Skip to content

fix(http/unstable): make parseCacheControl() lenient instead of throwing - #7249

Open
tomas-zijdemans wants to merge 5 commits into
denoland:mainfrom
tomas-zijdemans:lenient-cache-control
Open

fix(http/unstable): make parseCacheControl() lenient instead of throwing#7249
tomas-zijdemans wants to merge 5 commits into
denoland:mainfrom
tomas-zijdemans:lenient-cache-control

Conversation

@tomas-zijdemans

@tomas-zijdemans tomas-zijdemans commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Makes parseCacheControl() ignore malformed directives instead of throwing SyntaxError. Groundwork for stabilizing the module (previous attempt: #7068).

Problem. The parser's job is untrusted input: our own doc example feeds it req.headers.get("cache-control") with no try/catch. Any client sending max-age=abc would throw mid-handler and 500 the server. RFC 9111 doesn't require strict parsing here. Unknown directives must be ignored (§5.2.3), and §4.2.1 encourages caches to treat invalid freshness info as stale, not to reject the message. Browsers and CDNs parse leniently. We were the odd one out.

What changed:

  • Known directives with a malformed or missing value (max-age=abc, bare max-age) are now skipped, same as unknown directives. @throws {SyntaxError} is gone from the public contract.
  • Docs now point cache implementers at §4.2.1: treat a response whose freshness directives didn't survive parsing as stale.
  • Doc note on formatCacheControl(): an empty noCache/private array serializes as the bare directive, which parses back as true.

formatCacheControl() still throws. It takes programmer-constructed values, so loud failure stays correct there.

Two judgment calls for review:

  • max-stale=abc drops the whole directive rather than degrading to bare max-stale. A garbled value shouldn't grant unlimited staleness.
  • Duplicate tracking runs before validation, so max-age=abc, max-age=100 parses to {}. The malformed first occurrence consumes the first-wins slot (§4.2.1).

Testing: the six assertThrows parse tests became lenient-behavior assertions, plus two new cases covering the judgment calls above. 52 tests pass.

@github-actions github-actions Bot added the http label Jul 23, 2026
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.03%. Comparing base (cdb83c6) to head (63cf475).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7249      +/-   ##
==========================================
- Coverage   95.03%   95.03%   -0.01%     
==========================================
  Files         618      618              
  Lines       51596    51589       -7     
  Branches     9340     9336       -4     
==========================================
- Hits        49035    49027       -8     
  Misses       2021     2021              
- Partials      540      541       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bartlomieju

Copy link
Copy Markdown
Member

The leniency change is the right call — RFC 9111 §5.2.3 requires ignoring unrecognized directives and §4.2.1 advises treating invalid freshness information as stale rather than rejecting, and throwing on attacker-controlled header bytes was a genuine hazard given the module's own example pipes req.headers.get() in unguarded. Since the module is unstable_/@experimental this doesn't need a BREAKING prefix.

Two notes:

  1. The new doc block says known directives with a malformed or missing value are ignored, but bare max-stale is deliberately kept as true per §5.2.1.2. Could you carve that exception out in the wording so it doesn't read as contradicting the implementation?

  2. Worth flagging for a maintainer decision rather than blocking this PR: after this change a caller can't distinguish "no max-age was sent" from "max-age was malformed". The doc tells cache implementers to treat the malformed case as stale, but an absent maxAge normally means fall back to heuristic freshness, and the API now gives no signal to tell the two apart. Probably fine to leave as-is while unstable, but it should be settled before stabilization.

Also, the first-wins resolution for max-age=abc, max-age=100{} is defensible under §4.2.1 and yields the safe result, but it's currently only explained in the PR description — might be worth a line in the JSDoc.

@tomas-zijdemans

Copy link
Copy Markdown
Contributor Author

Thanks, all three points land.

1 and 3 are fixed in the doc block: bare max-stale now has an explicit carve-out (valid per §5.2.1.2, parses as true), and the first-wins duplicate behavior (max-age=abc, max-age=100 yields no maxAge) moved from the PR description into the JSDoc where it belongs.

On 2, agreed, and I'd rather not solve it in this PR. The honest state today: {} means "absent or malformed, you can't tell which". If callers should follow the §4.2.1 advice without re-parsing the raw header, the parser has to surface the failure somehow. An optional invalid: string[] on the result is the least invasive shape I can think of, but that's an API question for stabilization review, not this fix. I'll open a tracking issue so it doesn't get lost.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants