Closed Bug 1210985 Opened 9 years ago Closed 8 years ago

CORS: confusing error message when trying to use credentials and they're not allowed

Categories

(Core :: DOM: Security, defect, P3)

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: francois, Assigned: hchang)

References

(
URL
)

Details

(Whiteboard: [domsecurity-backlog1][domsecurity-active])

Attachments

(1 file)

Bug 1210985 - More specific error message for "useCredentials" and wildcard allowing origin.
58 bytes, text/x-review-board-request
ckerschb
: review+
Details 
The message you get in Firefox when specifying "use-credentials" despite it not being allowed on the server side is:

> Cross-Origin Request Blocked: The Same Origin Policy disallows reading the
> remote resource at https://fmarier.org/cors/anon.js. (Reason: CORS header
> 'Access-Control-Allow-Origin' does not match '*').

whereas Chrome has:

> Script from origin 'https://fmarier.org' has been blocked from
> loading by Cross-Origin Resource Sharing policy: A wildcard '*' cannot be
> used in the 'Access-Control-Allow-Origin' header when the credentials flag
> is true. Origin 'https://people.mozilla.org' is therefore not allowed
> access.

I created an online test case for it (see URL field on the bug):

  $ curl https://people.mozilla.org/~fmarier/cors/creds.html
  <script src="https://fmarier.org/cors/anon.js" crossorigin="use-credentials"></script>

  $ curl -i https://fmarier.org/cors/anon.js
  HTTP/1.1 200 OK
  ...
  Access-Control-Allow-Origin: *
  Content-Type: application/javascript

  alert('executed anon.js!');
Whiteboard: [domsecurity-backlog]
Priority: -- → P1
Priority: P1 → P3
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog1]
Got some spare cycles so will be looking into this bug.
Assignee: nobody → hchang
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][domsecurity-active]
The script link becomes invalid now. Using http://elefant.github.io/cors/Bug1210985.html to test this bug instead.
(In reply to Henry Chang [:henry][:hchang] from comment #3)
> The script link becomes invalid now. Using
> http://elefant.github.io/cors/Bug1210985.html to test this bug instead.

I mean the script link in comment 0.
Hi Chris,

Could you help review the patch I submitted? In the patch I specifically extract the special case from

if (mWithCredentials || !allowedOriginHeader.EqualsLiteral("*")) {
  // ...
}

to make it not too complicated to read. Otherwise I have to code like

if (A || !B) {
  // ...
  if (A && B) {
  } else if (...) {
    // ...
  }
}

Thanks :)
Comment on attachment 8778074 [details]
Bug 1210985 - More specific error message for "useCredentials" and wildcard allowing origin.

https://reviewboard.mozilla.org/r/69468/#review67604

::: dom/locales/en-US/chrome/security/security.properties:12
(Diff revision 1)
>  # LOCALIZATION NOTE: Do not translate "Access-Control-Allow-Origin", Access-Control-Allow-Credentials, Access-Control-Allow-Methods, Access-Control-Allow-Headers
>  CORSDisabled=Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at %1$S. (Reason: CORS disabled).
>  CORSRequestNotHttp=Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at %1$S. (Reason: CORS request not http).
>  CORSMissingAllowOrigin=Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at %1$S. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing).
>  CORSAllowOriginNotMatchingOrigin=Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at %1$S. (Reason: CORS header ‘Access-Control-Allow-Origin’ does not match ‘%2$S’).
> +CORSWildcardAllowOriginNotSupportingCredentials=Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at %1$S. (Reason: Credential is not supported if the CORS header 'Access-Control-Allow-Origin’ is '*').

Be careful with your single quotes, it seems the opening and closing quotes don't match. We have had issues before when they don't match. Please make sure you are consistent with what's already in the file.

Also, I would slightly prefer if you would add quotes around %1$S. I see the file sometimes uses it and sometimes doesn't, let's set a good example and surround our variables with quotes.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:594
(Diff revision 1)
>    }
>  
> +  // Bug 1210985 - Explicitly point out the error that the credential is
> +  // not supported if the allowing origin is '*'. Note that this check
> +  // has to be done before the following one.
> +  if (mWithCredentials && allowedOriginHeader.EqualsLiteral("*")) {

Arrh, I am with you, there is not an easy way to simplify that construct I suppose.

Please update the comment and replace 'has to be done before the following one'. In case someone adds code in between this check and the 'following' check then this comment might be confusing. I suppose the check needs to happen before checking 'Access-Control-Allow-Credentials', right?

Last nit: CORSWildcardAllowOriginNotSupportingCredentials feels super long, how about CORSNotSupportingCredentials?
Attachment #8778074 - Flags: review?(ckerschb) → review+
It seems reviewboard didn't publish my initial comment:

> Thanks for fixing, please incorporate my nits before landing though; r=me
Comment on attachment 8778074 [details]
Bug 1210985 - More specific error message for "useCredentials" and wildcard allowing origin.

https://reviewboard.mozilla.org/r/69468/#review67604

Thanks for the review, Christoph!

> Be careful with your single quotes, it seems the opening and closing quotes don't match. We have had issues before when they don't match. Please make sure you are consistent with what's already in the file.
> 
> Also, I would slightly prefer if you would add quotes around %1$S. I see the file sometimes uses it and sometimes doesn't, let's set a good example and surround our variables with quotes.

I will quote around %1$S absolutely :) However, I don't see any unmatching single quotes. Could you indicate the dangling one? Thanks!

> Arrh, I am with you, there is not an easy way to simplify that construct I suppose.
> 
> Please update the comment and replace 'has to be done before the following one'. In case someone adds code in between this check and the 'following' check then this comment might be confusing. I suppose the check needs to happen before checking 'Access-Control-Allow-Credentials', right?
> 
> Last nit: CORSWildcardAllowOriginNotSupportingCredentials feels super long, how about CORSNotSupportingCredentials?

The check has to happen before

```cpp
if (mWithCredentials || !allowedOriginHeader.EqualsLiteral("*"))
```

since it inlcudes 

```cpp
if (mWithCredentials && !allowedOriginHeader.EqualsLiteral("*"))
```

I would update the comment to

```cpp
// Bug 1210985 - Explicitly point out the error that the credential is
// not supported if the allowing origin is '*'. Note that this check
// has to be done before the condition
//
// >> if (mWithCredentials || !allowedOriginHeader.EqualsLiteral("*"))
//
// below since "if (A && B)" is included in "if (A || B)".
//
if (mWithCredentials && allowedOriginHeader.EqualsLiteral("*")) {
  LogBlockedRequest(aRequest, "CORSNotSupportingCredentials", nullptr);
  return NS_ERROR_DOM_BAD_URI;
}

if (mWithCredentials || !allowedOriginHeader.EqualsLiteral("*")) {
  nsAutoCString origin;
  ...
```

What do you think? :)
(In reply to Henry Chang [:henry][:hchang] from comment #9)
> I will quote around %1$S absolutely :) However, I don't see any unmatching
> single quotes. Could you indicate the dangling one? Thanks!

I think this one here | 'Access-Control-Allow-Origin’ |, no?

> // Bug 1210985 - Explicitly point out the error that the credential is
> // not supported if the allowing origin is '*'. Note that this check
> // has to be done before the condition
> What do you think? :)

I understand what the code is doing, I was just suggesting that you update the comment to:

// Bug 1210985 - Explicitly point out the error that credentials are not
// supported if the allowing origin is '*'. Note that this check has to
// happen before checking for the "Access-Control-Allow-Credentials" header.


Thanks for fixing!
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to Henry Chang [:henry][:hchang] from comment #9)
> > I will quote around %1$S absolutely :) However, I don't see any unmatching
> > single quotes. Could you indicate the dangling one? Thanks!
> 
> I think this one here | 'Access-Control-Allow-Origin’ |, no?

It's copied around the other descriptions and I don't even know what that character it is
and how to type it ^^"

I'll change it to the single quote that I could type :p. (But it's not consistent with the other ones. Is that okay?)

> 
> > // Bug 1210985 - Explicitly point out the error that the credential is
> > // not supported if the allowing origin is '*'. Note that this check
> > // has to be done before the condition
> > What do you think? :)
> 
> I understand what the code is doing, I was just suggesting that you update
> the comment to:
> 
> // Bug 1210985 - Explicitly point out the error that credentials are not
> // supported if the allowing origin is '*'. Note that this check has to
> // happen before checking for the "Access-Control-Allow-Credentials" header.
> 
> 
> Thanks for fixing!

The check doesn't just need to happen before "Access-Control-Allow-Credentials".
It has to happen even earlier.

// (a)
if (mWithCredentials && allowedOriginHeader.EqualsLiteral("*")) {
  // ...
}

// (b)
if (mWithCredentials || !allowedOriginHeader.EqualsLiteral("*")) {
  // ...
}

// (c)
if (mWithCredentials) {
  // ...
}

What I want to mention is (a) has to be appeared before (b) or the error message
in (a) wouldn't be printed.
Flags: needinfo?(ckerschb)
(In reply to Henry Chang [:henry][:hchang] from comment #12)
> It's copied around the other descriptions and I don't even know what that
> character it is
> and how to type it ^^"
> 
> I'll change it to the single quote that I could type :p. (But it's not
> consistent with the other ones. Is that okay?)

As long as there isn't any breakage it's fine by me. I just wanted to make sure that adding different kinds of single quotes didn't happen by accident.
 
> The check doesn't just need to happen before
> "Access-Control-Allow-Credentials".
> It has to happen even earlier.

I see. I trust your judgement. Works for me! thanks!
Flags: needinfo?(ckerschb)
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1e7184ec767
More specific error message for "useCredentials" and wildcard allowing origin. r=ckerschb
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/b83235ed0ea5
More specific error message for "useCredentials" and wildcard allowing origin. r=browser-chrome-fix
https://hg.mozilla.org/mozilla-central/rev/f1e7184ec767
https://hg.mozilla.org/mozilla-central/rev/b83235ed0ea5
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: