Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1714013002:
Load cookies once per 'document.cookie' read (Closed)
|
Created:
4 years, 10 months ago by Mike West Modified:
4 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@reallyonce Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLoad cookies once per 'document.cookie' read
This patch refactors RenderFrameMessageFilter to read cookies for a URL
once when requested via 'document.cookie', rather than reading them once
then checking cookie permissions, throwing the data away and requesting
it again to build the cookie string.
As a drive-by, it also fixes a bug in the logic which prevented
same-site cookies from being returned, and fixes a bug in a related
test for http-only cookies which turns out to have had no impact on
the feature, but meant that it's been basically untested for years.
BUG=581650
R=mmenke@chromium.org,rdsmith@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/df1821c63f29298b59528fdb5ab829f5898979f5
Cr-Commit-Position: refs/heads/master@{#377278}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Feedback. #
Total comments: 5
Patch Set 3 : Feedback. #Patch Set 4 : FILE_PATH_LITERAL #
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Load cookies once per 'document.cookie' read This patch refactors RenderFrameMessageFilter to read cookies for a URL once when requested via 'document.cookie', rather than reading them once then checking cookie permissions, throwing the data away and requesting it again to build the cookie string. As a drive-by, it also fixes a bug in the logic which prevented same-site cookies from being returned, and fixes a bug in a related test for http-only cookies which turns out to have had no impact on the feature, but meant that it's been basically untested for years. BUG=581650 R=mmenke@chromium.org,rdsmith@chromium.org ========== to ========== Load cookies once per 'document.cookie' read This patch refactors RenderFrameMessageFilter to read cookies for a URL once when requested via 'document.cookie', rather than reading them once then checking cookie permissions, throwing the data away and requesting it again to build the cookie string. As a drive-by, it also fixes a bug in the logic which prevented same-site cookies from being returned, and fixes a bug in a related test for http-only cookies which turns out to have had no impact on the feature, but meant that it's been basically untested for years. BUG=581650 R=mmenke@chromium.org,rdsmith@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Last one, I hope. Thanks for putting up with lots of reviews!
https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_message_filter.cc:399: // 'document.cookie', so do not include http-only cookies. nit: Avoid "we" in comments. https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_message_filter_browsertest.cc (right): https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_message_filter_browsertest.cc:56: GURL https_url = https_server.GetURL("/set_httponly_cookie.html"); What was wrong here? Looks like that was a built-in URL in the test server: https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_... https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_message_filter_browsertest.cc:113: // never allow this to be sent to any renderer process. This comment is incorrect. https://codereview.chromium.org/1714013002/diff/1/content/test/data/set_httpo... File content/test/data/set_httponly_cookie.html (right): https://codereview.chromium.org/1714013002/diff/1/content/test/data/set_httpo... content/test/data/set_httponly_cookie.html:10: I don't think any of this file is needed. The tests use NavigateToURL instead of waiting on the title, so this file can just be empty or "foo" or something. https://codereview.chromium.org/1714013002/diff/1/content/test/data/set_sames... File content/test/data/set_samesite_cookie.html (right): https://codereview.chromium.org/1714013002/diff/1/content/test/data/set_sames... content/test/data/set_samesite_cookie.html:10: I don't think any of this file is needed. The tests use NavigateToURL instead of waiting on the title, so this file can just be empty or "foo" or something.
And good job on catching this!
mkwst@chromium.org changed reviewers: + jochen@chromium.org
mkwst@chromium.org changed required reviewers: + mmenke@chromium.org
mmenke@: Thanks for the feedback, I figured out that the bug in the test was super simple, and we don't actually need the HTML files I added. Mind taking another look? jochen@: Would you mind stamping //content so I can land this once mmenke@ is happy with the actual change? https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_message_filter.cc:399: // 'document.cookie', so do not include http-only cookies. On 2016/02/19 at 17:34:42, mmenke wrote: > nit: Avoid "we" in comments. Done. https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_message_filter_browsertest.cc (right): https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_message_filter_browsertest.cc:56: GURL https_url = https_server.GetURL("/set_httponly_cookie.html"); On 2016/02/19 at 17:34:42, mmenke wrote: > What was wrong here? Looks like that was a built-in URL in the test server: https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_... Ah! It was failing because the HTTPS server wasn't set up correctly. I'll fix that instead of adding these files; thanks for poking! https://codereview.chromium.org/1714013002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_message_filter_browsertest.cc:113: // never allow this to be sent to any renderer process. On 2016/02/19 at 17:34:42, mmenke wrote: > This comment is incorrect. The dangers of cargo-culting a test... Thanks for catching it!
lgtm
I think it's worthwhile having a comment in the CL description that the behavior of cookies sent up to the delegate (and thence to extensions, correct?) has changed and no longer includes http only cookies. Just in case that breaks something and someone's searching through the commit history to figure out what could have done so. Beyond that, and with nit below, LGTM. Thanks! https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_message_filter.cc:399: // 'document.cookie', so do not include http-only cookies. It's sorta weird to have a comment like this without any indication of what piece of the code (IIUC, the default constructor for CookieOptions) is implementing it. Also, this comment feels like it's more about the transition from the old code to the new (fixing a bug) than they are about the new code in static form. Could you either add a note about the default constructor being what avoids inclusion of http-only cookies or remove the comment entirely if you agree with my transition comment?
LGTM https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_message_filter.cc:399: // 'document.cookie', so do not include http-only cookies. On 2016/02/22 15:30:58, Randy Smith - Not in Fridays wrote: > It's sorta weird to have a comment like this without any indication of what > piece of the code (IIUC, the default constructor for CookieOptions) is > implementing it. Also, this comment feels like it's more about the transition > from the old code to the new (fixing a bug) than they are about the new code in > static form. Could you either add a note about the default constructor being > what avoids inclusion of http-only cookies or remove the comment entirely if you > agree with my transition comment? Hrm....We may want to get just rid of the default values for CookieOptions, because it's unclear what they are, when looking at code that uses CookieOptions, which can lead to bugs. Maybe require them all to be explicitly specified. More wordy, but clearer... net::CookieOptions options(CookieOptions::INCLUDE_SAME_SITE, CookieOptions::INCLUDE_HTTP_ONLY, CookieOptions::SAVE_SOME_FOR_SANTA, ...); Also seems weird that the same struct is used for setting and getting cookies, despite some only applying to setters, some only to getters, and INCLUDE_HTTP_ONLY having a very different meaning for setters and getters. https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_message_filter_browsertest.cc (right): https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_message_filter_browsertest.cc:114: GURL url = embedded_test_server()->GetURL("/set-cookie?samesite=1;SameSite"); Should include url/gurl.h
On 2016/02/22 16:18:45, mmenke wrote: > LGTM > > https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... > File content/browser/frame_host/render_frame_message_filter.cc (right): > > https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... > content/browser/frame_host/render_frame_message_filter.cc:399: // > 'document.cookie', so do not include http-only cookies. > On 2016/02/22 15:30:58, Randy Smith - Not in Fridays wrote: > > It's sorta weird to have a comment like this without any indication of what > > piece of the code (IIUC, the default constructor for CookieOptions) is > > implementing it. Also, this comment feels like it's more about the transition > > from the old code to the new (fixing a bug) than they are about the new code > in > > static form. Could you either add a note about the default constructor being > > what avoids inclusion of http-only cookies or remove the comment entirely if > you > > agree with my transition comment? > > Hrm....We may want to get just rid of the default values for CookieOptions, > because it's unclear what they are, when looking at code that uses > CookieOptions, which can lead to bugs. > > Maybe require them all to be explicitly specified. More wordy, but clearer... > > net::CookieOptions options(CookieOptions::INCLUDE_SAME_SITE, > CookieOptions::INCLUDE_HTTP_ONLY, > CookieOptions::SAVE_SOME_FOR_SANTA, > ...); > > Also seems weird that the same struct is used for setting and getting cookies, > despite some only applying to setters, some only to getters, and > INCLUDE_HTTP_ONLY having a very different meaning for setters and getters. +1 to all these concerns. > > https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... > File content/browser/frame_host/render_frame_message_filter_browsertest.cc > (right): > > https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... > content/browser/frame_host/render_frame_message_filter_browsertest.cc:114: GURL > url = embedded_test_server()->GetURL("/set-cookie?samesite=1;SameSite"); > Should include url/gurl.h
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, rdsmith@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1714013002/#ps40001 (title: "Feedback.")
https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_message_filter.cc:399: // 'document.cookie', so do not include http-only cookies. On 2016/02/22 at 16:18:45, mmenke wrote: > On 2016/02/22 15:30:58, Randy Smith - Not in Fridays wrote: > > It's sorta weird to have a comment like this without any indication of what > > piece of the code (IIUC, the default constructor for CookieOptions) is > > implementing it. Also, this comment feels like it's more about the transition > > from the old code to the new (fixing a bug) than they are about the new code in > > static form. Could you either add a note about the default constructor being > > what avoids inclusion of http-only cookies or remove the comment entirely if you > > agree with my transition comment? > > Hrm....We may want to get just rid of the default values for CookieOptions, because it's unclear what they are, when looking at code that uses CookieOptions, which can lead to bugs. > > Maybe require them all to be explicitly specified. More wordy, but clearer... > > net::CookieOptions options(CookieOptions::INCLUDE_SAME_SITE, > CookieOptions::INCLUDE_HTTP_ONLY, > CookieOptions::SAVE_SOME_FOR_SANTA, > ...); > > Also seems weird that the same struct is used for setting and getting cookies, despite some only applying to setters, some only to getters, and INCLUDE_HTTP_ONLY having a very different meaning for setters and getters. I agree that something like this would make sense; I guess I'll poke at that as part of the "clean up this sprawling API" bug. Dropped the comment in the meantime. https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_message_filter_browsertest.cc (right): https://codereview.chromium.org/1714013002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_message_filter_browsertest.cc:114: GURL url = embedded_test_server()->GetURL("/set-cookie?samesite=1;SameSite"); On 2016/02/22 at 16:18:45, mmenke wrote: > Should include url/gurl.h Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714013002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, rdsmith@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1714013002/#ps60001 (title: "FILE_PATH_LITERAL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714013002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714013002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Load cookies once per 'document.cookie' read This patch refactors RenderFrameMessageFilter to read cookies for a URL once when requested via 'document.cookie', rather than reading them once then checking cookie permissions, throwing the data away and requesting it again to build the cookie string. As a drive-by, it also fixes a bug in the logic which prevented same-site cookies from being returned, and fixes a bug in a related test for http-only cookies which turns out to have had no impact on the feature, but meant that it's been basically untested for years. BUG=581650 R=mmenke@chromium.org,rdsmith@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Load cookies once per 'document.cookie' read This patch refactors RenderFrameMessageFilter to read cookies for a URL once when requested via 'document.cookie', rather than reading them once then checking cookie permissions, throwing the data away and requesting it again to build the cookie string. As a drive-by, it also fixes a bug in the logic which prevented same-site cookies from being returned, and fixes a bug in a related test for http-only cookies which turns out to have had no impact on the feature, but meant that it's been basically untested for years. BUG=581650 R=mmenke@chromium.org,rdsmith@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/df1821c63f29298b59528fdb5ab829f5898979f5 Cr-Commit-Position: refs/heads/master@{#377278} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/df1821c63f29298b59528fdb5ab829f5898979f5 Cr-Commit-Position: refs/heads/master@{#377278} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
