From bce7b75f900f37c5e08832ecd4adb5bbed2a9910 Mon Sep 17 00:00:00 2001 From: Khafra <42794878+KhafraDev@users.noreply.github.com> Date: Sat, 28 May 2022 11:12:54 -0400 Subject: [PATCH 1/5] feat: remove headers filtering --- README.md | 8 +++++ lib/fetch/constants.js | 31 ----------------- lib/fetch/headers.js | 41 +++++----------------- lib/fetch/request.js | 4 +-- lib/fetch/response.js | 37 ++++---------------- test/fetch/cookies.js | 50 ++++++++++++++++++++++++++ test/fetch/headers.js | 79 ------------------------------------------ 7 files changed, 75 insertions(+), 175 deletions(-) create mode 100644 test/fetch/cookies.js diff --git a/README.md b/README.md index b7a5ef1acd4..cbcfb0096c0 100644 --- a/README.md +++ b/README.md @@ -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. + ### `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. diff --git a/lib/fetch/constants.js b/lib/fetch/constants.js index 75c9265e8c1..4358cd3ae08 100644 --- a/lib/fetch/constants.js +++ b/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] @@ -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', @@ -86,12 +60,8 @@ const subresource = [ '' ] -const corsSafeListedResponseHeaderNames = [] // TODO - module.exports = { subresource, - forbiddenResponseHeaderNames, - corsSafeListedResponseHeaderNames, forbiddenMethods, requestBodyHeader, referrerPolicy, @@ -99,7 +69,6 @@ module.exports = { requestMode, requestCredentials, requestCache, - forbiddenHeaderNames, redirectStatus, corsSafeListedMethods, nullBodyStatus, diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index eb4eb47c4ec..3469c589114 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -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') @@ -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 } @@ -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 } @@ -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 } diff --git a/lib/fetch/request.js b/lib/fetch/request.js index 661f5814ba2..d17e3e3ccde 100644 --- a/lib/fetch/request.js +++ b/lib/fetch/request.js @@ -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) } } else { // 5. Otherwise, fill this’s headers with headers. @@ -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) } } diff --git a/lib/fetch/response.js b/lib/fetch/response.js index 03db7eb488e..582876e050a 100644 --- a/lib/fetch/response.js +++ b/lib/fetch/response.js @@ -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') @@ -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 @@ -411,12 +387,10 @@ 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" @@ -424,9 +398,10 @@ function filterResponse (response, type) { // 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 @@ -449,7 +424,7 @@ function filterResponse (response, type) { type: 'opaqueredirect', status: 0, statusText: '', - headersList: makeFilteredHeadersList(response.headersList, () => false), + headersList: [], body: null }) } else { diff --git a/test/fetch/cookies.js b/test/fetch/cookies.js new file mode 100644 index 00000000000..d17a8827df5 --- /dev/null +++ b/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() +}) diff --git a/test/fetch/headers.js b/test/fetch/headers.js index f84c6c23839..05abc4339cf 100644 --- a/test/fetch/headers.js +++ b/test/fetch/headers.js @@ -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) @@ -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() - }) -}) From f8d12a3611c437ad5bd74abbeba28d3501aaac33 Mon Sep 17 00:00:00 2001 From: Khafra <42794878+KhafraDev@users.noreply.github.com> Date: Sat, 28 May 2022 13:24:26 -0400 Subject: [PATCH 2/5] add wintercg issue to README --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index cbcfb0096c0..0329b969d76 100644 --- a/README.md +++ b/README.md @@ -293,6 +293,7 @@ const headers = await fetch(url) * 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 +* https://github.com/wintercg/fetch/issues/6 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. From 81cdb44d900ceb447d7a85ed7c86dede935e56c8 Mon Sep 17 00:00:00 2001 From: Khafra <42794878+KhafraDev@users.noreply.github.com> Date: Sat, 28 May 2022 16:53:25 -0400 Subject: [PATCH 3/5] update README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 0329b969d76..c8dc260e12a 100644 --- a/README.md +++ b/README.md @@ -295,7 +295,7 @@ const headers = await fetch(url) * https://fetch.spec.whatwg.org/#forbidden-response-header-name * https://github.com/wintercg/fetch/issues/6 -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. +The [Fetch Standard](https://fetch.spec.whatwg.org) requires implementations to exclude certain headers from requests and responses. In browser environments, some headers are forbidden so the user agent remains in full control over them. In Undici, these constraints are removed to give more control to the user. ### `undici.upgrade([url, options]): Promise` From e500a5872414e55b54487ae549d1047132883611 Mon Sep 17 00:00:00 2001 From: Khafra <42794878+KhafraDev@users.noreply.github.com> Date: Sat, 28 May 2022 18:23:35 -0400 Subject: [PATCH 4/5] fix(Headers): properly use/set "this's headers" --- lib/fetch/request.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/fetch/request.js b/lib/fetch/request.js index d17e3e3ccde..161db6c1741 100644 --- a/lib/fetch/request.js +++ b/lib/fetch/request.js @@ -383,10 +383,10 @@ class Request { // 30. Set this’s headers to a new Headers object with this’s relevant // Realm, whose header list is request’s header list and guard is // "request". - this[kHeaders] = new Headers() + this[kHeaders] = new Headers(request.headersList) this[kHeaders][kGuard] = 'request' - this[kHeaders][kHeadersList] = request.headersList this[kHeaders][kRealm] = this[kRealm] + this[kState].headersList = this[kHeaders][kHeadersList] // 31. If this’s request’s mode is "no-cors", then: if (mode === 'no-cors') { @@ -406,7 +406,7 @@ class Request { if (Object.keys(init).length !== 0) { // 1. Let headers be a copy of this’s headers and its associated header // list. - let headers = new Headers(this.headers) + let headers = new Headers(this[kHeaders]) // 2. If init["headers"] exists, then set headers to init["headers"]. if (init.headers !== undefined) { @@ -414,18 +414,18 @@ class Request { } // 3. Empty this’s headers’s header list. - this[kState].headersList = new HeadersList() - this[kHeaders][kHeadersList] = this[kState].headersList + this[kHeaders] = new Headers() + this[kState].headersList = this[kHeaders][kHeadersList] // 4. If headers is a Headers object, then for each header in its header // 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[kState].headersList.append(key, val) + for (const [key, val] of headers) { + this[kHeaders].append(key, val) } } else { // 5. Otherwise, fill this’s headers with headers. - fillHeaders(this[kState].headersList, headers) + fillHeaders(this[kHeaders], headers) } } @@ -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[kState].headersList.append('content-type', contentType) + this[kHeaders].append('content-type', contentType) } } From f460eaeb06af80c8fd75af0f0b459a7198f76a60 Mon Sep 17 00:00:00 2001 From: Khafra <42794878+KhafraDev@users.noreply.github.com> Date: Sun, 29 May 2022 21:36:16 -0400 Subject: [PATCH 5/5] fix: remove broken guard checks --- lib/fetch/headers.js | 17 +++++------------ lib/fetch/request.js | 7 +++---- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index 3469c589114..0c6e5584d33 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -111,6 +111,11 @@ class HeadersList { } } + clear () { + this[kHeadersMap].clear() + this[kHeadersSortedMap] = null + } + append (name, value) { this[kHeadersSortedMap] = null @@ -210,12 +215,8 @@ class Headers { // Note: undici does not implement forbidden header names if (this[kGuard] === 'immutable') { throw new TypeError('immutable') - } else if (this[kGuard] === 'request') { - return } else if (this[kGuard] === 'request-no-cors') { // TODO - } else if (this[kGuard] === 'response') { - return } return this[kHeadersList].append(String(name), String(value)) @@ -236,12 +237,8 @@ class Headers { // Note: undici does not implement forbidden header names if (this[kGuard] === 'immutable') { throw new TypeError('immutable') - } else if (this[kGuard] === 'request') { - return } else if (this[kGuard] === 'request-no-cors') { // TODO - } else if (this[kGuard] === 'response') { - return } return this[kHeadersList].delete(String(name)) @@ -292,12 +289,8 @@ class Headers { // Note: undici does not implement forbidden header names if (this[kGuard] === 'immutable') { throw new TypeError('immutable') - } else if (this[kGuard] === 'request') { - return } else if (this[kGuard] === 'request-no-cors') { // TODO - } else if (this[kGuard] === 'response') { - return } return this[kHeadersList].set(String(name), String(value)) diff --git a/lib/fetch/request.js b/lib/fetch/request.js index 161db6c1741..42c7eb447a2 100644 --- a/lib/fetch/request.js +++ b/lib/fetch/request.js @@ -383,10 +383,10 @@ class Request { // 30. Set this’s headers to a new Headers object with this’s relevant // Realm, whose header list is request’s header list and guard is // "request". - this[kHeaders] = new Headers(request.headersList) + this[kHeaders] = new Headers() + this[kHeaders][kHeadersList] = request.headersList this[kHeaders][kGuard] = 'request' this[kHeaders][kRealm] = this[kRealm] - this[kState].headersList = this[kHeaders][kHeadersList] // 31. If this’s request’s mode is "no-cors", then: if (mode === 'no-cors') { @@ -414,8 +414,7 @@ class Request { } // 3. Empty this’s headers’s header list. - this[kHeaders] = new Headers() - this[kState].headersList = this[kHeaders][kHeadersList] + this[kHeaders][kHeadersList].clear() // 4. If headers is a Headers object, then for each header in its header // list, append header’s name/header’s value to this’s headers.