Skip to content

fix(serve-static): correct Range header parsing edge cases#372

Merged
yusukebe merged 4 commits into
honojs:mainfrom
otnc:fix/serve-static-range-parsing
Jul 12, 2026
Merged

fix(serve-static): correct Range header parsing edge cases#372
yusukebe merged 4 commits into
honojs:mainfrom
otnc:fix/serve-static-range-parsing

Conversation

@otnc

@otnc otnc commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

serveStatic's Range parsing had three edge cases that didn't match RFC 9110:

  • bytes=-N (suffix range) returned the first N+1 bytes instead of the last N.
  • bytes=0-0 returned the whole file instead of 1 byte (parseInt(...) || size - 1 treated a valid 0 as falsy).
  • An unsatisfiable range (start beyond EOF, or start > end, e.g. bytes=100-200 on a 17-byte file) crashed createReadStream with RangeError instead of returning 416.

Replaced the ad-hoc parsing with a parseRange() helper that handles all three correctly while keeping the existing behavior for malformed headers (e.g. Range: hello still serves the whole file, per the existing test).

Added tests for each case; all existing tests still pass unchanged.

- `bytes=-N` (suffix range) now returns the last N bytes instead of
  the first N+1 bytes.
- `bytes=0-0` now returns 1 byte instead of the whole file (the old
  `parseInt(...) || size - 1` treated a valid 0 as falsy).
- An unsatisfiable range (start beyond EOF, or start > end) now
  returns 416 instead of crashing createReadStream with a RangeError.
@yusukebe

Copy link
Copy Markdown
Member

@otnc Thank you for the PR.

Hey @usualoma , what do you think of this PR's direction? I think these are actual bugs. If this PR is acceptable, I'll review it.

@usualoma

Copy link
Copy Markdown
Member

Hi @otnc
cc: @yusukebe

Thank you for creating the PR. I think this is a great way to implement it!

I have two concerns:

Malformed ranges

Now that range parsing has been extracted into its own helper, I think bytes=x-1 and bytes=0-1x should be treated as malformed rather than partially parsed. Currently, both appear to be interpreted as 0-1.

Empty files

For an empty file, Range: hello returns { start: 0, end: -1 }, causing createReadStream() to throw and respond with 500. We should add a regression test for this case.

…rash

- Range segments with trailing garbage (e.g. `bytes=0-1x`) were silently
  truncated by parseInt into `0-1` instead of being treated as malformed.
  first-pos/last-pos/suffix-length are now validated as `1*DIGIT`.
- A Range request on an empty file fell through to createReadStream with
  end=-1 and crashed with 500 instead of returning 416.

Addresses review feedback from @usualoma on honojs#372.
@otnc

otnc commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@usualoma
cc: @yusukebe
Thank you for your review!
Fixed a mistake in the code.

@yusukebe yusukebe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yusukebe

Copy link
Copy Markdown
Member

@usualoma Could you review this again?

@usualoma

Copy link
Copy Markdown
Member

Hi @otnc,

Thank you for the update! I believe the functionality is now implemented correctly.

Sorry to ask for one more thing, but I would prefer to merge this after a small refactor. I opened otnc#1 with a proposal. Could you please review it and merge it if it looks good to you?

The main points are:

  • Although this is an internal utility, parsing the Range syntax and resolving/validating it against the file size are logically separate responsibilities. I think it is clearer to split parseRange into two small functions.
  • This part comes from the existing implementation, but replace, split, and the subsequent isDigits checks can be expressed with a single anchored regular expression. This avoids intermediate operations and repeated validation, so it should also be slightly more efficient, though the difference is admittedly small.

I could also merge this PR first and handle the refactoring separately, but I wanted to share the proposal in advance because I thought you might prefer to see it before this PR is merged. Apologies if this was unnecessary.

refactor(serve-static): separate range parsing and resolution
@otnc

otnc commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Hi @usualoma !
Your PR has been accepted. I think it's a very good change!

@usualoma

Copy link
Copy Markdown
Member

Hi @otnc,
Thank you for accepting my PR.
I think this PR is ready to be merged as is!

@yusukebe yusukebe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yusukebe

Copy link
Copy Markdown
Member

@otnc @usualoma

Thank you for the PR and review. Looks good. Let's go.

@yusukebe yusukebe merged commit fdb87ba into honojs:main Jul 12, 2026
5 checks passed
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.

3 participants