-
-
Notifications
You must be signed in to change notification settings - Fork 361
Fix cookie validation, config merge, and error handler promise in compose.ts #1452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix cookie validation, config merge, and error handler promise in compose.ts #1452
Conversation
WalkthroughUpdated imports and formatting in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Compose as composeHandler
participant CookieUtil as parseCookie/validate
participant Handler as RouteHandler
participant Response
Client->>Compose: HTTP request (with cookies)
Compose->>CookieUtil: parseCookie(rawCookies)
alt cookie schema present
CookieUtil->>CookieUtil: decode/transform & validate per schema
opt validation fails
CookieUtil-->>Compose: validation error (422)
Compose->>Response: send 422 error
Response-->>Client: 422
end
opt validation succeeds
CookieUtil-->>Compose: parsed & transformed cookie values
Compose->>Handler: invoke route handler with cookies
Handler-->>Compose: handler result
Compose->>Response: format response (apply encodings/cookies)
Response-->>Client: 200
end
else no cookie schema
Compose->>Handler: invoke route handler
Handler-->>Compose: handler result
Compose->>Response: send response
Response-->>Client: 200
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 review |
✅ Actions performedReview triggered.
|
Due to merge conflicts and lots of file changes, I have created a separate commit here with your test case |
This PR fixes #1451 several issues in
compose.ts
affecting cookie validation, cookie configuration merging, handler performance, and error handling. These bugs could lead to incorrect cookie validation, misapplied configurations, inefficient handler code, or unresolved promises in error handling.Issues Fixed
1. Cookie Validation Bug
Location:
src/compose.ts
- Lines 1493-1535 (Cookie Validator Section)Problem:
Cookie validation incorrectly checked
validator.body?.schema?.noValidate
instead ofvalidator.cookie?.schema?.noValidate
. This caused:noValidate: true
to fail validation.Fix:
2. Cookie Config Merge Bug
Location:
src/compose.ts
- Lines 1495-1498Problem:
Cookie config was being merged with itself, preventing application-level cookie configs (
app.config.cookie
) from being applied.Fix:
3. AfterHandle Loop Optimization
Location:
src/compose.ts
- Lines 1617-1624Problem:
c.response = be
was being set inside the loop repeatedly, causing redundant assignments and minor performance issues.Fix:
Moved the assignment outside the loop:
4. Error Handler Promise Bug
Location:
src/compose.ts
- Line 1939Problem:
The wrong variable was awaited (
er
instead ofmep
), potentially leaving promises unresolved in async error handlers.Fix:
Why These Changes Matter
noValidate
flags.Testing Recommendations
noValidate
flags.Files Changed
src/compose.ts
– Main fixestest/cookie/validation.test.ts
– Added/updated tests for cookie validationBackward Compatibility
All changes are fully backward compatible. They improve reliability without changing the public API.
Related Issues
Summary by CodeRabbit
Tests
Chores