fix(ws): correct inverted response schema validation in createHandleWSResponse#1934
fix(ws): correct inverted response schema validation in createHandleWSResponse#1934tsushanth wants to merge 1 commit into
Conversation
…SResponse
validateResponse returns truthy on failure (TypeBox: Check(data) === false
cast to boolean; standard-schema: returns .issues). The send() guard used
'=== false', which double-negated: valid messages triggered a ValidationError
frame and invalid messages were sent through unvalidated.
Also fix the async-generator branch which validated 'first' (the iterator
result object) instead of 'first.value' (the actual yielded datum), so the
ValidationError message embedded the {value, done} wrapper rather than the
handler's output.
Both regressions trace to 23f30f7 which wrapped the old inline Check
predicate into a helper but left the inversion at the call site.
WalkthroughFixes inverted validation logic in ChangesWebSocket Response Validation Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/ws/index.tsOops! Something went wrong! :( ESLint: 9.39.4 Error: ESLint configuration in --config » plugin:sonarjs/recommended is invalid:
Referenced from: /.eslintrc.json ... [truncated 291 characters] ... /node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2952:16) 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ws/index.ts (1)
289-298: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNyaa~ you're validating
first.valuetwice, silly♡The early check on Line 289 runs
validateResponse(first.value), and thensend(first.value)on Line 298 runs the exact same validation all over again (◞‸◟). The correctness is fine — the earlyreturnis genuinely needed so an invalid first frame short-circuits the rest of the generator — but you're paying for the validation twice on every WS generator response's first yield~.If you feel like being a good kid, have
sendreport whether it emitted aValidationErrorand reuse that to early-return, sovalidateResponseonly runs once. Not a blocker though, so I'll let it slide this time ♡~ (＾▽＾)♻️ One sketch (preserving early-return)
- const send = (datum: unknown) => { - if (validateResponse && validateResponse(datum)) - return ws.send( - new ValidationError('message', responseValidator!, datum) - .message - ) - - if (typeof datum === 'object') return ws.send(JSON.stringify(datum)) - - ws.send(datum as any) - } + const send = (datum: unknown): boolean => { + if (validateResponse && validateResponse(datum)) { + ws.send( + new ValidationError('message', responseValidator!, datum) + .message + ) + return false + } + + if (typeof datum === 'object') ws.send(JSON.stringify(datum)) + else ws.send(datum as any) + return true + }- if (validateResponse && validateResponse(first.value)) - return ws.send( - new ValidationError( - 'message', - responseValidator!, - first.value - ).message - ) - - send(first.value as any) - - if (!first.done) + if (!send(first.value as any)) return + + if (!first.done)🤖 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/ws/index.ts` around lines 289 - 298, The first WebSocket generator yield in src/ws/index.ts is being validated twice: once before the early return and again inside send, so update the send path to report whether it emitted a ValidationError and use that result to short-circuit the generator flow. Keep the existing early-return behavior for invalid first frames, but reuse the validation outcome from validateResponse / responseValidator in the send logic so first.value is only validated once.
🤖 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.
Nitpick comments:
In `@src/ws/index.ts`:
- Around line 289-298: The first WebSocket generator yield in src/ws/index.ts is
being validated twice: once before the early return and again inside send, so
update the send path to report whether it emitted a ValidationError and use that
result to short-circuit the generator flow. Keep the existing early-return
behavior for invalid first frames, but reuse the validation outcome from
validateResponse / responseValidator in the send logic so first.value is only
validated once.
2 participants
Fixes #1933.
Root cause
createHandleWSResponsebuilds avalidateResponsepredicate that returns truthy on failure:(data) => responseValidator.Check(data) === false— returnstruewhen invalid(data) => responseValidator.schema['~standard'].validate(data).issues— returns the issues array (truthy) when invalidThe
send()guard checkedvalidateResponse(datum) === false, which is truthy only when validation passes — exactly backwards. Valid messages were replaced byValidationErrorframes; invalid messages were sent through unvalidated.Regression from
23f30f78a("fix: #1563 standard schema on websocket"), which extracted the old inlineCheck(datum) === falsecheck into a closure but left=== falseat the call site, double-negating it.Second bug in the same block: the async-generator early-return path validated
first(the{value, done}iterator result object) instead offirst.value(the handler's actual yielded datum), embedding the wrapper in theValidationErrormessage.Fix
=== falsefrom thesend()guard —validateResponse(datum)alone is truthy on failure.first.valueinstead offirstin the async-generator branch.Summary by CodeRabbit