Closed Bug 1225053 Opened 4 years ago Closed 4 years ago

gfxSVGGlyphs.cpp should use correct origin attributes to create principal

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 --- affected
firefox47 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted[OA])

Attachments

(1 file)

Whiteboard: gfx-noted
Assignee: nobody → allstars.chh
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)
Attached patch PatchSplinter Review
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)
https://hg.mozilla.org/mozilla-central/rev/e46b45235799
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Whiteboard: gfx-noted → gfx-noted[OA]
You need to log in before you can comment on or make changes to this bug.