feat(event-handler): add CORS middleware support #4477
Conversation
- Add comprehensive CORS middleware with configurable options - Support string, array, and function-based origin validation - Handle OPTIONS preflight requests with 204 status - Add CORS headers to regular responses - Include comprehensive test suite (26 tests) - Follow Python implementation defaults for consistency - Support global and route-specific middleware usage Signed-off-by: Daniel ABIB <daniabib@amazon.com>
b511608 to
2a9fd4c
Compare
packages/event-handler/tests/unit/rest/Router/cors-integration.test.ts
Outdated
Show resolved
Hide resolved
packages/event-handler/tests/unit/rest/Router/cors-integration.test.ts
Outdated
Show resolved
Hide resolved
packages/event-handler/tests/unit/rest/Router/cors-integration.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank you for the PR.
There are still a few gaps to address before we can merge. In particular:
- The module’s execution flow and middleware order need to be aligned with our conventions (e.g., destructuring of
reqCtx, middleware ordering, etc.). - Please run the linter and fix notices/warnings/errors. Import order is unsorted in several files.
- Please follow project conventions and place constants, types, and modules in appropriate files; follow test style and conventions
- Remove the example files from this PR; we’ll add examples in a separate documentation-focused PR.
Meta/process note:
I’ve noticed several recent PRs landing with red CI (linting and tests). To keep reviews efficient, please ensure before requesting review that:
- All tests pass locally and in CI.
- Linting is clean (no warnings/errors).
- Pre-commit hooks run successfully.
If CI is red, we’ll pause reviews until it’s green. If this remains an issue across PRs, we may close the PR and handle the changes internally to keep momentum.
If you’re running into setup issues, let us know - we’re happy to help you get the local environment/hooks configured.
|
Hi @dcabib, we would like to merge this today since we are on a tight schedule and we have other PRs in the pipeline that will impact areas of the code changed in this one. Let us know if you're able to address the feedback or have questions. Thank you! |
- Moved CorsOptions type to types/rest.ts - Moved DEFAULT_CORS_OPTIONS to constants.ts - Fixed middleware to mutate existing headers - Updated tests with Prepare/Act/Assess pattern - Removed example files (will add in docs PR) - Fixed all linting issues - Converted functions to arrow functions
|
Hey @svozza @dreamorosi! 👋 Just pushed all the requested changes. Man, you guys caught some good stuff - totally missed that I was creating new Headers instead of mutating the existing ones 🤦♂️ Here's what I fixed:
All 26 CORS tests are passing! The other test failures are from that missing testing-utils dependency that was already broken on main. Build works great, both ESM and CJS are happy. Should be good to merge now! Let me know if you spot anything else - ready to fix whatever's needed to get this in today 🚀 |
|
Hi @dcabib, there were some feedbacks r ED4F emaining and some missing test coverage which I've now addressed due to our schedule. |
There was a problem hiding this comment.
Left a couple of final comments, I'll leave the actual approval to @svozza.
Besides these two comments:
- make sure we're checking not only default
corsoptions in the tests (if I've missed it, ignore this comment) - update PR body to reflect the final spec (not everything mentioned in the PR body is present in the final version)
Left a general comment since all previous changes I requested were addressed. Will leave final review to other maintainer.
|
Description
This PR implements CORS middleware support for the Event Handler as requested in issue #4446.
Implementation
Configuration Options
The middleware supports all standard CORS options:
origin: string | string[]methods: string[]allowedHeaders: string[]exposedHeaders: string[]credentials: booleanmaxAge: numberUsage
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Closes #4446