Bug 86067 – [BlackBerry] Possible to clobber httponly cookie.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Jason Liu]

The httponly cookies should be changed by javaScript.

[Jason Liu]

(In reply to comment #0)
> The httponly cookies should be changed by javaScript.

Sorry. The httponly cookies should not be changed by javaScript.

[Jason Liu]

Created attachment 142647 [details]
Patch

Comment on attachment 142647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142647&action=review

> Source/WebCore/platform/blackberry/CookieManager.h:82
> +    void setCookies(const KURL&, const String& value, bool isHttpCookie = true);

Would it be more consistent with the other methods in this class to define an enum instead of using a bool?

[Jason Liu]

Created attachment 142673 [details]
Patch

[George Staikos]

Comment on attachment 142647 [details]
Patch

This seems incredibly inefficient. Is this the best we can do?

Comment on attachment 142673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142673&action=review

> Source/WebCore/platform/blackberry/CookieManager.cpp:513
> +    getRawCookies(cookies, url, WithHttpOnlyCookies);

the only way I see this could be optimized is to move getRawCookies out of the loop. In the end, to insert a cookie, you'll have to check all cookies for the given url, so there's not really much to be improved

> LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.php:17
> +

nit. no empty lines at eof

[Jason Liu]

(In reply to comment #3)
> (From update of attachment 142647 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142647&action=review
> 
> > Source/WebCore/platform/blackberry/CookieManager.h:82
> > +    void setCookies(const KURL&, const String& value, bool isHttpCookie = true);
> 
> Would it be more consistent with the other methods in this class to define an enum instead of using a bool?

(In reply to comment #5)
> (From update of attachment 142647 [details])
> This seems incredibly inefficient. Is this the best we can do?

I thought the cookies are always set by http. And they are seldom set by JavaScript. So I didn't want to add more code into setCookies' deeper code path which may decrease the performance of http cookies.

Maybe above idea is wrong or there is a better method. 
Let me think it over.

[Jason Liu]

(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 142647 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=142647&action=review
> > 
> > > Source/WebCore/platform/blackberry/CookieManager.h:82
> > > +    void setCookies(const KURL&, const String& value, bool isHttpCookie = true);
> > 
> > Would it be more consistent with the other methods in this class to define an enum instead of using a bool?
> 
> (In reply to comment #5)
> > (From update of attachment 142647 [details] [details])
> > This seems incredibly inefficient. Is this the best we can do?
> 
> I thought the cookies are always set by http. And they are seldom set by JavaScript. So I didn't want to add more code into setCookies' deeper code path which may decrease the performance of http cookies.
> 
> Maybe above idea is wrong or there is a better method. 
> Let me think it over.

I will change my patch.

[Jason Liu]

Created attachment 143215 [details]
Patch

Comment on attachment 143215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143215&action=review

> Source/WebCore/platform/blackberry/CookieMap.cpp:57
> +ParsedCookie* CookieMap::addOrReplaceCookie(ParsedCookie* cookie, CookieFilter filter)

I think the semantics that returning cookie means the cookie was not added is very unclear.

Can you make it return a bool indicating whether the cookie was added or not, and add a parameter ParsedCookie** replacedCookie which is set to the replaced cookie if the return value is true?

[Jason Liu]

Created attachment 143218 [details]
Patch

[Jason Liu]

(In reply to comment #10)
> (From update of attachment 143215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143215&action=review
> 
> > Source/WebCore/platform/blackberry/CookieMap.cpp:57
> > +ParsedCookie* CookieMap::addOrReplaceCookie(ParsedCookie* cookie, CookieFilter filter)
> 
> I think the semantics that returning cookie means the cookie was not added is very unclear.
> 
> Can you make it return a bool indicating whether the cookie was added or not, and add a parameter ParsedCookie** replacedCookie which is set to the replaced cookie if the return value is true?

Let me have a look. Thanks for your high-speed reply and this good suggestion.

[Jason Liu]

Created attachment 143241 [details]
Patch

The change looks good to me, thanks

(I'm no reviewer, so I can't r+ it..)

[Jason Liu]

(In reply to comment #14)
> The change looks good to me, thanks
> 
> (I'm no reviewer, so I can't r+ it..)
Thanks. ：）

[Rob Buis]

Comment on attachment 143241 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 143241 [details]
Patch

Clearing flags on attachment: 143241

Committed r118105: <http://trac.webkit.org/changeset/118105>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 87255

[Jason Liu]

(In reply to comment #15)
> (In reply to comment #14)
> > The change looks good to me, thanks
> > 
> > (I'm no reviewer, so I can't r+ it..)
> Thanks. ：）

Any idea about my test case?
I don't think it can cause the other test case failing.

Thanks.

[Jason Liu]

Will I commit my test case in our own platform

(In reply to comment #21)
> Will I commit my test case in our own platform

I think that's not really desirable. It seems that no port should have the ability to override http cookies from javascript.

Do you have any idea why the test passed on the EWS (at least the cr-linux bot also executes tests) but then failed when you committed it? can you reproduce this locally on any port?

[Jason Liu]

(In reply to comment #22)
> (In reply to comment #21)
> > Will I commit my test case in our own platform
> 
> I think that's not really desirable. It seems that no port should have the ability to override http cookies from javascript.
> 
> Do you have any idea why the test passed on the EWS (at least the cr-linux bot also executes tests) but then failed when you committed it? can you reproduce this locally on any port?

Maybe there are problems in their own platform DRT running.
I don't have the other platform's environment. So can't try to reproduce it.
I really can't find any problems in my test case.
: (

[Jason Liu]

Created attachment 145520 [details]
Patch

[Jason Liu]

(In reply to comment #24)
> Created an attachment (id=145520) [details]
> Patch

I can't test all the other platforms. And so don't know why (In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Will I commit my test case in our own platform
> > 
> > I think that's not really desirable. It seems that no port should have the ability to override http cookies from javascript.
> > 
> > Do you have any idea why the test passed on the EWS (at least the cr-linux bot also executes tests) but then failed when you committed it? can you reproduce this locally on any port?
> 
> Maybe there are problems in their own platform DRT running.
> I don't have the other platform's environment. So can't try to reproduce it.
> I really can't find any problems in my test case.
> : (

I put this test case to blackberry platform.

(In reply to comment #25)
> I put this test case to blackberry platform.

Given that the test case described behavior that should work on all platforms, and should work the same on all platforms, I think this is the wrong solution.

What operating system are you developing on? Building other ports usually doesn't require much effort

[Jason Liu]

(In reply to comment #26)
> (In reply to comment #25)
> > I put this test case to blackberry platform.
> 
> Given that the test case described behavior that should work on all platforms, and should work the same on all platforms, I think this is the wrong solution.
> 
> What operating system are you developing on? Building other ports usually doesn't require much effort

[Jason Liu]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > I put this test case to blackberry platform.
> > 
> > Given that the test case described behavior that should work on all platforms, and should work the same on all platforms, I think this is the wrong solution.
> > 
> > What operating system are you developing on? Building other ports usually doesn't require much effort

Please have a look at 87208. They are Mac, GTK, and qt.
Would you help me if you have these platform's building already?

Comment on attachment 145520 [details]
Patch

In order to successfully contribute to WebKit, you should be able to build a few other ports..

View in context: https://bugs.webkit.org/attachment.cgi?id=145520&action=review

> LayoutTests/platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie.php:2
> +setcookie("httpOnlyCookie", "value", time()[phone], "", "", 0, 1);

you should use the cookie scripts located in http/tests/cookies/resources as in your previos patch to set the cookie.

The bug in your previous patch was that you use php to set the cookie, instead of the setCookie helper function. Therefore, the call to clearCookies() had no effect.

Please revert to the previous version, but change your test to not use PHP to set the cookie. Please also remove the baselines committed by other ports (such as LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt)

[Rob Buis]

Comment on attachment 145520 [details]
Patch

R- based on Jochen's comments.

[Jason Liu]

Created attachment 145748 [details]
Patch

Comment on attachment 145748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145748&action=review

> Source/WebCore/ChangeLog:11
> +        Test: platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie.php

nit: the test is not in platform/blackberry anymore

> LayoutTests/ChangeLog:8
> +        * platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt: Added.

also here. Please also delete the old test expectations/unskip the test and update this list accordingly

> LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.html:1
> +<!DCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

[Jason Liu]

Created attachment 145753 [details]
Patch

[Jason Liu]

(In reply to comment #32)
> (From update of attachment 145748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145748&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Test: platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie.php
> 
> nit: the test is not in platform/blackberry anymore
> 
> > LayoutTests/ChangeLog:8
> > +        * platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt: Added.
> 
> also here. Please also delete the old test expectations/unskip the test and update this list accordingly
> 
> > LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.html:1
> > +<!DCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> <!DOCTYPE html>

Thanks a lot for your help. : )

Comment on attachment 145753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review

> LayoutTests/ChangeLog:9
> +        * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added.

there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt

can you delete that file as part of this CL? otherwise, it won't pass on the mac bots

[Jason Liu]

(In reply to comment #35)
> (From update of attachment 145753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added.
> 
> there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt
> 
> can you delete that file as part of this CL? otherwise, it won't pass on the mac bots

I think mac will fail with the new test, too. 
So LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt is needed to make mac pass.

(In reply to comment #36)
> (In reply to comment #35)
> > (From update of attachment 145753 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review
> > 
> > > LayoutTests/ChangeLog:9
> > > +        * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added.
> > 
> > there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt
> > 
> > can you delete that file as part of this CL? otherwise, it won't pass on the mac bots
> 
> I think mac will fail with the new test, too. 
> So LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt is needed to make mac pass.

Why do you think so?

[Jason Liu]

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > (From update of attachment 145753 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review
> > > 
> > > > LayoutTests/ChangeLog:9
> > > > +        * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added.
> > > 
> > > there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt
> > > 
> > > can you delete that file as part of this CL? otherwise, it won't pass on the mac bots
> > 
> > I think mac will fail with the new test, too. 
> > So LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt is needed to make mac pass.
> 
> Why do you think so?

In my opinion, mac added this *.txt to make the old test *.php pass.
Although I changed the test case, mac's result will be also the same as *.txt.
So it is needed for mac all the same.
Do you agree with me?

[Jason Liu]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > (In reply to comment #35)
> > > > (From update of attachment 145753 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review
> > > > 
> > > > > LayoutTests/ChangeLog:9
> > > > > +        * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added.
> > > > 
> > > > there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt
> > > > 
> > > > can you delete that file as part of this CL? otherwise, it won't pass on the mac bots
> > > 
> > > I think mac will fail with the new test, too. 
> > > So LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt is needed to make mac pass.
> > 
> > Why do you think so?
> 
> In my opinion, mac added this *.txt to make the old test *.php pass.
> Although I changed the test case, mac's result will be also the same as *.txt.
> So it is needed for mac all the same.
> Do you agree with me?

If I remove it, mac will fail again.

(In reply to comment #38)
> In my opinion, mac added this *.txt to make the old test *.php pass.
> Although I changed the test case, mac's result will be also the same as *.txt.
> So it is needed for mac all the same.
> Do you agree with me?

I'd hope that now that you use the cookies testing framework, the test passes on all platforms.

[Jason Liu]

(In reply to comment #40)
> (In reply to comment #38)
> > In my opinion, mac added this *.txt to make the old test *.php pass.
> > Although I changed the test case, mac's result will be also the same as *.txt.
> > So it is needed for mac all the same.
> > Do you agree with me?
> 
> I'd hope that now that you use the cookies testing framework, the test passes on all platforms.

I think mac fails because its httpOnly cookies can be changed by JavaScript.

(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #38)
> > > In my opinion, mac added this *.txt to make the old test *.php pass.
> > > Although I changed the test case, mac's result will be also the same as *.txt.
> > > So it is needed for mac all the same.
> > > Do you agree with me?
> > 
> > I'd hope that now that you use the cookies testing framework, the test passes on all platforms.
> 
> I think mac fails because its httpOnly cookies can be changed by JavaScript.

Ok, I manually verified this.

Note that the platform/mac suppression will also apply to other ports, so when you land this change, there will be some redness.

Anyway, can you comment on https://bugs.webkit.org/show_bug.cgi?id=87208 that the failure on mac is because of an actual bug in their cookie jar?

otherwise, the patch looks good

[Jason Liu]

(In reply to comment #42)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > (In reply to comment #38)
> > > > In my opinion, mac added this *.txt to make the old test *.php pass.
> > > > Although I changed the test case, mac's result will be also the same as *.txt.
> > > > So it is needed for mac all the same.
> > > > Do you agree with me?
> > > 
> > > I'd hope that now that you use the cookies testing framework, the test passes on all platforms.
> > 
> > I think mac fails because its httpOnly cookies can be changed by JavaScript.
> 
> Ok, I manually verified this.
> 
> Note that the platform/mac suppression will also apply to other ports, so when you land this change, there will be some redness.
> 
> Anyway, can you comment on https://bugs.webkit.org/show_bug.cgi?id=87208 that the failure on mac is because of an actual bug in their cookie jar?
> 
> otherwise, the patch looks good

I've added comments for 87208.

Thanks a lot for your help.

[WebKit Review Bot]

Comment on attachment 145753 [details]
Patch

Attachment 145753 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12905704

New failing tests:
http/tests/cookies/js-get-and-set-http-only-cookie.html

[WebKit Review Bot]

Created attachment 145936 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jason Liu]

(In reply to comment #44)
> (From update of attachment 145753 [details])
> Attachment 145753 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12905704
> 
> New failing tests:
> http/tests/cookies/js-get-and-set-http-only-cookie.html

This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt.

I have to delete it since it effects chromium's right result.

[Jason Liu]

Created attachment 145945 [details]
Patch

(In reply to comment #46)
> (In reply to comment #44)
> > (From update of attachment 145753 [details] [details])
> > Attachment 145753 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/12905704
> > 
> > New failing tests:
> > http/tests/cookies/js-get-and-set-http-only-cookie.html
> 
> This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt.
> 
> I have to delete it since it effects chromium's right result.

I think the better solution is to add the right results to platform/chromium/http/tests/cookies/... because mac will need to failing results

[Jason Liu]

(In reply to comment #48)
> (In reply to comment #46)
> > (In reply to comment #44)
> > > (From update of attachment 145753 [details] [details] [details])
> > > Attachment 145753 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > Output: http://queues.webkit.org/results/12905704
> > > 
> > > New failing tests:
> > > http/tests/cookies/js-get-and-set-http-only-cookie.html
> > 
> > This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt.
> > 
> > I have to delete it since it effects chromium's right result.
> 
> I think the better solution is to add the right results to platform/chromium/http/tests/cookies/... because mac will need to failing results

And shall we do this on the other platforms? 
Why the other ports follow  LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt?

It seems better to remove this *.txt if there are more ports failing under this wrong result.
I think it is better to delete this wrong result and ask mac to fix the cookieJar's bug.

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #46)
> > > (In reply to comment #44)
> > > > (From update of attachment 145753 [details] [details] [details] [details])
> > > > Attachment 145753 [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > > Output: http://queues.webkit.org/results/12905704
> > > > 
> > > > New failing tests:
> > > > http/tests/cookies/js-get-and-set-http-only-cookie.html
> > > 
> > > This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt.
> > > 
> > > I have to delete it since it effects chromium's right result.
> > 
> > I think the better solution is to add the right results to platform/chromium/http/tests/cookies/... because mac will need to failing results
> 
> And shall we do this on the other platforms? 
> Why the other ports follow  LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt?

Because mac is somewhat like the "main" port.


> 
> It seems better to remove this *.txt if there are more ports failing under this wrong result.
> I think it is better to delete this wrong result and ask mac to fix the cookieJar's bug.

Until they fix this, they will add back the suppression. Anyway, that's what I meant in comment 42 - just leave the expectations as is, and let the gardeners figure out what to do

[Jason Liu]

(In reply to comment #50)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > (In reply to comment #46)
> > > > (In reply to comment #44)
> > > > > (From update of attachment 145753 [details] [details] [details] [details] [details])
> > > > > Attachment 145753 [details] [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > > > Output: http://queues.webkit.org/results/12905704
> > > > > 
> > > > > New failing tests:
> > > > > http/tests/cookies/js-get-and-set-http-only-cookie.html
> > > > 
> > > > This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt.
> > > > 
> > > > I have to delete it since it effects chromium's right result.
> > > 
> > > I think the better solution is to add the right results to platform/chromium/http/tests/cookies/... because mac will need to failing results
> > 
> > And shall we do this on the other platforms? 
> > Why the other ports follow  LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt?
> 
> Because mac is somewhat like the "main" port.
> 
> 
> > 
> > It seems better to remove this *.txt if there are more ports failing under this wrong result.
> > I think it is better to delete this wrong result and ask mac to fix the cookieJar's bug.
> 
> Until they fix this, they will add back the suppression. Anyway, that's what I meant in comment 42 - just leave the expectations as is, and let the gardeners figure out what to do

Ok, I will add the right result to chromium's platform.

[Jason Liu]

Created attachment 145962 [details]
Patch

(In reply to comment #52)
> Created an attachment (id=145962) [details]
> Patch

That patch looks good to me

[Rob Buis]

Comment on attachment 145962 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 145962 [details]
Patch

Clearing flags on attachment: 145962

Committed r119947: <http://trac.webkit.org/changeset/119947>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

This appears to have broken the EFL and GTK+ bots:

http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/33967/steps/layout-test/logs/stdio
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/1051/steps/layout-test/logs/stdio

The error diffs can be obtained via http://build.webkit.org/results/.