Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove headers filtering #1469

Merged
merged 5 commits into from May 30, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.md
Expand Up @@ -288,6 +288,14 @@ const headers = await fetch(url)
.then(res => res.headers)
```

##### Forbidden and Safelisted Header Names

* https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name
* https://fetch.spec.whatwg.org/#forbidden-header-name
* https://fetch.spec.whatwg.org/#forbidden-response-header-name

The [Fetch Standard](https://fetch.spec.whatwg.org) requires implementations to exclude certain headers from requests and responses. Undici does not filter out these headers.
KhafraDev marked this conversation as resolved.
Show resolved Hide resolved

KhafraDev marked this conversation as resolved.
Show resolved Hide resolved
### `undici.upgrade([url, options]): Promise`

Upgrade to a different protocol. See [MDN - HTTP - Protocol upgrade mechanism](https://developer.mozilla.org/en-US/docs/Web/HTTP/Protocol_upgrade_mechanism) for more details.
Expand Down
31 changes: 0 additions & 31 deletions lib/fetch/constants.js
@@ -1,28 +1,5 @@
'use strict'

const forbiddenHeaderNames = [
'accept-charset',
'accept-encoding',
'access-control-request-headers',
'access-control-request-method',
'connection',
'content-length',
'cookie',
'cookie2',
'date',
'dnt',
'expect',
'host',
'keep-alive',
'origin',
'referer',
'te',
'trailer',
'transfer-encoding',
'upgrade',
'via'
]

const corsSafeListedMethods = ['GET', 'HEAD', 'POST']

const nullBodyStatus = [101, 204, 205, 304]
Expand Down Expand Up @@ -58,9 +35,6 @@ const requestCache = [
'only-if-cached'
]

// https://fetch.spec.whatwg.org/#forbidden-response-header-name
const forbiddenResponseHeaderNames = ['set-cookie', 'set-cookie2']

const requestBodyHeader = [
'content-encoding',
'content-language',
Expand All @@ -86,20 +60,15 @@ const subresource = [
''
]

const corsSafeListedResponseHeaderNames = [] // TODO

module.exports = {
subresource,
forbiddenResponseHeaderNames,
corsSafeListedResponseHeaderNames,
forbiddenMethods,
requestBodyHeader,
referrerPolicy,
requestRedirect,
requestMode,
requestCredentials,
requestCache,
forbiddenHeaderNames,
redirectStatus,
corsSafeListedMethods,
nullBodyStatus,
Expand Down
41 changes: 9 additions & 32 deletions lib/fetch/headers.js
Expand Up @@ -6,10 +6,6 @@ const { validateHeaderName, validateHeaderValue } = require('http')
const { kHeadersList } = require('../core/symbols')
const { kGuard } = require('./symbols')
const { kEnumerableProperty } = require('../core/util')
const {
forbiddenHeaderNames,
forbiddenResponseHeaderNames
} = require('./constants')

const kHeadersMap = Symbol('headers map')
const kHeadersSortedMap = Symbol('headers map sorted')
Expand Down Expand Up @@ -211,21 +207,14 @@ class Headers {
)
}

const normalizedName = normalizeAndValidateHeaderName(String(name))

// Note: undici does not implement forbidden header names
if (this[kGuard] === 'immutable') {
throw new TypeError('immutable')
} else if (
this[kGuard] === 'request' &&
forbiddenHeaderNames.includes(normalizedName)
) {
} else if (this[kGuard] === 'request') {
return
} else if (this[kGuard] === 'request-no-cors') {
// TODO
} else if (
this[kGuard] === 'response' &&
forbiddenResponseHeaderNames.includes(normalizedName)
) {
} else if (this[kGuard] === 'response') {
return
}

Expand All @@ -244,21 +233,14 @@ class Headers {
)
}

const normalizedName = normalizeAndValidateHeaderName(String(name))

// Note: undici does not implement forbidden header names
if (this[kGuard] === 'immutable') {
throw new TypeError('immutable')
} else if (
this[kGuard] === 'request' &&
forbiddenHeaderNames.includes(normalizedName)
) {
} else if (this[kGuard] === 'request') {
return
} else if (this[kGuard] === 'request-no-cors') {
// TODO
} else if (
this[kGuard] === 'response' &&
forbiddenResponseHeaderNames.includes(normalizedName)
) {
} else if (this[kGuard] === 'response') {
return
}

Expand Down Expand Up @@ -307,19 +289,14 @@ class Headers {
)
}

// Note: undici does not implement forbidden header names
if (this[kGuard] === 'immutable') {
throw new TypeError('immutable')
} else if (
this[kGuard] === 'request' &&
forbiddenHeaderNames.includes(String(name).toLocaleLowerCase())
) {
} else if (this[kGuard] === 'request') {
return
} else if (this[kGuard] === 'request-no-cors') {
// TODO
} else if (
this[kGuard] === 'response' &&
forbiddenResponseHeaderNames.includes(String(name).toLocaleLowerCase())
) {
} else if (this[kGuard] === 'response') {
return
}

Expand Down
4 changes: 2 additions & 2 deletions lib/fetch/request.js
Expand Up @@ -421,7 +421,7 @@ class Request {
// list, append header’s name/header’s value to this’s headers.
if (headers.constructor.name === 'Headers') {
for (const [key, val] of headers[kHeadersList] || headers) {
this[kHeaders].append(key, val)
this[kState].headersList.append(key, val)
ronag marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
// 5. Otherwise, fill this’s headers with headers.
Expand Down Expand Up @@ -461,7 +461,7 @@ class Request {
// not contain `Content-Type`, then append `Content-Type`/Content-Type to
// this’s headers.
if (contentType && !this[kHeaders].has('content-type')) {
this[kHeaders].append('content-type', contentType)
this[kState].headersList.append('content-type', contentType)
}
}

Expand Down
37 changes: 6 additions & 31 deletions lib/fetch/response.js
Expand Up @@ -8,9 +8,7 @@ const { kEnumerableProperty } = util
const { responseURL, isValidReasonPhrase, toUSVString, isCancelled, isAborted, serializeJavascriptValueToJSONString } = require('./util')
const {
redirectStatus,
nullBodyStatus,
forbiddenResponseHeaderNames,
corsSafeListedResponseHeaderNames
nullBodyStatus
} = require('./constants')
const { kState, kHeaders, kGuard, kRealm } = require('./symbols')
const { kHeadersList } = require('../core/symbols')
Expand Down Expand Up @@ -380,28 +378,6 @@ function makeFilteredResponse (response, state) {
})
}

function makeFilteredHeadersList (headersList, filter) {
return new Proxy(headersList, {
get (target, prop) {
// Override methods used by Headers class.
if (prop === 'get' || prop === 'has') {
const defaultReturn = prop === 'has' ? false : null
return (name) => filter(name) ? target[prop](name) : defaultReturn
} else if (prop === Symbol.iterator) {
return function * () {
for (const entry of target) {
if (filter(entry[0])) {
yield entry
}
}
}
} else {
return target[prop]
}
}
})
}

// https://fetch.spec.whatwg.org/#concept-filtered-response
function filterResponse (response, type) {
// Set response to the following filtered response with response as its
Expand All @@ -411,22 +387,21 @@ function filterResponse (response, type) {
// and header list excludes any headers in internal response’s header list
// whose name is a forbidden response-header name.

// Note: undici does not implement forbidden response-header names
return makeFilteredResponse(response, {
type: 'basic',
headersList: makeFilteredHeadersList(
response.headersList,
(name) => !forbiddenResponseHeaderNames.includes(name.toLowerCase())
)
headersList: response.headersList
})
} else if (type === 'cors') {
// A CORS filtered response is a filtered response whose type is "cors"
// and header list excludes any headers in internal response’s header
// list whose name is not a CORS-safelisted response-header name, given
// internal response’s CORS-exposed header-name list.

// Note: undici does not implement CORS-safelisted response-header names
return makeFilteredResponse(response, {
type: 'cors',
headersList: makeFilteredHeadersList(response.headersList, (name) => !corsSafeListedResponseHeaderNames.includes(name))
headersList: response.headersList
})
} else if (type === 'opaque') {
// An opaque filtered response is a filtered response whose type is
Expand All @@ -449,7 +424,7 @@ function filterResponse (response, type) {
type: 'opaqueredirect',
status: 0,
statusText: '',
headersList: makeFilteredHeadersList(response.headersList, () => false),
headersList: [],
body: null
})
} else {
Expand Down
50 changes: 50 additions & 0 deletions test/fetch/cookies.js
@@ -0,0 +1,50 @@
'use strict'

const { once } = require('events')
const { createServer } = require('http')
const { test } = require('tap')
const { fetch, Headers } = require('../..')

test('Can receive set-cookie headers from a server using fetch - issue #1262', async (t) => {
const server = createServer((req, res) => {
res.setHeader('set-cookie', 'name=value; Domain=example.com')
res.end()
}).listen(0)

t.teardown(server.close.bind(server))
await once(server, 'listening')

const response = await fetch(`http://localhost:${server.address().port}`)

t.equal(response.headers.get('set-cookie'), 'name=value; Domain=example.com')

const response2 = await fetch(`http://localhost:${server.address().port}`, {
credentials: 'include'
})

t.equal(response2.headers.get('set-cookie'), 'name=value; Domain=example.com')

t.end()
})

test('Can send cookies to a server with fetch - issue #1463', async (t) => {
const server = createServer((req, res) => {
t.equal(req.headers.cookie, 'value')
res.end()
}).listen(0)

t.teardown(server.close.bind(server))
await once(server, 'listening')

const headersInit = [
new Headers([['cookie', 'value']]),
{ cookie: 'value' },
[['cookie', 'value']]
]

for (const headers of headersInit) {
await fetch(`http://localhost:${server.address().port}`, { headers })
}

t.end()
})
79 changes: 0 additions & 79 deletions test/fetch/headers.js
Expand Up @@ -8,12 +8,6 @@ const {
fill
} = require('../../lib/fetch/headers')
const { kGuard } = require('../../lib/fetch/symbols')
const {
forbiddenHeaderNames,
forbiddenResponseHeaderNames
} = require('../../lib/fetch/constants')
const { createServer } = require('http')
const { fetch } = require('../../index')

tap.test('Headers initialization', t => {
t.plan(7)
Expand Down Expand Up @@ -661,76 +655,3 @@ tap.test('request-no-cors guard', (t) => {
t.doesNotThrow(() => { headers.delete('key') })
t.end()
})

tap.test('request guard', (t) => {
const headers = new Headers(forbiddenHeaderNames.map(k => [k, 'v']))
headers[kGuard] = 'request'
headers.set('set-cookie', 'val')

for (const name of forbiddenHeaderNames) {
headers.set(name, '1')
headers.append(name, '1')
t.equal(headers.get(name), 'v')
headers.delete(name)
t.equal(headers.has(name), true)
}

t.equal(headers.get('set-cookie'), 'val')
t.equal(headers.has('set-cookie'), true)

t.end()
})

tap.test('response guard', (t) => {
const headers = new Headers(forbiddenResponseHeaderNames.map(k => [k, 'v']))
headers[kGuard] = 'response'
headers.set('key', 'val')
headers.set('keep-alive', 'val')

for (const name of forbiddenResponseHeaderNames) {
headers.set(name, '1')
headers.append(name, '1')
t.equal(headers.get(name), 'v')
headers.delete(name)
t.equal(headers.has(name), true)
}

t.equal(headers.get('keep-alive'), 'val')
t.equal(headers.has('keep-alive'), true)

t.end()
})

tap.test('set-cookie[2] in Headers constructor', (t) => {
const headers = new Headers(forbiddenResponseHeaderNames.map(k => [k, 'v']))

for (const header of forbiddenResponseHeaderNames) {
t.ok(headers.has(header))
t.equal(headers.get(header), 'v')
}

t.end()
})

// https://github.com/nodejs/undici/issues/1328
tap.test('set-cookie[2] received from server - issue #1328', (t) => {
const server = createServer((req, res) => {
res.setHeader('set-cookie', 'my-cookie; wow')
res.end('Goodbye!')
}).unref()
t.teardown(server.close.bind(server))

server.listen(0, async () => {
const { headers } = await fetch(`http://localhost:${server.address().port}`)

t.notOk(headers.has('set-cookie'))
t.notOk(headers.has('Set-cookie'))
t.notOk(headers.has('sEt-CoOkIe'))

t.equal(headers.get('set-cookie'), null)
t.equal(headers.get('Set-cookie'), null)
t.equal(headers.get('sEt-CoOkIe'), null)

t.end()
})
})