Repository navigation
Conversation
RedisClient accepts Blob for any KeyLike parameter, but the argument
converter read the Blob's in-memory view via shared_view(), which is
empty for a file- or S3-backed store. So
await redis.set("k", Bun.file("page.html"))
sent SET "k" "" on the wire, resolved "OK", and silently dropped the
file contents. Same for a Bun.file used as a key (became the empty key),
a single-part new Blob([Bun.file(p)]) (shares the file store), and a
nonexistent path (no ENOENT).
The converter now checks needs_to_read_file() / is_s3() on the Blob arm
and throws ERR_INVALID_ARG_TYPE telling the user to pass
await blob.bytes(), matching ServerWebSocket.send and the MySQL binder.
|
Status: fix verified locally, waiting on CI. Reproduced with the inline RESP3 recorder against stock bun (sends |
WalkthroughChangesValkey Blob validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
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 `@test/js/valkey/unit/buffer-operations.test.ts`:
- Around line 209-224: Extend the Blob conversion tests around the existing
file-backed cases to assert ERR_INVALID_ARG_TYPE for both file-backed and
S3-backed Blobs. Add a hermetic Bun.S3Client.file("some-key") case, verify the
thrown error code for each storage type, and retain the existing no-frame
assertion.
🪄 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: 24b6ee62-aa67-456a-9dc5-c1c87a84add8
📒 Files selected for processing (2)
src/runtime/valkey_jsc/js_valkey_functions.rstest/js/valkey/unit/buffer-operations.test.ts
There was a problem hiding this comment.
LGTM — small, focused input-validation guard following the existing ServerWebSocket pattern.
What was reviewed:
- Verified
from_jsis the single shared converter for all prototype methods and.send(), so the guard covers every argument position. - Confirmed
throw_invalid_arguments→JscError::INVALID_ARG_TYPE→ERR_INVALID_ARG_TYPE, matching the test assertion. - Checked
BlobOrStringOrBuffer::slice()callsshared_view()on the Blob arm, confirming the empty-string bug the guard prevents. - Tests are hermetic (local RESP3 recorder on port 0, no real server), assert both the throw and that no frame reaches the wire, and include a regression guard for in-memory Blobs.
Extended reasoning...
Overview
Adds a 6-line guard in js_valkey_functions.rs::from_js that rejects file- and S3-backed Blob arguments to RedisClient with ERR_INVALID_ARG_TYPE, instead of letting them fall through to shared_view() (which is empty for those stores) and serialize as a zero-length bulk string. Adds two dockerless tests in buffer-operations.test.ts using an in-process RESP3 recorder.
Security risks
None. This is a validation guard that fails closed — it turns silent data loss into a synchronous throw. No new parsing of untrusted input.
Level of scrutiny
Low-medium. The Rust change is a small, additive guard on a single helper, following the exact pattern already established in ServerWebSocket.rs:208-211. I traced:
BlobOrStringOrBuffer::Blob(Box<Blob>)is the matched variant;needs_to_read_file()/is_s3()are inherentBlobmethods checkingstore::Data::File/store::Data::S3.throw_invalid_arguments→to_invalid_arguments→JscError::INVALID_ARG_TYPE.fmt(), so the test'scode: "ERR_INVALID_ARG_TYPE"assertion holds.slice()on the Blob arm callsblob.shared_view(), confirming the root cause.- The throw is synchronous (before
send_cmd), soexpect(() => ...).toThrow()is the right shape.
Other factors
Tests are well-constructed: hermetic local Bun.listen({port: 0}) recorder, tempDir from harness, resources released in finally before assertions, autoReconnect: false. The negative test asserts the exact error code and message and proves no wire frame was produced (wire equals only the trailing sentinel SET after ok). The positive test guards against regressing in-memory / sliced Blobs. S3 case constructs a lazy S3Client#file() — no network, throw happens in argument conversion. All prior review threads (comment-cop on the explanatory comment, coderabbit on S3 coverage) are resolved by follow-up commits.
What does this PR do?
RedisClientacceptsBlobfor anyKeyLikeparameter (key or value). The argument converter read the Blob's bytes viashared_view(), which is empty for a file- or S3-backed store. So:sent
SET "tpl" ""on the wire, resolved"OK", and silently dropped the file contents. The same applied to:Bun.file()as a key (became the empty key"")new Blob([Bun.file(p)])(shares the file store)ENOENT, just"")Wire-verified on stock 1.4.0-canary and release-asan main; deterministic.
Cause
js_valkey_functions.rs::from_jscallsBlobOrStringOrBuffer::from_js_maybe_file(global, value, false). Thefalseskips the file-blob guard infrom_js_maybe_file_maybe_async, so a file-backed Blob falls through toSelf::Blob(blob.dupe())andslice()returns its emptyshared_view(). (Theallow_fileparameter there is named the opposite of its effect; left alone to keep the diff small.)Fix
After conversion, check
needs_to_read_file() || is_s3()on the Blob arm and throwERR_INVALID_ARG_TYPEwith "pass await blob.bytes() instead", matchingServerWebSocket.sendand the MySQL parameter binder. All ~50 prototype methods and.send()route through this one helper, so the check covers every argument position.Not changed
A
Uint8Arrayover a detachedArrayBufferstill serializes as"". Per spec a detached view hasbyteLength === 0, and Node treats it as empty in comparable APIs, so sending zero bytes is arguably correct; happy to reject it too if preferred.How did you verify your code works?
Added two dockerless tests to
test/js/valkey/unit/buffer-operations.test.tsthat stand up a minimal RESP3 recorder:Bun.file()as value, as key, wrapped, after.text(), nonexistent path, and viagetBuffer/appendall throw and produce no wire frame[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file