fix: handle invalid if-modified-since - #1426
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — a small, correct fix for RFC 9110-compliant handling of an invalid If-Modified-Since header.
What was reviewed: the new parseIfModifiedSince helper and its use in both the iceberg and normal GetObject handlers in get-object.ts; traced the resulting Date through s3-handler.ts (toISOString()) down to the backend adapter to confirm the string/undefined expectations in the new test match runtime behavior; confirmed the pre-existing parseDateHeader (used for response-expires) is untouched and unrelated to this change.
Extended reasoning...
Overview
The diff adds a parseIfModifiedSince(input?: string) helper in src/http/routes/s3/commands/get-object.ts that returns undefined for an invalid date string instead of an "Invalid Date" object, per RFC 9110 §13.1.3. Both the iceberg and normal GetObject handlers were switched from manually constructing new Date(ifModifiedSince) to calling this helper, and the now-dead local variable was removed. router.test.ts adds a parametrized it.each test covering an invalid header ("not a date" -> undefined) and a valid RFC-formatted header (-> correct ISO date passed to the backend).
Security risks
None identified. This is a request-header parsing fix with no auth, permission, or data-exposure implications. Previously the bug could propagate an invalid Date object to the storage backend, which is a correctness/availability issue (potential 500 or wrong behavior) rather than a security issue.
Level of scrutiny
Low. The change is small, self-contained, mechanical, and matches an established pattern already used elsewhere in the file (parseDateHeader). It is directly covered by a new test that exercises the actual route through s3Router and backend mock, verifying both the invalid and valid cases end-to-end.
Other factors
I traced the data flow (get-object.ts -> s3-handler.ts's command.IfModifiedSince?.toISOString() -> backend adapter) to confirm the test's expected values (undefined and the ISO string) are consistent with actual runtime behavior, not just the helper's isolated logic. No CODEOWNERS file was found restricting these paths, and no prior review comments exist on this PR to check for being addressed.
Coverage Report for CI Build 35702101685Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.03%) to 83.371%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
35cb07d to
d76c842
Compare
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Invalid if-modified-since header causes 500.
What is the new behavior?
Invalid value is ignored per RFC.
Additional context
https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.3