stricter Headers brand checks in cookies (#3500)

nodejs · Aug 24, 2024 · ee45443 · ee45443
1 parent 91230e8
commit ee45443
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 17 deletions.
diff --git a/lib/web/cookies/index.js b/lib/web/cookies/index.js
@@ -26,7 +26,7 @@ const { Headers } = require('../fetch/headers')
 function getCookies (headers) {
   webidl.argumentLengthCheck(arguments, 1, 'getCookies')
 
-  webidl.brandCheck(headers, Headers, { strict: false })
+  webidl.brandCheckMultiple(headers, [Headers, globalThis.Headers])
 
   const cookie = headers.get('cookie')
 
@@ -53,7 +53,7 @@ function getCookies (headers) {
  * @returns {void}
  */
 function deleteCookie (headers, name, attributes) {
-  webidl.brandCheck(headers, Headers, { strict: false })
+  webidl.brandCheckMultiple(headers, [Headers, globalThis.Headers])
 
   const prefix = 'deleteCookie'
   webidl.argumentLengthCheck(arguments, 2, prefix)
@@ -78,7 +78,7 @@ function deleteCookie (headers, name, attributes) {
 function getSetCookies (headers) {
   webidl.argumentLengthCheck(arguments, 1, 'getSetCookies')
 
-  webidl.brandCheck(headers, Headers, { strict: false })
+  webidl.brandCheckMultiple(headers, [Headers, globalThis.Headers])
 
   const cookies = headers.getSetCookie()
 
@@ -97,7 +97,7 @@ function getSetCookies (headers) {
 function setCookie (headers, cookie) {
   webidl.argumentLengthCheck(arguments, 2, 'setCookie')
 
-  webidl.brandCheck(headers, Headers, { strict: false })
+  webidl.brandCheckMultiple(headers, [Headers, globalThis.Headers])
 
   cookie = webidl.converters.Cookie(cookie)
 

diff --git a/lib/web/fetch/webidl.js b/lib/web/fetch/webidl.js
@@ -34,18 +34,24 @@ webidl.errors.invalidArgument = function (context) {
 
 // https://webidl.spec.whatwg.org/#implements
 webidl.brandCheck = function (V, I, opts) {
-  if (opts?.strict !== false) {
-    if (!(V instanceof I)) {
-      const err = new TypeError('Illegal invocation')
-      err.code = 'ERR_INVALID_THIS' // node compat.
-      throw err
-    }
-  } else {
-    if (V?.[Symbol.toStringTag] !== I.prototype[Symbol.toStringTag]) {
-      const err = new TypeError('Illegal invocation')
-      err.code = 'ERR_INVALID_THIS' // node compat.
-      throw err
-    }
+  if (opts?.strict !== false && !(V instanceof I)) {
+    const err = new TypeError('Illegal invocation')
+    err.code = 'ERR_INVALID_THIS' // node compat.
+    throw err
+  } else if (V?.[Symbol.toStringTag] !== I.prototype[Symbol.toStringTag]) {
+    // TODO(@KhafraDev): this check does not work properly on extended classes
+
+    const err = new TypeError('Illegal invocation')
+    err.code = 'ERR_INVALID_THIS' // node compat.
+    throw err
+  }
+}
+
+webidl.brandCheckMultiple = function (V, List) {
+  if (List.every((clazz) => !(V instanceof clazz))) {
+    const err = new TypeError('Illegal invocation')
+    err.code = 'ERR_INVALID_THIS' // node compat.
+    throw err
   }
 }
 

diff --git a/test/cookie/global-headers.js b/test/cookie/global-headers.js
@@ -53,3 +53,16 @@ describe('Using global Headers', async () => {
     assert.equal(headers.get('Set-Cookie'), 'undici=setCookie')
   })
 })
+
+describe('Headers check is not too lax', () => {
+  class Headers {}
+  Object.defineProperty(Headers.prototype, Symbol.toStringTag, {
+    value: 'Headers',
+    configurable: true
+  })
+
+  assert.throws(() => getCookies(new Headers()), { code: 'ERR_INVALID_THIS' })
+  assert.throws(() => getSetCookies(new Headers()), { code: 'ERR_INVALID_THIS' })
+  assert.throws(() => setCookie(new Headers(), { name: 'a', value: 'b' }), { code: 'ERR_INVALID_THIS' })
+  assert.throws(() => deleteCookie(new Headers(), 'name'), { code: 'ERR_INVALID_THIS' })
+})
diff --git a/types/webidl.d.ts b/types/webidl.d.ts
@@ -167,7 +167,9 @@ export interface Webidl {
    * @description Performs a brand-check on {@param V} to ensure it is a
    * {@param cls} object.
    */
-  brandCheck <Interface>(V: unknown, cls: Interface, opts?: { strict?: boolean }): asserts V is Interface
+  brandCheck <Interface extends new () => unknown>(V: unknown, cls: Interface, opts?: { strict?: boolean }): asserts V is Interface
+
+  brandCheckMultiple <Interfaces extends (new () => unknown)[]> (V: unknown, list: Interfaces): asserts V is Interfaces[number]
 
   /**
    * @see https://webidl.spec.whatwg.org/#es-sequence