Skip to content

Validate media type names, and fix Accept-Patch/Post/Query syntax checking - #157

Merged
mnot merged 4 commits into
mainfrom
claude/media-type-parsing-validation-c8d11d
Aug 8, 2026
Merged

Validate media type names, and fix Accept-Patch/Post/Query syntax checking#157
mnot merged 4 commits into
mainfrom
claude/media-type-parsing-validation-c8d11d

Conversation

@mnot

@mnot mnot commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Started from a review of 92fe78c (Accept-Patch / Accept-Post / Accept-Query), which factored media-type parsing into parse_media_type(). That function only checked that a value contained a /, leaving everything else to each field's ABNF regex — and the three new fields had syntax = False, so they got no character-level checking at all. @@@/###, "quoted"/thing, text/, a/b/c and */* all passed silently.

Verifying the ABNF against the specs turned up two further problems, which are the second and third commits.

Added: check media type names, and the syntax of Accept-Patch/Accept-Post

Accept-Patch and Accept-Post now declare the syntax their specs actually define — RFC 5789 is 1#media-type, LDP §7.1.1 is #media-range, so Accept-Post keeps wildcards — and HttpListField runs the check.

Media-type checking proper moves into check_media_type():

  • The shape check requires exactly one / with a non-empty name either side, instead of a / somewhere.
  • allow_wildcard now does something. It only exempted a bare * before, so */* and type/* were accepted everywhere, including on Accept-Patch, which lists media types rather than ranges. It now gates * in either position, and */subtype is rejected in all cases.
  • Type and subtype names are checked against restricted-name from RFC 6838 §4.2, added as httplint/syntax/rfc6838.py. This was unchecked for every media-type field, not just the new ones: HTTP's token permits %, ', *, `, |, ~ and a non-alphanumeric first character, none of which RFC 6838 allows. So Content-Type: ~text/pl%in was clean before and is now MEDIA_TYPE_BAD_NAME; names over 127 characters are MEDIA_TYPE_LONG_NAME.

Both new notes are WARN, not BAD — such a value is legal HTTP and recipients will accept it, it just can't be registered with IANA. Names that aren't valid tokens are left to the field's own syntax check, so there's no double reporting of a single problem.

I did not implement RFC 6838's SHOULD-level 64-character limit. Registered types such as application/vnd.openxmlformats-officedocument.presentationml.slideshow sit close enough to it that the note would fire on legitimate values.

Also fixes split_list_field(), which tested each match for truth before stripping it, so a whitespace-only element survived as "" and produced a spurious bad-syntax note for e.g. Accept-Patch: text/plain, .

Changed: parse Accept-Query as a Structured Field, per RFC 10008

Accept-Query isn't a comma-separated media-type list. RFC 10008 §3 defines it as a List Structured Field of Tokens or Strings, each naming a media range without parameters; type parameters ride along as SF parameters. The RFC's own example was mis-parsed by the old code:

Accept-Query: "application/jsonpath", application/sql;charset="UTF-8"

Rebased on StructuredField with sf_type = "list", checking each member with check_media_type(). Wildcards are allowed here (the RFC permits */* and xxxx/*). Members that are neither Tokens nor Strings get the new ACCEPT_QUERY_BAD_TYPE.

Breaking: the field's parsed value goes from a list of (media type, params) tuples to the Structured Field list http_sf returns.

The reference also moves from the datatracker copy of draft-ietf-httpbis-safe-method-w-body to RFC 10008, published June 2026. (92fe78c's message cited RFC 9694 for this field; that's Guidelines for the Definition of New Top-Level Media Types — commit message only, the code pointed at the draft.)

Fixed: substitute the media type into the Accept q value note

BAD_Q_VALUE's summary used str.format syntax where Note summaries are %-formatted, so it rendered literally as The q value on '{media_type}' is invalid.

Reviewer notes

  • Behaviour was checked by driving the linter over a table of good and bad values before and after each change, not just by the unit tests. make test, make lint and make typecheck are clean.
  • One thing deliberately left alone: RFC 10008 says Accept-Query members carry no inline parameters, but that can't be checked — SF parsing consumes ; as parameter syntax before the value reaches us, so a sender writing application/sql;charset=UTF-8 produces the conformant structure by accident.
  • make tidy wants to reformat note.py, status.py and set_cookie.py, which this branch doesn't touch. Reverted to keep the diff clean; the repo has some pre-existing black drift.

Written by Claude Code (Opus 5) at mnot's direction. mnot asked for an assessment of the media-type validation gap first, reviewed and approved the proposed approach before any code was written, then approved the two follow-up fixes and the RFC reference update. The analysis, code, and spec citations are Claude's; each spec claim was verified against the RFC or W3C text during the session rather than recalled.

🤖 Generated with Claude Code

mnot and others added 3 commits August 8, 2026 15:26
…Post

parse_media_type() only checked that a media type contained a "/", leaving
everything else to each field's ABNF regex. Accept-Patch, Accept-Post and
Accept-Query had syntax = False, so they got no character-level checking at
all: "@@@/###", '"quoted"/thing', "text/" and "a/b/c" all passed silently.

Give Accept-Patch and Accept-Post the syntax their specs define (RFC 5789 is
1#media-type; LDP is #media-range, so it keeps wildcards), and move the
media-type checks proper into check_media_type():

* The shape check now requires exactly one "/" with a non-empty name on
  each side, rather than just a "/" somewhere.
* allow_wildcard now means something. It only exempted a bare "*" before,
  so "*/*" and "type/*" were accepted everywhere -- including on
  Accept-Patch, which lists media types, not media ranges. It now gates "*"
  in either position, and "*/subtype" is rejected in all cases.
* Type and subtype names are checked against restricted-name from RFC 6838,
  Section 4.2, added as httplint/syntax/rfc6838.py. HTTP's token production
  is more permissive than the registry's rules, so "~text/pl%in" was clean
  before and is now MEDIA_TYPE_BAD_NAME; names over 127 characters are
  MEDIA_TYPE_LONG_NAME. Both are WARN, since such a value is legal HTTP and
  recipients will accept it -- it just can't be registered with IANA. Names
  that aren't valid tokens are left to the field's own syntax check, so
  there's no double reporting.

Accept-Query keeps syntax = False; the current draft defines it as a
Structured Field, so the media-type ABNF would reject its valid forms.

Also fix split_list_field(), which tested each match for truth before
stripping it, so a whitespace-only element survived as "". That produced a
spurious bad-syntax note for values with trailing whitespace after a comma,
e.g. "Accept-Patch: text/plain, ".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Accept-Query was implemented as a comma-separated list of media types, but
it isn't one. RFC 10008, Section 3 defines it as a List Structured Field
whose members are Tokens or Strings, each naming a media range without
parameters; media type parameters are carried as Structured Field
parameters. Its own example, which the old code mis-parsed, is:

    Accept-Query: "application/jsonpath", application/sql;charset="UTF-8"

Rebase the field on StructuredField with sf_type = "list", and check each
member with check_media_type(). Members that are neither Tokens nor Strings
are reported with the new ACCEPT_QUERY_BAD_TYPE. Wildcards are allowed --
the RFC permits "*/*" and "xxxx/*" -- so ACCEPT_QUERY_BAD_SYNTAX now talks
about media ranges rather than media types.

This changes the field's parsed value from a list of (media type, params)
tuples to the Structured Field list that http_sf returns.

The QUERY method was published as RFC 10008 in June 2026, so the reference
moves from the datatracker copy of draft-ietf-httpbis-safe-method-w-body to
the RFC.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BAD_Q_VALUE's summary used str.format syntax, but Note summaries are
%-formatted, so it rendered literally as:

    The q value on '{media_type}' is invalid.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mnot

mnot commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Review

Verified the spec claims against the sources and ran the checks on the branch (make test, make lint, make typecheck all clean; the black drift is genuinely pre-existing and limited to note.py and set_cookie.py). Also drove the linter over a table of edge cases rather than relying on the unit tests alone.

The three load-bearing citations hold up:

  • LDP §7.1.1 — Accept-Post = "Accept-Post" ":" #media-range. ✅ #media-range, wildcards allowed.
  • RFC 10008 §3 — List Structured Field of Tokens or Strings, media range without parameters, wildcards limited to */* and xxxx/*, type parameters as SF parameters. ✅
  • RFC 5789 §3.1 — 1#media-type. ✅

One detail worth calling out because it's easy to get wrong: */* does match rfc9110.media_type (since * is a valid tchar), so AcceptPatchWildcardTest correctly expects ACCEPT_PATCH_BAD_SYNTAX without BAD_SYNTAX — the allow_wildcard gate is what actually does the work there, not the ABNF. Also correct: Token is a UserString, not a str subclass, so isinstance(val, (Token, str)) genuinely needs both arms.

1. Non-token names in Accept-Query escape every check

httplint/field/utils.py:

# Names that aren't valid HTTP tokens are already reported by the field's own
# syntax check; RFC 6838 only adds information for the ones that are.
names = [name for name in names if re.match(rf"^{rfc9110.token}$", name, RE_FLAGS)]

That premise doesn't hold for Accept-Query, which now has syntax = False because it's a Structured Field. There's no ABNF check to defer to, so the filter drops the name and nothing reports it:

Accept-Query: "text/pl(in"    -> NO NOTES
Accept-Query: "text/pl in"    -> NO NOTES

This is the same silent-pass class the PR set out to close, and it lands in the worst spot: RFC 10008 Strings exist precisely to carry names that aren't valid Tokens, so a String member is where a bad name is most likely to appear. "~text/pl%in" is caught only because ~ and % happen to be tchars.

Fix could be a check_token: bool = True parameter on check_media_type() that accept_query sets to False, or emitting bad_syntax_note for non-token names when the caller signals it has no ABNF backstop.

2. Bare * accepted for Accept-Query

if allow_wildcard and media_type == "*":
    return

Accept-Query: * produces no note, but RFC 10008 permits only */* and xxxx/*. For Accept and Accept-Post this early return is masked by the field's ABNF regex (BAD_SYNTAX fires); Accept-Query has none.

Neither RFC 9110's media-range nor LDP's permits a bare * either, so this escape hatch may have no legitimate users left — dropping it looks better than special-casing it. The check_media_type docstring advertises it (so , /andtype/ are accepted), so that would need updating too.

3. Translation catalogs go stale

translate() is gettext keyed on the English source string, so changing BAD_Q_VALUE._summary from {media_type} to %(media_type)s orphans the existing msgid — The q value on '{media_type}' is invalid. is still what sits in fr/es/ja/zh, and those four now fall back to English for that string. Not a regression in correctness (the translated strings carried the same broken {media_type}), and the repo handles catalogs in separate "Update translations" commits, so this needn't block — just flagging that a refresh is owed, along with entries for MEDIA_TYPE_BAD_NAME, MEDIA_TYPE_LONG_NAME, ACCEPT_QUERY_BAD_TYPE and the reworded ACCEPT_QUERY_BAD_SYNTAX.

Smaller things

  • RESTRICTED_NAME_CHARS in utils.py is the whole unbounded restricted-name, not a character class — sitting one line from rfc6838.restricted_name_chars, which is one, the near-identical name is confusing. RESTRICTED_NAME_UNBOUNDED reads better.
  • rfc6838.restricted_name / type_name / subtype_name are unused. Consistent with how the other syntax modules mirror full ABNF (and the regex checker covers them), so probably keep — noting in case you'd rather not carry unused productions.
  • RESTRICTED_NAME_MAX_LEN is the only non-regex constant in a syntax module. Minor layering wobble; could live next to its single use in utils.py.
  • check_media_type(str(val).lower(), …) in accept_query.evaluate lowercases for checking, but self.value keeps the original case, unlike every other media-type field (parse_media_type lowercases what it stores). RFC 10008 members are case-insensitive media ranges; normalising the stored value would make the fields consistent.

Test coverage

Good coverage of what was added; the gaps line up with the issues above.

  • No test for a String member whose name isn't a valid HTTP token — that's issue 1, and a test would have caught it.
  • No test for Accept-Query: * (issue 2).
  • Every MEDIA_TYPE_BAD_NAME test exercises the subtype half. One on the type half (~text/plain) would guard the names = [type_name, subtype_name] symmetry.
  • The expected_out: Any annotations in content_type.py are load-bearing, not noise — mypy needs them for the empty-dict literals. Confirmed by removing them.

Verdict

Merge-worthy after 1. 2 is a one-line follow-on worth folding in at the same time; 3 is routine catalog maintenance. Nothing that reads as a security or performance concern — the regexes are anchored, linear, and bounded by header size.


Review written by Claude Code (Opus 5) at mnot's direction. mnot asked for a review of this PR and has not yet read the findings below in detail; the analysis, the spec verification (LDP and RFC 10008 fetched and checked during the session, not recalled) and the edge-case probing against a local build are Claude's.

check_media_type() dropped names that aren't valid HTTP tokens, on the
grounds that the field's own ABNF check reports them. That doesn't hold for
Accept-Query, which has syntax = False because it's a Structured Field, so
nothing reported them at all:

    Accept-Query: "text/pl(in"    -> no notes
    Accept-Query: "text/pl in"    -> no notes

RFC 10008 Strings exist precisely to carry names that aren't valid Tokens,
so that's where a bad name is most likely to turn up. Add a check_token
argument for callers with no ABNF backstop, which reports such names with
the field's own bad-syntax note, and set it in accept_query.

Also drop the bare "*" escape hatch. Neither RFC 9110's media-range, LDP's
nor RFC 10008's permits it; for Accept and Accept-Post the field's ABNF
masked it, but "Accept-Query: *" passed silently.

While here:

* Rename RESTRICTED_NAME_CHARS to RESTRICTED_NAME_UNBOUNDED. It's a whole
  restricted-name, not a character class, and sat one line from
  rfc6838.restricted_name_chars, which is one.
* Lowercase the media ranges accept_query stores, as the other media-type
  fields do; RFC 10008 members are case-insensitive.
* Cover a String member with a non-token name, a bare "*", case
  normalisation, and a bad name in the type half rather than the subtype.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mnot

mnot commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Reproduced both issues before fixing; addressed in 927fbc5.

1. Non-token names in Accept-Query — fixed

Confirmed exactly as described:

Accept-Query: "text/pl(in"    -> NO NOTES
Accept-Query: "text/pl in"    -> NO NOTES
Accept-Query: "text/pl@in"    -> NO NOTES

Took the second of the two suggested routes: check_media_type() gets a check_token argument, defaulting to False, which callers with no ABNF backstop set. When it's on, a name that isn't a valid HTTP token gets the field's own bad_syntax_note instead of being dropped.

Chose that over reporting it as MEDIA_TYPE_BAD_NAME because that note's text says HTTP's syntax is more permissive so recipients are likely to accept the value — true of ~text/pl%in, plainly false of text/pl in. All three now report ACCEPT_QUERY_BAD_SYNTAX at BAD, while "~text/pl%in" still reports MEDIA_TYPE_BAD_NAME at WARN. The distinction survives.

2. Bare * — fixed

Dropped the escape hatch rather than special-casing it, per your read. Nothing that reaches check_media_type permits a bare *, and the shape check already rejects it once the early return is gone. Docstring updated.

Fallout on the two fields that were masking it: Accept: * and Accept-Post: * now report their field-specific note alongside BAD_SYNTAX. That matches how text/ already behaved, so it looks like the right shape rather than new noise.

3. Translation catalogs — agreed, not in this PR

Confirmed the convention from the history — catalogs move in their own Update translations commits (291d766, b75c9de, fce3477), and tools.i18n.check passes on the branch. Owed on a refresh: the reworded BAD_Q_VALUE summary, MEDIA_TYPE_BAD_NAME, MEDIA_TYPE_LONG_NAME, ACCEPT_QUERY_BAD_TYPE, and the reworded ACCEPT_QUERY_BAD_SYNTAX.

Smaller things

  • RESTRICTED_NAME_CHARSRESTRICTED_NAME_UNBOUNDED. Agreed, the old name read as a character class.
  • Media ranges stored by accept_query are now lowercased, matching parse_media_type. type(val)(media_range) keeps the Token/String distinction, so APPLICATION/SQL, "TEXT/Plain" stores [(Token("application/sql"), {}), ('text/plain', {})].
  • Kept the unused rfc6838.restricted_name / type_name / subtype_name, on the reasoning you gave — the other syntax modules mirror full ABNF whether or not every production is referenced.
  • Kept RESTRICTED_NAME_MAX_LEN in the syntax module. It's the ABNF's own *126 bound plus the first character, so it belongs next to the rule it comes from; SPEC_URL is already a non-regex string in rfc9110.py, so it isn't a new kind of thing there. Happy to move it if you'd rather.

Tests

All four gaps covered: AcceptQueryBadStringTest (String member with a non-token name — this one would indeed have caught issue 1), AcceptQueryBareStarTest, AcceptQueryCaseTest, and CTBadTypeNameTest for the type half.

make test, make lint, make typecheck clean; 352 tests, up from 348.


Written by Claude Code (Opus 5) at mnot's direction. mnot asked for the review comments to be checked and acted on, and has not yet read this reply or the resulting commit; the repro, the fixes and the judgement calls above are Claude's.

@mnot
mnot merged commit 8be9bfc into main Aug 8, 2026
5 checks passed
@mnot
mnot deleted the claude/media-type-parsing-validation-c8d11d branch August 8, 2026 06:00
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.

1 participant