Skip to content
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

Merged
merged 5 commits into from
Oct 26, 2024

Conversation

SeokHoChoi
Copy link
Contributor

@SeokHoChoi SeokHoChoi commented Aug 25, 2024

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

  • Updated 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!

- 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.
if (!authHeader) {
throw Error(`Authorization header required`);
if (!authHeader && !authCookie) {
throw Error(`Authorization header or cookie required`);
Copy link
Owner

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/

Copy link
Contributor Author

@SeokHoChoi SeokHoChoi Oct 18, 2024

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/

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 SeokHoChoi requested a review from cdimascio October 20, 2024 12:19
@cdimascio
Copy link
Owner

@SeokHoChoi there are two tests that are failing, can you have a look and resolve. once, working, i'll get this merged in

  431 passing (6s)
  4 pending
  2 failing

  1) security.defaults
       should skip validation, even if auth header is missing for basic auth:

      AssertionError: expected 'Cannot read properties of undefined (…' to equal 'Authorization header required'
      + expected - actual

      -Cannot read properties of undefined (reading 'includes')
      +Authorization header required
      
      at /home/runner/work/express-openapi-validator/express-openapi-validator/test/security.defaults.spec.ts:43:17
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  2) security.handlers
       should return 401 if auth header is missing for basic auth:
     AssertionError: expected 'Cannot read properties of undefined (…' to include 'Authorization'
      at /home/runner/work/express-openapi-validator/express-openapi-validator/test/security.handlers.spec.ts:204:43
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      ```

- 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
@SeokHoChoi
Copy link
Contributor Author

@SeokHoChoi there are two tests that are failing, can you have a look and resolve. once, working, i'll get this merged in

  431 passing (6s)
  4 pending
  2 failing

  1) security.defaults
       should skip validation, even if auth header is missing for basic auth:

      AssertionError: expected 'Cannot read properties of undefined (…' to equal 'Authorization header required'
      + expected - actual

      -Cannot read properties of undefined (reading 'includes')
      +Authorization header required
      
      at /home/runner/work/express-openapi-validator/express-openapi-validator/test/security.defaults.spec.ts:43:17
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

  2) security.handlers
       should return 401 if auth header is missing for basic auth:
     AssertionError: expected 'Cannot read properties of undefined (…' to include 'Authorization'
      at /home/runner/work/express-openapi-validator/express-openapi-validator/test/security.handlers.spec.ts:204:43
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      ```

Thank you for pointing out the failing tests! I've updated the code to fix the issues.

스크린샷 2024-10-26 오후 3 54 17

I've restructured the validation logic to handle both Bearer and Basic authentication properly:

  1. For Basic auth, I now check for the existence of the authorization header first before attempting to access its properties
  2. For Bearer auth, I've maintained the existing behavior while adding cookie support when specified

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!

@SeokHoChoi SeokHoChoi requested a review from cdimascio October 26, 2024 07:00
Copy link
Owner

@cdimascio cdimascio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@cdimascio cdimascio merged commit 00d070b into cdimascio:master Oct 26, 2024
5 checks passed
aloisklink added a commit to aloisklink/express-openapi-validator that referenced this pull request Oct 28, 2024
[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
cdimascio pushed a commit that referenced this pull request Oct 30, 2024
[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
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.

2 participants