fix(http/unstable): make parseCacheControl() lenient instead of throwing - #7249
fix(http/unstable): make parseCacheControl() lenient instead of throwing#7249tomas-zijdemans wants to merge 5 commits into
parseCacheControl() lenient instead of throwing#7249Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
|
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 Two notes:
Also, the first-wins resolution for |
|
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. |
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:
formatCacheControl() still throws. It takes programmer-constructed values, so loud failure stays correct there.
Two judgment calls for review:
Testing: the six assertThrows parse tests became lenient-behavior assertions, plus two new cases covering the judgment calls above. 52 tests pass.