1254689 - Remove SEC_NORMAL where loadingPrincipal is SystemPrincipal or NullPrincipal

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1254689_remove_sec_normal_where_loading_is_system_or_null.patch

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8728083 [details] [diff] [review]
bug_1254689_remove_sec_normal_where_loading_is_system_or_null.patch

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

r=me with the below fixed

::: netwerk/base/nsIOService.cpp
@@ +691,5 @@
>    return NewChannelFromURI2(aURI,
>                              nullptr, // aLoadingNode
>                              nullptr, // aLoadingPrincipal
>                              nullptr, // aTriggeringPrincipal
> +                            nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

I don't think we should make this change. Same-origin makes no sense if we don't have a principal to check the origin against.

We should just deprecate this function and eventually remove it.

@@ +886,5 @@
>                                            aProxyFlags,
>                                            nullptr, // aLoadingNode
>                                            nullptr, // aLoadingPrincipal
>                                            nullptr, // aTriggeringPrincipal
> +                                          nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

Sane here

@@ +936,5 @@
>                       aBaseURI,
>                       nullptr, // aLoadingNode
>                       nullptr, // aLoadingPrincipal
>                       nullptr, // aTriggeringPrincipal
> +                     nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

And here

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1254689_remove_sec_normal_where_loading_is_system_or_null.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> > +                            nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,
> 
> I don't think we should make this change. Same-origin makes no sense if we
> don't have a principal to check the origin against.
> 
> We should just deprecate this function and eventually remove it.


Agreed, it doesn't make sense, but on the other hand this function is deprecated and not used anymore. I just wanted to eliminate all of SEC_NORMAL where there is no need for it anymore. Anyway, I am fine with filing a follow up and remove the deprecated versions of NS_NewChannel...()-API

Comment 4

[Carsten Book [:Tomcat]]

has problems to apply:

renamed 1254689 -> bug_1254689_remove_sec_normal_where_loading_is_system_or_null.patch
applying bug_1254689_remove_sec_normal_where_loading_is_system_or_null.patch
patching file js/xpconnect/loader/mozJSComponentLoader.cpp
Hunk #1 FAILED at 232
1 out of 1 hunks FAILED -- saving rejects to file js/xpconnect/loader/mozJSComponentLoader.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cbab9ec76d568da7079604bf5a3dc8c72198671
Bug 1254689 - Remove SEC_NORMAL where loadingPrincipal is SystemPrincipal or NullPrincipal (r=sicking)

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Carsten Book [:Tomcat] from comment #4)
> has problems to apply:
> 
> renamed 1254689 ->
> bug_1254689_remove_sec_normal_where_loading_is_system_or_null.patch
> applying bug_1254689_remove_sec_normal_where_loading_is_system_or_null.patch
> patching file js/xpconnect/loader/mozJSComponentLoader.cpp
> Hunk #1 FAILED at 232
> 1 out of 1 hunks FAILED -- saving rejects to file
> js/xpconnect/loader/mozJSComponentLoader.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory

Thanks Carsten!

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e26f9e5f41cd for breaking some devtools tests: https://treeherder.mozilla.org/logviewer.html#?job_id=23366109&repo=mozilla-inbound

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1254689_remove_sec_normal_where_loading_is_system_or_null.patch

Bug 1254752 was responsible for the backout, not this one. Rebased the patch. Carrying over r+.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37b591f54363

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37b591f54363