Closed Bug 1671147 Opened 2 years ago Closed 8 months ago

Network monitor should show when responses are inaccessible to the requesting site due to Cross-Origin Request Blocking

Categories

(DevTools :: Netmonitor, enhancement, P3)

Desktop
All
enhancement

Tracking

(firefox94 fixed)

RESOLVED FIXED
94 Branch
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.

  1. We could use similar UI as suggested for exposing anti-tracking classification flag in bug 1537627
  2. 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)
  3. Or both

Honza

Attached image image.png

I am attaching a mockup of how the UI should look like.

Honza

Mentor: hmanilla

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

Attached image Initial UI

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

Flags: needinfo?(hmanilla)

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: nobody → delosrogers
Flags: needinfo?(hmanilla)

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

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

Status: NEW → ASSIGNED

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

Flags: needinfo?(hmanilla)
Duplicate of this bug: 1709395
Duplicate of this bug: 1709749
Attached image CORS notification.png

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

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

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

Attached image CORS error v2.png

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

Attachment #9220487 - Attachment description: Bug 1671147 - Style CORS blocked notification and add MDN link. r=bomsy → Bug 1671147 - Add MDNLink and displayCloseButton options to NotificationBox. r=bomsy
See Also: → 1716136

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.

Flags: needinfo?(hmanilla)
Flags: needinfo?(delosrogers)

Bomsy, please test, but it looks like we can land the patches.
Thank you!

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
Flags: needinfo?(hmanilla)
Regressions: 1732635
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch
Duplicate of this bug: 1716136
Flags: needinfo?(delosrogers)
You need to log in before you can comment on or make changes to this bug.