Skip to content

DiagnosticUrlRedactor: replace four spelling-specific scanners with one normalize-then-scan pass #4990

Description

@justin808

Context

ReactOnRails::DiagnosticUrlRedactor currently redacts userinfo with four parallel,
spelling-specific mechanisms:

  • the main module's literal-scheme regex path (FLEXIBLE_HTTP_URL_SCHEME_PATTERN)
  • NetworkUrlStartScanner / EncodedNetworkUrlRewriter (encoded scheme + authority)
  • AuthorityRelativeRewriter (bare literal //)
  • EncodedAuthorityRedactor (bare encoded %2F%2F, /%2F, %2F/)

Each independently re-solves "find the authority, strip its userinfo" for a different
spelling of scheme / slash / colon / whitespace.

Raised in review on #4863 (multiple threads).

Why this is worth revisiting

#4863 went through five review rounds, and each round found a new bypass that was closed by
adding another spelling-specific pattern or module rather than by closing the underlying
class:

  1. encoded authority terminator before an encoded userinfo delimiter
  2. encoded / mixed authority-relative starts
  3. sanitized_unprotected_prefix returning an unsanitized prefix
  4. prose path recognizing only literal // starts
  5. an all-numeric password read as a port

That is real evidence the mechanism does not generalize on its own. Plausible future
spellings that would each need another special case: double-encoded %252F%252F,
mixed-case percent hex in an unanticipated position, an unhandled whitespace variant.

The three files are ~550 + 110 + 120 lines, which is a lot of surface for a reviewer to
convince themselves is exhaustive.

Proposed direction

A single normalize-then-scan pass: decode percent-encoding once into a byte-tagged
intermediate form that records, per byte, whether it was literal or encoded; then run one
boundary-finding scan over that form. Spelling stops being a dimension of the problem, and
the fail-closed rules apply once instead of N times.

This is a larger refactor than belonged in a security forward-port, which is why #4863
closed the individual bypasses instead.

Smaller items to fold in

These were raised in the same review round and are cheap alongside the refactor:

  1. Bounded-time regression for the @-free shape. malformed_authority_pattern chains
    two unbounded (?:(?!SCHEME|//).)* groups. I measured it and it is linear, not
    quadratic — the shared // lookahead marker self-bounds each attempt:

    input length time
    "// " + "x " * 10_000 20 KB 0.011 s
    "// " + "x " * 50_000 100 KB 0.051 s
    "// " + "x " * 100_000 200 KB 0.094 s
    "// " + %(a "b 'c ) * 50_000 400 KB 0.152 s

    10x input gives ~8.5x time. No fix needed, but the existing Timeout.timeout(1) specs
    all use inputs containing a trailing @, so this shape has no permanent coverage. Add
    one spec with a large //-prefixed, @-free, whitespace/quote-heavy string.

  2. detected_authority_start over-redacts benign prose. For input starting with a
    literal // where the first scheme span is not at byte 0 (e.g.
    "//foo@bar http%3a%2f%2fuser:pass@host/path"), it falls back to offset 2 and strips
    foo@ as if it were credentials. The security property holds — the real credential is
    still removed by the subsequent rewrite_all — but unrelated diagnostic text is silently
    deleted. Guard the fallback so it only fires when no scheme span is found at all.

  3. Duplicate accumulation loop. ByteSpanRewriter.rewrite and
    rewrite_at_offsets implement the same "buffer / byteslice prefix / yield span / advance
    / flush tail" pattern, differing only in how the next boundary is produced, and have
    already diverged slightly in early-return handling. Parameterize on "next boundary".

  4. Cross-gem duplication. sanitized_single_network_url reimplements
    ReactOnRailsPro::Configuration#strip_renderer_url_userinfo
    (react_on_rails_pro/lib/react_on_rails_pro/configuration.rb), including the
    password-before-user clearing order that URI requires. A correctness fix to one will
    not propagate to the other.

Not in scope

The sanitize entry-point API shape is tracked separately in #4988.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    P1Target this sprintcomplexity:simplifyRemoves code, duplicated rules, states or maintenance obligations. Not a merge-readiness verdict.triage:simplify-nextFocused simplification after overlapping work is resolved; reduce code and obligations.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions