Empty xlink:href treated like absent xlink:href on SVG <script>

RESOLVED FIXED in mozilla10

Status

()

Core
SVG
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: hsivonen, Assigned: Robert Longson)

Tracking

(Blocks: 1 bug)

Trunk
mozilla10
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Build id: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20091105 Minefield/3.7a1pre

Steps to reproduce:
 1) Load http://hsivonen.iki.fi/test/moz/svg-script-empty-xlink-href.svg

Actual result:
Alert saying FAIL.

Expected results:
Expected the document to load self as JS and JS parsing to fail.
This might be desired behavior. See bug 531327. Though of course we shouldn't execute any inline scripts, for example for:

<svg:script xlink:href="">
thisShouldNotExecute();
</svg:script>
(Assignee)

Comment 2

6 years ago
Created attachment 565793 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #565793 - Flags: review?(dholbert)
(Assignee)

Comment 3

6 years ago
Created attachment 565879 [details] [diff] [review]
replace string override too
Attachment #565793 - Attachment is obsolete: true
Attachment #565879 - Flags: review?(dholbert)
Attachment #565793 - Flags: review?(dholbert)
The chunks from the original patch look good.

I don't understand the addition in the new patch (DidChangeString --> AfterSetAttr).  Could you explain what that's for?
(Assignee)

Comment 5

6 years ago
I'm trying to get rid of all DidChangeXXX overrides. When I do so I can make all the 20 or so methods non-virtual and save 128 bytes per class (not per instance unfortunately). We'll get performance improvements too, over 1% on tSVG which isn't much but it all helps.
Ok -- I think I'd prefer if that change were in a separate patch, then (or ideally a separate bug, but I don't care too much on that part), since it's a nontrivial change that's unrelated to the actual fix here.  If we have to back it out for some reason, or if we trace a regression back to it, we'd want to back out just that part without the meat of this bug's fix.

Marking r+ on the original attachment, with just the fix for this bug (and pending r=me on the AfterSetAttr chunk as a separate patch, either on this bug or in a new one).
Attachment #565793 - Attachment is obsolete: false
Attachment #565793 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #565879 - Attachment is obsolete: true
Attachment #565879 - Flags: review?(dholbert)
(Assignee)

Comment 7

6 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/c49fd40bbd2b

I'll create a new bug for the AfterSetAttr change.
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
Robert, I couldn't see you on IRC (or more likely you are using something other than longsonr now).

This has caused a new M1 orange:
{
96930 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_nonAnimStrings.xhtml | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: XML can't be the whole program at http://mochi.test:8888/tests/content/svg/content/test/test_nonAnimStrings.xhtml:79
}
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c49fd40bbd2b
https://tbpl.mozilla.org/php/getParsedLog.php?id=6853293&tree=Mozilla-Inbound
Backed out for comment 8 orange:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4166640d1b5c
Status: NEW → ASSIGNED
(Assignee)

Comment 10

6 years ago
The patch exposed a bug in the mochitest. 

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/15d3df58e1e4 with the mochitest fixed.
(In reply to Robert Longson from comment #10)
> The patch exposed a bug in the mochitest.

Are you sure? I'm not convinced that the mochitest was buggy...

The mochitest did have <script id="script" xlink:href="">, but it didn't depend on any scripts *inside* of that <script> element running (that'd be the sort of test bug I'd expect this patch to expose).

Rather, it just tested that it was able to <animate> the xlink:href attribute (which apparently works, but we now trigger a security error for some reason).

Might merit a followup bug to investigate...  (I'm happy to file, I just want to make sure I'm not missing something)
(Assignee)

Comment 12

6 years ago
After this patch xlink:href="" means reload this file as javascript and run it, that's where we get the security error now.
(In reply to Robert Longson from comment #12)
> After this patch xlink:href="" means reload this file as javascript and run
> it

Ah, ok -- and that's because href="" resolves to the URI of the host document?
(Assignee)

Comment 14

6 years ago
it does now. As per comment 0 in fact.
Ah, of course, this exactly matches "expected results" from comment 0. :)

(I was recalling comment 1's "we shouldn't execute any inline scripts" as being the entirety of the expected results, and had forgotten about the other part.)
Please note that elsewhere we have moved towards letting <someelement src=""> not load anything. This because in the vast majority of cases the above markup intends to not actually load anything and is instead often a result of serverside code like:

response.write("<someelement src=\"" + somevalue + "\">");

So in this case I think that we should *not* execute the contents of a <svg:script xlink:href="">. But we also shouldn't attempt to resolve a url and load it.

I'm not 100% sure that the HTML5 spec has been updated to require this behavior (though I think so), but that was definitely the agreement on the lists.
Blocks: 694670
(In reply to Jonas Sicking (:sicking) from comment #16)
> But we also shouldn't attempt to resolve a url and load it.

I've filed bug 694670 on that.
https://hg.mozilla.org/mozilla-central/rev/15d3df58e1e4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.