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: validate authorization schema #167

Merged
merged 2 commits into from
Dec 27, 2023

Conversation

dancastillo
Copy link
Member

Checklist

This PR fixes issue where any string with same length as bearer passes validation ex: AAAAAA auth_key.
fixes #164

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Minor things

lib/verifyBearerAuthFactory.js Outdated Show resolved Hide resolved
lib/verifyBearerAuthFactory.js Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

I dont like it for the reason that it is doing over and over unnecessary string operations.

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 27, 2023

@mcollina

Should we use @fastify/error to create FST_BEARER_AUTH_MISSING_AUTHORIZATION_HEADER and FST_BEARER_AUTH_INVALID_AUTHORIZATION_HEADER and use that instead? Would also be better passing them to the callback instead of native Node Errors?

@mcollina
Copy link
Member

  1. yes
  2. as you prefer, what is there now is ok.

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 27, 2023

I am ok with this. I would probably create a follow up PR in the few days to straighten this up.

@mcollina mcollina merged commit 25edf58 into fastify:master Dec 27, 2023
19 checks passed
@olivierchatry
Copy link

Is verifying authtype suppose to be case sensitive ? I'm think it should not be. Found this : lexik/LexikJWTAuthenticationBundle#411

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.

Authorization scheme not validated
6 participants