-
Notifications
You must be signed in to change notification settings - Fork 22.4k
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
Mixed Content update to new spec #33786
Conversation
Preview URLs
Flaws (1)Note! 2 documents with no flaws that don't need to be listed. 🎉 URL:
External URLs (4)URL:
(comment last updated: 2024-06-11 22:59:27) |
642ebb5
to
dfca50d
Compare
@mozfreddyb If you have a moment, I'd appreciate your thoughts on this first draft of the updated Mixed content page. I'm not sure how far to go with this - the old version was very high level, and this might verge on too much depth/historical context. I find the terminology switch between mixed context and mixed content requests a little uncomfortable - so I might edit a bit more. Note, I still haven't updated the "how to fix a website topic". Job for tomorrow. Notes
|
Great, thanks for the ping! I will give this a more thorough review soon, just noting that Firefox also disallows mixed download. They still show up in the downloads panel, effectively hiding them behind additional clicks. |
1c8c635
to
a94969c
Compare
@mozfreddyb Thanks very much. I've fixed the mixed downloads stuff and also removed the "how to fix" document, merging the relevant parts into the mixed content doc (not much was - we no longer live in the world where sites are newly migrating from HTTP to HTTPS). @pepelsbey Can you please review this too. I'd like to have any iteration done well before the release happens. |
All CORS-enabled mixed-content requests are blocked, whether or not the request would be upgradable if not using CORS. | ||
For example, `<img crossorigin src=...>` will be blocked, while `<img src=...>` is not. | ||
|
||
## Examples of mixed content requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I am not in love with this section, but I wanted to add some practical examples and also have a place to capture other things we might want to say that could be unexpected. In particular I've
- mentioned plugins and workers below, since those are also part of the secure context.
- Mentioned that local testing counts as secure, at least according to the spec. I did test http from a local file, and that does count as mixed content, as you would expect.
The current examples are a modification of https://www.w3.org/TR/mixed-content/#categorize-settings-object - I don't use "evil.com" because they are just misunderstood :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading w3c/webappsec-mixed-content#67, I believe we changed the spec such that CORS-enabled requests are upgraded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mozfreddyb Hmmm, I based this off section 3.1. Upgradeable Content which says:
We further limit this category in § 4.4 Should fetching request be blocked as mixed content? by force-failing any CORS-enabled request. This means, for example, that mixed content images loaded via <img crossorigin ...> will be blocked.
However if you look at § 4.4 Should fetching request be blocked as mixed content? it does not appear to look at CORS.
So you're right I think, but shouldn't this section of the spec be cleaned up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the spec appears to not upgrade requests that have IP addresses, so presumably they are always blocked even for images, videos etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed these with the two suggestions in comments linked above:
- https://github.com/mdn/content/pull/33786/files#r1630486539
- https://github.com/mdn/content/pull/33786/files#r1630484789
Are these OK, and does this otherwise seem sufficient and technically correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mozfreddyb @pepelsbey I merged those two comments above. I think they are correct.
69fd20c
to
40a5a4b
Compare
e080025
to
b950793
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor suggestions, but it looks good to me overall :) Thanks!
Co-authored-by: Vadim Makeev <hi@pepelsbey.dev>
fb6dd00
to
0c9845a
Compare
Thanks @pepelsbey - I have accepted all. Ready for merge is you're happy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you :)
@Josh-Cena Yes it should. @pepelsbey @mozfreddyb - thanks for the reviews. |
FF127 auto upgrades insecure (HTTP) requests in secure pages to secure requests (HTTPS) for most img, video, audio in https://bugzilla.mozilla.org/show_bug.cgi?id=1779757
As part of that work I am updating the Mixed content topics to match the spec
Related docs work in #33592
Fix #14131
Fix #33696