1272594 - Remove special cookie policy handling within InternalRequest

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

The special code landed within Bug 1188822 [1]. We now have support for asyncOpen2() for stylesheets as well as imgLoader (See Bug 1206961 and Bug 1195173). I suppose we can remove that code now.

[1] https://hg.mozilla.org/mozilla-central/rev/74ab992b6952#l2.24

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

Ben, I am not entirely sure, but I suppose we can remove that special handling now that we support asyncOpen2() for images and styles also, right? Or am I missing something?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Christoph, what code still uses AsyncOpen() in the tree?

If there is still a call site that uses AsyncOpen() and can be intercepted by service workers then we need to keep this code.  That particular test exercised images and stylesheets, but there could be other types of loads without test coverage.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Kelly [:bkelly] from comment #3)
> Christoph, what code still uses AsyncOpen() in the tree?

We still have to convert:
* TYPE_OBJECT
* TYPE_DOCUMENT
* TYPE_WEBSOCKET

If you think we should rather wait till we have converted all the callsites to use AsyncOpen2(), then that's fine with me. We can keep this bug around and land once we are done converting the whole tree to rely on AsyncOpen2().

Feel free to clear the r?.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8752102 [details] [diff] [review]
bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

TYPE_OBJECT and TYPE_WEBSOCKET are not intercepted by service workers.  We need to wait for TYPE_DOCUMENT to be converted to AsyncOpen2(), though.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

Ben, the old patch is outdated. I rebased the logic and I think we can only remove the switch statement as of now, right?

I think all the other parts need to be fixed within Bug 1189945.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8823579 [details] [diff] [review]
bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

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

Sorry, I think this is removing the wrong code.  We need to modify MapChannelToRequestCredentials() not MapChannelToRequestMode().  We want to remove this bit:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.cpp#446-464

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

(In reply to Ben Kelly [:bkelly] from comment #7)
> Sorry, I think this is removing the wrong code.  We need to modify
> MapChannelToRequestCredentials() not MapChannelToRequestMode().  We want to
> remove this bit:

Facepalm - you are absolutely right. Never rebase patches in the middle of the night. Here we go!

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8823802 [details] [diff] [review]
bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

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

Can you add a `MOZ_DIAGNOSTIC_ASSERT(loadInfo->GetSecurityMode() != nsILoadInfo::SEC_NORMAL)`? r=me with that change.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f67c56b19396
Remove special cookie policy handling within InternalRequest (r=bkelly)

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f67c56b19396