csrf: parse the encoding option in one place - #39140
Conversation
Bun.CSRF.generate and Bun.CSRF.verify each read the `encoding` option with the same copy of the Encoding to TokenFormat mapping and its error. Move it into get_optional_token_format, next to get_optional_int_u64, and call it from both. No behavior change. Drop the now clean same_match_twice entry for this file from the mordant baseline, and fix the comment on the reverse mapping, which still named TokenFormat::to_node_encoding (removed in #35002).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: ready for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical dedup of two identical encoding option-parsing blocks into one helper; no behavior change.
What was reviewed:
- Traced old vs new control flow: the
None(unknown name) andSome(non-token encoding)cases both still hit the same error, and property-read order at both call sites is unchanged. - Checked that the helper mirrors the existing
get_optional_int_u64pattern and that the reverse-mapping comment update is accurate. - New test asserts the exact error message for both
generateandverifyand the""→ base64url default; loop is non-empty so assertions can fail. mordant-baseline.tomlentry removal matches the deduplicated site.
Extended reasoning...
Overview
This PR extracts a ~20-line block that appeared twice — once in csrf__generate and once in csrf__verify — into a single get_optional_token_format helper in src/runtime/api/csrf_jsc.rs. The block reads the encoding option, parses it via NodeEncoding::from_js_with_default_on_empty, maps it to csrf::TokenFormat, and throws on unsupported values. It also removes the corresponding same_match_twice entry from mordant-baseline.toml, updates a stale comment referring to a removed TokenFormat::to_node_encoding helper, and adds one test in test/js/bun/util/csrf.test.ts pinning the error message and the empty-string default.
Security risks
None. The CSRF token generation and verification logic (csrf::generate / csrf::verify) is untouched. Only the option-parsing plumbing that maps a user-supplied encoding name to a TokenFormat enum is deduplicated, and its input/output behavior is byte-for-byte identical: unknown names and non-token Buffer encodings still throw the same error, "" still defaults to base64url, and the three accepted formats still map to the same variants.
Level of scrutiny
Low. This is a textbook "second time it appears in your diff, extract a named helper" refactor from the review guide. The helper is placed next to the existing get_optional_int_u64 helper it mirrors, is called at the same point in the option-read sequence in both functions (so observable getter-invocation order is preserved), and the old two-error-site structure (let Some(...) else { err } + match { _ => err }) is collapsed into one _ arm on a match over Option<NodeEncoding> — semantically equivalent.
Other factors
The added test is well-formed: it iterates a fixed non-empty list (so the toThrow assertions inside the loop always run), asserts the exact error string rather than a bare toThrow(), exercises both generate and verify, and covers both the "known encoding but not a token format" (utf8, latin1, buffer) and "not an encoding at all" (bogus) branches that now share one error arm. The PR description documents that the test passes both before and after (correct for a refactor) and that behavior parity was verified against a release build. No CODEOWNERS apply to the touched files.
2 participants
Problem
Bun.CSRF.generateandBun.CSRF.verifyeach read theencodingoption with their own copy of the same code:get("encoding"),Encoding::from_js_with_default_on_empty, theEncodingtocsrf::TokenFormatmatch, and theInvalid formaterror (which appeared four times).src/runtime/api/csrf_jsc.rs:99-120and:254-275before this change.same_match_twicereports the second copy; it is thesame_match_twice:src/runtime/api/csrf_jsc.rsentry inmordant-baseline.toml.Fix
get_optional_token_format(options, global)helper incsrf_jsc.rs, next to the existingget_optional_int_u64, called from both functions at the same point in the option-reading order. TheNonefrom an unknown encoding name and the non-token encodings share the one error arm, so the message exists once.csrf_jsc.rsrather than on an enum:TokenFormatlives inbun_csrf, which cannot seecrate::node::Encoding, and this file isTokenFormat's only user. The comment on the reverse mapping (used to encode the token) still namedTokenFormat::to_node_encoding, removed in Remove ~39k lines of dead Rust across the workspace #35002; reworded.bun run rust:mordantover the workspace is clean with it removed; with the entry removed but the old source restored it reports the one finding atcsrf_jsc.rs:265, so the entry was this site.utf8,latin1,buffer,bogus,null, a number, an object), case-insensitive names,"",undefined, and the order in which the option properties are read by both functions: identical output.bun bd test test/js/bun/util/csrf.test.ts(25 pass). Added one test pinning the error message for both functions and the""default, since that path was not covered; it passes before and after, as expected for a refactor.Background
crate::node::Encodingis the Buffer encoding enum (utf8,hex,base64, ...).from_js_with_default_on_emptyparses a JS value as an encoding name, returning the given default for""andNonefor names it does not know.csrf::TokenFormatis the three-variant enum (Base64,Base64Url,Hex) thebun_csrfcrate encodes and decodes tokens with. The CSRF API accepts the encoding option as a Buffer encoding name and narrows it to aTokenFormat, which is the mapping this PR deduplicates.bun run rust:mordant;mordant-baseline.tomlrecords the accepted count of findings per (lint, file), and CI fails only on findings above the recorded count, so fixing a finding means deleting its entry.csrf_jsc.rshunk becomes redundant with this one.