search

mozilla-services / autopush-rs

Push Server in Rust
Mozilla Public License 2.0
199 stars 16 forks source link

Autoendpoint ignores configuration file settings (including `vapid_aud`) when validating VAPID claims #770

Closed ncalexan closed 1 month ago

ncalexan commented 1 month ago

validate_vapid_jwt creates a new Settings object solely from the environment -- ignoring any configuration file settings -- at https://github.com/mozilla-services/autopush-rs/blob/9afbe422c6c46a9babca3a4fcb4ef180480c3790/autoendpoint/src/extractors/subscription.rs#L292

In practice, this object defaults to having vapid_aud as set here. This is both incorrect and inefficient.

┆Issue is synchronized with this Jira Task

jrconlin commented 1 month ago

Sigh. Good catch.

(I keep thinking that was fixed in a prior version, but clearly it's not now.)

ncalexan commented 1 month ago

Worth a test that has a domain that is not push.services.mozilla.com, I think.

jrconlin commented 1 month ago

I'm not so sure.

The unit test submits a header that uses and aud of example.com where the settings uses push.services.mozilla.com. The actual validation of the aud is performed in jsonwebtoken::decode within subscription.rs. I feel we can presume that their unit tests work correctly and that strings that do not match would be properly flagged.

The local aud is specified as a setting, since any hard-coded value would be flagged in review. Is there a specific error case you're thinking of?

ncalexan commented 1 month ago

I saw that one, but AFAICT the tests don't actually exercise the situation that I hit: a custom vapid_aud that does validate a custom aud. Shouldn't the unit tests ensure there isn't a future regression?

jrconlin commented 1 month ago

I'm not 100% sold on the value, but I do understand the desire. Adding the test is cheap. I'll do a follow up PR that includes it.

jrconlin commented 1 month ago

Ah! Sure enough, there was a bug, and it was a sneaky one too. There was some legacy code that was double-checking the aud against the endpoint_url setting.

Since the jwt does the validation already, no need to double check (and the associated error metrics would never fire anyway).