Skip to content

test: stop asserting an OCSP URI on the live bun.sh certificate - #32789

Merged
Jarred-Sumner merged 1 commit into
mainfrom
farm/536da82d/tls-connect-live-cert-ocsp
Jun 26, 2026
Merged

Jarred-Sumner merged 1 commit into
mainfrom
farm/536da82d/tls-connect-live-cert-ocsp

Conversation

@robobun

@robobun robobun commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

node-tls-connect.test.ts > should have peer certificate is currently failing on every CI lane, on main and on every open PR (for example https://buildkite.com/bun/bun/builds/65076):

313 |         expect(infoAccess["OCSP - URI"]).toBeDefined();
                                               ^
error: expect(received).toBeDefined()

Received: undefined

Cause

The test connects to the live bun.sh:443 and asserts fields of whatever certificate the host presents. bun.sh's certificate was reissued (Google Trust Services WE1, valid to Sep 2026) and its Authority Information Access extension now carries only a CA Issuers entry, with no OCSP responder URI:

Authority Information Access:
    CA Issuers - URI:http://i.pki.goog/we1.crt

Public CAs (Let's Encrypt, Google Trust Services) stopped including OCSP URIs in newly issued certificates in 2025 in favor of CRLs, so the field is not coming back. The comment above the assertion already says the contents "can change over time"; this is that.

Fix

Drop the OCSP - URI assertion. Keep the CA Issuers - URI one, strengthened from toBeDefined() to "a non-empty array of http(s) URIs".

This loses no meaningful coverage: the exact infoAccess key/value parsing (including the OCSP key) is already tested hermetically against fixed fixtures in test/js/node/test/parallel/test-tls-translate-peer-certificate.js, test/js/node/test/parallel/test-crypto-x509.js, and test/regression/issue/21274.test.ts. The live test only needs to prove the end-to-end shape.

Verification

To reproduce without depending on the live host, I minted a leaf certificate matching the live one's identity and AIA (CN=bun.sh, SAN bun.sh, *.bun.sh, AIA with a CA Issuers entry and no OCSP entry) and served it on bun.sh:443 through an /etc/hosts override.

  • Unmodified test: both variants fail at line 313 with the exact output above.
  • With this change: bun bd test test/js/node/tls/node-tls-connect.test.ts passes the whole file, 30 pass / 1 skip (intentional) / 0 fail.

No other test asserts on the live bun.sh certificate's contents; every other getPeerCertificate() test in the suite uses a local fixture cert.

bun.sh's reissued certificate (Google Trust Services WE1) has no OCSP
responder URI in its Authority Information Access extension, so the
live-host assertion in node-tls-connect.test.ts fails on every lane.
Assert the CA Issuers entry instead, which is still present, and check
its shape rather than just that it is defined. Exact infoAccess parsing
stays covered by the fixed-fixture x509 tests.
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 2 minutes and 38 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1a09fd50-6502-4998-a690-5e27a4682770

📥 Commits

Reviewing files that changed from the base of the PR and between ba72810 and 33ffcf0.

📒 Files selected for processing (1)
  • test/js/node/tls/node-tls-connect.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:31 PM PT - Jun 26th, 2026

@robobun, your commit 33ffcf0 is building: #65124

@Jarred-Sumner
Jarred-Sumner merged commit 034f693 into main Jun 26, 2026
6 of 8 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/536da82d/tls-connect-live-cert-ocsp branch June 26, 2026 20:32

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — straightforward test-only fix dropping a live-cert assertion that became stale when public CAs stopped emitting OCSP URIs.

Extended reasoning...

Overview

This PR touches a single test file, test/js/node/tls/node-tls-connect.test.ts, and modifies one assertion block inside the "should have peer certificate" test that connects to the live bun.sh:443 host. It drops the infoAccess["OCSP - URI"] assertion (which now fails on every CI lane because the reissued bun.sh certificate from Google Trust Services no longer carries an OCSP responder URI) and slightly strengthens the surviving CA Issuers - URI assertion from toBeDefined() to expect.arrayContaining([expect.stringMatching(/^https?:\/\//)]). The accompanying comment is updated to explain why.

Security risks

None. This is a test-only change with no production/runtime code touched. It neither weakens TLS verification nor alters any crypto logic — it simply stops asserting on a field of an externally-controlled live certificate whose contents legitimately changed.

Level of scrutiny

Low. The diff is 5 lines in a test, the rationale (CAs dropping OCSP in 2025) is factually correct and well-documented in the PR description, and the exact infoAccess parsing behavior remains covered by fixed-fixture tests elsewhere (test-tls-translate-peer-certificate.js, test-crypto-x509.js, 21274.test.ts). The replacement assertion is strictly stronger than the original toBeDefined() for the field that does still exist.

Other factors

The file is not covered by CODEOWNERS. The bug-hunting system found no issues. There are no outstanding human review comments — only bot build/rate-limit notices in the timeline. Given this is unblocking CI across all lanes with a minimal, obviously-correct adjustment to an environment-dependent assertion, it's safe to approve without further human review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants