1364367 - rewrite test_bug379959.html

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

test_bug379959.html is considering data: URI as the same origin, we should revise it.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8867091 [details] [diff] [review]
Patch.

Yoshi, here is what I think we need to do: In those cases where we are actually test data: URI inheritance behavior like in the specific case of this bug, we need to clone the test. In more detail:

1) Keep that test as is but *explicitly* use:
>  SpecialPowers.setBoolPref("security.data_uri.inherit_security_context", true);

2) Clone that test (call it something like test_bug379959_data_not_inherits.html), use:
>  SpecialPowers.setBoolPref("security.data_uri.inherit_security_context", false);
and change the test like you did within this patch.

This way we will always have test coverage for both versions (data inherits and data does not inherit) depending on the actual value of the pref. Once we have flipped the pref and are confident that everything works as expected we can eliminate all the 'old' tests.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Comment on attachment 8867091 [details] [diff] [review]
> Patch.
> 
> Yoshi, here is what I think we need to do: In those cases where we are
> actually test data: URI inheritance behavior like in the specific case of
> this bug, we need to clone the test. In more detail:
> 
> 1) Keep that test as is but *explicitly* use:
> >  SpecialPowers.setBoolPref("security.data_uri.inherit_security_context", true);
I am not sure is it worth doing this?
Data URI should NOT inherit the origin per W3C spec, that's why we are fixing it now.

Why do we still need to spend time to maintain the 'WRONG' behavior?
Given that our priority should be 
1. To use the CORRECT behavior (bug 1324406)
2. To add tests to test the CORRECT behavior (bug 1365145)

And to be honest, the two items above will take a lots of time and effort, now we still spend more time to maintain old one, and once we fix all of them we also need to spend time to remove old ones....

I understand your concern but it doesn't seem to be so realistic to me.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Yoshi Huang [:allstars.chh] from comment #3)
> Why do we still need to spend time to maintain the 'WRONG' behavior?

Yoshi, as discussed in our meeting have to go down that route. In case we have to revert the pref for whatever reasons we need test coverage, otherwise we end up in a limbo state where we can't get out of.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch v2

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8868406 [details] [diff] [review]
Patch v2

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

Yoshi, thanks, that looks great. Should we potentially use _data_uri_legacy.* as the postfix for all of those tests? Or are you keeping a list of all of those tests which we can remove eventually. In fact, I think we could also just do a DXR search for |security.data_uri.unique_opaque_origin| and just check where the pref is false, right?

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Or are you keeping
> a list of all of those tests which we can remove eventually. In fact, I
> think we could also just do a DXR search for
> |security.data_uri.unique_opaque_origin| and just check where the pref is
> false, right?
Yeah, I think a DXR search is easier so I didn't maintain the list.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22c50e492cd2
rewrite test_bug379959.html. r=ckerschb

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22c50e492cd2