deps: update lshpack to v2.3.5 - #31315
Conversation
|
Updated 10:52 PM PT - Aug 8th, 2026
❌ @robobun, your commit d8e7f43 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31315That installs a local version of the PR into your bun-31315 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR retargets the LSHPACK_COMMIT constant to a new upstream commit SHA and updates the ChangesDependency Update
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@robobun fix process.test.js in this branch |
|
✅ Fixed at d8e7f43 — Twelfth re-apply. The weekly #29295 is the permanent fix (the test derives its expected hashes from The open review thread about |
c098aeb to
08ba1cb
Compare
ca8280f to
64dd4b3
Compare
dc19304 to
752291f
Compare
72d5979 to
a2df3f1
Compare
056cc44 to
3eb7950
Compare
3dfe472 to
3757eb1
Compare
|
Re-applied the test hash for the seventh time (59cabbc) after this Sunday's The loop is the Two ways out, either is fine:
|
59cabbc to
6bacbd2
Compare
| import type { Dependency, DirectBuild } from "../source.ts"; | ||
|
|
||
| const LSHPACK_COMMIT = "8905c024b6d052f083a3d11d0a169b3c2735c8a1"; | ||
| const LSHPACK_COMMIT = "cf0f70dd10b352194c97448eb5d00b4aa484f531"; |
There was a problem hiding this comment.
🟡 The earlier (resolved) comment on v2.3.5's removed trailing-whitespace strip is now stale for the two paths it named — both have since gained full tchar validation — but a fourth decode loop was missed: handle_push_promise_frame (src/runtime/api/bun/h2_frame_parser.rs:3678-3712) still surfaces header.name/header.value straight to JS with no is_malformed_field_name/is_malformed_field_value check between self.decode() at :3679 and create_utf8_for_js at :3697. After this bump a PUSH_PROMISE literal name like "cookie \t\r\n" or " " (previously trimmed/rejected by lshpack itself) reaches the node:http2 client's 'push' event verbatim. Client-only and requires a non-compliant server sending PUSH_PROMISE, so not blocking — but it's the one residual gap in the class fix; mirroring decode_header_block's guard (h2_frame_parser.rs:3828-3836) after :3692 would close it.
Extended reasoning...
What changed and why the earlier comment is stale
The resolved 2026-05-24 inline comment on this PR flagged that lshpack v2.3.5 (cf0f70dd) deletes from lshpack_dec_decode() the block while(len > 0 && isspace(*(name + len - 1))) --len; if (len == 0) return LSHPACK_ERR_BAD_DATA;, so literal header names with trailing whitespace now decode verbatim instead of being trimmed/rejected. Upstream's own new test test_hdec_trailing_whitespace_name asserts a literal name of cookie \t\v\f\r\n is returned with name_len == 12 and rc == 0 — validation is now the caller's job.
That comment named two Bun call sites (dispatch.zig:546, h2_frame_parser.zig:1918-1965). Both are now covered by full RFC 9110 tchar validation — the class fix landed:
src/http/h2_client/dispatch.rs:621→is_malformed_response_field(dispatch.rs:737-762) rejects any byte outside the lowercase-tchar setsrc/runtime/api/bun/h2_frame_parser.rs:3829—decode_header_blockcallsis_malformed_field_name(header.name) || is_malformed_field_value(header.value)(defined at :627-656)src/runtime/api/bun/h2/connection.rs:910calls the same helper
Grep confirms is_malformed_field_name is called at exactly those sites — and not in handle_push_promise_frame.
The residual gap
handle_push_promise_frame at h2_frame_parser.rs:3678-3712 is the fourth HPACK decode loop, and it was missed by the class fix:
while off < payload.len() {
let header = match self.decode(&payload[off..]) { // :3679
Ok(h) => h,
Err(_) => { /* GOAWAY COMPRESSION_ERROR */ return Ok(end); }
};
off += header.next; // :3692
let js_name = match get_http2_common_string(...) {
Some(cached) => cached,
None => create_utf8_for_js(&global_object, header.name)?, // :3697
};
headers.push(&global_object, js_name)?;
headers.push(&global_object, create_utf8_for_js(&global_object, header.value)?)?; // :3703
...
}Between self.decode() and create_utf8_for_js there is no call to is_malformed_field_name / is_malformed_field_value, no pseudo-header check, no connection-specific-field check, and no maxHeaderListSize/maxHeaderListPairs accounting. Everything decoded is pushed straight into the JS headers array and dispatched via onStreamPush. Contrast with the sibling loop at :3828-3836, which gates on is_malformed_field_name(header.name) || is_malformed_field_value(header.value) || (header.name.first() == Some(&b':') && !is_valid_..._pseudo_header(...)) before surfacing anything.
The only remaining upstream guard is lshpack.rs:67's name_len == 0 → EmptyHeaderName, which catches truly-empty names but not whitespace-bearing ones (name_len > 0 after v2.3.5).
Interaction with this PR (step-by-step)
- A server sends a PUSH_PROMISE frame whose HPACK block contains a literal-header-field with a non-Huffman name string of
63 6f 6f 6b 69 65 20 09 0d 0a("cookie \t\r\n", 10 bytes). - Before this bump (8905c024):
lshpack_dec_decodedecodeslen = 10, the trailing-whitespace loop strips the last four bytes,len = 6, name is"cookie".get_http2_common_stringmatches the well-known index, and JS sees the canonical key. - After this bump (cf0f70dd): the loop is gone,
lenstays 10,header.name = b"cookie \t\r\n".well_knowis 0 (no static-table hit for a 10-byte name), so control falls through tocreate_utf8_for_js(&global_object, header.name)at :3697 and JS receives a header keyed"cookie \t\r\n". - All-whitespace case: a literal name of
20 20 20(" "). Before, lshpack stripped tolen = 0→return LSHPACK_ERR_BAD_DATA→Err(_)at :3681 →COMPRESSION_ERRORGOAWAY. After,name_len = 3→ passes lshpack.rs:67 → surfaces to JS as a header keyed" ".
So the LSHPACK_COMMIT bump at line 13 observably widens what a node:http2 client's 'push' event sees, on this one path — it is not purely pre-existing.
Why nothing else catches it
is_malformed_field_name (:627-656) is a lowercase-tchar allowlist and would reject space/tab/CR/LF — it just isn't wired into this loop. The path is client-only (is_server sends GOAWAY at :3648-3656), and the frame-type dispatch calls handle_push_promise_frame unconditionally for type 0x05 with no enable_push gate, so it's reachable from any server the client connects to.
Impact
Low. Requires a deliberately non-compliant server sending PUSH_PROMISE (a largely-deprecated feature) with a crafted literal header name. Worst outcome is an odd header key surfaced to JS — no crash, no memory issue, no security boundary crossed. The missing maxHeaderListSize/maxHeaderListPairs accounting on this path is fully pre-existing and independent of this bump. Not worth blocking a routine dep bump; flagged so the one-line guard can be added to complete the class fix.
Suggested fix
Mirror decode_header_block's guard — after h2_frame_parser.rs:3692:
if is_malformed_field_name(header.name) || is_malformed_field_value(header.value) {
// set a malformed flag and continue decoding (to keep HPACK dynamic-table state in sync),
// then RST/GOAWAY PROTOCOL_ERROR after the loop instead of dispatching onStreamPush
}
162ae3a to
c4c22bf
Compare
1f9e6a7 to
ee439df
Compare
d324c10 to
88e1ac3
Compare
a5a4c26 to
a39bf77
Compare
2 participants
What does this PR do?
Updates lshpack to version v2.3.5
Compare: litespeedtech/ls-hpack@8905c02...cf0f70d
Auto-updated by this workflow
no test proof · iteration 27 · Platform-specific test-only change; deferring to CI.