242564 - Default p3p cookie action changed to reject

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Daniel Veditz [:dveditz]]

The fix in bug 225298 changed the default P3P cookie action from UNKNOWN to
REJECT for default error cases. This is overly harsh considering p3p is the
default cookieBehavior and that the harshest action in the default policy is
FLAGGED.

This also happens to break a legacy proprietary extension that needs to retrieve
and set a cookie without having a channel, which could be a useful thing for
other similar service add-ons.

Comment 1

[Daniel Veditz [:dveditz]]

Attachment: revert default action (ignore all.js pref change)

Comment 2

[Darin Fisher]

Comment on attachment 147631 [details] [diff] [review]
revert default action (ignore all.js pref change)

>Index: netwerk/cookie/src/nsCookieService.cpp

>     nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(aChannel);
>-    if (httpChannel) {
>-      // lazily init the P3P service
>-      if (!mP3PService)
>-        mP3PService = do_GetService(NS_COOKIECONSENT_CONTRACTID);
> 
>-      if (mP3PService) {
>-        // get the site policy and a status decision for the cookie
>-        PRBool isForeign = IsForeign(aHostURI, aFirstURI);
>-        mP3PService->GetConsent(aHostURI, httpChannel, isForeign, &aPolicy, &p3pStatus);
>-      }
>+    // lazily init the P3P service
>+    if (!mP3PService)
>+      mP3PService = do_GetService(NS_COOKIECONSENT_CONTRACTID);
>+
>+    if (mP3PService) {
>+      // get the site policy and a status decision for the cookie
>+      PRBool isForeign = IsForeign(aHostURI, aFirstURI);
>+      mP3PService->GetConsent(aHostURI, httpChannel, isForeign, &aPolicy, &p3pStatus);
>     }

is this part of the patch really necessary?  why do we need to
consult the P3P service in the non-HTTP case?  why isn't the
change to all.js sufficient?

Comment 3

[Daniel Veditz [:dveditz]]

The change to all.js is not sufficient because it's still broken for users who
switch back to the current default setting. The pref change, in fact, is
possibly best left to the "remove p3p" bug.

The meat of the fix is changing the initial value for p3pstatus. The part you
quote is mostly whitespace change, it just drops the httpChannel check and lets
the p3p service deal with that case to be more in-line with the pre-225298
logic. nsP3PService::GetConsent explicitly handles no httpChannel (not as an
error) and there's no point in that code if the new cookie code prevented it
getting used.

Comment 4

[[e-mail]]

Comment on attachment 147631 [details] [diff] [review]
revert default action (ignore all.js pref change)

i recall noticing this back when i wrote the original patches, but i neglected
to remedy the logic change... your patch here looks spot-on, thanks for fixing
it.

although, please leave out the pref change in your patch, we have "other plans"
for that. ;)

r=dwitte with pref change omitted

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: same patch -w for review, dropping pref change

same patch minus the pref change (which won't be checked in), -w to make the
actual fix stand out from the outdenting noise.

Comment 6

[Darin Fisher]

Comment on attachment 147748 [details] [diff] [review]
same patch -w for review, dropping pref change

sr=darin

ok, sounds good.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 147748 [details] [diff] [review]
same patch -w for review, dropping pref change

Carrying over dwitte's r=

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 147748 [details] [diff] [review]
same patch -w for review, dropping pref change

a=asa (on behalf of drivers) for checkin to 1.7

Comment 9

[Daniel Veditz [:dveditz]]

Checked in

/cvsroot/mozilla/netwerk/cookie/src/nsCookieService.cpp,v <-- nsCookieService.cpp
new revision: 1.27; previous revision: 1.26

/cvsroot/mozilla/netwerk/cookie/src/nsCookieService.cpp,v <-- nsCookieService.cpp
new revision: 1.22.2.1; previous revision: 1.22