From f2b9846906187b992b619973ea3aeadf2c0c62e5 Mon Sep 17 00:00:00 2001 From: Khafra Date: Thu, 16 May 2024 11:29:17 -0400 Subject: [PATCH] use private properties in Headers --- benchmarks/fetch/headers-length32.mjs | 8 +-- benchmarks/fetch/headers.mjs | 10 +-- lib/core/symbols.js | 1 - lib/web/cookies/util.js | 8 ++- lib/web/fetch/headers.js | 90 ++++++++++++++++++--------- lib/web/fetch/request.js | 22 +++---- lib/web/fetch/response.js | 16 ++--- lib/web/fetch/symbols.js | 1 - lib/web/websocket/connection.js | 5 +- test/fetch/headers.js | 7 +-- test/fetch/issue-3267.js | 18 ++++++ test/fetch/request.js | 8 +-- test/fetch/response.js | 8 +-- 13 files changed, 119 insertions(+), 83 deletions(-) create mode 100644 test/fetch/issue-3267.js diff --git a/benchmarks/fetch/headers-length32.mjs b/benchmarks/fetch/headers-length32.mjs index a9b7a52a40e..16062022111 100644 --- a/benchmarks/fetch/headers-length32.mjs +++ b/benchmarks/fetch/headers-length32.mjs @@ -1,5 +1,5 @@ import { bench, run } from 'mitata' -import { Headers } from '../../lib/web/fetch/headers.js' +import { Headers, getHeadersList } from '../../lib/web/fetch/headers.js' const headers = new Headers( [ @@ -38,11 +38,7 @@ const headers = new Headers( ].map((v) => [v, '']) ) -const kHeadersList = Reflect.ownKeys(headers).find( - (c) => String(c) === 'Symbol(headers list)' -) - -const headersList = headers[kHeadersList] +const headersList = getHeadersList(headers) const kHeadersSortedMap = Reflect.ownKeys(headersList).find( (c) => String(c) === 'Symbol(headers map sorted)' diff --git a/benchmarks/fetch/headers.mjs b/benchmarks/fetch/headers.mjs index d529370c40c..7f9047b6e2e 100644 --- a/benchmarks/fetch/headers.mjs +++ b/benchmarks/fetch/headers.mjs @@ -1,5 +1,5 @@ import { bench, group, run } from 'mitata' -import { Headers } from '../../lib/web/fetch/headers.js' +import { Headers, getHeadersList } from '../../lib/web/fetch/headers.js' const characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789' const charactersLength = characters.length @@ -27,13 +27,9 @@ for (const [name, length] of Object.entries(settings)) { const headersSorted = new Headers(headers) - const kHeadersList = Reflect.ownKeys(headers).find( - (c) => String(c) === 'Symbol(headers list)' - ) - - const headersList = headers[kHeadersList] + const headersList = getHeadersList(headers) - const headersListSorted = headersSorted[kHeadersList] + const headersListSorted = getHeadersList(headersSorted) const kHeadersSortedMap = Reflect.ownKeys(headersList).find( (c) => String(c) === 'Symbol(headers map sorted)' diff --git a/lib/core/symbols.js b/lib/core/symbols.js index 879ba833de6..b58fc90a69f 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -8,7 +8,6 @@ module.exports = { kQueue: Symbol('queue'), kConnect: Symbol('connect'), kConnecting: Symbol('connecting'), - kHeadersList: Symbol('headers list'), kKeepAliveDefaultTimeout: Symbol('default keep alive timeout'), kKeepAliveMaxTimeout: Symbol('max keep alive timeout'), kKeepAliveTimeoutThreshold: Symbol('keep alive timeout threshold'), diff --git a/lib/web/cookies/util.js b/lib/web/cookies/util.js index d49857175a4..8c6a77bc717 100644 --- a/lib/web/cookies/util.js +++ b/lib/web/cookies/util.js @@ -1,7 +1,7 @@ 'use strict' const assert = require('node:assert') -const { kHeadersList } = require('../../core/symbols') +const { getHeadersList: internalGetHeadersList } = require('../fetch/headers') /** * @param {string} value @@ -278,8 +278,10 @@ function stringify (cookie) { let kHeadersListNode function getHeadersList (headers) { - if (headers[kHeadersList]) { - return headers[kHeadersList] + try { + return internalGetHeadersList(headers) + } catch { + // fall-through } if (!kHeadersListNode) { diff --git a/lib/web/fetch/headers.js b/lib/web/fetch/headers.js index d726b5a5d10..5e2321b8292 100644 --- a/lib/web/fetch/headers.js +++ b/lib/web/fetch/headers.js @@ -2,8 +2,7 @@ 'use strict' -const { kHeadersList, kConstruct } = require('../../core/symbols') -const { kGuard } = require('./symbols') +const { kConstruct } = require('../../core/symbols') const { kEnumerableProperty } = require('../../core/util') const { iteratorMixin, @@ -103,19 +102,18 @@ function appendHeader (headers, name, value) { // 3. If headers’s guard is "immutable", then throw a TypeError. // 4. Otherwise, if headers’s guard is "request" and name is a // forbidden header name, return. + // 5. Otherwise, if headers’s guard is "request-no-cors": + // TODO // Note: undici does not implement forbidden header names - if (headers[kGuard] === 'immutable') { + if (getHeadersGuard(headers) === 'immutable') { throw new TypeError('immutable') - } else if (headers[kGuard] === 'request-no-cors') { - // 5. Otherwise, if headers’s guard is "request-no-cors": - // TODO } // 6. Otherwise, if headers’s guard is "response" and name is a // forbidden response-header name, return. // 7. Append (name, value) to headers’s header list. - return headers[kHeadersList].append(name, value, false) + return getHeadersList(headers).append(name, value, false) // 8. If headers’s guard is "request-no-cors", then remove // privileged no-CORS request headers from headers @@ -357,16 +355,20 @@ class HeadersList { // https://fetch.spec.whatwg.org/#headers-class class Headers { + #guard + #headersList + constructor (init = undefined) { if (init === kConstruct) { return } - this[kHeadersList] = new HeadersList() + + this.#headersList = new HeadersList() // The new Headers(init) constructor steps are: // 1. Set this’s guard to "none". - this[kGuard] = 'none' + this.#guard = 'none' // 2. If init is given, then fill this with init. if (init !== undefined) { @@ -416,22 +418,20 @@ class Headers { // 5. Otherwise, if this’s guard is "response" and name is // a forbidden response-header name, return. // Note: undici does not implement forbidden header names - if (this[kGuard] === 'immutable') { + if (this.#guard === 'immutable') { throw new TypeError('immutable') - } else if (this[kGuard] === 'request-no-cors') { - // TODO } // 6. If this’s header list does not contain name, then // return. - if (!this[kHeadersList].contains(name, false)) { + if (!this.#headersList.contains(name, false)) { return } // 7. Delete name from this’s header list. // 8. If this’s guard is "request-no-cors", then remove // privileged no-CORS request headers from this. - this[kHeadersList].delete(name, false) + this.#headersList.delete(name, false) } // https://fetch.spec.whatwg.org/#dom-headers-get @@ -454,7 +454,7 @@ class Headers { // 2. Return the result of getting name from this’s header // list. - return this[kHeadersList].get(name, false) + return this.#headersList.get(name, false) } // https://fetch.spec.whatwg.org/#dom-headers-has @@ -477,7 +477,7 @@ class Headers { // 2. Return true if this’s header list contains name; // otherwise false. - return this[kHeadersList].contains(name, false) + return this.#headersList.contains(name, false) } // https://fetch.spec.whatwg.org/#dom-headers-set @@ -518,16 +518,14 @@ class Headers { // 6. Otherwise, if this’s guard is "response" and name is a // forbidden response-header name, return. // Note: undici does not implement forbidden header names - if (this[kGuard] === 'immutable') { + if (this.#guard === 'immutable') { throw new TypeError('immutable') - } else if (this[kGuard] === 'request-no-cors') { - // TODO } // 7. Set (name, value) in this’s header list. // 8. If this’s guard is "request-no-cors", then remove // privileged no-CORS request headers from this - this[kHeadersList].set(name, value, false) + this.#headersList.set(name, value, false) } // https://fetch.spec.whatwg.org/#dom-headers-getsetcookie @@ -538,7 +536,7 @@ class Headers { // 2. Return the values of all headers in this’s header list whose name is // a byte-case-insensitive match for `Set-Cookie`, in order. - const list = this[kHeadersList].cookies + const list = this.#headersList.cookies if (list) { return [...list] @@ -549,8 +547,8 @@ class Headers { // https://fetch.spec.whatwg.org/#concept-header-list-sort-and-combine get [kHeadersSortedMap] () { - if (this[kHeadersList][kHeadersSortedMap]) { - return this[kHeadersList][kHeadersSortedMap] + if (this.#headersList[kHeadersSortedMap]) { + return this.#headersList[kHeadersSortedMap] } // 1. Let headers be an empty list of headers with the key being the name @@ -559,14 +557,14 @@ class Headers { // 2. Let names be the result of convert header names to a sorted-lowercase // set with all the names of the headers in list. - const names = this[kHeadersList].toSortedArray() + const names = this.#headersList.toSortedArray() - const cookies = this[kHeadersList].cookies + const cookies = this.#headersList.cookies // fast-path if (cookies === null || cookies.length === 1) { // Note: The non-null assertion of value has already been done by `HeadersList#toSortedArray` - return (this[kHeadersList][kHeadersSortedMap] = names) + return (this.#headersList[kHeadersSortedMap] = names) } // 3. For each name of names: @@ -596,16 +594,38 @@ class Headers { } // 4. Return headers. - return (this[kHeadersList][kHeadersSortedMap] = headers) + return (this.#headersList[kHeadersSortedMap] = headers) } [util.inspect.custom] (depth, options) { options.depth ??= depth - return `Headers ${util.formatWithOptions(options, this[kHeadersList].entries)}` + return `Headers ${util.formatWithOptions(options, this.#headersList.entries)}` + } + + static getHeadersGuard (o) { + return o.#guard + } + + static setHeadersGuard (o, guard) { + o.#guard = guard + } + + static getHeadersList (o) { + return o.#headersList + } + + static setHeadersList (o, list) { + o.#headersList = list } } +const { getHeadersGuard, setHeadersGuard, getHeadersList, setHeadersList } = Headers +Reflect.deleteProperty(Headers, 'getHeadersGuard') +Reflect.deleteProperty(Headers, 'setHeadersGuard') +Reflect.deleteProperty(Headers, 'getHeadersList') +Reflect.deleteProperty(Headers, 'setHeadersList') + Object.defineProperty(Headers.prototype, util.inspect.custom, { enumerable: false }) @@ -631,8 +651,12 @@ webidl.converters.HeadersInit = function (V, prefix, argument) { // A work-around to ensure we send the properly-cased Headers when V is a Headers object. // Read https://github.com/nodejs/undici/pull/3159#issuecomment-2075537226 before touching, please. - if (!util.types.isProxy(V) && kHeadersList in V && iterator === Headers.prototype.entries) { // Headers object - return V[kHeadersList].entriesList + if (!util.types.isProxy(V) && iterator === Headers.prototype.entries) { // Headers object + try { + return getHeadersList(V).entriesList + } catch { + // fall-through + } } if (typeof iterator === 'function') { @@ -654,5 +678,9 @@ module.exports = { // for test. compareHeaderName, Headers, - HeadersList + HeadersList, + getHeadersGuard, + setHeadersGuard, + setHeadersList, + getHeadersList } diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index c6d42fd36d6..07a87f66d47 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -3,7 +3,7 @@ 'use strict' const { extractBody, mixinBody, cloneBody } = require('./body') -const { Headers, fill: fillHeaders, HeadersList } = require('./headers') +const { Headers, fill: fillHeaders, HeadersList, setHeadersGuard, getHeadersGuard, setHeadersList, getHeadersList } = require('./headers') const { FinalizationRegistry } = require('./dispatcher-weakref')() const util = require('../../core/util') const nodeUtil = require('node:util') @@ -25,10 +25,10 @@ const { requestDuplex } = require('./constants') const { kEnumerableProperty } = util -const { kHeaders, kSignal, kState, kGuard, kDispatcher } = require('./symbols') +const { kHeaders, kSignal, kState, kDispatcher } = require('./symbols') const { webidl } = require('./webidl') const { URLSerializer } = require('./data-url') -const { kHeadersList, kConstruct } = require('../../core/symbols') +const { kConstruct } = require('../../core/symbols') const assert = require('node:assert') const { getMaxListeners, setMaxListeners, getEventListeners, defaultMaxListeners } = require('node:events') @@ -445,8 +445,8 @@ class Request { // Realm, whose header list is request’s header list and guard is // "request". this[kHeaders] = new Headers(kConstruct) - this[kHeaders][kHeadersList] = request.headersList - this[kHeaders][kGuard] = 'request' + setHeadersList(this[kHeaders], request.headersList) + setHeadersGuard(this[kHeaders], 'request') // 31. If this’s request’s mode is "no-cors", then: if (mode === 'no-cors') { @@ -459,13 +459,13 @@ class Request { } // 2. Set this’s headers’s guard to "request-no-cors". - this[kHeaders][kGuard] = 'request-no-cors' + setHeadersGuard(this[kHeaders], 'request-no-cors') } // 32. If init is not empty, then: if (initHasKey) { /** @type {HeadersList} */ - const headersList = this[kHeaders][kHeadersList] + const headersList = getHeadersList(this[kHeaders]) // 1. Let headers be a copy of this’s headers and its associated header // list. // 2. If init["headers"] exists, then set headers to init["headers"]. @@ -519,7 +519,7 @@ class Request { // 3, If Content-Type is non-null and this’s headers’s header list does // not contain `Content-Type`, then append `Content-Type`/Content-Type to // this’s headers. - if (contentType && !this[kHeaders][kHeadersList].contains('content-type', true)) { + if (contentType && !getHeadersList(this[kHeaders]).contains('content-type', true)) { this[kHeaders].append('content-type', contentType) } } @@ -785,7 +785,7 @@ class Request { } // 4. Return clonedRequestObject. - return fromInnerRequest(clonedRequest, ac.signal, this[kHeaders][kGuard]) + return fromInnerRequest(clonedRequest, ac.signal, getHeadersGuard(this[kHeaders])) } [nodeUtil.inspect.custom] (depth, options) { @@ -894,8 +894,8 @@ function fromInnerRequest (innerRequest, signal, guard) { request[kState] = innerRequest request[kSignal] = signal request[kHeaders] = new Headers(kConstruct) - request[kHeaders][kHeadersList] = innerRequest.headersList - request[kHeaders][kGuard] = guard + setHeadersList(request[kHeaders], innerRequest.headersList) + setHeadersGuard(request[kHeaders], guard) return request } diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index 81c32fe3e51..8c00835698e 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -1,6 +1,6 @@ 'use strict' -const { Headers, HeadersList, fill } = require('./headers') +const { Headers, HeadersList, fill, getHeadersGuard, setHeadersGuard, setHeadersList } = require('./headers') const { extractBody, cloneBody, mixinBody } = require('./body') const util = require('../../core/util') const nodeUtil = require('node:util') @@ -19,11 +19,11 @@ const { redirectStatusSet, nullBodyStatus } = require('./constants') -const { kState, kHeaders, kGuard } = require('./symbols') +const { kState, kHeaders } = require('./symbols') const { webidl } = require('./webidl') const { FormData } = require('./formdata') const { URLSerializer } = require('./data-url') -const { kHeadersList, kConstruct } = require('../../core/symbols') +const { kConstruct } = require('../../core/symbols') const assert = require('node:assert') const { types } = require('node:util') const { isDisturbed, isErrored } = require('node:stream') @@ -141,8 +141,8 @@ class Response { // Realm, whose header list is this’s response’s header list and guard // is "response". this[kHeaders] = new Headers(kConstruct) - this[kHeaders][kGuard] = 'response' - this[kHeaders][kHeadersList] = this[kState].headersList + setHeadersGuard(this[kHeaders], 'response') + setHeadersList(this[kHeaders], this[kState].headersList) // 3. Let bodyWithType be null. let bodyWithType = null @@ -255,7 +255,7 @@ class Response { // 3. Return the result of creating a Response object, given // clonedResponse, this’s headers’s guard, and this’s relevant Realm. - return fromInnerResponse(clonedResponse, this[kHeaders][kGuard]) + return fromInnerResponse(clonedResponse, getHeadersGuard(this[kHeaders])) } [nodeUtil.inspect.custom] (depth, options) { @@ -522,8 +522,8 @@ function fromInnerResponse (innerResponse, guard) { const response = new Response(kConstruct) response[kState] = innerResponse response[kHeaders] = new Headers(kConstruct) - response[kHeaders][kHeadersList] = innerResponse.headersList - response[kHeaders][kGuard] = guard + setHeadersList(response[kHeaders], innerResponse.headersList) + setHeadersGuard(response[kHeaders], guard) if (hasFinalizationRegistry && innerResponse.body?.stream) { registry.register(response, innerResponse.body.stream) diff --git a/lib/web/fetch/symbols.js b/lib/web/fetch/symbols.js index 7dd5fc8912b..32e360e490f 100644 --- a/lib/web/fetch/symbols.js +++ b/lib/web/fetch/symbols.js @@ -5,6 +5,5 @@ module.exports = { kHeaders: Symbol('headers'), kSignal: Symbol('signal'), kState: Symbol('state'), - kGuard: Symbol('guard'), kDispatcher: Symbol('dispatcher') } diff --git a/lib/web/websocket/connection.js b/lib/web/websocket/connection.js index 85b471e6e0b..664fc3f0780 100644 --- a/lib/web/websocket/connection.js +++ b/lib/web/websocket/connection.js @@ -13,9 +13,8 @@ const { channels } = require('../../core/diagnostics') const { CloseEvent } = require('./events') const { makeRequest } = require('../fetch/request') const { fetching } = require('../fetch/index') -const { Headers } = require('../fetch/headers') +const { Headers, getHeadersList } = require('../fetch/headers') const { getDecodeSplit } = require('../fetch/util') -const { kHeadersList } = require('../../core/symbols') const { WebsocketFrameSend } = require('./frame') /** @type {import('crypto')} */ @@ -59,7 +58,7 @@ function establishWebSocketConnection (url, protocols, client, ws, onEstablish, // Note: undici extension, allow setting custom headers. if (options.headers) { - const headersList = new Headers(options.headers)[kHeadersList] + const headersList = getHeadersList(new Headers(options.headers)) request.headersList = headersList } diff --git a/test/fetch/headers.js b/test/fetch/headers.js index 8536dea7175..38dd6b16886 100644 --- a/test/fetch/headers.js +++ b/test/fetch/headers.js @@ -3,8 +3,7 @@ const { test } = require('node:test') const assert = require('node:assert') const { tspl } = require('@matteo.collina/tspl') -const { Headers, fill } = require('../../lib/web/fetch/headers') -const { kGuard } = require('../../lib/web/fetch/symbols') +const { Headers, fill, setHeadersGuard } = require('../../lib/web/fetch/headers') const { once } = require('node:events') const { fetch } = require('../..') const { createServer } = require('node:http') @@ -610,7 +609,7 @@ test('various init paths of Headers', () => { test('immutable guard', () => { const headers = new Headers() headers.set('key', 'val') - headers[kGuard] = 'immutable' + setHeadersGuard(headers, 'immutable') assert.throws(() => { headers.set('asd', 'asd') @@ -627,7 +626,7 @@ test('immutable guard', () => { test('request-no-cors guard', () => { const headers = new Headers() - headers[kGuard] = 'request-no-cors' + setHeadersGuard(headers, 'request-no-cors') assert.doesNotThrow(() => { headers.set('key', 'val') }) assert.doesNotThrow(() => { headers.append('key', 'val') }) }) diff --git a/test/fetch/issue-3267.js b/test/fetch/issue-3267.js new file mode 100644 index 00000000000..8b7515e62af --- /dev/null +++ b/test/fetch/issue-3267.js @@ -0,0 +1,18 @@ +'use strict' + +const { Headers } = require('../..') +const { test } = require('node:test') +const assert = require('node:assert') + +test('Spreading a Headers object yields 0 symbols', (t) => { + const baseHeaders = { 'x-foo': 'bar' } + + const requestHeaders = new Headers({ 'Content-Type': 'application/json' }) + const headers = { + ...baseHeaders, + ...requestHeaders + } + + assert.deepStrictEqual(headers, { 'x-foo': 'bar' }) + assert.doesNotThrow(() => new Headers(headers)) +}) diff --git a/test/fetch/request.js b/test/fetch/request.js index 4e007da2b19..2cc1f6437de 100644 --- a/test/fetch/request.js +++ b/test/fetch/request.js @@ -15,8 +15,8 @@ const { Blob: ThirdPartyBlob, FormData: ThirdPartyFormData } = require('formdata-node') -const { kState, kGuard, kSignal, kHeaders } = require('../../lib/web/fetch/symbols') -const { kHeadersList } = require('../../lib/core/symbols') +const { kState, kSignal, kHeaders } = require('../../lib/web/fetch/symbols') +const { getHeadersGuard, getHeadersList } = require('../../lib/web/fetch/headers') const hasSignalReason = 'reason' in AbortSignal.prototype @@ -497,6 +497,6 @@ test('fromInnerRequest', () => { // check property assert.strictEqual(request[kState], innerRequest) assert.strictEqual(request[kSignal], signal) - assert.strictEqual(request[kHeaders][kHeadersList], innerRequest.headersList) - assert.strictEqual(request[kHeaders][kGuard], 'immutable') + assert.strictEqual(getHeadersList(request[kHeaders]), innerRequest.headersList) + assert.strictEqual(getHeadersGuard(request[kHeaders]), 'immutable') }) diff --git a/test/fetch/response.js b/test/fetch/response.js index e0c046ab833..912c24a40e3 100644 --- a/test/fetch/response.js +++ b/test/fetch/response.js @@ -12,8 +12,8 @@ const { Blob: ThirdPartyBlob, FormData: ThirdPartyFormData } = require('formdata-node') -const { kState, kGuard, kHeaders } = require('../../lib/web/fetch/symbols') -const { kHeadersList } = require('../../lib/core/symbols') +const { kState, kHeaders } = require('../../lib/web/fetch/symbols') +const { getHeadersGuard, getHeadersList } = require('../../lib/web/fetch/headers') test('arg validation', async () => { // constructor @@ -282,6 +282,6 @@ test('fromInnerResponse', () => { // check property assert.strictEqual(response[kState], innerResponse) - assert.strictEqual(response[kHeaders][kHeadersList], innerResponse.headersList) - assert.strictEqual(response[kHeaders][kGuard], 'immutable') + assert.strictEqual(getHeadersList(response[kHeaders]), innerResponse.headersList) + assert.strictEqual(getHeadersGuard(response[kHeaders]), 'immutable') })