Remove SEC_NORMAL where loadingPrincipal is SystemPrincipal or NullPrincipal

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
Status: NEW → ASSIGNED
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
Attachment #8728083 - Flags: review?(jonas) → review+
(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
Attachment #8728083 - Attachment is obsolete: true
Attachment #8728141 - Flags: review+
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
Flags: needinfo?(mozilla)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cbab9ec76d568da7079604bf5a3dc8c72198671
Bug 1254689 - Remove SEC_NORMAL where loadingPrincipal is SystemPrincipal or NullPrincipal (r=sicking)
(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!
Flags: needinfo?(mozilla)
Bug 1254752 was responsible for the backout, not this one. Rebased the patch. Carrying over r+.
Attachment #8728141 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
Attachment #8728627 - Flags: review+

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37b591f54363
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.