Skip to content

fix(server): follow RFC 9110 for if-modified-since - #346

Open
lprnmns wants to merge 1 commit into
unjs:mainfrom
lprnmns:fix/if-modified-since
Open

lprnmns wants to merge 1 commit into
unjs:mainfrom
lprnmns:fix/if-modified-since

Conversation

@lprnmns

@lprnmns lprnmns commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

The if-modified-since check in the handler has two problems:

1. It never matches a real file's mtime. last-modified is sent with mtime.toUTCString(), which has second precision. The check then compares the client's echoed date against the full mtime, new Date(ifModifiedSince) >= sourceMeta.mtime. File mtimes almost always have a sub-second part, so the echoed date is always slightly earlier and the request is answered with a full 200. Revalidation only works when the client also sends if-none-match.

With ipxFSStorage over test/assets:

mtime 2026-10-23T13:36:13.675Z
GET /w_10/giphy.gif                                        -> 200, last-modified: Fri, 23 Oct 2026 13:36:13 GMT
GET /w_10/giphy.gif  if-modified-since: <that value>       -> 200 (expected 304)

2. It runs before, and regardless of, if-none-match. RFC 9110 §13.1.3 says a recipient MUST ignore If-Modified-Since if the request contains If-None-Match, because the entity tag is the more accurate validator. Here a request whose if-none-match doesn't match, but whose if-modified-since is not older than the mtime, gets a 304. The client then keeps a response the ETag says is stale:

GET /w_10/giphy.gif  if-none-match: "other"  if-modified-since: <tomorrow>  -> 304 (expected 200)

The fix reads if-modified-since only when there is no if-none-match, and compares it against the mtime truncated to whole seconds. Truncating matches what last-modified sends; Date.parse() returns NaN for an invalid date, so the comparison is false as before. This is the same behavior h3 (utils/internal/cache.ts) and srvx (static.ts) already implement.

Tests: two cases in the conditional requests block of test/server.test.ts, one with a .500 mtime and one with a non-matching if-none-match plus if-modified-since. Both fail without the change. The existing if-modified-since test used a whole-second mtime, which is why it passed. pnpm test passes (lint, tsc --noEmit after pnpm build for the examples, and vitest with coverage).

Summary by CodeRabbit

  • Bug Fixes
    • Corrected conditional response handling for cached content. Modification dates are now compared at whole-second precision, so matching dates return a not-modified response even when the underlying timestamp includes milliseconds.
    • When a request includes both an entity tag and a modification date, the entity tag takes precedence. A non-matching entity tag now correctly results in the full response rather than an incorrect not-modified response.

@lprnmns
lprnmns requested a review from pi0 as a code owner October 23, 2026 13:39
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b754538c-a0f7-4e6d-93cf-2650828e2e86
📥 Commits

Reviewing files that changed from the base of the PR and between 28295a0 and e23fc16.

📒 Files selected for processing: 2
  • src/server.ts
  • test/server.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The server now ignores If-Modified-Since when If-None-Match is present. Otherwise, it compares the parsed date with the source modification time rounded down to whole seconds. Tests cover both behaviors.

Changes: Conditional request handling

Layer / File(s) Summary
Conditional header checks
src/server.ts, test/server.test.ts
The server checks If-Modified-Since only when If-None-Match is absent and compares dates at whole-second precision. Tests cover a modification time with milliseconds and a nonmatching ETag.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to e23fc

The conditional-request change is ready to merge after normal checks. Clients using only Last-Modified retain its standard one-second ambiguity.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: updating server handling of If-Modified-Since to follow RFC 9110, including conditional-request precedence behavior.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.76%. Comparing base (28295a0) to head (e23fc16).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #346      +/-   ##
==========================================
+ Coverage   95.74%   95.76%   +0.01%     
==========================================
  Files           9        9              
  Lines        1011     1015       +4     
  Branches      352      357       +5     
==========================================
+ Hits          968      972       +4     
  Misses         41       41              
  Partials        2        2              

☔ 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.

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