Skip to content

fix(ext/node): brand-check SecureContext._external accessor#33569

Merged
bartlomieju merged 1 commit intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter43
Apr 28, 2026
Merged

fix(ext/node): brand-check SecureContext._external accessor#33569
bartlomieju merged 1 commit intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter43

Conversation

@nathanwhitbot
Copy link
Copy Markdown
Contributor

Summary

Node's tls.createSecureContext().context._external is a C++ accessor that returns the underlying external pointer; reading it on a non-context receiver hits an internal-slot check and throws TypeError. Deno had no _external accessor at all, so:

const pctx = tls.createSecureContext().context;
const cctx = { __proto__: pctx };
cctx._external;  // expected: TypeError; actual: silently undefined

This patch defines _external as a non-enumerable own getter on the context object, brand-checked against a private WeakSet of contexts the SecureContext constructor created. Reading it on the original context returns the context (we have no real external pointer); reading it via prototype-tampering throws TypeError("Illegal invocation"), matching Node's behavior.

Enables parallel/test-tls-external-accessor.js in the Node compat suite.

Test plan

  • cargo test --test node_compat -- test-tls-external-accessor (was failing on main, passes here)
  • cargo build --bin deno clean
  • tools/format.js and tools/lint.js --js clean
  • Sibling enabled TLS tests still pass: test-tls-basic-validations, test-tls-alert, test-tls-cert-regression, test-tls-honorcipherorder, test-tls-startcom-wosign-whitelist, test-tls-check-server-identity, test-tls-connect-simple.

Node's `_external` accessor on a TLS SecureContext returns the C++
external pointer; reading it on a non-context receiver hits a slot
check and throws. Deno had no such accessor at all, so accessing
`secureContext.context._external` returned `undefined` regardless of
the receiver — silently passing prototype-tampering tests instead of
matching Node's behaviour.

Define `_external` as an own getter on the context that brand-checks
the receiver via a `WeakSet` and throws `TypeError` when it's not the
same context object the SecureContext constructed.

Enables `parallel/test-tls-external-accessor.js`.
Copy link
Copy Markdown
Contributor Author

@nathanwhitbot nathanwhitbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Targeted fix for the exact symptom — prototype-tampering reads of
_external on a non-pctx receiver now throw TypeError instead of
silently returning undefined.

The brand check via a module-scope WeakSet is the right shape:

  • Constructor adds this.context to the set and installs the getter.
  • Getter compares this (the property-access receiver) against the set —
    inherited objects, Reflect.get with a different receiver, etc. all fail
    the check.
  • Returning this from the live path is fine because deno has no real
    external pointer and the upstream test only asserts "no throw" for that
    branch.

Worth verifying once at the implementation level:

  • The descriptor uses configurable: true, which is more permissive than
    Node's C++ accessor — it could be deleted or redefined by user code.
    Probably fine since the test doesn't exercise it, but the safer match is
    configurable: false.
  • __proto__: null on the descriptor is a nice touch — it dodges the
    Object.prototype pollution path that the recent child_process PR was
    fixing in the same neighborhood.
  • No set is defined, so writes will throw in strict mode and silently
    no-op otherwise. Matches Node's accessor (no setter).

The WeakSet is module-scope and weakly held, so contexts won't leak after
their owning SecureContext is GC'd.

LGTM.

@bartlomieju bartlomieju merged commit 0c476ad into denoland:main Apr 28, 2026
112 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants