-
Notifications
You must be signed in to change notification settings - Fork 15
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: enforce VAPID aud
#225
Conversation
marked as draft until mozilla-services/autopush#1432 lands |
Adding directly to review queue since the source is bugzilla. |
validate_vapid_jwt(vapid)?; | ||
validate_vapid_jwt( | ||
vapid, | ||
&format!("{}://{}", &state.settings.scheme, &state.settings.host), |
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.
Can you include the non standard ports in here (like the python now does)? Otherwise I think a mismatch could potentially hit someone when testing vapid locally.
Might make sense to make the formatted version w/ port a var or method on Settings.
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.
Ah cool, we already have that method.
It does not hide the standard ports though, can we fix that here? Or making it a separate issue might make more sense in case it affects something else.
autopush already has logic for this.
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.
Oh, cool. Didn't know about that function.
I'll make a separate issue to fix that. #229
Issue: bug 1663922
Issue: bug 1663922
Note The failing python tests may be fixed by mozilla-services/autopush#1432