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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: dholbert, Assigned: longsonr)

Tracking

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Whiteboard: [todo after Firefox 4 branches]
Posted patch warning.txt (obsolete) — Splinter Review
Attachment #8570935 - Flags: review?(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*.)
OS: Linux → All
Hardware: x86 → All
Whiteboard: [todo after Firefox 4 branches]
Assignee: nobody → longsonr
Posted patch warning.txtSplinter Review
Of the two suggestions I picked the one I preferred.
Attachment #8570935 - Attachment is obsolete: true
Attachment #8570935 - Flags: review?(dholbert)
Attachment #8572863 - Flags: review?(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.)
Attachment #8572863 - Flags: review?(dholbert) → review+
Will this require translation? If so do I need to do set anything in the bug to indicate this?
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.
Flags: needinfo?(jwatt)
Correct, as long as you don't plan on uplifting, this can just land.
Flags: needinfo?(jwatt)
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.
https://hg.mozilla.org/mozilla-central/rev/7ba19816e1e4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.