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

Validate the aud claim of JwtAccessToken #38

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

timhallmann
Copy link
Contributor

No description provided.

@timhallmann
Copy link
Contributor Author

Wait a second... aud is still optional, meaning client_id="example_client" and aud=None validates successfully. Even if the issuer is already validated, that shouldn't happen.

@timhallmann timhallmann force-pushed the validate_aud_of_jwt_access_token branch from 0ea65d5 to c6700b9 Compare March 4, 2025 13:07
@lilioid
Copy link
Member

lilioid commented Mar 10, 2025

Is there a reason why you implemented client_id as an optional parameter?

The aud description says the following after all:

Each principal intended to process the JWT MUST identify itself with a value in the audience claim. If the principal processing the claim does not identify itself with a value in the 'aud' claim when this claim is present, then the JWT MUST be rejected.

Which I would interpret to mean that a client_id must always be given and must match (with the exception that it can be ignored if no aud claim is present in the token)

@timhallmann
Copy link
Contributor Author

timhallmann commented Mar 10, 2025

There is a reason, and I should have added a comment: I wanted to keep it backwards compatible. By itself, it doesn't seem important enough to break the interface.

Though... what is your policy regarding breaking changes?

@lilioid
Copy link
Member

lilioid commented Mar 10, 2025

There is a reason, and I should have added a comment: I wanted to keep it backwards compatible.

That's kind of what I thought ^^

Though... what is your policy regarding breaking changes?

I am pretty much of the opinion that you shouldn't be scared of increasing major versions. If something breaks the interface, the version goes up.
I'd rather have a correct implementation than a backlog of we'll have to remember to change this when eventually we do bump the major version stuff.

So I'd say make the attribute mandatory and then I'll bump the version.

@timhallmann timhallmann force-pushed the validate_aud_of_jwt_access_token branch from c6700b9 to afe6bbc Compare March 10, 2025 17:46
@timhallmann
Copy link
Contributor Author

Sounds good to me, let's do that. :)

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