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

Check for secure context before installing web app #421

Merged
merged 1 commit into from
Nov 18, 2023

Conversation

iTrooz
Copy link
Contributor

@iTrooz iTrooz commented Nov 16, 2023

PWA should only be installable in a secure context: https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/Guides/Making_PWAs_installable#secure_context

Initially, I wanted the replace the current https check by secure context, because I couldn't install a PWA from localhost. But it turned out to be more complicated than this, because then we could load the manifest over http.
But checking for secure context in itself is still probably a good thing to do

@filips123
Copy link
Owner

Is is possible that the document has HTTPS URL, but it's still not a secure context? Also, is there some way we could check if manifest is in a secure context (without checking for HTTPS, to also allow localhost and other URLs)?

@iTrooz
Copy link
Contributor Author

iTrooz commented Nov 16, 2023

Is is possible that the document has HTTPS URL, but it's still not a secure context?

I think so, yeah: "it must be served in a secure context. This usually means that it must be served over HTTPS"

Also, is there some way we could check if manifest is in a secure context

I don't think so, that's the reason I kept the https checks ):

@filips123
Copy link
Owner

This could probably still be simplified to:

manifestUrl && manifestUrl.protocol === 'https:' && isSecureContext

So, without the document URL HTTPS check. It shouldn't change anything (as the code was supposed to check for secure context in any case), but it's a bit shorter and cleaner.

@iTrooz
Copy link
Contributor Author

iTrooz commented Nov 16, 2023

It won't change anything for localhost/etc.. because the manifest url will still need HTTPS.
And I think keeping that redundant security condition just in case is good

But indeed, this normally shouldn't be any less secure than before. Do you want me to do that ?

@filips123 filips123 self-requested a review November 18, 2023 15:51
Copy link
Owner

@filips123 filips123 left a comment

Choose a reason for hiding this comment

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

It's probably fine.

@filips123 filips123 merged commit ff86359 into filips123:main Nov 18, 2023
2 checks passed
@filips123 filips123 added this to the 2.9.0 milestone Nov 18, 2023
@filips123 filips123 added the enhancement New feature or request label Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants