Summary: | [BlackBerry] Possible to clobber httponly cookie. | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Liu <jasonliuwebkit> | ||||||||||||||||||||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, dannin, dglazkov, jochen, joenotcharles, pnormand, rakuco, rwlbuis, tonikitoo, webkit.review.bot, zan | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||||||
OS: | Other | ||||||||||||||||||||||||||
Bug Depends on: | 87208, 87255 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Jason Liu
2012-05-10 01:30:11 PDT
(In reply to comment #0) > The httponly cookies should be changed by javaScript. Sorry. The httponly cookies should not be changed by javaScript. 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? Created attachment 142673 [details]
Patch
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 (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. (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. 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? Created attachment 143218 [details]
Patch
(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. Created attachment 143241 [details]
Patch
The change looks good to me, thanks (I'm no reviewer, so I can't r+ it..) (In reply to comment #14) > The change looks good to me, thanks > > (I'm no reviewer, so I can't r+ it..) Thanks. :) Comment on attachment 143241 [details]
Patch
Looks good.
Comment on attachment 143241 [details] Patch Clearing flags on attachment: 143241 Committed r118105: <http://trac.webkit.org/changeset/118105> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 87255 (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. 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? (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. : ( Created attachment 145520 [details]
Patch
(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 (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 (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()+36000000, "", "", 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) Comment on attachment 145520 [details]
Patch
R- based on Jochen's comments.
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> Created attachment 145753 [details]
Patch
(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 (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? (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? (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. (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 (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. 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 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
(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. 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 (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 (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. Created attachment 145962 [details]
Patch
(In reply to comment #52) > Created an attachment (id=145962) [details] > Patch That patch looks good to me Comment on attachment 145962 [details]
Patch
Looks good.
Comment on attachment 145962 [details] Patch Clearing flags on attachment: 145962 Committed r119947: <http://trac.webkit.org/changeset/119947> All reviewed patches have been landed. Closing bug. 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/. |