Hardening: input validation and protocol tightening across 24 subsystems (round 7)#31495
Conversation
|
Updated 5:19 PM PT - May 28th, 2026
❌ @Jarred-Sumner, your commit a4d0ca0 has some failures in 🧪 To try this PR locally: bunx bun-pr 31495That installs a local version of the PR into your bun-31495 --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:
WalkthroughThis PR hardens security and protocol robustness across network, package manager, database, and parsing layers. Changes include TLS read-state preservation during dispatch, owned SocketAddress types, absolute-form URL routing normalization, HTTP/2 connection-specific header rejection, YAML/XML/Markdown parsing bounds enforcement, SQLite thread-safe handle registry, lockfile trust verification for non-npm resolutions, tarball and registry token validation, and comprehensive regression tests covering all changes. ChangesNetwork Stack Security: TLS, Socket Lifecycle, URL Routing, HTTP Protocol and Parsing
Package Manager Security: Trust Scoping, Tarball Validation, Registry Control
SQLite Thread Safety and Handle Registry
File Permissions, Filesystem Bounds, and Credential Security
Comprehensive Regression Test Coverage
Possibly Related PRs
Suggested Reviewers
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Confirmed and fixed in 0a37560. clippy's undocumented_unsafe_blocks only accepts a SAFETY comment on the lines directly above the unsafe block (or above a let/const), so prepending the OWNS_HANDLERS check pushed the unsafe block off the line below the existing comment — at all three sites this change touched (Listener.rs ~1077, ~1177, 1434), not just 1434. Moved the existing SAFETY comments to sit directly above the |
| )); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This isn't the right way to address this - we can keep storing the scripts in the binary lockfile.
There was a problem hiding this comment.
Reverted in defda3d — bun.lockb keeps storing the lifecycle scripts and the install path loads and uses them exactly as on main; the load-time clearing in Package.rs is gone and the test that asserted that behavior is removed. One question: would you like a follow-up that re-validates the stored script text against the installed package's package.json at execution time, or do you prefer leaving this exactly as on main?
|
r7t4-01: Confirmed and fixed in 9c01179. The Content-Length change in build_request() also applied to node:http's client (it goes through nodeHttpClient → the same request builder), so the conflicting "Content-Length: 1, 2" that test-http-double-content-length.js relies on never reached the wire and the server accepted the request instead of emitting clientError. The request builder now distinguishes the nodeHttpClient entry point via a flag threaded through FetchOptions: the Node client forwards an explicitly set Content-Length on bodyless requests exactly as Node does (and as main did), while fetch() keeps refusing to emit a Content-Length that doesn't describe the body it actually sends. Verified with test/js/node/test/parallel/test-http-double-content-length.js, fetch.test.ts -t "Content-Length", and node-http.test.ts. |
|
r7t4-02: Reworked as requested in d4184b8: the Response constructor and Response.json() no longer throw for 101/103/204/205/304 — the body is accepted and dropped, so response.body ends up null and Bun.serve never has body bytes to write for those statuses (the on-wire property from the original change is preserved without the breaking TypeError). The regression test in test/js/web/fetch/response.test.ts now asserts ignore semantics (.body === null, text() === ""), the serve.test.ts status-code workaround is reverted back to |
|
Re: the websocket LSan report (r7t6-01) — confirmed and fixed in ca57e25. The LSan report on debian-13-x64-asan (build 58567) is the 32-byte The fix keeps the detach for a still-running VM (the queued microtask remains the single owner and frees the box, so the mid-tick close path covered by websocket-client-short-read.test.ts stays free of the original double-free) and additionally reclaims the box in |
|
Re: the 28632 RSS regression (r7t6-02) — confirmed and fixed in 6801c8c. The header-time reset made every re-execution of a reused prepared statement drop its cached row shape, so the duplicate-name pass and structure rebuild ran per query and the resulting per-execution alloc/free churn pushes test/regression/issue/28632.test.ts past its ASAN bound (the same churn class that test was added for — see the elision note in ColumnDefinition41.rs). Fixed by detecting changes where they are actually observable: ColumnDefinition41::decode now reports whether the column identifier differs from the previously decoded one, and the connection only resets cached_structure/fields_flags (and re-arms the duplicate check) when a column changed or the column count changed. Same-count renames still invalidate, so the new "rebuilds row object shape when a reused statement's result columns change" coverage in sql-mysql.test.ts keeps passing, while identical re-executions reuse the cached shape as before. Verified locally against the dockerized MySQL: 28632 passes and the full sql-mysql.test.ts shape-rebuild coverage passes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/parsers/yaml.rs (1)
2326-2359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the alias budget per document.
parse_stream()reuses oneParserfor the whole YAML stream, but this counter is only initialized once here. A large first document can therefore consume budget and make a later document fail withExcessiveAliasingeven though each document stays within the intended per-document limit.Suggested fix
fn parse_document(&mut self) -> Result<Document, ParseError> { let mut directives: Vec<Directive> = Vec::new(); self.anchors.clear(); self.tag_handles.clear(); + self.alias_expansion_budget = Self::MAX_ALIAS_EXPANSION;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parsers/yaml.rs` around lines 2326 - 2359, The alias expansion budget is only set once in Parser::init so parse_stream() reusing the Parser across multiple documents can run out of budget on later documents; update the start-of-document path (where parse_stream begins processing each new document) to reset parser.alias_expansion_budget = Parser::MAX_ALIAS_EXPANSION (and optionally parser.merge_props_budget = MappingProps::MAX_MERGED_PROPERTIES) so each document gets a fresh per-document budget; locate the document-start code invoked by parse_stream and add this reset before parsing the document body.src/runtime/cli/pack_command.rs (1)
2252-2274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-run the restricted-access scoped-name check after lifecycle scripts.
Line 2254 reloads
name, but theAccess::Restricted/ scoped-package validation from the first-read path is not repeated here. Aprepack/prepublishOnlyscript can change@scope/pkgtopkg, and this branch will keep going withRestrictedUnscopedPackagebypassed.Suggested fix
package_name = package_name_expr .as_string_cloned(bump)? .ok_or(PackError::InvalidPackageName)?; if package_name.is_empty() || has_unsafe_tarball_filename_part(package_name) { return Err(PackError::InvalidPackageName); } + if FOR_PUBLISH { + let is_scoped = bun_install::dependency::is_scoped_package_name(package_name) + .map_err(|_| PackError::InvalidPackageName)?; + if let Some(access) = manager.options.publish_config.access { + if access == bun_install::Access::Restricted && !is_scoped { + return Err(PackError::RestrictedUnscopedPackage); + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/pack_command.rs` around lines 2252 - 2274, After re-reading package_name/version you must re-run the restricted-access scoped-name validation so lifecycle scripts can't remove a required scope; after the package_name is reassigned (the block that sets package_name from json.root.get("name") and validates emptiness), check if access == Access::Restricted and if so validate package_name still has a scope and otherwise return PackError::RestrictedUnscopedPackage (same logic used in the first-read path). Place this check immediately after the package_name re-read/validation and mirror the original scoped-name validation routine used earlier.src/jsc/bindings/bun-spawn.cpp (1)
41-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t hard-cap the fd fallback sweep at 65536 in
bun-spawn
getMaxFd()clampsmaxfdto65536, andcloseRangeLoop()iterates withfd < maxfd, so65536becomes an exclusive upper bound.- The cloexec fallback call is
closeRangeOrLoop(current_max_fd + 1, INT_MAX, true), andend == INT_MAXbypasses theend + 1logic—so the loop sweep stops at the65536clamp, leaving fds>= 65536untouched (non-Linux always; Linux only whenbun_close_range()fails).- Use
getrlimit(RLIMIT_NOFILE)(soft limit) to size the sweep, with a small conservative default only when querying it fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/bun-spawn.cpp` around lines 41 - 45, The current fd-sweep clamps maxfd to 65536 in getMaxFd(), causing closeRangeLoop()/closeRangeOrLoop() to stop at that exclusive bound and leave fds >= 65536 open; update getMaxFd() (and any place using the 65536 magic) to query the process soft limit via getrlimit(RLIMIT_NOFILE) and use that as the sweep upper bound, falling back to a small conservative default only if getrlimit fails, and preserve the existing logic that treats end == INT_MAX as “no end” so end+1 isn't applied when calling closeRangeOrLoop()/closeRangeLoop(); ensure bun_close_range() failure still triggers the loop fallback that uses the getrlimit-derived bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/lockfile.rs`:
- Around line 3413-3418: The code indexes resolutions[package_id as usize]
without ensuring package_id is within resolutions.len(), which can panic; before
using package_id (from self.buffers.resolutions[dep_id as usize]) add a bounds
check (e.g., if package_id as usize >= resolutions.len() { continue } or
otherwise fail-closed) so out-of-range IDs are rejected; update the block around
package_id, invalid_package_id, resolutions, and the eql(resolution, buf, buf)
check to perform this check and avoid panics.
In `@src/install/yarn.rs`:
- Around line 1200-1218: Extract the duplicated remote URL + integrity
validation into a helper function (e.g., validate_remote_url_or_integrity) that
accepts the same inputs used in both blocks: resolved, name_to_use, &entry,
&manager; move the logic that checks starts_with("https://"/"http://"),
manager.scope_for_package_name(...).url.href(),
Integrity::parse(...).tag.is_supported(),
lockfile::bun_lock::url_is_under_registry(...), npm::Registry::DEFAULT_URL and
the codeload.github.com exception into that helper and have it return a
Result<(), bun_core::Error> (or bool) so both original sites call it and
preserve the existing Err(bun_core::err!("InvalidYarnLockfile")) behavior when
validation fails.
In `@src/jsc/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 264-271: The current databaseForHandle(int32_t handle) returns a
VersionSqlite3* while holding databasesLock but releases it before callers
access versionDB->db, allowing close() to null/close the DB and create a
TOCTOU/use-after-close; change the design so handle resolution and DB liveness
are atomic by adding a pin/unpin or reference count inside VersionSqlite3 (e.g.,
add methods like retain() / release() or a mutex inside VersionSqlite3) and
modify databaseForHandle to acquire that pin under databasesLock (or return a
scoped guard object) so callers can safely use versionDB->db without races, and
update close() to wait for or clear that pin before closing the sqlite handle;
apply the same fix pattern where databaseForHandle is used (including the other
referenced block around lines 1799-1821).
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 656-665: The helper is_forbidden_connection_specific_header
currently whitelists "te: trailers" unconditionally which lets malformed
response header blocks through; change its API to accept an is_request (or
is_response) boolean (e.g., fn is_forbidden_connection_specific_header(name:
&[u8], value: &[u8], is_request: bool) -> bool) and only allow the special-case
"te: trailers" when is_request == true; for responses treat any "te" header as
forbidden. Update every call site that currently invokes
is_forbidden_connection_specific_header (including the spots that validate
inbound requests and inbound responses) to pass the appropriate is_request flag
so responses will be rejected with PROTOCOL_ERROR. Ensure behaviour and tests
reflect the tightened response validation.
In `@src/runtime/cli/pack_command.rs`:
- Around line 3055-3059: The function has_unsafe_tarball_filename_part currently
lets values containing '/' through if they aren't '.' or '..', which allows
inputs like package version "1/2" or unscoped names "foo/bar" to become path
separators later in fmt_tarball_filename; change
has_unsafe_tarball_filename_part to accept a flag allow_scope_separator: bool
and treat any '/' in value as unsafe unless allow_scope_separator is true, keep
the existing checks for "."/".." components and for matches!(c, b'\\' | b':' |
0), and then update the call sites so you pass allow_scope_separator = true when
validating package_name and allow_scope_separator = false when validating
package_version.
In `@src/url/lib.rs`:
- Around line 428-434: The s3_path method is using an off-by-one offset when
slicing href: because parse_protocol() stores protocol without the colon, the
separator is "://", so change the slice offset in s3_path from
self.protocol.len() + 2 to self.protocol.len() + 3 to skip the full "://";
update the return branch in pub fn s3_path(&self) -> &'a [u8] that currently
uses self.protocol.len() + 2 to use +3 so s3://bucket/key yields "/bucket/key"
instead of starting with "//".
In `@test/cli/install/bun-install-lifecycle-scripts.test.ts`:
- Around line 3861-3867: Move the assertion that checks the process exit code
(the `expect(await exited).toBe(0)` call) to occur immediately after the
stdout/stderr assertions and before any filesystem checks; specifically, in the
test blocks where `exists(join(packageDir, ...))` is asserted (including the
other occurrence near the same install phase), place `expect(await
exited).toBe(0)` before the `exists(...)` assertions so exit failure is reported
before downstream file-state assertions.
In `@test/cli/install/bun-install.test.ts`:
- Line 9415: Replace the overly-broad assertion on the error (the
expect(err).toMatch(/Invalid dependency name|is not a valid install folder
name/)) with an exact check for the extraction-layer refusal message: assert
that the error equals or contains the exact string "Refusing to install package
with invalid name" so the test fails if a different layer or message triggers
the rejection.
In `@test/cli/install/migration/yarn-lock-migration.test.ts`:
- Around line 1509-1521: The test currently awaits each process's exit
(rejectedResult.exited and the other spawn's exited) before consuming their
stdout/stderr, which reduces diagnostic output; update both spawned migrate runs
(references: rejectedResult and the other spawn result) to first read stdout and
stderr via new Response(<result>.stdout).text() and new
Response(<result>.stderr).text() and only then await <result>.exited so that
stdout/stderr are captured and included in assertions or error messages.
In `@test/js/bun/http/serve.test.ts`:
- Around line 1134-1149: The raw-connection promise created via
Promise.withResolvers<void>() used with Bun.connect currently wires the socket
handlers data/end/close to resolve but doesn't handle socket errors; add an
error handler on the socket object passed to Bun.connect that calls the promise
reject (from promise.withResolvers) so any socket "error" event rejects the
promise (use the same reject used alongside resolve) to avoid hanging tests;
look for the Promise.withResolvers and the Bun.connect call and add
socket.error(...) => reject(err) (or equivalent) alongside the existing
data/end/close handlers.
In `@test/js/bun/sqlite/sqlite.test.js`:
- Around line 1857-1860: The test currently collects proc.stdout, proc.stderr
and proc.exited into stdout, stderr and exitCode but only asserts stdout and
exitCode; add an assertion for stderr (e.g. expect(stderr.trim()).toBe("") or
another appropriate expectation) so failures show stderr output like the other
tests; update the block around the Promise.all result and the expect calls
referencing stdout, stderr, exitCode to include this new stderr assertion.
---
Outside diff comments:
In `@src/jsc/bindings/bun-spawn.cpp`:
- Around line 41-45: The current fd-sweep clamps maxfd to 65536 in getMaxFd(),
causing closeRangeLoop()/closeRangeOrLoop() to stop at that exclusive bound and
leave fds >= 65536 open; update getMaxFd() (and any place using the 65536 magic)
to query the process soft limit via getrlimit(RLIMIT_NOFILE) and use that as the
sweep upper bound, falling back to a small conservative default only if
getrlimit fails, and preserve the existing logic that treats end == INT_MAX as
“no end” so end+1 isn't applied when calling
closeRangeOrLoop()/closeRangeLoop(); ensure bun_close_range() failure still
triggers the loop fallback that uses the getrlimit-derived bound.
In `@src/parsers/yaml.rs`:
- Around line 2326-2359: The alias expansion budget is only set once in
Parser::init so parse_stream() reusing the Parser across multiple documents can
run out of budget on later documents; update the start-of-document path (where
parse_stream begins processing each new document) to reset
parser.alias_expansion_budget = Parser::MAX_ALIAS_EXPANSION (and optionally
parser.merge_props_budget = MappingProps::MAX_MERGED_PROPERTIES) so each
document gets a fresh per-document budget; locate the document-start code
invoked by parse_stream and add this reset before parsing the document body.
In `@src/runtime/cli/pack_command.rs`:
- Around line 2252-2274: After re-reading package_name/version you must re-run
the restricted-access scoped-name validation so lifecycle scripts can't remove a
required scope; after the package_name is reassigned (the block that sets
package_name from json.root.get("name") and validates emptiness), check if
access == Access::Restricted and if so validate package_name still has a scope
and otherwise return PackError::RestrictedUnscopedPackage (same logic used in
the first-read path). Place this check immediately after the package_name
re-read/validation and mirror the original scoped-name validation routine used
earlier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21411465-93d5-40f8-a9c4-7960234a2e6d
📒 Files selected for processing (78)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpParser.hsrc/http/HTTPContext.rssrc/http/lib.rssrc/http_jsc/websocket_client.rssrc/install/PackageManager/PackageManagerOptions.rssrc/install/PackageManager/security_scanner.rssrc/install/TarballStream.rssrc/install/extract_tarball.rssrc/install/lockfile.rssrc/install/yarn.rssrc/io/PipeReader.rssrc/js/internal/assert/assertion_error.tssrc/js/internal/sql/shared.tssrc/js/node/_http2_upgrade.tssrc/js/node/_http_client.tssrc/js/node/net.tssrc/jsc/bindings/FormatStackTraceForJS.cppsrc/jsc/bindings/SQLClient.cppsrc/jsc/bindings/bun-spawn.cppsrc/jsc/bindings/sqlite/JSSQLStatement.cppsrc/md/line_analysis.rssrc/md/links.rssrc/md/ref_defs.rssrc/parsers/yaml.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/package_manager_command.rssrc/runtime/cli/repl.rssrc/runtime/crypto/pwhash.rssrc/runtime/node/assert/myers_diff.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/server/RequestContext.rssrc/runtime/socket/Listener.rssrc/runtime/socket/socket_body.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/list_objects.rssrc/sql/mysql/protocol/ColumnDefinition41.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/standalone_graph/StandaloneModuleGraph.rssrc/url/lib.rssrc/uws/lib.rssrc/uws_sys/Response.rssrc/uws_sys/h3.rstest/cli/install/bun-install-lifecycle-scripts.test.tstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install.test.tstest/cli/install/bun-pack.test.tstest/cli/install/bun-pm.test.tstest/cli/install/migration/yarn-lock-migration.test.tstest/js/bun/http/bun-serve-routes.test.tstest/js/bun/http/serve.test.tstest/js/bun/md/md-edge-cases.test.tstest/js/bun/net/socket.test.tstest/js/bun/repl/repl.test.tstest/js/bun/s3/s3-list-encode-overflow.test.tstest/js/bun/s3/s3-list-objects.test.tstest/js/bun/s3/s3-requester-pays.test.tstest/js/bun/sqlite/sqlite.test.jstest/js/bun/util/password.test.tstest/js/bun/yaml/yaml.test.tstest/js/node/assert/assert-typedarray-deepequal.test.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2.test.jstest/js/node/tls/node-tls-server.test.tstest/js/node/watch/fs.watch.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/sql.test.tstest/js/sql/tls-sql.test.tstest/js/web/fetch/fetch.test.tstest/js/web/fetch/response.test.tstest/js/web/websocket/websocket-client-short-read.test.ts
|
The package-binary-size failure on build 58594 is baseline drift, not growth from this PR. The check compares against the latest finished main build at the time it runs: build 58594 compared against main #58579 (commit 3ec6669, PR #31456 "embed browser polyfills zstd-compressed"), which shrank every release binary by ~830 KB (bun-linux-x64: 74,758,936 -> 73,906,968). The sibling builds 58512/58552 passed because they ran before #58579 existed and compared against main #58481 (4ee835b, this branch's merge-base), showing +0.0 KB. Measured against its own merge-base build #58481, this PR's binaries are byte-identical on all linux glibc/android/baseline targets (e.g. bun-linux-x64: 74,758,936 on both) and at most +16 KB on the others (freebsd/musl +16,384 B, windows-x64 +12 KB, darwin-aarch64 +16 B) — far under the 0.5 MB threshold. There is nothing in this PR's compiled code to shrink; merging current main (which contains #31456) into the branch will make the check compare cleanly on the next run. |
| "repository": "https://github.com/elysiajs/elysia", | ||
| "tag": "1.4.28" | ||
| "tag": "1.4.28", | ||
| "skipTests": { |
There was a problem hiding this comment.
Undo these changes. We should not add breaking changes like this that nobody asked for.
There was a problem hiding this comment.
Reverted. The Response constructor and Response.json() accept bodies on 101/103/204/205/304 again exactly as released 1.4 does (new Response("x", { status: 204 }).text() still resolves to "x", .body is unchanged), and the Elysia skipTests entries are gone from test/vendor.json — the vendored Elysia suites pass with no skips. The only remaining piece is on the write side of Bun.serve: for those statuses no message-body bytes are written to the socket (status line and headers are untouched), which is not observable from JS, plus a raw-socket test for it in serve.test.ts. If you'd rather the wire output stay exactly as before too, I can drop that part as well — leaving this thread open for you.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/server/RequestContext.rs`:
- Around line 3563-3566: The short-circuit for status codes 101/103/204/205/304
currently calls do_render_blob(), which eventually invokes render_bytes() and
can write self.blob — causing leaked/stale body bytes; change the no-body path
so it does not call do_render_blob() (or implement a no_body_render variant) and
ensure render_bytes() is not invoked for these status codes (or make
do_render_blob() check response.status_code() and skip writing blob for
101/103/204/205/304), and ensure headers/body reflect a true empty body (e.g.,
no Content-Length/body bytes) to avoid writing stale state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63086dc1-96ae-49b8-b56f-856a1a84032f
📒 Files selected for processing (2)
src/runtime/server/RequestContext.rstest/js/bun/http/serve.test.ts
|
Adjusted per maintainer direction in aecab51: explicit tls/ssl options now map to the require mode only — the connection is aborted if the server declines TLS (postgres 'N' SSLRequest reply / MySQL without CLIENT_SSL) before any credentials are sent. Certificate verification behavior is unchanged from main: it is still only enabled when explicitly requested via ca, rejectUnauthorized, or a verify-* sslmode, and a comment documenting this now sits at the mapping site in src/js/internal/sql/shared.ts. The test option changes that were only needed for verify-by-default were reverted (tls-sql.test.ts getOptions back to |
|
Actionable comments posted: 0 |
|
re: src/install/yarn.rs — yarn.lock migration handling of off-registry tarball URLs Reverted as requested in 7834157: yarn.lock migration once again accepts off-registry tarball URLs without integrity, exactly as on main. The check_remote_tarball_url helper and both of its (new-in-this-PR) call sites are removed, and a short comment now documents that the lenient handling is intentional (yarn v1 lockfiles legitimately contain entries without integrity and migration must not fail closed). The regression test asserting the rejected behavior was removed from yarn-lock-migration.test.ts; migrate.test.ts was never changed on this branch. |
|
re: src/js/node/_http_client.ts — absolute URI in the Removed per the directive in 16589e7: the branch's wire behavior does not match Node exactly, so r7-32 is reverted (_http_client.ts back to main, its test removed from node-http.test.ts). Fixture (raw net server echoing the request line + Host header; client: Node v24.3.0:
Branch debug build (with r7-32):
USE_SYSTEM_BUN (main behavior), for reference:
Since Node sends the absolute form verbatim for the https:// case while the branch rewrites it to path-only, the change fails the parity bar and is removed rather than kept. |
|
re: src/runtime/api/bun/h2_frame_parser.rs — server-side header validation Ran the requested comparison against real Node (v24.3.0, http2.createServer, raw frame client: preface + SETTINGS + HEADERS(+DATA), same HPACK encoding as the in-tree test):
Branch Bun for the cases this PR changed: each new condition (connection-specific fields incl. te != trailers, repeated pseudo/content-length fields, pseudo-header after a regular field) feeds the pre-existing malformed path, which sends RST_STREAM with PROTOCOL_ERROR on that stream only (no GOAWAY) and never dispatches the headers to JS — the same observable behavior as Node above. The in-tree test "http2 server rejects requests carrying connection-specific or repeated pseudo-headers" drives the identical raw-frame client against Bun's server and asserts exactly that (0 delivered, RST code NGHTTP2_PROTOCOL_ERROR, no HEADERS response); I added a On (c): this PR does not add any DATA-vs-content-length mismatch enforcement. The only content-length change is rejecting a repeated content-length header field, which Node also rejects (RST code 1, not delivered). Bun's behavior for a content-length value that disagrees with the DATA length is unchanged by this branch (request still delivered, no reset), so nothing was removed or adjusted there; matching Node's end-of-stream length check would be a separate change. Keeping the validation. node-http2.test.js passes in full with the added case. |
|
re: src/js/node/net.ts — server-side Verified against real Node.js as requested — the r7-33 change matches Node and is kept (no code change in this round). Fixture (Node v24.3.0, branch debug build, system Bun 1.4.0; agent1/ca1 certs from test/js/node/tls/fixtures), server-side socket.authorized:
The branch matches Node on all four cases, including the http2-over-TLS upgrade path; current Bun does not on (a) and (d). This mirrors Node's |
|
Actionable comments posted: 0 |
Jarred-Sumner
force-pushed
the
claude/security-round-7
branch
from
28e34ef to
372f68c
Compare
Jarred-Sumner
force-pushed
the
claude/security-round-7
branch
from
372f68c to
a4d0ca0
Compare
| let mut dir = PathBuffer::uninit(); | ||
| let fd = get_cache_directory(pm); | ||
| let outpath = match bun_sys::get_fd_path(fd, &mut dir) { | ||
| Ok(p) => &p[..], | ||
| Err(err) => { | ||
| Output::pretty_errorln(format_args!( | ||
| "{} getting cache directory", | ||
| bun_core::Error::from(err).name(), | ||
| )); | ||
| Global::crash(); | ||
| } | ||
| }; | ||
| let _ = Output::writer().write_all(outpath); |
There was a problem hiding this comment.
🟡 nit (description text only): the parenthetical "(bun pm cache prints the same path it would delete)" no longer matches the design settled in the resolved thread on this file — bun pm cache intentionally keeps the project-config-honoring get_cache_directory(pm) path while bun pm cache rm uses the process-env-only resolution, so the two can differ when a project-local BUN_INSTALL_CACHE_DIR/.npmrc cache= is set. Consider dropping or rewording the parenthetical (e.g. "bun pm cache continues to print the project-effective cache path").
Extended reasoning...
What this is
This is a documentation-accuracy nit on the PR description text only — the code is correct and intentional. The PR description's pm: bullet reads:
pm:
bun pm cache rmresolves the cache directory from the process environment, ignoring project-local overrides for the destructive delete (bun pm cacheprints the same path it would delete)
The bolded parenthetical is no longer accurate after the review thread on this file was resolved. Per the implementation at src/runtime/cli/package_manager_command.rs:474-486, bun pm cache (display) calls get_cache_directory(pm), which honors project .env / bunfig / .npmrc cache= overrides; while bun pm cache rm (lines 376-409) builds a fresh process-env-only loader and resolves via fetch_cache_directory_path(&mut process_env, None). The author's own resolution of review thread 3315091045 states this divergence is by design:
The print at line ~489 intentionally keeps the project-config-honoring resolution:
bun pm cachereports the cache directory this project's installs actually use … The non-destructive print and the destructive delete answer different questions by design here.
So when a project-local cache override exists, bun pm cache and bun pm cache rm resolve different paths — the parenthetical asserts the opposite.
Step-by-step example
- A project's checked-in
.npmrccontainscache=/tmp/project-cache; the process environment has onlyBUN_INSTALL=/home/user/.bun. bun pm cachereaches line 475, callsget_cache_directory(pm)(which usespm.envwith the project's.npmrcmerged), and prints/tmp/project-cache. This is the behaviortest/cli/install/npmrc.test.ts("should convert to utf8 if BOM") locks in.bun pm cache rmreaches line 378, builds a freshbun_dotenv::Loader, calls onlyload_process(), andfetch_cache_directory_pathreturns/home/user/.bun/install/cache, which is what gets deleted.- The path printed in step 2 is not the path deleted in step 3.
Addressing the "not actionable / author already knows" objection
One verifier argued this is redundant meta-commentary because the author wrote both the description and the resolution, so he's aware of the behavior. That's true — but knowing the code behavior and noticing that one parenthetical in a long, actively-maintained description has gone stale are different things. This PR description is unusually carefully curated: it already has a "Changes tried and reverted during review (rationale)" section that was added as the branch evolved, and the pm: bullet itself was clearly written for an earlier iteration before thread 3315091045 settled on "they answer different questions by design." It's the same class of drift the author has been correcting elsewhere in the description.
This is also not a duplicate of the resolved thread (3314890827 / 3315091045): that thread was about the code divergence and was resolved as by-design; this is about the PR description text not having been updated to reflect that resolution.
Why nothing else catches it
PR description text isn't covered by tests or CI. The new regression tests in test/cli/install/bun-pm.test.ts correctly assert the rm behavior, and npmrc.test.ts covers the display behavior — both pass; only the prose disagrees.
Impact and fix
Nit only — PR description prose doesn't ship. Worth a one-line edit because (a) the description is otherwise being maintained as an accurate change record, and (b) a reader skimming the description for "what changed in bun pm cache" would take away the wrong invariant. Suggested rewording:
pm:
bun pm cache rmresolves the cache directory from the process environment, ignoring project-local overrides for the destructive delete (bun pm cachecontinues to print the project-effective cache path)
or simply drop the parenthetical.
…(round 8) (#31559) Tightens input validation, bounds checking, and pointer/lifetime handling across the package manager, node:http/http2, the debug adapter, the V8 shim, S3, and path/fs internals. Continuation of #31417 and #31495 (rounds 6–7); same structure — small per-area commits with regression tests included. ### Package manager / build - install: validate dependency-alias-derived names at the remaining cache-index call site; validate git/github resolved values when loading `bun.lockb` (same check the text lockfile applies) - compile: bounds-check Mach-O segment offsets/sizes with overflow-checked arithmetic before patching; a corrupted template now errors instead of crashing - webview: the default Chrome profile directory is created with an unpredictable name and owner-only permissions ### node:http / node:http2 / debugger / V8 shim - node:http: a `host` option containing URL-reserved delimiters is treated as an unresolvable hostname instead of being re-parsed into a different target - node:http2: the raw-socket TLS upgrade path forwards the server's `requestCert`/`rejectUnauthorized` configuration into the native TLS config explicitly - debug adapter: only allowlisted inspector event domains from the debug target are dispatched; the diagnostics signal listener binds to loopback - v8 shim: `String::Utf8Length`/`WriteUtf8` saturate at `INT_MAX` instead of wrapping for very large strings ### Runtime / fs / S3 / path - node:fs: the read path re-fetches the destination buffer after argument coercion can run user code; buffer-backed path arguments are pinned for the duration of async operations - S3: a malformed multipart upload ID from the server surfaces as a request error instead of aborting the process - node:path: `path.format` sizes its scratch buffer for the implicitly inserted extension separator, removing a reachable abort ### Not changed The bin-link containment behavior for dotless symlinked targets and the binary-lockfile trust policy are unchanged (both previously decided); the file-backed Blob deserialization finding is already covered by the open #31329. ### Tests 9 validated regression tests (each fails on the released build — two of them by crashing it — and passes here), plus 2 regression-locked tests for repo-TypeScript debugger changes. The Chrome profile-dir and h2-upgrade changes ship without standalone tests (no non-flaky observable in existing files; noted per commit). `cargo check` and the cross-target checks pass on all 10 CI targets; the touched suites pass locally with no new failures versus main. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…ms (round 9) (#31606) Tightens input validation, pointer/lifetime handling, and Node-compatibility across the package manager, bun-lambda, node:zlib/net/http, Glob, the transpiler cache, postgres, and HTMLRewriter. Continuation of #31417, #31495, and #31559; same structure — small per-area commits with regression tests included. ### Scaffolding / Lambda - create: dependency names detected from scanned source files are passed to the spawned installer as positionals (after `--`), so a specifier can never be interpreted as a flag - bun-lambda: event-to-Request conversion builds the URL from the request context's domain; client-supplied path bytes can no longer change the URL authority ### node runtime - node:zlib: native compression handles re-resolve the caller-supplied output array on every write completion instead of caching a raw pointer, and reject detached/resized buffers - node:net: BlockList subnet matching handles IPv4-mapped IPv6 rules the same way Node does (verified against Node's behavior) - node:http: duplicate request headers follow Node's IncomingMessage precedence — first value wins for singleton headers, others are joined, `set-cookie` is arrayed (verified against Node's behavior) - Glob: an oversized `cwd` option throws a catchable error instead of aborting the process ### Runtime internals - transpiler: cached module metadata records with out-of-range string indices are rejected at load (previously a corrupted cache record could crash the process) - postgres: unknown backend messages are skipped by exactly their declared length, and NotificationResponse is consumed through the protocol decoder, so the stream can no longer desynchronize - HTMLRewriter: `comment.replace()` removes the original comment and emits only the replacement (previously both appeared in the output) ### Not changed The bin-link containment behavior for symlinked targets is unchanged (previously decided); the recursive-delete traversal change is already covered by the open #31337. ### Tests 11 regression tests across the touched areas. Most are validated to fail on the released build and pass here — including one where the released build segfaults on a corrupted cache record and one where it hangs on an oversized Glob cwd. The zlib write-state tests are regression-only (the released build does not exhibit the failure deterministically without ASAN). `cargo check` and the cross-target checks pass on all 10 CI targets; the touched suites pass locally with no new failures versus main. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
2 participants
Tightens input validation, bounds checking, protocol framing, and state handling across the package manager, HTTP stack, sockets/TLS, SQL clients, and parsers. Continuation of #31417 (round 6); same structure — small per-area commits with regression tests included.
Package manager / CLI
bun pm cache rmresolves the cache directory from the process environment, ignoring project-local overrides for the destructive delete (bun pm cacheprints the same path it would delete)HTTP / HTTP2 / WebSocket / S3
request.urlderived from Host; no message-body bytes are written for null-body statuses (101/103/204/205/304) — Response construction is unchanged?/#characters end-to-end; list-response parsing is linear; the remote-address accessor copies into caller-owned storageSockets / TLS / SQL / runtime
tls/ssloption aborts the connection before credentials are sent if the server declines TLS; certificate-verification behavior is unchanged. Row construction is bounded by the declared structure; MySQL rebuilds cached row shapes when result columns change; postgres protocol counts are validated against the received bytesauthorized: trueafter a client certificate was requested and verified (matches observed Node behavior)Parsers / shell / node
Tests
33 new regression tests across the touched areas (validated to fail on the released build and pass here), plus a handful of regression-only cases where the released build does not exhibit the behavior deterministically. A few fixes have no standalone test where the change is not observable from JS.
cargo checkand the cross-target checks pass on all 10 CI targets; the touched suites pass locally with no new failures versus main. The branch is rebased on current main.Changes tried and reverted during review (rationale)
package.jsoninstead of the deserialized lockfile data; reverted becausebun.lockbintentionally stores and uses the script text, and that design stays.file:, codeload) and migration must not fail closed. The lenient handling is now documented with a comment at the site.Responseand null-body statuses — both the body-dropping and the later spec-styleTypeErrorfor non-null bodies with 101/103/204/205/304 were reverted: existing code and frameworks (including Elysia) construct such responses and expect the body to be ignored, so changing construction is a breaking change nobody asked for. Only the wire-level behavior (no body bytes emitted) is kept.tls/ssloption imply certificate verification; narrowed to abort-on-decline only because the unverified default for these options is intentional and matches other JavaScript database clients.pathwith an absolute URI — an earlier commit rewrote such requests to a path-only target; removed because measured Node behavior does not do that, and Node compatibility takes precedence.