Walkthrough

Consolidates CSP nonce sanitization into a new OSS-exported sanitizeNonce function; Pro packages import and use this shared implementation, removing the duplicate Pro helper. Adds tests and a package subpath export; minor CLI typing change and changelog entry included.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I found two nonces in the grass,

I hopped them into one clean pass.
A tidy function, tests that sing,
One little hop — secure everything! 🥕

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

size-limit report 📦

Greptile Summary

This PR eliminates a duplicated CSP nonce sanitization implementation by extracting it into a single shared sanitizeNonce module (react-on-rails/sanitizeNonce) and updating both RenderUtils.ts (OSS) and two Pro-package consumers to import from it. The refactor is a clean extraction with no behavior change — the regex logic is byte-for-byte identical to both prior copies, and the new module is properly registered in package.json's exports map so the Pro package can resolve it.

• New packages/react-on-rails/src/sanitizeNonce.ts module with clear JSDoc describing the sanitize-then-validate policy
• 11-case test suite added covering valid nonces, padding variants, edge cases, and injection attempts
• RenderUtils.ts inline sanitization replaced with a call to the shared module
• Pro injectRSCPayload.ts and transformRSCStreamAndReplayConsoleLogs.ts updated to import from OSS instead of local utils.ts
• ./sanitizeNonce export entry added to package.json
• CHANGELOG.md entry uses a [PR PENDING] placeholder that should be updated to [PR 2828]

Confidence Score: 5/5

• Safe to merge — pure extraction refactor with no behavior change and comprehensive test coverage.
• The logic is identical to both prior implementations, the new export is correctly wired up in package.json, and 11 tests verify all edge cases. The only open item is a trivial CHANGELOG placeholder ([PR PENDING]) that can be fixed in this PR or immediately after merge.
• No files require special attention.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["RenderUtils.ts (OSS)"]
    B["injectRSCPayload.ts (Pro)"]
    C["transformRSCStreamAndReplayConsoleLogs.ts (Pro)"]
    D["sanitizeNonce.ts\n(react-on-rails/sanitizeNonce)"]

    subgraph Before
        A1["RenderUtils.ts\n(inline regex logic)"]
        B1["injectRSCPayload.ts\nimports from utils.ts"]
        C1["transformRSCStream…\nimports from utils.ts"]
        U["utils.ts (Pro)\nexports sanitizeNonce"]
        B1 --> U
        C1 --> U
    end

    subgraph After
        A --> D
        B --> D
        C --> D
    end

_{Reviews (1): Last reviewed commit: "Update CHANGELOG with actual PR number (..." | Re-trigger Greptile}

Review: Consolidate CSP nonce sanitization into shared module

Overall this is a clean, well-structured refactor. The extraction eliminates real duplication, the test coverage is solid (11 cases covering the key paths), and the Pro package correctly imports from the shared OSS module. A few points worth addressing:

Docstring accuracy: The JSDoc says 'If the sanitized value does not match, undefined is returned and no nonce attribute will be emitted'. This overstates the drop guarantee. The function can return a sanitized-but-garbage value when stripped remnants of malformed input happen to match the validation regex. Input ><script>alert(1)</script> after stripping becomes scriptalert1/script, which passes validation and is returned as the nonce value. The behavior is injection-safe (all dangerous chars are stripped), but the docstring implies a stricter drop policy than the code enforces.

Misleading test case: The test 'strips dangerous characters from injection attempts' and its comment 'After stripping non-base64 chars, the remaining alphanumeric string is valid' reads as if returning a garbage nonce is expected/desirable. In practice scriptalert1/script will be emitted as a nonce attribute value — it won't match any real browser nonce so CSP blocks the script. That is fine security-wise, but a clearer comment noting the output is not meaningful as a real nonce would help future readers.

Public export surface: sanitizeNonce is now a named public export under ./sanitizeNonce. If end-users are expected to import it directly, document it in the public API. If it is only meant for Pro package internal use, consider using the @internal export prefix (similar to ./@internal/base/client) to avoid accidentally committing to this as a stable API.

Minor: The = character is included in the strip allowlist ([^a-zA-Z0-9+/=_-]) but is restricted to trailing position in the validate regex (^[a-zA-Z0-9+/_-]+={0,2}$). This is correct, but a short comment on the two-phase treatment of = would help future readers.

The core change is correct and the existing tests confirm security-critical paths work as intended. The above are documentation and API surface concerns rather than functional bugs.

Overall: Clean, well-motivated refactor that eliminates the keep-in-sync comment by actually sharing code. Tests cover the core scenarios well.

One security observation worth calling out: the sanitize-then-validate approach means certain injection-looking strings can pass through rather than being rejected. The test at line 38 of sanitizeNonce.test.ts documents this edge case. After stripping non-base64 characters from a script-tag payload, the remaining string scriptalert1/script clears the validation regex and becomes the emitted nonce value. This is not a practical XSS risk since CSP nonces come from the server-generated Content-Security-Policy header (an attacker controlling the nonce value implies a much larger compromise), but it is surprising behavior that a security reviewer could flag.

Two small improvements worth considering: (1) The test name "strips dangerous characters from injection attempts" implies the function rejects the input, but it actually accepts the stripped result. A clearer name like "produces stripped nonce from base64-passable injection string" matches the assertion. (2) The module JSDoc says malformed nonces are dropped, which is only partially true. Nonces that produce a valid base64-like string after stripping are NOT dropped. One sentence acknowledging this edge case would prevent future confusion.

Minor nit: the test "returns undefined for a nonce that is only padding characters" passes "===". Since = is preserved by the strip regex but only allowed at the end of the validate regex, this test relies on the interaction of both steps. A brief inline comment explaining why === returns undefined (validate requires at least one non-padding char before any =) would help readers understand the two-step logic.

Everything else looks correct and consistent with the pre-existing regex. Changelog entry is accurate and the package.json exports entry follows the existing pattern.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Overall this is a clean, well-scoped refactor. The regex is preserved exactly from both call sites so behaviour is genuinely unchanged, coverage is thorough, and the TypeScript narrowing in create-app.ts is a nice improvement. Three things worth addressing:

API surface (medium): sanitizeNonce is exported at the public subpath react-on-rails/sanitizeNonce, but Pro already uses react-on-rails/@internal/base/client and react-on-rails/@internal/base/full for cross-package utilities not part of the user-facing API. sanitizeNonce is the same kind of internal utility — using @internal/sanitizeNonce would be consistent with that convention and avoids locking in a public path for an implementation detail. See inline comment on package.json.

Missing edge-case test (minor): The strip regex allows = anywhere, but the validate regex only permits trailing = padding. A nonce like 'abc=def' will silently return undefined. A test for this would make the contract explicit and guard against future regex edits. See inline comment on sanitizeNonce.ts.

Partially-stripped injection result (documentation): The test confirming '><script>alert(1)</script>' returns 'scriptalert1/script' is correct and safe, but the comment should explain why it is safe: the returned value can never match a server-issued nonce, so CSP still blocks the script regardless. Without that context a future reader might flag it as a security hole. See inline comment on the test file.

Code Review — PR #2828: Consolidate CSP nonce sanitization

Overall this is a clean, well-structured refactor. The new sanitizeNonce module is correct, the tests are comprehensive, and the Pro package imports are updated properly. Three issues need attention before merge.

🔴 CHANGELOG structural bug — duplicate #### Changed section + duplicated entry

The PR adds a second #### Changed block inside the existing [16.5.0] release, and it also duplicates the Async::Variable → Async::Promise entry that already exists earlier in that same release section (under the first #### Changed). This creates two #### Changed subsections in one release and repeats an entry from a different PR (2832).

The fix is to:

1. Remove the new #### Changed header that this PR adds (there is already one in [16.5.0]).
2. Remove the duplicate Async::Variable bullet from this PR's additions.
3. Append only the nonce sanitization bullet to the existing #### Changed block.

🟡 CHANGELOG policy — refactor-only entry

Per the project's CHANGELOG guidelines: "Update CHANGELOG.md for user-visible changes only. Do NOT add entries for: linting, formatting, refactoring, tests, or documentation fixes."

The nonce sanitization entry explicitly states "Behavior is unchanged", which flags it as a refactor note. However, the PR also adds ./sanitizeNonce as a named public export in package.json — that is a user-visible API addition. Consider either:

• Removing the entry entirely (if the intent is purely internal), and removing the public ./sanitizeNonce export (keeping the module internal), or
• Rewriting the changelog entry to highlight the new public react-on-rails/sanitizeNonce export as an Added item, without the "behavior is unchanged" framing.

🟢 sanitizeNonce.ts — regex character class clarity

In the validation regex /^[a-zA-Z0-9+/_-]+={0,2}$/, the - at the end of the character class is parsed as a literal (correct), but the sequence _- reads like a range from _ (95) to - (45), which is an invalid range and could cause confusion during code review. Moving - to the very start of the class ([-a-zA-Z0-9+/_]) or escaping it ([a-zA-Z0-9+/\_\-]) makes the intent unambiguous. (This is a clarity nit; the current code is functionally correct in JavaScript because a trailing - is always literal.)

Review: Consolidate CSP nonce sanitization into shared module

Overall: Clean refactor, good test coverage. One CHANGELOG issue and one behavioural question worth addressing before merge.

Summary of changes

The extraction of duplicate nonce sanitization logic into react-on-rails/sanitizeNonce is well-motivated and the implementation is correct. The Pro package correctly imports from the OSS module, and the new export is wired up in package.json. TypeScript type tightening in create-app.ts is a nice bonus.

Issues found

1. CHANGELOG duplicate / wrong PR — needs fix

The #### Changed block added at the bottom of the [16.5.0] section is identical to the entry already present at line 46 and references PR 2832 (the Async::Variable → Async::Promise migration), not this PR. Looks like it was accidentally carried over when branching from that work. See inline comment on CHANGELOG.md.

2. Sanitize-then-validate: mangled injection inputs can still emit a nonce

'><script>alert(1)</script>' strips to 'scriptalert1/script' → passes validation → returned as the nonce value. This is attribute-injection safe (all HTML metacharacters are stripped), but it means a clearly corrupted nonce is emitted rather than silently dropped. See inline comment on sanitizeNonce.ts for a validate-only alternative. If the current behaviour is intentional, the doc comment should make that explicit.

Minor observations (no action required)

• No minimum-length guard: A single-character string like 'a' passes through. CSP nonces should be ≥128 bits; the function accepts anything matching the base64 alphabet. Fine for an injection-safety sanitizer, but worth noting if this function is ever reused as a validator.
• Test coverage is solid — 11 cases covering injection attempts, padding variants, and the full base64url alphabet.

Code Review: Clean refactoring of CSP nonce sanitization. Behavior preserved exactly from both source files. Three observations: (1) The sanitize-then-validate approach can emit a stripped but non-matching nonce (e.g. scriptalert1/script from injection input) which is attribute-safe but still causes CSP nonce mismatch - script is blocked, but the nonce attr in HTML may confuse debugging. (2) No test covers equals-sign in middle position (abc=def) - strip keeps it, validation rejects it, returns undefined, which is correct but undocumented. (3) Minor: regex accepts mixed base64 and base64url chars simultaneously (harmless superset). Overall: solid work - well-documented, tests thorough, no behavioral change.

Review

Overall: clean consolidation, good test coverage — a few security-policy questions worth settling before merge.

What's good

• The extraction removes real duplication and the "NOTE: keep this in sync" comment is gone, which is always a win.
• The new export path (react-on-rails/sanitizeNonce) is correctly added to package.json exports.
• 11 test cases is solid for a function this small.
• create-app.ts simplification is correct and the narrower literal-union type ('--rsc' | '--pro' | null) is a genuine improvement.

Concerns

1. Sanitize-then-validate policy leaves a silent-accept path uncovered by tests

The strip regex and validate regex are intentionally asymmetric: stripping is permissive (keeps = anywhere), but validation requires = only at the end. As a side effect, every injection test in the suite happens to fail for a structural reason (e.g. = in onload=alert ends up mid-string, breaking the pattern). There is currently no test exercising the other branch of the policy: a nonce that is stripped and still passes validation.

Example: sanitizeNonce('abc123"') → 'abc123'. The trailing " is silently dropped and the stripped value is used. This is documented, but it means a server bug that appends an extra character to the nonce value would be silently "fixed" rather than surfaced. A test explicitly covering this case would make the intent unambiguous.

2. The XSS-payload test produces a non-empty output

sanitizeNonce('"><script>alert(1)</script>') returns 'scriptalert1/script' — attribute-safe, and will never match a real CSP nonce, so no security issue in practice. But it means the function emits a nonce attribute for a value that was constructed entirely from an XSS payload. An alternative design — reject if any character was stripped — would return undefined for all tainted inputs and be simpler to reason about. The current design trades that predictability for not breaking renders when a nonce has minor encoding noise. Whether that trade-off is right for this codebase is worth an explicit comment in the source.

3. Hybrid base64 / base64url acceptance

Both +/ (standard base64) and -_ (base64url) are allowed simultaneously. A real CSP nonce uses one alphabet. This is harmless but the permissiveness is implicit. See inline comment for details.

Minor: the new ./sanitizeNonce entry in package.json exports is inserted between ./buildConsoleReplay and ./ReactDOMServer — no alphabetical ordering issue per se, but if the project has a convention for export ordering it's worth checking.

Review: Consolidate CSP nonce sanitization into shared module

Overall: Good consolidation of duplicate logic — the implementation is functionally identical to what it replaces, the test coverage is a clear improvement, and the @internal naming on the export signals intent appropriately. A few issues worth addressing before merge.

Security concern: sanitize-then-validate vs. validate-then-reject

The core function (inline comment on sanitizeNonce.ts:16) strips invalid characters from the input, then validates the result. This means:

• 'abc 123' → 'abc123' (silently returns a different nonce than the server issued)
• '"><script>alert(1)</script>' → 'scriptalert1/script' (adversarial input partially passes through)

The mutation path is attribute-injection-safe (no " or > can survive), but it creates subtle failure modes: if the server somehow emits a nonce with extraneous characters (encoding artifact, middleware bug), the sanitized value won't match the CSP header and the script will be silently blocked, with the nonce appearing valid in the HTML. A validate-then-reject policy (reject the entire nonce if any invalid character is present) would be stricter and easier to reason about — see the inline comment for a concrete alternative.

Behavior parity confirmed ✓

The new sanitizeNonce.ts is byte-for-byte equivalent to the removed logic in both RenderUtils.ts and react-on-rails-pro/src/utils.ts. No behavior change introduced.

Minor: @internal export is publicly accessible

The ./@internal/sanitizeNonce export in package.json is not enforced by Node's package system — any consumer can import it. Worth a changelog note flagging it as an unstable internal API with no SemVer guarantees.

Nit: test description

The test case at sanitizeNonce.test.ts:36 ('strips invalid chars from injection input and returns the remaining nonce text') describes the mechanism but not the actual output. A description like 'returns a stripped nonce derived from XSS input (attribute-safe)' would make the behavior immediately obvious from the test name without reading the body.

Address-review checkpoint posted at 2026-04-09T08:02:52Z.

Scan window: full PR history (2026-03-24T06:50:15Z → 2026-04-08T07:37:26Z) because no previous <!-- address-review-summary --> comment existed.
Cutoff mode: default workflow cutoff (no check all reviews override used).

Mattered

• Confirmed and closed addressed blocking items from earlier review passes:
  • ./sanitizeNonce export concern was handled by using ./@internal/sanitizeNonce.
  • abc=def middle-padding behavior now has explicit test coverage.
  • CSP-mismatch implication is now called out in sanitizer test comments.
  • Duplicate CHANGELOG.md section/commentary was removed.
• Policy-change suggestions (sanitize-then-validate vs validate-then-reject) were reviewed and intentionally deferred: this PR remains behavior-preserving consolidation.

Skipped

• Regex character-class formatting nit (- placement) with no behavioral impact.
• Test-name wording preferences where test body/comments already capture behavior.
• General notes about @internal discoverability/changelog annotation for internal-only surfaces.
• Duplicate/status-only bot summary comments that did not add new actionable deltas.

Follow-up issue: none created (no deferred must-fix items).

Future full-PR scans should start after this comment unless check all reviews is explicitly requested.