Skip to content

Reject invalid percent-encoding in ISO-8859-1 ext-value#138

Open
spokodev wants to merge 2 commits into
jshttp:masterfrom
spokodev:fix/iso-8859-1-malformed-pct
Open

Reject invalid percent-encoding in ISO-8859-1 ext-value#138
spokodev wants to merge 2 commits into
jshttp:masterfrom
spokodev:fix/iso-8859-1-malformed-pct

Conversation

@spokodev

Copy link
Copy Markdown

Problem

decodeExtended() handles a malformed percent-encoding (a % not followed by two hex digits) inconsistently between the two RFC 8187 charsets:

  • UTF-8 path → decodeURIComponent throws → the value is rejected (undefined).
  • ISO-8859-1 path (decodeHexEscapes) → keeps the literal % and returns a string.

RFC 8187 §3.2.1 defines value-chars = *( pct-encoded / attr-char ) with pct-encoded = "%" HEXDIG HEXDIG, and attr-char does not include %, so a bare or short % is non-conformant for both charsets; the two paths should behave the same.

Because a successfully decoded filename* overrides filename (RFC 6266 §4.3), the lenient ISO-8859-1 path lets a malformed filename* clobber a valid filename fallback:

parse('attachment; filename="invoice.pdf"; filename*=ISO-8859-1\'\'report%2').parameters.filename
// before: 'report%2'   (malformed ext-value overrides the fallback)
// after:  'invoice.pdf'

This asymmetry was introduced in #127, which tightened only the UTF-8 branch.

Fix

Make decodeHexEscapes return undefined on a malformed percent-encoding, mirroring the UTF-8 path. decodeExtended already propagates undefined, so the unprocessable ext-value is ignored and the filename fallback is preserved. Valid percent-encoding and bare-ASCII ext-values are unaffected.

Verification

  • Two new tests: a malformed ISO-8859-1 filename* is ignored, and a valid filename fallback is preserved. Both fail on the base branch and pass with the fix.
  • Independent RFC 8187 §3.2.1 reference decoder, fuzzed over malformed/valid percent inputs: the ISO-8859-1 path now matches the reference (0 divergences) and agrees with the UTF-8 path on the reject decision; previously-valid inputs are unchanged.
  • Full suite: 193/193 (191 existing + 2 new).

decodeExtended() enforces strict percent-encoding for UTF-8 ext-values
(a "%" not followed by two hex digits makes decodeURIComponent throw, so
the value is rejected) but the ISO-8859-1 path kept the literal "%" and
returned a value. RFC 8187 3.2.1 defines pct-encoded = "%" HEXDIG HEXDIG
for both charsets, so a malformed "%" is non-conformant regardless of
charset.

Because a successfully decoded filename* overrides filename (RFC 6266
4.3), the lenient path let a malformed ISO-8859-1 filename* clobber a
valid filename fallback:

  parse('attachment; filename="invoice.pdf"; '
      + "filename*=ISO-8859-1''report%2").parameters.filename
  // before: 'report%2'   after: 'invoice.pdf'

Make decodeHexEscapes return undefined on malformed percent-encoding,
matching the UTF-8 path, so an unprocessable ext-value is ignored.

@blakeembrey blakeembrey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but can you correct the formatting so tests pass?

Comment thread src/index.ts
// malformed percent-encoding: reject per RFC 8187 3.2.1, matching the
// UTF-8 path, so the unprocessable ext-value is ignored
return undefined;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Prefer to return early, e.g. if (idx + 2 > str.length || !isHexDegit(...) || ...) return undefined first and avoid the need for an else.

@spokodev

Copy link
Copy Markdown
Author

Done - the failing check was prettier on the new test line; reformatted it (single quote + wrapped to printWidth), no logic change. Tests pass now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants