Network monitor should show when responses are inaccessible to the requesting site due to Cross-Origin Request Blocking
Categories
(DevTools :: Netmonitor, enhancement, P3)
Tracking
(firefox94 fixed)
Tracking | Status | |
---|---|---|
firefox94 | --- | fixed |
People
(Reporter: Gijs, Assigned: delosrogers, Mentored)
References
(Blocks 1 open bug, Regressed 1 open bug)
Details
Attachments
(6 files)
Although we warn about this in the console, we do not show anything in the network monitor.
Splitting this out from a bug that's confidential, as these bits don't need to be. Relevant bits of discussion:
(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #3)
(In reply to :Gijs (he/him) from comment #1)
We can probably add some notification to the network tab to list that access to the CORS request from JS is blocked, in addition to the console warnings around this, but we should avoid doing exactly what Chrome is doing and show the same kind of "blocked" UI that we do when the request actually does not make it to the server (due to CSP, or an add-on blocking requests, or some other reason).
Yes, agree.
(In reply to :Gijs (he/him) from comment #2)
The devtools side of this looks like a regression, cf. bug 1638313. Honza?
That bug only added platform support to identify requests "blocked" by CORS. The DevTools side wasn't completed (see bug 1640099).
So, let's use this bug to implement the notification.
- We could use similar UI as suggested for exposing anti-tracking classification flag in bug 1537627
- Or we could show an icon indicating "this request is inaccessible due to CORS" in the Status column (the same place where we show the Request Blocked icon)
- Or both
Honza
Comment 1•4 years ago
|
||
I am attaching a mockup of how the UI should look like.
Honza
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
I would be interested in working on this issue. Do you know which files I should look in to add the new functionality and UI? Also, to clarify this bug isn't about showing cross-origin blocking in the summary panel but adding more details about the blocking to the headers panel when you click for more info on the blocked request.
Mattias
Assignee | ||
Comment 3•4 years ago
|
||
I implemented the initial functionality of adding a message in the headers panel that the request was blocked. Right now it will add the message from BLOCKED_REASON_MESSAGES
in devtools/client/netmonitor/src/constants.js
for all blocked requests but I'm not sure if it's better to limit the scope right now to just requests blocked by CORS.
I was also wondering if there is a premade component or similar for rendering the warning box or if I should write my own.
Mattias
Assignee | ||
Updated•4 years ago
|
Comment 4•3 years ago
•
|
||
Thanks starting on this Mattias!
I implemented the initial functionality of adding a message in the headers panel that the request was blocked. Right now it will add the message from BLOCKED_REASON_MESSAGES in devtools/client/netmonitor/src/constants.js for all blocked requests but I'm not sure if it's better to limit the scope right now to just requests blocked by CORS.
We only the need the notification for requests blocked by CORS, so lets limit it to that. I'm not sure if we want it for all types of CORS errors but we can look at that later.
I was also wondering if there is a premade component or similar for rendering the warning box or if I should write my own.
You can use this NotificationBox https://searchfox.org/mozilla-central/search?q=NotificationBox&redirect=false
Feel free to open an in-progress patch.
Assignee | ||
Comment 5•3 years ago
|
||
What is the correct way to apply styles to a component in the Devtools codebase? I've been able to get the NotificationBox to display but I'm struggling to get it styled and many online resources assume that you are using webpack.
Mattias
Comment 6•3 years ago
|
||
Hi Mattias,
You can add the NotificationBox style sheet here https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/devtools/client/netmonitor/src/assets/styles/netmonitor.css#5-14
@import "chrome://devtools/content/shared/components/NotificationBox.css"
Then you can reference the classes from one of the other netmonitor stylesheets. I guess this might be assest/styles/HeadersPanel.css
.
Hope this helps
Also feel free to open a work in progress patch and it's easier to help when looking at the code.
Thanks
Assignee | ||
Comment 7•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D114377
Assignee | ||
Comment 9•3 years ago
|
||
I want to write a test for the new UI but I'm unsure how to simulate a request blocked by CORS so that it has the correct blocked reason. Is there a good way to emulate a CORS blocked request in the test setup?
Mattias
Comment 10•3 years ago
|
||
Hi Mattias,
Here is a simple cors test https://searchfox.org/mozilla-central/rev/3151f97de27730793c2e298716df760999423f26/devtools/client/netmonitor/test/browser_net_cors_requests.js
Thanks
Comment 13•3 years ago
|
||
Related webcompat issue filed https://webcompat.com/issues/72813
Assignee | ||
Comment 14•3 years ago
|
||
Here is what the UI is looking like right now. I'm not sure how well the NotificationBox
works for two reasons. First, I don't know if the warning should be closable. Second, at least in the way I was doing it, I couldn't reuse the existing devtools learn-more link code and had to partially reimplement it to get a learn more button.
Mattias
Comment 15•3 years ago
|
||
This looks great Mattias, thank you!
(In reply to Mattias de los Rios Rogers [:delosrogers] from comment #14)
First, I don't know if the warning should be closable.
Good point. Agree, the close button should not be there.
I think we could introduce new displayCloseButton
prop in the NotificationBox component (true by default)
Second, at least in the way I was doing it, I couldn't reuse the existing devtools learn-more link code and had to partially reimplement it to get a learn more button.
Perhaps we could move this piece directly to the NotificationBox and introduce displayLearnMore
prop. If MDNLink
component can't be directly reused we can adjust/polish this piece in a follow up bug.
Honza
Assignee | ||
Comment 16•3 years ago
|
||
Ok, I'll start working on adding a displayCloseButton
prop and a displayLearnMore
prop. I think that I'll also have to add a displayButtons
prop because as the component is right now it will always display at least one general-purpose button (even if you pass an empty array of buttons) and I think that the learn-more link should be separate.
Mattias
Assignee | ||
Comment 17•3 years ago
|
||
Here is the new UI using the MDNLink
component as a learn more link and without a close button. I think looks much better and it's nice that it matches matches the learn more link of the status code. Also, I was wrong about needing a displayButtons
prop because NotificationBox
actually doesn't display any buttons when you pass an empty array, I'm not sure why I thought it did.
Mattias
Comment 18•3 years ago
|
||
It looks great!
Updated•3 years ago
|
Comment 19•3 years ago
|
||
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:delosrogers, could you have a look please?
For more information, please visit auto_nag documentation.
Comment 20•3 years ago
|
||
Bomsy, please test, but it looks like we can land the patches.
Thank you!
Comment 21•3 years ago
|
||
Pushed by hmanilla@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/810afa9a4fcd Add MDNLink and displayCloseButton options to NotificationBox. r=bomsy https://hg.mozilla.org/integration/autoland/rev/770b763ba0fd Show when responses have been blocked by CORS. r=bomsy
Updated•3 years ago
|
Comment 22•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/810afa9a4fcd
https://hg.mozilla.org/mozilla-central/rev/770b763ba0fd
Assignee | ||
Updated•3 years ago
|
Description
•