fix: security doc now provides solid url check #16775
Conversation
|
💖 Thanks for opening this pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
| @@ -278,7 +278,8 @@ session | |||
| callback(true) | |||
| } | |||
|
|
|||
| if (!url.startsWith('https://my-website.com')) { | |||
| // Verify URL | |||
| if (!url.startsWith('https://my-website.com/')) { | |||
There was a problem hiding this comment.
how does that extra / benefit?
There was a problem hiding this comment.
As I said in #16774, the current example (if followed by the reader) can be bypassed with an URL which starts with the required string, but may be a subdomain (or part of the domain) like https://my-website.com.attacker.com, which passes the check.
Also, this follows the example provided in point 11 (https://github.com/electron/electron/blob/master/docs/tutorial/security.md#how-9). Another option would be to parse the URL and strictly check the origin like in point 12 (https://github.com/electron/electron/blob/master/docs/tutorial/security.md#how-10).
|
Release Notes Persisted
|
|
Congrats on merging your first pull request! 🎉🎉🎉 |
Description of Change
The security doc now provides a solid check, as in point 11 and 12.
Release Notes
Notes: fixes issue #16774