Escape key add `pointer-events: none` to body when closing a dialog in a dropdown menu

[besirThor]

The issue is that when the dialog is opened and I use the escape key to close the dialog it adds to the body pointer-events: none and the whole page become unusable.

I have seen multiple solutions but I cannot seem to find this. How can I prevent the dropdown to close, when the dialog is opened and I press the escape key.

This is the code when I am having the problem on.

Modal is a custom component built on top of the dialog modal.

[mcollina]

This module does not claim to have this protection in any way.

I think it should be possible to implement this with an interceptor: https://github.com/nodejs/undici/blob/main/docs/api/DispatchInterceptor.md.

We might want to develop some mitigation via a custom agent. wdyt @RafaelGSS @ronag?

[ronjouch]

@mcollina thx for the fast reply!

This module does not claim to have this protection in any way.

Okay. Not surprised, given that fetch comes foremost from a browser world.

I think it should be possible to implement this with an interceptor: https://github.com/nodejs/undici/blob/main/docs/api/DispatchInterceptor.md . We might want to develop some mitigation via a custom agent.

👍, super interested in this discussion. But in an interceptor world, if I read correctly your types at https://github.com/nodejs/undici/blob/7276126945c52cf7ec61460b36d19f882e1bab82/types/dispatcher.d.ts#L96-L98 , I see that a DispatchInterceptor is being fed a DispatchOptions with an origin URL and path, whereas I need a host/IP. So, from my understanding, I'd still have to do my own dns lookup here (which undici will repeat afterwards), which means this API isn't exactly set up at the right point I need in the request lifecycle (after { DNS resolution, redirects followup }, before firing the request) 😕. Am I correct?

---

In the meantime, and putting aside interceptors, my plan was to do the SSRF check manually before calling fetch, repeating what ssrf-req-filter does with help from ipaddr.js:

const ip = await require('dns').promises.lookup(maybeMaliciousUrl).address;
const isSuspicious = require('ipaddr.js').parse(ip).range() !== 'unicast';
if (isSuspicious) { throw 'why u ssrf'; }

I actually like this thing, it's straightforward and explicit, more than the plugins.

However, I dislike that it forces an extra dns lookup, whereas a "plugin" agent solution integrated to fetch (similar to what request & axios do) does the lookup, and feeds it the agent/interceptor the IP, eliminating the extra dns resolution.

Any opinion/comment about it? Or should I just not care because the dns lookup fast/cached?

[mcollina]

I think we might have to provide a checkup option in Client for you to plug into.

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[ronjouch]

I think we might have to provide a checkup option in Client for you to plug into.

@mcollina not sure I understand you: what do you mean by a checkup option in Client ?

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina considering it. As mentioned in https://github.com/nodejs/undici/issues/2019#issuecomment-1478534997 , I except for my DNS resolve question I also somehow like a dumb pre-call check (wrappable in a helper function). But I don't fully understand yet how e.g. request does its agent thing, and if it's anymore DNS-lookup-efficient than a dumb pre-call check.

I'll be researching what request does, and if still convinced by the agent approach, will give a PR a try. No commitment as long as I have no code to show, and passersby very welcome to jump on it and do it, of course.

Thanks for your halp.

[mcollina]

By looking at how ssrf-req-filter is implemented, we need to add an option to Client so that we can install the same lookup event handler inside our connect function.

[andsens]

@ronjouch

I actually like this thing, it's straightforward and explicit, more than the plugins.

However, I dislike that it forces an extra dns lookup, whereas a "plugin" agent solution integrated to fetch (similar to what request & axios do) does the lookup, and feeds it the agent/interceptor the IP, eliminating the extra dns resolution.

Any opinion/comment about it? Or should I just not care because the dns lookup fast/cached?

This is actually insecure because of a TOCTOU (time of check, time of use) vulnerability (owasp link). You can create e.g. a round robin A record where only one of the addresses is for an internal IP while the others are fine. So when you check you get one IP and when you run it you get another.

[nunofgs]

FWIW I was able to pass a custom lookup function to get this functionality:

import dns from 'node:dns';

const lookup = function (hostname, options, callback) {
  dns.lookup(hostname, options, (err, address, family) => {
    if (err) {
      return callback(err, address, family);
    }

    if (address === '127.0.0.1') {
      return callback(new Error('not allowed'), address, family);
    }

    return callback(err, address, family);
  })
}

const agent = new Agent({
  connect: {
    lookup
  }
});

setGlobalDispatcher(agent);

[matthieusieben]

@nunofgs

FWIW I was able to pass a custom lookup function to get this functionality:

Using a global dispatcher might be problematic as, in some cases, you do might want to be able to contact other micro services. Also, using only the lookup function as protection would allow an attacker to circumvent the protection by using an IP in the URI (e.g. http://[::1]/foo).

[matthieusieben]

I personally use a wrapper around fetch that can be used as follows:

const safeFetch = ssrfFetchWrap({ fetch: globalThis.fetch })
safeFetch(new Request('http://[::1]/foo')) // Will throw

fetch() can still be used for trusted hosts (e.g. micro services) and safeFetch() for user provided endpoints.

Here is what the implementation looks like:

import dns, { promises as dnsPromises } from 'node:dns'

import ipaddr from 'ipaddr.js'

const { IPv4, IPv6 } = ipaddr

type Fetch = (
  this: void | null | typeof globalThis,
  input: Request,
) => Promise<Response>
class FetchError extends Error {
  constructor(
    public readonly status: number,
    message: string,
    public readonly context: Record<string, unknown>,
  ) {
    super(message)
  }
}

export type SsrfSafeFetchWrapOptions = NonNullable<
  Parameters<typeof ssrfFetchWrap>[0]
>

/**
 * @see {@link https://owasp.org/Top10/A10_2021-Server-Side_Request_Forgery_%28SSRF%29/}
 */
export const ssrfFetchWrap = ({
  fetch = globalThis.fetch as Fetch,
} = {}): Fetch => {
  const ssrfSafeFetch: Fetch = async (request) => {
    const { protocol, hostname } = new URL(request.url)

    if (protocol === 'http:' || protocol === 'https:') {
      if (request.redirect === 'follow') {
        // Owasp recommends to "Disable HTTP redirections"
        throw new FetchError(
          500,
          'Request redirect must be "error" or "manual" when SSRF is enabled',
          { request },
        )
      }

      // Make sure the hostname is a unicast IP address
      await verifyHostnameIps(hostname)
    } else if (protocol === 'data:') {
      // No SSRF issue
    } else {
      // blob: about: file: all should be rejected
      throw new FetchError(400, `Forbidden protocol ${protocol}`, { request })
    }

    return fetch(request)
  }

  return ssrfSafeFetch
}

async function verifyHostnameIps(hostname: string) {
  for await (const ip of hostnameLookup(hostname)) {
    if (ip.range() !== 'unicast') {
      throw new FetchError(400, `Invalid hostname IP address ${ip}`, {})
    }
  }
}

async function* hostnameLookup(
  hostname: string,
): AsyncGenerator<ipaddr.IPv4 | ipaddr.IPv6> {
  if (IPv4.isIPv4(hostname)) {
    return yield IPv4.parse(hostname)
  }

  if (hostname.startsWith('[') && hostname.endsWith(']')) {
    return yield IPv6.parse(hostname.slice(1, -1))
  }

  return yield* domainLookup(hostname)
}

async function* domainLookup(
  domain: string,
): AsyncGenerator<ipaddr.IPv4 | ipaddr.IPv6> {
  const results = await dnsPromises
    .lookup(domain, {
      hints: dns.ADDRCONFIG | dns.V4MAPPED,
      all: true,
    })
    .catch((cause) => {
      throw cause?.code === 'ENOTFOUND'
        ? new FetchError(400, `Invalid hostname ${domain}`, {
            cause,
          })
        : new FetchError(500, `Unable resolve DNS for ${domain}`, {
            cause,
          })
    })

  for (const addr of results) {
    const ip =
      addr.family === 4 ? IPv4.parse(addr.address) : IPv6.parse(addr.address)

    if (ip instanceof IPv6 && ip.isIPv4MappedAddress()) {
      yield ip.toIPv4Address()
    } else {
      yield ip
    }
  }
}

[andsens]

@matthieusieben see my previous comment https://github.com/nodejs/undici/issues/2019#issuecomment-1706572083 as to why this is unsafe.