gfxSVGGlyphs.cpp should use correct origin attributes to create principal

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: allstars.chh, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox47 fixed)

Details

(Whiteboard: gfx-noted[OA])

Attachments

(1 attachment)

(Assignee)

Updated

3 years ago
Depends on: 1209162
Whiteboard: gfx-noted
(Assignee)

Updated

3 years ago
Blocks: 1153435

Updated

3 years ago
Blocks: 1218479
(Assignee)

Updated

3 years ago
Assignee: nobody → allstars.chh
(Assignee)

Updated

3 years ago
Summary: gfxSVGGlyphs.cpp should use correct principal → gfxSVGGlyphs.cpp should use correct origin attributes to create principal
PrincipalOriginAttributes will be initialized with default value, appId=0 and inBrowser=false.

And gfxSVGGlyphsDocument seems not related to any origin attributes-specific things,
so the code in https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxSVGGlyphs.cpp#351 should be no harm, 

Jonas, do you agree?

Thanks
Flags: needinfo?(jonas)
If the gfxSVGGlyphsDocument makes any network requests (to download subresources) we'll need it to have the right origin attributes so that it uses the right cookies when making those network requests.

Similarly if the document does any sort of permission checks, we'll want to make sure that those checks use the right permissions.

In general, I would be very worried that if we have an incorrect principal here we'll end up in trouble either now or in the future. It just feels very unsafe to do.

If we truly don't think that this document will ever need to make network requests or check permissions, then we should use a nullprincipal instead.
Flags: needinfo?(jonas)
Created attachment 8721231 [details] [diff] [review]
Patch

Hi, Edwin
You changed to use a non-null principal back in bug 801467, and now I tried to move the null principal back and the tests looks okay, can you help to check if we can use null principal now?

Thanks
Attachment #8721231 - Flags: feedback?(edwin)
Comment on attachment 8721231 [details] [diff] [review]
Patch

Review of attachment 8721231 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Yoshi Huang[:allstars.chh] from comment #3)
> You changed to use a non-null principal back in bug 801467, and now I tried
> to move the null principal back and the tests looks okay, can you help to
> check if we can use null principal now?

Hm, no idea why I did that. This makes sense AFAICT from the spec, which states that rendering be done with "no script execution, external references, interactivity, or link traversal."
Attachment #8721231 - Flags: feedback?(edwin) → feedback+
Comment on attachment 8721231 [details] [diff] [review]
Patch

Review of attachment 8721231 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Edwin
Ask for r? for the same patch, as it looks changing to use null principal should be enough.

I'd change patch subject later.

Thanks
Attachment #8721231 - Flags: review?(edwin)

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e46b45235799
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

3 years ago
Whiteboard: gfx-noted → gfx-noted[OA]
You need to log in before you can comment on or make changes to this bug.