Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563083002:
[DevTools] show warning if 'Set-Cookie' header length is > 4096 symbols (Closed)
|
Created:
4 years ago by kozy Modified:
4 years ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] show warning if 'Set-Cookie' header length is > 4096 symbols
BUG=chromium:643400
R=allada@chromium.org
Committed: https://crrev.com/37be34422bf87bbaaeb668c628c2fa494c820299
Cr-Commit-Position: refs/heads/master@{#439309}
Patch Set 1 #
Total comments: 4
Patch Set 2 : addressed comments #
Total comments: 10
Patch Set 3 : addressed comments #
Total comments: 6
Patch Set 4 : addressed all comments #
Messages
Total messages: 29 (16 generated)
Blaise, please take a look.
Can we also write a test? https://codereview.chromium.org/2563083002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2563083002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:376: if (response.headers['Set-Cookie'].length > 4096) { Lets put both if statements on same line. https://codereview.chromium.org/2563083002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:376: if (response.headers['Set-Cookie'].length > 4096) { Can we also put a comment in here to where/what the 4096 number is tracking in net stack?
Patchset #2 (id:20001) has been deleted
all done, please take another look. Programming in PHP is great :) https://codereview.chromium.org/2563083002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2563083002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:376: if (response.headers['Set-Cookie'].length > 4096) { On 2016/12/09 at 18:26:50, Blaise wrote: > Lets put both if statements on same line. Done. https://codereview.chromium.org/2563083002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:376: if (response.headers['Set-Cookie'].length > 4096) { On 2016/12/09 18:26:50, Blaise wrote: > Can we also put a comment in here to where/what the 4096 number is tracking in > net stack? Done.
Looks good, I especially like the PHP work! ;-) https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-long-cookie.php (right): https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-long-cookie.php:1: <?php Maybe it's better to just modify this to meet all our needs: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/warning-for-long-cookie.html (right): https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/warning-for-long-cookie.html:11: xhr.open('GET', `http://localhost:8000/inspector/network/resources/set-long-cookie.php?length=${length}`, true); We are trying to stay away from using localhost in tests because sometimes they resolve to [::1] and sometimes 127.0.0.1 (and who knows in the future!?), so we try to hard code ip addresses instead. They seem much safer. https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/warning-for-long-cookie.html:17: InspectorTest.addConsoleViewSniffer(dumpMessages); I'm not against this, however the test will timeout easier any time there would be a failure to log because we would never receive a console log event (or simply fail if we print other messages about his request). Can we instead add a sniffer on network request, something like: InspectorTest.makeFetch("network/resources/set-long-cookie.php?length=4097", {}, dumpMessages); https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:378: 'Set-Cookie header is ignored for response from url: %s. Cookie length should be less then or equal to 4096 symbols.', I think we should rename symbols to "characters" or "bytes", I think netstack does it's count in characters/bytes not symbols. https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:378: 'Set-Cookie header is ignored for response from url: %s. Cookie length should be less then or equal to 4096 symbols.', nit: s/for/in/
kozyatinskiy@chromium.org changed reviewers: + lushnikov@chromium.org
thanks, all done. Andrey, please take a look too, I need owner lgtm. https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-long-cookie.php (right): https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-long-cookie.php:1: <?php On 2016/12/10 09:48:22, allada wrote: > Maybe it's better to just modify this to meet all our needs: > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... Done. https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/warning-for-long-cookie.html (right): https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/warning-for-long-cookie.html:11: xhr.open('GET', `http://localhost:8000/inspector/network/resources/set-long-cookie.php?length=${length}`, true); On 2016/12/10 09:48:22, allada wrote: > We are trying to stay away from using localhost in tests because sometimes they > resolve to [::1] and sometimes 127.0.0.1 (and who knows in the future!?), so we > try to hard code ip addresses instead. They seem much safer. Done. https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/warning-for-long-cookie.html:17: InspectorTest.addConsoleViewSniffer(dumpMessages); On 2016/12/10 09:48:22, allada wrote: > I'm not against this, however the test will timeout easier any time there would > be a failure to log because we would never receive a console log event (or > simply fail if we print other messages about his request). Can we instead add a > sniffer on network request, something like: > > InspectorTest.makeFetch("network/resources/set-long-cookie.php?length=4097", {}, > dumpMessages); Done. https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:378: 'Set-Cookie header is ignored for response from url: %s. Cookie length should be less then or equal to 4096 symbols.', On 2016/12/10 09:48:22, allada wrote: > I think we should rename symbols to "characters" or "bytes", I think netstack > does it's count in characters/bytes not symbols. Done. https://codereview.chromium.org/2563083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:378: 'Set-Cookie header is ignored for response from url: %s. Cookie length should be less then or equal to 4096 symbols.', On 2016/12/10 09:48:22, allada wrote: > nit: s/for/in/ Done.
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
some nits, but lgtm https://codereview.chromium.org/2563083002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-cookie.php (right): https://codereview.chromium.org/2563083002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-cookie.php:6: header("Access-Control-Allow-Origin:http://127.0.0.1:8000"); nit: space after : https://codereview.chromium.org/2563083002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-cookie.php:7: if (!isset($_GET['length'])) { Lets use: $length = isset($_GET["length"]) ? (int) $_GET["length"] : 0; if ($length) { ... This will ensure we do not send bad headers because if you send a url param of "length=hello" it will convert it to int(0) causing the header to be sent as: header("Set-Cookie: "); which is invalid. https://codereview.chromium.org/2563083002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-cookie.php:11: for ($i = 0; $i < $_GET["length"]; $i++) { PHP is weird with it's soft variable types. Lets use: for ($i = 0; $i < $length; $i++) { for safety.
thanks, all done. https://codereview.chromium.org/2563083002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-cookie.php (right): https://codereview.chromium.org/2563083002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-cookie.php:6: header("Access-Control-Allow-Origin:http://127.0.0.1:8000"); On 2016/12/16 22:33:21, allada wrote: > nit: space after : Done. https://codereview.chromium.org/2563083002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-cookie.php:7: if (!isset($_GET['length'])) { On 2016/12/16 22:33:21, allada wrote: > Lets use: > > $length = isset($_GET["length"]) ? (int) $_GET["length"] : 0; > > if ($length) { > ... > > This will ensure we do not send bad headers because if you send a url param of > "length=hello" it will convert it to int(0) causing the header to be sent as: > header("Set-Cookie: "); which is invalid. Done. https://codereview.chromium.org/2563083002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/set-cookie.php:11: for ($i = 0; $i < $_GET["length"]; $i++) { On 2016/12/16 22:33:21, allada wrote: > PHP is weird with it's soft variable types. Lets use: > > for ($i = 0; $i < $length; $i++) { > > for safety. Done.
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks!
The CQ bit was unchecked by kozyatinskiy@chromium.org
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from allada@chromium.org Link to the patchset: https://codereview.chromium.org/2563083002/#ps80001 (title: "addressed all comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481939636280180,
"parent_rev": "e9780157d4ac1229bf5f74dd415bd6da83e8a870", "commit_rev":
"2cd4bcc05cf2d8e3197dead3a5eda5e6fec2b46f"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] show warning if 'Set-Cookie' header length is > 4096 symbols BUG=chromium:643400 R=allada@chromium.org ========== to ========== [DevTools] show warning if 'Set-Cookie' header length is > 4096 symbols BUG=chromium:643400 R=allada@chromium.org Review-Url: https://codereview.chromium.org/2563083002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] show warning if 'Set-Cookie' header length is > 4096 symbols BUG=chromium:643400 R=allada@chromium.org Review-Url: https://codereview.chromium.org/2563083002 ========== to ========== [DevTools] show warning if 'Set-Cookie' header length is > 4096 symbols BUG=chromium:643400 R=allada@chromium.org Committed: https://crrev.com/37be34422bf87bbaaeb668c628c2fa494c820299 Cr-Commit-Position: refs/heads/master@{#439309} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/37be34422bf87bbaaeb668c628c2fa494c820299 Cr-Commit-Position: refs/heads/master@{#439309} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
