629682 - Add a better warning message for SVG-as-an-image external resources being blocked

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Bug 628747 imposed some restrictions, essentially preventing SVG-as-an-image from loading external resources, including resources loaded from the same domain (since those could actually translate into resources from a remote domain in some cases, for sites with open redirectors).

Right now, we're posting a "CheckSameOriginError" warning in the error console to notify users about the blocked resource, to aid in website debugging:
>          nsScriptSecurityManager::ReportError(
>            nsnull, NS_LITERAL_STRING("CheckSameOriginError"), principalURI,
>            aContentLocation);
http://hg.mozilla.org/mozilla-central/rev/21f96c423923

However, "CheckSameOriginError" is technically not the right error message to show, since in most cases the resource will probably *be coming from the same domain* (and we're just blocking it to be on the safe side).

Luckily, the actual longhand text of this error is pretty vague, so it'll still be helpful for now:
> Security Error: Content at %S may not load data from %S.
... but nonetheless, we should probably add a more specific, clearer error message for this situation, once we're not in an l10n string freeze.

Comment 1

[Robert Longson [:longsonr]]

Attachment: warning.txt

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8570935 [details] [diff] [review]
warning.txt

Thanks for picking this up!

>+++ b/dom/locales/en-US/chrome/security/caps.properties
> CheckSameOriginError = Security Error: Content at %S may not load data from %S.
>+ExternalDataError = Security Error: Content at %S may not load external data.

We probably should still include the external resource URI somehow, for two reasons:
 (1) It'll be useful for the author -- they may have a huge .svg file, with an external resource buried in there somewhere, with no way of finding it if we don't give them a hint at what to search for.

 (2) The function we're using -- nsScriptSecurityManager::ReportError() -- expects to be providing both URIs to the localized string (and you're passing in both URIs, too) -- so it's a bit bogus that we're providing these strings and then not using them. It's effectively doing printf("blah %s", localURI, resourceURI), with one more arg than "%s". Probably not a big deal, but a bit hacky.


So, maybe reword as:
 Security Error: Content at %S may not load external data; blocking its attempt to load %S.
...or:
 Security Error: Content at %S attempted to load %S, but may not load external data when being used as an image.
...or maybe you can think of better wording that includes the resource URL.


(I also thought of "may not load external data from %S", but that gives the impression that it *just* can't load external data from *this one place*.)

Comment 3

[Robert Longson [:longsonr]]

Attachment: warning.txt

Of the two suggestions I picked the one I preferred.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8572863 [details] [diff] [review]
warning.txt

r=me. Thanks!

(If you haven't already, please test the final patch locally & make sure the message actually shows up correctly in error console, before landing.)

Comment 5

[Robert Longson [:longsonr]]

Will this require translation? If so do I need to do set anything in the bug to indicate this?

Comment 6

[Daniel Holbert [:dholbert]]

Yes, I believe it will. I think localizers look for new strings and translate them proactively, and you only need to set flags if it's a late-breaking string change (e.g. for something being backported).

jwatt can confirm/correct my interpretation on this; I'm pretty sure I've seen him adding strings recently.

Comment 7

[Jonathan Watt [:jwatt]]

Correct, as long as you don't plan on uplifting, this can just land.

Comment 8

[Robert Longson [:longsonr]]

Local testing...

Security Error: Content at http://www.standardista.com/clowncar/clown.svg attempted to load http://www.standardista.com/clowncar/images/small.png, but may not load external data when being used as an image.

Comment 9

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba19816e1e4

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ba19816e1e4