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

Whitelist webpush endpoints #182

Merged
merged 12 commits into from
Mar 22, 2021
Merged

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Mar 22, 2021

through an optional allowed_endpoints configuration key

I added support for globs because some browser vendor endpoints seem to contain sharding identifiers in the subdomain.

@bwindels bwindels marked this pull request as ready for review March 22, 2021 14:52
@bwindels bwindels force-pushed the bwindels/whitelist-webpush-endpoints branch from 56f6114 to 1f55017 Compare March 22, 2021 14:57
@bwindels bwindels requested a review from clokep March 22, 2021 15:09
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks good overall.

sygnal/webpushpushkin.py Show resolved Hide resolved
sygnal/webpushpushkin.py Outdated Show resolved Hide resolved
@clokep
Copy link
Member

clokep commented Mar 22, 2021

Does it make sense to include some info on Google/Microsoft/Mozilla here so people know what a reasonable value is?

@bwindels bwindels force-pushed the bwindels/whitelist-webpush-endpoints branch from ed3b438 to 329f3aa Compare March 22, 2021 16:30
@bwindels
Copy link
Contributor Author

Does it make sense to include some info on Google/Microsoft/Mozilla here so people know what a reasonable value is?

Added this to the docs 👍

@bwindels bwindels requested a review from clokep March 22, 2021 17:57
@bwindels
Copy link
Contributor Author

How would I fix this error?:

sygnal/webpushpushkin.py:108: error: Incompatible types in assignment (expression has type "None", variable has type "List[Any]")  [assignment]

@bwindels bwindels force-pushed the bwindels/whitelist-webpush-endpoints branch from d8c69b0 to a8765c1 Compare March 22, 2021 18:13
@clokep
Copy link
Member

clokep commented Mar 22, 2021

How would I fix this error?:

sygnal/webpushpushkin.py:108: error: Incompatible types in assignment (expression has type "None", variable has type "List[Any]")  [assignment]

When you define self.allowed_endpoints you need to add a type to it. I usually do this as:

self.foo = None  # type: Optional[List[str]]
if blah:
    self.foo = list(...)

You'll also need a from typing import List, Optional at the top.

Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks good! I think my other comment will fix the mypy issue.

changelog.d/182.feature Outdated Show resolved Hide resolved
bwindels and others added 3 commits March 22, 2021 20:14
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

LGTM!

@clokep clokep merged commit 189f091 into master Mar 22, 2021
@clokep clokep deleted the bwindels/whitelist-webpush-endpoints branch March 22, 2021 19:23
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