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

Ignore authorization headers if not Bearer #325

Merged
merged 6 commits into from
Jan 7, 2024

Conversation

cberescu
Copy link
Contributor

@cberescu cberescu commented Jan 5, 2024

Fixes: #318

Checklist

Screenshot 2024-01-05 155257

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

@mcollina
Copy link
Member

mcollina commented Jan 5, 2024

I'm a bit conflicted if this is major or not. Anybody has an opinion?

test/jwt.test.js Outdated Show resolved Hide resolved
test/jwt.test.js Outdated Show resolved Hide resolved
@climba03003
Copy link
Member

I'm a bit conflicted if this is major or not. Anybody has an opinion?

Although it stills throw Error, but changing the return from 400 to 401 status code is absolutely a major.

jwt.js Outdated Show resolved Hide resolved
Co-authored-by: KaKa <[email protected]>
Signed-off-by: cberescu <[email protected]>
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

Co-authored-by: Gürgün Dayıoğlu <[email protected]>
Signed-off-by: cberescu <[email protected]>
@cberescu cberescu requested a review from gurgunday January 6, 2024 12:33
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

looks good to me

jwt.js Outdated Show resolved Hide resolved
@cberescu
Copy link
Contributor Author

cberescu commented Jan 6, 2024

@gurgunday not sure why one of the tests failed... any ideas ?

@gurgunday gurgunday requested a review from mcollina January 7, 2024 09:47
@gurgunday
Copy link
Member

@gurgunday not sure why one of the tests failed... any ideas ?

It was a CI issue, fixed now

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

@mcollina mcollina merged commit 3bab18e into fastify:master Jan 7, 2024
19 checks passed
@willpoorman
Copy link

willpoorman commented Aug 19, 2024

Hey guys. Appreciate the work guys have done with this package. Could you potentially update the changelog for the 8.0.0 to clarify what the exact breaking change is and what users need to adjust when upgrading? https://github.com/fastify/fastify-jwt/releases/tag/v8.0.0. I had to dig in to find this PR to read the comments to find out what was considered BREAKING about it: #325 (comment)

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.

If authorization is not of type Bearer to ignore it and check the cookie
6 participants