1601371 - We should have better error wording for load failures that fail same-origin checks

Status: NEW

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

Description

[Boris Zbarsky [:bzbarsky]]

Consider these three testcases:

<video>
  <track src="https://example.com" default>
</video>

and

<video crossorigin>
  <track src="https://example.com" default>
</video>

and

<script>
  var xhr = new XMLHttpRequest();
  xhr.open("GET", "https://example.com");
  xhr.send();
</script>

In all three, the load gets blocked by same-origin policies: that first load is happening with SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS while the latter two are happening with SEC_REQUIRE_CORS_DATA_INHERITS. So as long as the HTML is not living on example.com, the loads will fail.

In Firefox, those load failures are silent as far as I can tell.

In Chrome, the first failure logs an error to the console that looks like this:

Unsafe attempt to load URL https://example.com/ from frame with URL <whatever>. Domains, protocols and ports must match.

the second logs an error that looks like this:

Access to text track at 'https://example.com/' from origin '<whatever>' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

and the third logs an error that looks like this:

Access to XMLHttpRequest at 'https://example.org/' from origin '<whatever>' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

It would be nice if the content security manager did something similar. It has enough information in the loadinfo (the type can be used to determine things like "text track" vs "XMLHttpRequest", for example) to provide these sorts of error messages...

Pages can detect these errors in various ways, of course (e.g. error event listeners on <track>), but having this sort of thing in the console would make for a much nicer debugging experience. And pages don't have access to details of why a CORS check failed, whereas content security manager does (e.g. missing Access-Control-Allow-Origin vs the origin not being allowed, etc).

I'm actually a little surprised that the CORS cases don't log anything, given the existence of nsCORSListenerProxy::LogBlockedCORSRequest. I'll dig into that a bit.

Comment 1

[:Harald Kirschner :digitarald]

Bug 1556451 is also related as network panel does not show an error for this failed request but a broken in-progress request.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm actually a little surprised that the CORS cases don't log anything, given the existence of nsCORSListenerProxy::LogBlockedCORSRequest. I'll dig into that a bit.

Ah, I see. It's logged as a warning, not an error, and I had warnings turned off because they were so spammy. Maybe we should report CORS failures as an error, not a warning?

And even then the warning does not include the origin making the request; perhaps it should. And it doesn't say what kinds of load it was that got blocked; again it probably should.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So for the "access to text track" thing, what's a sane way to localize it? We want to interpolate the "text track" string into a more generic string that explains why it was blocked; see the CORS* lines in dom/locales/en-US/chrome/security/security.properties

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, this is hilarious. For the non-CORS case we already do log the error, but without a proper windowid, so it goes to the brower console, not the web console!

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

So for the "access to text track" thing, what's a sane way to localize it?

Are you asking how to compose such message, or which API to use?

For Fluent, I think that if the number of permutations is low, we'd be ok with WET over DRY logic [*], but if the number of permutations is high, or will grow, it would make a good case to prioritize adding support for dynamic message references [**] which would allow us to do:

msg = Access to { $type } at '{ $domain }' from origin '{ $origin }' has been blocked by CORS policy: { $policy }

## Types

msg-type-text = text track
msg-type-another = another track

## Policies

msg-policy-header = No '{ $header }' header is present on the requested resource.
...

and that would allow you to do:

let type = switch () {
  case "text-track": FluentMessageReference("msg-type-text"),
  case "another-track": FluentMessageReference("msg-type-another"),
  case "XMLHttpRequest": "XMLHttpRequest",
}

let policy = switch () {
  case "header": FluentMessageReference("msg-policy-header"),
  ...
}

let string = bundle.formatValue("msg", {
  type,
  domain,
  origin,
  policy,
  header: "Access-Control-Allow-Origin"
});

As you can see, this feature would allow you to compose a message from dynamically selected references to other messages and variables. The type here is interesting because it allows you to either use a reference or a hardcoded string (because I doubt we'd want to localize "XMLHttpRequest").

@bz - does it seem like it would fit your needs here?
@stas - WDYT?

[*] https://github.com/projectfluent/fluent/wiki/Good-Practices-for-Developers#prefer-wet-over-dry
[**] https://github.com/projectfluent/fluent/issues/80

Comment 6

[Boris Zbarsky [:bzbarsky]]

So first of all, the relevant code creating these message strings is in C++, not JS. I could have it call out to JS to get the string if I have to, but it would be moderately annoying in various ways.

In terms of what I am asking, I am asking what the "right" way of doing this is. That includes what API to use, etc. In particular, localizers would need to be aware of the context their translation of the type string gets interpolated into, right? As long as that's the case, clearly I can do this on top of .properties files as well as anything else, as long as the translation for all the types would be the same for all the strings they get interpolated into. Which is not 100% obvious to me across all our languages...

In terms of WET vs DRY, we have ~20 types and 15 existing CORS* strings in security.properties. Writing out all 300 strings may be more wetness than I want to deal with. ;)

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Totally fair. We should be able to call it from c++. As for architecture, stas is the right person to advise here. Let's wait for him

Comment 8

[Boris Zbarsky [:bzbarsky]]

I spun off bug 1602090 and bug 1602093 on changes to which errors show up and how; leaving this open for discussion about the actual wording of the errors.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.