Skip to content

fix(ext/node): improve node:tls test compatibility#34067

Merged
bartlomieju merged 7 commits into
denoland:mainfrom
nathanwhitbot:codex/aged-b0274a27-177
May 15, 2026
Merged

fix(ext/node): improve node:tls test compatibility#34067
bartlomieju merged 7 commits into
denoland:mainfrom
nathanwhitbot:codex/aged-b0274a27-177

Conversation

@nathanwhitbot
Copy link
Copy Markdown
Contributor

Summary

  • Improves node:tls CA certificate handling, including default CA overrides, system CA flags, CA list validation/deduping, and getCACertificates() default/system/bundled/extra behavior.
  • Adds SecureContext CA mutation support and verifier error tracking so explicit CA options and rejectUnauthorized: false align more closely with Node.
  • Fills selected TLS compatibility gaps for protocol-method errors, ALPN setKeyCert(), ticket key APIs, and related TLS option plumbing.
  • Enables newly passing TLS compatibility tests in config.jsonc.

Validation

  • cargo build --bin deno
  • cargo fmt --check
  • deno fmt --check tests/node_compat/config.jsonc
  • cargo test --test node_compat -p node_compat_tests -- 'test-tls-client-verify'
  • cargo test --test node_compat -p node_compat_tests -- 'test-tls-get-ca-certificates'
  • cargo test --test node_compat -p node_compat_tests -- 'test-tls' reports 151 passing and 74 remaining failures across the TLS filter; the remaining failures are existing unsupported TLS clusters.

@nathanwhit nathanwhit changed the title Improve node:tls Test Compatibility fix(ext/node): improve node:tls test compatibility May 14, 2026
@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

CI note: the latest lint title check is passing for fix(ext/node): improve node:tls test compatibility. The current completed failure, deno_core test linux-x86_64, appears to be runner infrastructure only: its check annotation reports No space left on device while writing the GitHub Actions runner diagnostic log. I did not make code changes.

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Follow-up CI note: after local validation I made one small test stability fix for tests/unit_node/tls_test.ts: bind the TLSSocket.alpnProtocol listener to ::1 because the test connects to ::1, avoiding localhost address-family dependence.

Focused validation now passes: ./x test-node tls, ./x test-compat test-tls-get-ca-certificates-system, and ./x test-compat test-tls-server-setkeycert.

The remaining completed CI failure is still deno_core test linux-x86_64, whose annotation is runner disk exhaustion (No space left on device) before the test step ran.

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

CI follow-up: on latest head c3a4ea55, deno_core test linux-x86_64 is now passing in run https://github.com/denoland/deno/actions/runs/25876485382/job/76045159501.

The earlier failed job https://github.com/denoland/deno/actions/runs/25875470576/job/76041640412 was on prior SHA eb1a412c and failed during cache restore due to GitHub runner disk exhaustion (No space left on device) before the deno_core test step ran. No additional code change from me in this pass; this is ready for CI monitoring while the remaining queued/in-progress jobs finish.

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

CI diagnosis for the current test node_compat (3/3) debug macos-x86_64 failure: it is isolated to parallel/test-tls-get-ca-certificates-system-without-flag.js. The failing assertion compares tls.getCACertificates('system') in the parent process with the same call in a spawned child; the log shows a certificate-array ordering mismatch, not a TLS verification/runtime failure.

I prepared a bounded fix in ext/node/ops/tls.rs to sort PEMs returned from load_native_certs() before exposing system CAs, and to reuse that path when system CAs are folded into the default CA list. This keeps Deno's process-to-process output deterministic for the Node test while preserving the same certificate set.

Local validation: cargo fmt --check passed; cargo check -p deno_node passed. The exact ./x test-compat test-tls-get-ca-certificates-system-without-flag rerun was blocked locally first by an uninitialized Node test submodule, then after initializing it by a missing debug deno binary; cargo build --bin deno could not complete because this VM hit No space left on device.

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

CI follow-up for test node_compat (3/3) debug macos-x86_64: the failure is isolated to parallel/test-tls-get-ca-certificates-system-without-flag.js and is caused by process-to-process ordering instability in the system CA PEM list, not by TLS verification behavior.

I applied a bounded fix in ext/node/ops/tls.rs: route system CA loading through one helper that converts native roots to PEM and sorts the PEM strings before returning them. This preserves the certificate set while making tls.getCACertificates('system') deterministic for the Node test and for default CA lists that include the system store.

Validation passed locally:

  • cargo fmt --check
  • cargo check -p deno_node
  • cargo build --bin deno
  • ./x test-compat test-tls-get-ca-certificates-system-without-flag
  • ./x test-compat test-tls-get-ca-certificates

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Current CI follow-up on head 158507f7: no code change from this pass.

Current status:

  • lint title is passing.
  • deno_core test linux-x86_64 is passing in the latest CI run.
  • The prior test node_compat (3/3) debug macos-x86_64 CA-ordering failure is not present in the latest completed release node_compat shards; the debug shard is still pending.
  • The current completed blocker is lint debug windows-x86_64, but it failed during the denoland/setup-deno “Install Deno” step before test_format.js, jsdoc_checker.js, or lint.js ran. The Linux/macOS lint jobs were cancelled after that failure, not failed by lint output.

Local validation in this pass: cargo fmt --check passed. ./x test-compat test-tls-get-ca-certificates-system-without-flag could not execute because this workspace does not have the Node test suite submodule initialized (tests/node_compat/runner/suite/test is missing).

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Current CI/local follow-up on head 158507f7: no code change from this pass.

All latest-head node_compat checks are now passing, including the prior test node_compat (3/3) debug macos-x86_64 TLS CA-ordering shard. Local validation also passes after initializing the declared test submodules and building the required local helpers:

  • cargo fmt --check
  • cargo build --bin deno
  • cargo build --bin test_server
  • ./x test-compat test-tls-get-ca-certificates-system-without-flag
  • ./x test-compat test-tls-get-ca-certificates
  • ./x test-node tls

The remaining PR blocker is CI infrastructure/setup: lint debug windows-x86_64 failed in the denoland/setup-deno “Install Deno” step before test_format.js, jsdoc_checker.js, or lint.js ran. The aggregate ci status check is failing because of that job; the Linux/macOS lint jobs were cancelled after it.

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Latest CI follow-up on head 158507f7: no code change from this pass.

The previously reported blockers have been superseded on the latest checks:

  • lint title is passing.
  • deno_core test linux-x86_64 is passing.
  • All node_compat shards are passing, including the prior macOS TLS CA-ordering shard.
  • The earlier failed/cancelled lint jobs have been rerun; the current lint debug jobs for linux, macOS, and Windows are pending/in progress rather than failing with actionable lint output.

Review state: no review submissions or review threads are currently present. The PR is mergeable at the git level, but GitHub still reports it blocked while the pending CI jobs finish.

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Latest CI follow-up on head 158507f7: no code change from this pass.

All latest checks are now green, including ci status, lint title, deno_core test linux-x86_64, every test node_compat shard, and all three lint debug jobs. Review state is also clean: no review submissions or review threads are currently present. The PR is mergeable at the git level.

Comment thread ext/node/ops/node_cli_parser.rs
@nathanwhit
Copy link
Copy Markdown
Member

@fibibot review this pr

@bartlomieju
Copy link
Copy Markdown
Member

@fibibot are you reviewing this one?


const nodeOptions = Deno.env.get("NODE_OPTIONS");
const args = nodeOptions ? splitNodeOptions(nodeOptions) : [];
const nodeOptions = globalThis.process?.env?.NODE_OPTIONS ??
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should not re-read mutable runtime state after startup. With this change, user code can mutate process.env.NODE_OPTIONS or process.execArgv after node:tls has loaded and then change TLS defaults such as tls.DEFAULT_MIN_VERSION or default CA selection. Node snapshots these option effects from startup state; for example, setting process.env.NODE_OPTIONS = "--tls-min-v1.0 --use-system-ca" at runtime does not change tls.DEFAULT_MIN_VERSION or tls.getCACertificates("default") in Node. Since this affects protocol and trust-store behavior, we should keep these options sourced from startup/translated options, not live mutable JS state.

@bartlomieju
Copy link
Copy Markdown
Member

Pushed a follow-up fix in 50e19c24d3f458559decf139fc2ae7a647e390b3.

This centralizes the TLS-related option reads on the existing internal_binding/node_options.ts path and snapshots startup options, so later mutations of process.env.NODE_OPTIONS or process.execArgv no longer affect node:tls defaults. Workers explicitly refresh the snapshot from their startup execArgv, which preserves the worker --use-system-ca / --no-use-system-ca behavior.

Validation run locally:

  • cargo build --bin deno
  • ./target/debug/deno test -A --no-check --config tests/config/deno.json tests/unit_node/tls_test.ts --filter "tls default options ignore runtime NODE_OPTIONS"
  • cargo test --test node_compat -p node_compat_tests -- 'test-tls-get-ca-certificates'
  • ./tools/format.js --check ext/node/polyfills/internal/options.ts ext/node/polyfills/internal_binding/node_options.ts ext/node/polyfills/tls.ts ext/node/polyfills/worker_threads.ts tests/unit_node/tls_test.ts && git diff --check

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Pushed 6120b80aa to fix the current lint failure. The CI lint job failed because ext/node/polyfills/internal/options.ts had one extra direct Deno.build access, tripping the node-polyfill Deno API usage count. I centralized the warmup check so the file stays at the existing allowed count.

Validation:

  • ./tools/format.js --check ext/node/polyfills/internal/options.ts && git diff --check
  • deno run --allow-write --allow-read --allow-run --allow-net --allow-env ./tools/lint.js
  • cargo test -p deno_node node_cli_parser --lib

@bartlomieju bartlomieju merged commit 1d8465f into denoland:main May 15, 2026
136 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