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

bug: Do not double-check VAPID aud #772

Merged
merged 3 commits into from
Oct 10, 2024
Merged

bug: Do not double-check VAPID aud #772

merged 3 commits into from
Oct 10, 2024

Conversation

jrconlin
Copy link
Member

@jrconlin jrconlin commented Oct 2, 2024

We were currently double-checking the VAPID aud via some legacy code which may produce invalid results if the declared vapid_aud setting and the endpoint_url did not match.

Closes: #770

We were currently double-checking the VAPID aud via some legacy code
which may produce invalid results if the declared `vapid_aud` setting
and the `endpoint_url` did not match.

Closes: #770
@jrconlin jrconlin requested review from pjenvey and taddes October 2, 2024 23:43

let domain = &settings.endpoint_url();

if domain != &aud {
Copy link
Member

Choose a reason for hiding this comment

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

I gather jsonwebtoken will now do an equivalent check against our settings.vapid_aud (via validation.set_audience()) -- however I don't see us assigning this new AUTOEND__VAPID_AUD setting anywhere, which I think means it's defaulting to incorrect values (not settings.endpoint_url()).

Do we even need settings.vapid_aud -- might we always set something along the lines of validation.set_audience(&[settings.endpoint_url()]) instead?

Copy link
Member

Choose a reason for hiding this comment

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

and prod's basically endpoint_url() for reference here

Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

I think the original check we did would be a little more lax with trailing /, since it compared Urls? But that doesn't matter, especially if we already have the jsonwebtoken's validation already occurring on prod (I can't remember if that's the case). Just noting it in case we see more validation errors e.g.

@jrconlin jrconlin merged commit ea04db6 into master Oct 10, 2024
1 check passed
@jrconlin jrconlin deleted the bug/770-aud_test branch October 10, 2024 17:44
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.

Autoendpoint ignores configuration file settings (including vapid_aud) when validating VAPID claims
2 participants