http2: remove prototype primordials

Co-authored-by: Yagiz Nizipli <[email protected]> PR-URL: #53696 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
nodejs · Jul 5, 2024 · 88beb76 · 88beb76
1 parent 5a775b3
commit 88beb76
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 125 deletions.
diff --git a/doc/contributing/primordials.md b/doc/contributing/primordials.md
@@ -4,8 +4,13 @@ The file `lib/internal/per_context/primordials.js` subclasses and stores the JS
 built-ins that come from the VM so that Node.js built-in modules do not need to
 later look these up from the global proxy, which can be mutated by users.
 
-Usage of primordials should be preferred for any new code, but replacing current
-code with primordials should be
+For some area of the codebase, performance and code readability are deemed more
+important than reliability against prototype pollution:
+
+* `node:http2`
+
+Usage of primordials should be preferred for new code in other areas, but
+replacing current code with primordials should be
 [done with care](#primordials-with-known-performance-issues). It is highly
 recommended to ping the relevant team when reviewing a pull request that touches
 one of the subsystems they "own".

diff --git a/lib/eslint.config_partial.mjs b/lib/eslint.config_partial.mjs
@@ -5,6 +5,32 @@ import {
   noRestrictedSyntaxCommonLib,
 } from '../tools/eslint/eslint.config_utils.mjs';
 
+const noRestrictedSyntax = [
+  'error',
+  ...noRestrictedSyntaxCommonAll,
+  ...noRestrictedSyntaxCommonLib,
+  {
+    selector: "CallExpression[callee.object.name='assert']:not([callee.property.name='ok']):not([callee.property.name='fail']):not([callee.property.name='ifError'])",
+    message: 'Please only use simple assertions in ./lib',
+  },
+  {
+    selector: 'NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError|AbortError|NodeAggregateError)$/])',
+    message: 'Use an error exported by the internal/errors module.',
+  },
+  {
+    selector: "CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']",
+    message: "Please use `require('internal/errors').hideStackFrames()` instead.",
+  },
+  {
+    selector: "AssignmentExpression:matches([left.object.name='Error']):matches([left.name='prepareStackTrace'], [left.property.name='prepareStackTrace'])",
+    message: "Use 'overrideStackTrace' from 'lib/internal/errors.js' instead of 'Error.prepareStackTrace'.",
+  },
+  {
+    selector: "ThrowStatement > NewExpression[callee.name=/^ERR_[A-Z_]+$/] > ObjectExpression:first-child:not(:has([key.name='message']):has([key.name='code']):has([key.name='syscall']))",
+    message: 'The context passed into SystemError constructor must have .code, .syscall and .message.',
+  },
+];
+
 export default [
   {
     files: ['lib/**/*.js'],
@@ -22,31 +48,7 @@ export default [
     rules: {
       'prefer-object-spread': 'error',
       'no-buffer-constructor': 'error',
-      'no-restricted-syntax': [
-        'error',
-        ...noRestrictedSyntaxCommonAll,
-        ...noRestrictedSyntaxCommonLib,
-        {
-          selector: "CallExpression[callee.object.name='assert']:not([callee.property.name='ok']):not([callee.property.name='fail']):not([callee.property.name='ifError'])",
-          message: 'Please only use simple assertions in ./lib',
-        },
-        {
-          selector: 'NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError|AbortError|NodeAggregateError)$/])',
-          message: 'Use an error exported by the internal/errors module.',
-        },
-        {
-          selector: "CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']",
-          message: "Please use `require('internal/errors').hideStackFrames()` instead.",
-        },
-        {
-          selector: "AssignmentExpression:matches([left.object.name='Error']):matches([left.name='prepareStackTrace'], [left.property.name='prepareStackTrace'])",
-          message: "Use 'overrideStackTrace' from 'lib/internal/errors.js' instead of 'Error.prepareStackTrace'.",
-        },
-        {
-          selector: "ThrowStatement > NewExpression[callee.name=/^ERR_[A-Z_]+$/] > ObjectExpression:first-child:not(:has([key.name='message']):has([key.name='code']):has([key.name='syscall']))",
-          message: 'The context passed into SystemError constructor must have .code, .syscall and .message.',
-        },
-      ],
+      'no-restricted-syntax': noRestrictedSyntax,
       'no-restricted-globals': [
         'error',
         {
@@ -487,4 +489,19 @@ export default [
       'node-core/set-proto-to-null-in-object': 'error',
     },
   },
+  {
+    files: [
+      'lib/internal/http2/*.js',
+      'lib/http2.js',
+    ],
+    rules: {
+      'no-restricted-syntax': [
+        ...noRestrictedSyntax,
+        {
+          selector: 'VariableDeclarator:has(.init[name="primordials"]) Identifier[name=/Prototype/]:not([name=/^(Object|Reflect)(Get|Set)PrototypeOf$/])',
+          message: 'We do not use prototype primordials in this file',
+        },
+      ],
+    },
+  },
 ];
diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
@@ -2,19 +2,13 @@
 
 const {
   ArrayIsArray,
-  ArrayPrototypePush,
   Boolean,
-  FunctionPrototypeBind,
   ObjectAssign,
+  ObjectHasOwn,
   ObjectKeys,
-  ObjectPrototypeHasOwnProperty,
   Proxy,
   ReflectApply,
   ReflectGetPrototypeOf,
-  SafeArrayIterator,
-  StringPrototypeIncludes,
-  StringPrototypeToLowerCase,
-  StringPrototypeTrim,
   Symbol,
 } = primordials;
 
@@ -89,7 +83,7 @@ let statusConnectionHeaderWarned = false;
 const assertValidHeader = hideStackFrames((name, value) => {
   if (name === '' ||
       typeof name !== 'string' ||
-      StringPrototypeIncludes(name, ' ')) {
+      name.includes(' ')) {
     throw new ERR_INVALID_HTTP_TOKEN.HideStackFramesError('Header name', name);
   }
   if (isPseudoHeader(name)) {
@@ -153,8 +147,7 @@ function onStreamTrailers(trailers, flags, rawTrailers) {
   const request = this[kRequest];
   if (request !== undefined) {
     ObjectAssign(request[kTrailers], trailers);
-    ArrayPrototypePush(request[kRawTrailers],
-                       ...new SafeArrayIterator(rawTrailers));
+    request[kRawTrailers].push(...rawTrailers);
   }
 }
 
@@ -216,7 +209,7 @@ const proxySocketHandler = {
       case 'end':
       case 'emit':
       case 'destroy':
-        return FunctionPrototypeBind(stream[prop], stream);
+        return stream[prop].bind(stream);
       case 'writable':
       case 'destroyed':
         return stream[prop];
@@ -229,8 +222,8 @@ const proxySocketHandler = {
       case 'setTimeout': {
         const session = stream.session;
         if (session !== undefined)
-          return FunctionPrototypeBind(session.setTimeout, session);
-        return FunctionPrototypeBind(stream.setTimeout, stream);
+          return session.setTimeout.bind(session);
+        return stream.setTimeout.bind(stream);
       }
       case 'write':
       case 'read':
@@ -242,7 +235,7 @@ const proxySocketHandler = {
           stream.session[kSocket] : stream;
         const value = ref[prop];
         return typeof value === 'function' ?
-          FunctionPrototypeBind(value, ref) :
+          value.bind(ref) :
           value;
       }
     }
@@ -417,7 +410,7 @@ class Http2ServerRequest extends Readable {
 
   set method(method) {
     validateString(method, 'method');
-    if (StringPrototypeTrim(method) === '')
+    if (method.trim() === '')
       throw new ERR_INVALID_ARG_VALUE('method', method);
 
     this[kHeaders][HTTP2_HEADER_METHOD] = method;
@@ -578,7 +571,7 @@ class Http2ServerResponse extends Stream {
 
   setTrailer(name, value) {
     validateString(name, 'name');
-    name = StringPrototypeToLowerCase(StringPrototypeTrim(name));
+    name = name.trim().toLowerCase();
     assertValidHeader(name, value);
     this[kTrailers][name] = value;
   }
@@ -594,7 +587,7 @@ class Http2ServerResponse extends Stream {
 
   getHeader(name) {
     validateString(name, 'name');
-    name = StringPrototypeToLowerCase(StringPrototypeTrim(name));
+    name = name.trim().toLowerCase();
     return this[kHeaders][name];
   }
 
@@ -609,16 +602,16 @@ class Http2ServerResponse extends Stream {
 
   hasHeader(name) {
     validateString(name, 'name');
-    name = StringPrototypeToLowerCase(StringPrototypeTrim(name));
-    return ObjectPrototypeHasOwnProperty(this[kHeaders], name);
+    name = name.trim().toLowerCase();
+    return ObjectHasOwn(this[kHeaders], name);
   }
 
   removeHeader(name) {
     validateString(name, 'name');
     if (this[kStream].headersSent)
       throw new ERR_HTTP2_HEADERS_SENT();
 
-    name = StringPrototypeToLowerCase(StringPrototypeTrim(name));
+    name = name.trim().toLowerCase();
 
     if (name === 'date') {
       this[kState].sendDate = false;
@@ -638,7 +631,7 @@ class Http2ServerResponse extends Stream {
   }
 
   [kSetHeader](name, value) {
-    name = StringPrototypeToLowerCase(StringPrototypeTrim(name));
+    name = name.trim().toLowerCase();
     assertValidHeader(name, value);
 
     if (!isConnectionHeaderAllowed(name, value)) {
@@ -662,7 +655,7 @@ class Http2ServerResponse extends Stream {
   }
 
   [kAppendHeader](name, value) {
-    name = StringPrototypeToLowerCase(StringPrototypeTrim(name));
+    name = name.trim().toLowerCase();
     assertValidHeader(name, value);
 
     if (!isConnectionHeaderAllowed(name, value)) {