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: Address insecure sub header fetch. #803

Merged
merged 7 commits into from
Nov 21, 2024
Merged

Conversation

jrconlin
Copy link
Member

@jrconlin jrconlin commented Nov 20, 2024

When insecurely getting the sub for tracking and errors, we were not
properly decoding. This was resulting in a VAPID error being generated.
This should not impact processing, but was generating a lot of logging
messages.

  • FCM would reject TTLS > that 4 weeks, which is less than
    our 30 day max. Added a specific filter for that.

  • fixed some spelling mistakes.

Closes: SYNC-4514

When insecurely getting the `sub` for tracking and errors, we were not
properly decoding. This was resulting in a VAPID error being generated.
This should not impact processing, but was generating a lot of logging
messages.

* FCM would reject TTLS > that 4 weeks, which is less than
our 30 day max. Added a specific filter for that.

* fixed some spelling mistakes.

Closes: SYNC-4514
@jrconlin jrconlin requested review from pjenvey and taddes November 20, 2024 17:55
Copy link
Contributor

@taddes taddes left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this! Just a few minor things.

@@ -44,7 +44,10 @@ pub mod util;
/// "abandoned" and any router info assigned to a User Agent that has not contacted
/// Autopush in 60 days can be discarded.
///
const ONE_DAY: u64 = 24 * 60 * 60;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's pretty clear from the surrounding comments that it's seconds, but maybe just for sanity: a docstring stating this is in seconds, opposed to renaming something like ONE_DAY_IN_SECONDS, just like the other consts below.

autoendpoint/src/headers/vapid.rs Outdated Show resolved Hide resolved
autoendpoint/src/headers/vapid.rs Outdated Show resolved Hide resolved
autoendpoint/src/routers/webpush.rs Outdated Show resolved Hide resolved
autopush-common/src/db/bigtable/mod.rs Outdated Show resolved Hide resolved
@jrconlin jrconlin requested a review from taddes November 21, 2024 19:31
Copy link
Contributor

@taddes taddes left a comment

Choose a reason for hiding this comment

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

👍

@jrconlin jrconlin merged commit 53d90aa into master Nov 21, 2024
1 check passed
@jrconlin jrconlin deleted the bug/SYNC-4514_vapid branch November 21, 2024 20:02
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.

3 participants