-
-
Notifications
You must be signed in to change notification settings - Fork 211
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: add cookie support for HTTP bearer authentication #949
fix: add cookie support for HTTP bearer authentication #949
Conversation
- Updated validateHttp() to handle bearer tokens in both authorization header and cookies. - Adapted logic to ensure flexibility for projects using HTTP-only cookies instead of headers for authentication.
src/middlewares/openapi.security.ts
Outdated
if (!authHeader) { | ||
throw Error(`Authorization header required`); | ||
if (!authHeader && !authCookie) { | ||
throw Error(`Authorization header or cookie required`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks so much for the CR!
i'm requesting a small update here.
Please retain the existing error message when !authHeader
(and the securitySchemw does not specify in: cookie
. When in: cookie
is specified by the securityScheme throw Cookie authentication required.
ref: https://swagger.io/docs/specification/authentication/cookie-authentication/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks so much for the CR! i'm requesting a small update here. Please retain the existing error message when
!authHeader
(and the securitySchemw does not specifyin: cookie
. Whenin: cookie
is specified by the securityScheme throwCookie authentication required.
ref: https://swagger.io/docs/specification/authentication/cookie-authentication/
Thanks for the detailed code review.!!
Implemented the requested changes:
- Updated auth validation in AuthValidator class
- Added checks for cookie-based authentication
- Refined error handling for header and cookie auth
- Updated error messages as suggested
PR updated and ready for another look. Let me know if anything else needs adjustment.
- Maintain existing error for missing Authorization header - Add specific error for cookie authentication when specified in security scheme - Consider both Authorization header and cookie for bearer token validation
@SeokHoChoi there are two tests that are failing, can you have a look and resolve. once, working, i'll get this merged in
|
- Restructure Basic auth validation to check header existence first - Maintain original error messages for non-cookie authentication - Add proper cookie authentication check when specified - Fix undefined.includes() error in Basic auth validation
Thank you for pointing out the failing tests! I've updated the code to fix the issues. I've restructured the validation logic to handle both Bearer and Basic authentication properly:
The changes should now pass all tests while maintaining the expected error messages. Please let me know if you'd like me to make any additional adjustments! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
[express-openapi-validator v5.8.3][1] and 00d070b (fix: add cookie support for HTTP bearer authentication (cdimascio#949), 2024-10-27) breaks HTTP bearer authentication when the `cookie-parser` middleware is not present (and therefore `req.cookies` is not present). [1]: https://github.com/cdimascio/express-openapi-validator/releases/tag/v5.3.8 Fixes: 00d070b
[express-openapi-validator v5.8.3][1] and 00d070b (fix: add cookie support for HTTP bearer authentication (#949), 2024-10-27) breaks HTTP bearer authentication when the `cookie-parser` middleware is not present (and therefore `req.cookies` is not present). [1]: https://github.com/cdimascio/express-openapi-validator/releases/tag/v5.3.8 Fixes: 00d070b
Overview
In my project, we use HTTP-only cookies exclusively for sending authentication tokens, and the Authorization header is not required. I encountered issues with the current
validateHttp()
implementation because it only validates tokens in the Authorization header. To address this, I made a minor adjustment to check for tokens in both the Authorization header and cookies.Changes
validateHttp()
to look for bearer tokens in cookies, in addition to the Authorization header.Reason for Change
Typically, HTTP authentication relies on the Authorization header, as token-based authentication often uses headers according to REST API standards. However, when using HTTP-only cookies—especially in setups that employ CSRF protection—this is a common approach for managing tokens.
I’ve made these changes to provide more flexibility for projects that use cookies for authentication. I wanted to propose this adjustment and get feedback on whether this approach aligns with broader use cases or if there are other considerations.
I didn't even touch the associated test code for quick post-application feedback!
Let me know your thoughts on this modification!