-
Notifications
You must be signed in to change notification settings - Fork 259
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
Support audience
option in authenticate request
#1004
Conversation
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.
✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆
packages/backend/src/tokens/jwt.ts
Outdated
@@ -148,31 +152,39 @@ export async function verifyJwt( | |||
} | |||
|
|||
// Verify audience claim (aud) | |||
if (typeof aud === 'string') { | |||
if (aud !== audience) { | |||
const audiences = audience && Array.isArray(audience) ? audience : [audience]; |
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.
@dimkl we can simplify the checks by wrapping the audience claim (string or string[]) and then do audiences = [audience].flat().filter(aud => !!aud)
. That way, the following checks will need to assert only on array emptiness.
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.
Please review the assertions.test.ts
to verify the expected behaviour of our verification audience implementation.
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.
@SokratisVidros are we okay with the tests and the expected behaviour? If so, i will proceed with merging this.
bc0c178
to
c72a9f5
Compare
edda3c4
to
c4d7ad0
Compare
@dimkl Is this ready for a final review? |
Yes. A rebase is required before merge but the implementation is finalized. |
b1e0da4
to
2a8b4e9
Compare
4d6a00d
to
aa1ab98
Compare
942c258
to
84f0ea2
Compare
265d940
to
5042091
Compare
🦋 Changeset detectedLatest commit: ce2e061 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Also there was some changes in the code: - aud claim with empty values does NOT throw verification error
d71e341
to
6afa5ec
Compare
This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Type of change
Packages affected
@clerk/clerk-js
@clerk/clerk-react
@clerk/nextjs
@clerk/remix
@clerk/types
@clerk/themes
@clerk/localizations
@clerk/clerk-expo
@clerk/backend
@clerk/clerk-sdk-node
@clerk/shared
@clerk/fastify
gatsby-plugin-clerk
build/tooling/chore
Description
npm test
runs as expected.npm run build
runs as expected.manual testing
unit tests