bluesky-social / atproto

Social networking technology created by Bluesky
Other
7.29k stars 515 forks source link

`safeFetchWrap()` may destroy or drain response before it can be read #2851

Closed devinivy closed 1 month ago

devinivy commented 1 month ago

Describe the bug

The safeFetchWrap() utility from @atproto/fetch-node seems to misbehave when certain parts of SSRF protection are enabled (specifically allowPrivateIps: false) and a custom fetch is provided. In this case response bodies are not able to be read at all. This seems to have been introduced recently in v0.1.1.

To Reproduce

Steps to reproduce the behavior:

import { safeFetchWrap } from '@atproto/fetch-node'

const safeFetch = safeFetchWrap({
  // set to false
  allowPrivateIps: false,
  // set to anything except globalThis.fetch
  fetch: async (input, init) => {
    return globalThis.fetch(input, init)
  },
})

const res = await safeFetch(
  // must use a domain here, not an ip address
  'https://identity.foundation/.well-known/did.json',
  { redirect: 'manual' },
)

await res.json()
/**
 * TypeError: terminated
 *       at Request.onError (node_modules/.pnpm/undici@6.19.8/node_modules/undici/lib/core/request.js:299:27)
 *       at Object.errorRequest (node_modules/.pnpm/undici@6.19.8/node_modules/undici/lib/core/util.js:638:13)
 *       at abort (node_modules/.pnpm/undici@6.19.8/node_modules/undici/lib/dispatcher/client-h2.js:277:10)
 *       at ClientHttp2Stream.<anonymous> (node_modules/.pnpm/undici@6.19.8/node_modules/undici/lib/dispatcher/client-h2.js:452:5)
 */

Expected behavior

It's expected that await res.json() would succeed rather than throw with the TypeError indicated above.

Details

Additional context

Removing these lines seems to resolve the issue, though I'm not sure if it's a legitimate fix: https://github.com/bluesky-social/atproto/blob/a611a5fe56bda94dea1eda5ab9f355e156f00ad7/packages/internal/fetch-node/src/unicast.ts#L132-L135

This implies that destroying the single-use dispatcher before reading the body will cause the body to be destroyed or drained.

matthieusieben commented 1 month ago

Tracked to a bug in undici https://github.com/nodejs/undici/issues/3671