Skip to content

Conversation

ShejanMahamud
Copy link

@ShejanMahamud ShejanMahamud commented Oct 2, 2025

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 of validator.cookie?.schema?.noValidate. This caused:

  • Optional cookies or cookies with noValidate: true to fail validation.
  • Potential security issues or runtime errors.

Fix:

} else if (validator.cookie?.schema?.noValidate !== true) {
    fnLiteral +=
        `if(validator.cookie.Check(cookieValue)===false){` +
        validation.validate('cookie', 'cookieValue') +
        '}'

2. Cookie Config Merge Bug

Location: src/compose.ts - Lines 1495-1498

Problem:
Cookie config was being merged with itself, preventing application-level cookie configs (app.config.cookie) from being applied.

Fix:

validator.cookie.config = mergeCookie(
    validator.cookie.config,
    app.config.cookie ?? {}
)

3. AfterHandle Loop Optimization

Location: src/compose.ts - Lines 1617-1624

Problem:
c.response = be was being set inside the loop repeatedly, causing redundant assignments and minor performance issues.

Fix:
Moved the assignment outside the loop:

if (hooks.afterHandle?.length) {
    fnLiteral += `c.response = be\n`  // Set once before loop
    for (let i = 0; i < hooks.afterHandle.length; i++) {
        // ...hook handling
    }
}

4. Error Handler Promise Bug

Location: src/compose.ts - Line 1939

Problem:
The wrong variable was awaited (er instead of mep), potentially leaving promises unresolved in async error handlers.

Fix:

if(mep instanceof Promise) mep = await mep

Why These Changes Matter

  • Cookie validation: Ensures correct behavior for optional cookies and noValidate flags.
  • Config merge: Properly applies app-level cookie settings.
  • AfterHandle loop: Improves code efficiency and readability.
  • Error handling: Ensures async hooks are correctly awaited, preventing race conditions.

Testing Recommendations

  • Validate cookies with optional fields and noValidate flags.
  • Check application-level cookie config overrides.
  • Test afterHandle hooks for correct response assignment.
  • Test error handlers for async hooks and promise resolution.

Files Changed

  • src/compose.ts – Main fixes
  • test/cookie/validation.test.ts – Added/updated tests for cookie validation

Backward Compatibility

All changes are fully backward compatible. They improve reliability without changing the public API.


Related Issues

  • Incorrect cookie validation
  • Missing application-level cookie config
  • Performance inefficiency in afterHandle hooks
  • Async error handling issues

Summary by CodeRabbit

  • Tests

    • Added a comprehensive cookie validation test suite covering required/optional cookies, multi-cookie scenarios, type-specific validation (number, boolean, string, object, array, union), constraints (length, range, format), nested schemas, transforms (encode/decode), optional/empty behaviors, and app-level config merges. Verifies correct HTTP statuses and response bodies.
  • Chores

    • Internal import and formatting cleanup, minor internal error/cookie handling tweaks; no public API or runtime behavior changes.

Copy link

coderabbitai bot commented Oct 2, 2025

Walkthrough

Updated imports and formatting in src/compose.ts and added comprehensive cookie validation tests in test/cookie/validation.test.ts, covering parsing, schema validation, transforms, optional/required behavior, and expected HTTP responses.

Changes

Cohort / File(s) Summary
Compose module import/formatting adjustments
src/compose.ts
Adjusted imports (added local ParseError, status, isBun, expanded utils imports, parseCookie/CookieOptions usage, changed Value import path), reordered/cleaned code and minor control-flow/format tweaks; no public API changes.
Cookie validation test suite
test/cookie/validation.test.ts
Added extensive tests for cookie parsing and validation: required vs optional, numeric/boolean/string/object/array/union types, constraints (length/range/format), transforms (encode/decode), nested schemas, guarded/merged configs, and expected 200/422 responses.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I nibbled through imports, tidy and neat,
Then hopped to cookies, a crunchy treat.
Schemas checked, transforms set just right,
Responses warm in the soft moonlight.
Tests sing true — the burrow bakes tonight. 🐇🍪

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and specifically summarizes the primary changes by naming the key fixes—cookie validation, configuration merging, and error handler promise—in the relevant file, making it concise and directly related to the pull request’s content.
Linked Issues Check ✅ Passed The pull request implements all coding requirements from issue #1451 by correcting the noValidate check to use validator.cookie, merging validator.cookie.config with app.config.cookie via mergeCookie, optimizing the afterHandle loop to avoid redundant assignments, and fixing the error handler to await the proper promise, in addition to adding comprehensive cookie validation tests.
Out of Scope Changes Check ✅ Passed All modifications in src/compose.ts and the new validation tests strictly address the linked issue objectives around cookie validation, configuration merging, afterHandle performance, and error handling, with no unrelated or extraneous changes detected.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a06dc2f and 840e6e2.

📒 Files selected for processing (1)
  • src/compose.ts (23 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/compose.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ShejanMahamud ShejanMahamud changed the title Multiple Issues in compose.ts (Cookie Validation, Config, Handlers) Fix cookie validation, config merge, and error handler promise in compose.ts Oct 2, 2025
@ShejanMahamud
Copy link
Author

@coderabbitai review

Copy link

coderabbitai bot commented Oct 7, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@SaltyAom
Copy link
Member

Due to merge conflicts and lots of file changes, I have created a separate commit here with your test case
3cb3b18

@SaltyAom SaltyAom closed this Oct 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cookie validation and handler issues in Elysia.js

2 participants