Closed Bug 1121824 Opened 5 years ago Closed 5 years ago

Improve CORS console messages when request is blocked

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(2 files, 6 obsolete files)

Currently we only have the following error message when a request is blocked by CORS:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at %1$S. This can be fixed by moving the resource to the same domain or enabling CORS.


Based on my own experience it's sometimes tricky to figure out why a request is blocked, even if you do use CORS, but incorrectly. Also, Jeff Griffith told us the other day, that 'better' CORS console messages is one of the number most desired features from web devs for Firefox.

Also, I wrote some testcases where we can not query the innerWindowID, in such cases Firefox does not print any error message to the console at all. I think we should fall back to the browser console instead of having no message at all if we can not query the innerWindowID.
Blocks: 713980
Jonas, what do you think?
Attachment #8549379 - Flags: review?(jonas)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment on attachment 8549379 [details] [diff] [review]
bug_1121824_improve_cors_console_messages.patch

Review of attachment 8549379 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsCORSListenerProxy.cpp
@@ +511,1 @@
>      return NS_ERROR_DOM_BAD_URI;

To block the network request?
Attachment #8549379 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #2)
> ::: dom/security/nsCORSListenerProxy.cpp
> @@ +511,1 @@
> >      return NS_ERROR_DOM_BAD_URI;
> 
> To block the network request?

Facepalm - of course - if CORS is disabled it should block and not allow the load.
Just deleted the question in the comment. Carrying over r+ from sicking.
Attachment #8549379 - Attachment is obsolete: true
Attachment #8549744 - Flags: review+
Jeff, before I am going to land this patch, is this what you were looking for? Or any additional logging needed?
Flags: needinfo?(jgriffiths)
One problem that I just realized though is that this means that these strings aren't localizable. Probably we should simply use different messages, that live entirely in security.properties, to solve this.
(In reply to Jonas Sicking (:sicking) from comment #6)
> One problem that I just realized though is that this means that these
> strings aren't localizable. Probably we should simply use different
> messages, that live entirely in security.properties, to solve this.

Yeah, thanks for pointing that out. Maybe we can at least re-use the actual header name, so we don't end up having so many new strings in security.properties. I will refactor and flag you again.
Comment on attachment 8549744 [details] [diff] [review]
bug_1121824_improve_cors_console_messages.patch

Changing review+ to feedback+ until we get the localication problem resolved.
Attachment #8549744 - Flags: review+ → feedback+
Jonas, I had to restructure and I am using FormatStringFromName() instead of FormatLocalizedString(), because I want to pass an array of arguments in some cases.

Anyway, the localization problems are gone now.
Attachment #8549744 - Attachment is obsolete: true
Attachment #8550135 - Flags: review?(jonas)
Comment on attachment 8550135 [details] [diff] [review]
bug_1121824_improve_cors_console_messages.patch

Review of attachment 8550135 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsCORSListenerProxy.cpp
@@ +527,5 @@
>    if (!mHasBeenCrossSite) {
>      return NS_OK;
>    }
>  
> +  // Generate error info in case we have to log to the console

Do this inside of LogBlockedRequest like before. It'll require a little more code to prepend the uri argument, but I think it's worth avoiding doing this every time.

Or, since you only ever send a single "extra" parameter, make LogBlockedRequest take just an nsIRequest and a nsACString*. Then just pass null when the extra string isn't needed. That way you can even do the UTF8toUTF16 conversion in LogBlockedRequest.
Attachment #8550135 - Flags: review?(jonas)
Jonas, you are right, we should only perform additional computations to generate error objects in case we do encounter an error. Here is an updated patch.
Attachment #8550135 - Attachment is obsolete: true
Attachment #8550845 - Flags: review?(jonas)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> Jeff, before I am going to land this patch, is this what you were looking
> for? Or any additional logging needed?

It's hard to tell, I'm not either an expert with CORS or a C++ programmer, but this looks like we're providing much more fine-grained logging of the various failure modes for requests caused by CORS interactions, yes? Go ahead for now, and we can handle feedback from web developers in followups once they've kicked the tires.
Flags: needinfo?(jgriffiths)
Duplicate of this bug: 1090227
For what it's worth, this is the kind of messages I was seeing in other browsers last time I was frustrated with our reporting. So this will be helpful.
(In reply to Anthony Ricaud (:rik) from comment #14)
> For what it's worth, this is the kind of messages I was seeing in other
> browsers last time I was frustrated with our reporting. So this will be
> helpful.

Good to know, thanks!
Comment on attachment 8550845 [details] [diff] [review]
bug_1121824_improve_cors_console_messages.patch

Review of attachment 8550845 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsCORSListenerProxy.cpp
@@ +524,5 @@
>      return NS_OK;
>    }
>  
>    if (gDisableCORS) {
> +    LogBlockedRequest(aRequest, "CORSDisabled", NS_LITERAL_STRING("").get());

Just pass null as third parameter. Same elsewhere.

@@ +596,5 @@
>          continue;
>        }
>        if (!NS_IsValidHTTPToken(method)) {
> +        NS_ConvertUTF8toUTF16 unicodeMethod(method);
> +        LogBlockedRequest(aRequest, "CORSInvalidAllowMethod", unicodeMethod.get());

NS_ConvertUTF8toUTF16(method).get();

Same elsewhere.
Attachment #8550845 - Flags: review?(jonas) → review+
Thanks Jonas - carrying over r+.
Attachment #8550845 - Attachment is obsolete: true
Attachment #8552858 - Flags: review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5756601&repo=mozilla-inbound
Flags: needinfo?(mozilla)
(In reply to Carsten Book [:Tomcat] from comment #19)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=5756601&repo=mozilla-
> inbound

Thanks Carsten for backing it out, sorry about that - I will take a look today - our tests are just to spread out within the codebase.
Flags: needinfo?(mozilla)
Jonas, currently the callsite of CheckRequestApproved logs an error to the console if CheckRequestApproved returns NS_FAILED().

We should be slighlty more conservative and always report to the console if something goes wrong. Hence I converted all the NS_ENSURE_SUCCESS() to if NS_FAILED() and log the error to the console. Even though that adds some more code I think it's worth doing. Nothing's worse than getting no error and having to guess what might have gone wrong. I hope you agree.
Attachment #8552858 - Attachment is obsolete: true
Attachment #8553432 - Flags: review?(jonas)
Missed a typo - here we go.
Attachment #8553432 - Attachment is obsolete: true
Attachment #8553432 - Flags: review?(jonas)
Attachment #8553433 - Flags: review?(jonas)
https://hg.mozilla.org/mozilla-central/rev/2eded1bead20
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Can anyone comment on the ending of this string?

CORSMissingAllowHeaderFromPreflight=Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at %1$S. (Reason: missing token '%2$S' in CORS header 'Access-Control-Allow-Headers' from CORS preflight channel"));

Bad copy and paste?
(In reply to Francesco Lodolo [:flod] from comment #25)
> Can anyone comment on the ending of this string?
> 
> CORSMissingAllowHeaderFromPreflight=Cross-Origin Request Blocked: The Same
> Origin Policy disallows reading the remote resource at %1$S. (Reason:
> missing token '%2$S' in CORS header 'Access-Control-Allow-Headers' from CORS
> preflight channel"));
> 
> Bad copy and paste?

Indeed. I am OOO this week, but I will follow up on this early next week.

Thanks for pointing that out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushing the follow up on incorrect line ending:
   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9bf9805a5e
https://hg.mozilla.org/mozilla-central/rev/0c9bf9805a5e
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 939201
Depends on: 1152574
Depends on: 1209094
You need to log in before you can comment on or make changes to this bug.