Skip to content

boringssl: restore is_safe_alt_name helper (main build break) - #36540

Merged
Jarred-Sumner merged 1 commit into
mainfrom
farm/4242dd82/boringssl-is-safe-alt-name
Jul 31, 2026
Merged

Jarred-Sumner merged 1 commit into
mainfrom
farm/4242dd82/boringssl-is-safe-alt-name

Conversation

@robobun

@robobun robobun commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

What

Fixes the E0433 compile error on main since ff512ea.

Why

Merge-order race: #36252 removed the x509::is_safe_alt_name helper and its use x509 as X509 alias (the only caller had been rewritten), and #36165 then landed a new call to X509::is_safe_alt_name in the NameBytes Display impl that was written against the pre-#36252 tree. Every lane on build 86151 fails with:

error[E0433]: cannot find type `X509` in this scope
   --> src/boringssl/lib.rs:584:12
    |
584 |         if X509::is_safe_alt_name(self.0, false) {

Fix

Restore the helper as a private fn is_safe_alt_name(name: &[u8]) -> bool next to NameBytes. The sole caller passed utf8 = false, so the UTF-8 branch is dropped; the body is otherwise the same port of Node's IsSafeAltName (src/jsc/bindings/ncrypto.cpp:753) that #36252 removed.

Verification

#36252 removed the x509::is_safe_alt_name helper (its only caller was
rewritten), then #36165 landed a new call to X509::is_safe_alt_name in the
NameBytes Display impl, written against the pre-#36252 tree. main stopped
compiling at ff512ea with E0433.

Restore the helper as a local fn next to NameBytes. The sole caller passed
utf8=false, so the utf8 branch is dropped; the body is otherwise a direct
port of Node's IsSafeAltName (src/jsc/bindings/ncrypto.cpp). Covered by the
existing Bun.connect altname-escaping tests and new unit tests in this crate.
@Jarred-Sumner
Jarred-Sumner merged commit befd269 into main Jul 31, 2026
8 of 11 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/4242dd82/boringssl-is-safe-alt-name branch July 31, 2026 07:42

@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 build-break fix restoring a removed helper.

What was reviewed:

  • Verified is_safe_alt_name matches Node's IsSafeAltName non-UTF8 branch in ncrypto.cpp:753 byte-for-byte (range ' '..='~', four excluded chars).
  • Confirmed no other callers of the removed X509::is_safe_alt_name remain in the tree.
  • Unit tests cover the range boundaries (\x1f, \x7f, \xe4) and all four excluded characters.
Extended reasoning...

Overview

This PR fixes a compile error on main caused by a merge-order race between #36252 (removed x509::is_safe_alt_name) and #36165 (added a new caller in NameBytes::fmt). The fix restores the helper as a private free function next to its sole caller, drops the unused utf8 parameter branch, and adds two unit tests.

Security risks

None. The helper is a pure byte-classification predicate used only to decide whether a certificate SAN entry needs JSON-style quoting in an error message. It does not participate in hostname verification, trust decisions, or any parsing of untrusted input beyond display formatting. The implementation was cross-checked against src/jsc/bindings/ncrypto.cpp:753 and matches the non-UTF8 branch exactly: printable ASCII 0x20..=0x7E minus ", \\, ,, '.

Level of scrutiny

Low. This is a build-break fix with main currently red on every lane. The change is a 4-line function plus tests; the function body is a direct port of code that was already in-tree until #36252 removed it. The only judgment call — dropping the utf8 parameter — is justified: the sole caller passed false, and NameBytes is documented as IA5/Latin-1.

Other factors

  • Grep confirms no other references to X509::is_safe_alt_name or the removed use x509 as X509 alias remain.
  • The new unit tests exercise both range boundaries (\x1f below, \x7f above, non-ASCII \xe4) and every excluded character, plus the end-to-end NameBytes quoting output including the CVE-2021-44532-relevant comma-injection case.
  • PR description cites cargo check/clippy/test and the integration test from #36165 all passing.

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