Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1322543003:
Fix Link header parsing bug with extension parameter parsing. (Closed)
|
Created:
5 years, 3 months ago by Yoav Weiss Modified:
5 years, 3 months ago CC:
blink-reviews, gavinp+loader_chromium.org, Nate Chapin, kinuko+watch, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix Link header parsing bug with extension parameter parsing.
The Link header wasn't handling well extensions parameters with no value
that were not followed by a white space.
This CL fixes that.
BUG=520698
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201642
Patch Set 1 #Patch Set 2 : Better comments #
Total comments: 1
Patch Set 3 : Changed valid char to match the spec #
Total comments: 2
Patch Set 4 : Use ASCIIType #
Total comments: 1
Patch Set 5 : Added comments, tests and aligned with the spec #
Total comments: 2
Messages
Total messages: 23 (4 generated)
yoav@yoav.ws changed reviewers: + japhet@chromium.org
Hey Nate! Can you take a look? (Mike did most of the previous reviews to that code, but he's off this week) Thanks :) Yoav
Looks reasonable. https://codereview.chromium.org/1322543003/diff/20001/Source/core/loader/Link... File Source/core/loader/LinkHeader.cpp (right): https://codereview.chromium.org/1322543003/diff/20001/Source/core/loader/Link... Source/core/loader/LinkHeader.cpp:28: return !isWhitespace(chr) && chr != '=' && chr != ';' && chr != ','; Is there a spec for this?
On 2015/08/27 21:39:16, Nate Chapin wrote: > Looks reasonable. > > https://codereview.chromium.org/1322543003/diff/20001/Source/core/loader/Link... > File Source/core/loader/LinkHeader.cpp (right): > > https://codereview.chromium.org/1322543003/diff/20001/Source/core/loader/Link... > Source/core/loader/LinkHeader.cpp:28: return !isWhitespace(chr) && chr != '=' && > chr != ';' && chr != ','; > Is there a spec for this? The spec for Link headers is https://tools.ietf.org/html/rfc5988 and parmname is defined in https://tools.ietf.org/html/rfc5987 Looking at it again, my implementation here does not match that definition. I'll match it and reupload.
On 2015/08/27 21:47:38, Yoav Weiss wrote: > On 2015/08/27 21:39:16, Nate Chapin wrote: > > Looks reasonable. > > > > > https://codereview.chromium.org/1322543003/diff/20001/Source/core/loader/Link... > > File Source/core/loader/LinkHeader.cpp (right): > > > > > https://codereview.chromium.org/1322543003/diff/20001/Source/core/loader/Link... > > Source/core/loader/LinkHeader.cpp:28: return !isWhitespace(chr) && chr != '=' > && > > chr != ';' && chr != ','; > > Is there a spec for this? > > The spec for Link headers is https://tools.ietf.org/html/rfc5988 and parmname is > defined in https://tools.ietf.org/html/rfc5987 > Looking at it again, my implementation here does not match that definition. I'll > match it and reupload. matched the implementation to the spec. Nate - can you take a second look?
https://codereview.chromium.org/1322543003/diff/40001/Source/core/loader/Link... File Source/core/loader/LinkHeader.cpp (right): https://codereview.chromium.org/1322543003/diff/40001/Source/core/loader/Link... Source/core/loader/LinkHeader.cpp:30: if ((chr >= 'a' && chr <= 'z') || (chr >= 'A' && chr <= 'Z') || (chr >= '0' && chr <= '9')) Can we simplify some of this function by reusing helpers in wtf/ASCIICType.h or similar? https://codereview.chromium.org/1322543003/diff/40001/Source/core/loader/Link... Source/core/loader/LinkHeader.cpp:35: || (chr >= '(' && chr <= '*') || chr == '\'' || chr == '"' || chr == '%') Nit: add {}
On 2015/08/27 23:11:48, Nate Chapin wrote: > https://codereview.chromium.org/1322543003/diff/40001/Source/core/loader/Link... > File Source/core/loader/LinkHeader.cpp (right): > > https://codereview.chromium.org/1322543003/diff/40001/Source/core/loader/Link... > Source/core/loader/LinkHeader.cpp:30: if ((chr >= 'a' && chr <= 'z') || (chr >= > 'A' && chr <= 'Z') || (chr >= '0' && chr <= '9')) > Can we simplify some of this function by reusing helpers in wtf/ASCIICType.h or > similar? I replaced the alpha numeric checks with an ASCIIType function. Not sure if there's an equivalent somewhere for the separators. > > https://codereview.chromium.org/1322543003/diff/40001/Source/core/loader/Link... > Source/core/loader/LinkHeader.cpp:35: || (chr >= '(' && chr <= '*') || chr == > '\'' || chr == '"' || chr == '%') > Nit: add {} added.
yoav@yoav.ws changed reviewers: + mkwst@chromium.org
+mkwst & a friendly ping :)
Still a little ugly, but I agree that there's not an obvious helper to solve the long list of symbols. LGTM
https://codereview.chromium.org/1322543003/diff/60001/Source/core/loader/Link... File Source/core/loader/LinkHeader.cpp (right): https://codereview.chromium.org/1322543003/diff/60001/Source/core/loader/Link... Source/core/loader/LinkHeader.cpp:36: return false; This requires way too much knowledge of ASCII. Like, what the heck is between '(' and '*'? :/ 1. Would you mind filing a bug to extract out all the character-categorizing methods (from here, and platform/network/ContentSecurityPolicyParsers.cpp, and various other places) into a single file somewhere in platform/ so that we can more easily determine that we _really_ need yet another method? 2. Would you mind adding tests with some of these characters?
On 2015/09/01 13:48:42, Mike West (buried) wrote: > https://codereview.chromium.org/1322543003/diff/60001/Source/core/loader/Link... > File Source/core/loader/LinkHeader.cpp (right): > > https://codereview.chromium.org/1322543003/diff/60001/Source/core/loader/Link... > Source/core/loader/LinkHeader.cpp:36: return false; > This requires way too much knowledge of ASCII. Like, what the heck is between > '(' and '*'? :/ I'll try to add comments that would ease the pain... > > 1. Would you mind filing a bug to extract out all the character-categorizing > methods (from here, and platform/network/ContentSecurityPolicyParsers.cpp, and > various other places) into a single file somewhere in platform/ so that we can > more easily determine that we _really_ need yet another method? > > 2. Would you mind adding tests with some of these characters? Will do x 2
On 2015/09/01 14:19:29, Yoav Weiss wrote: > On 2015/09/01 13:48:42, Mike West (buried) wrote: > > > https://codereview.chromium.org/1322543003/diff/60001/Source/core/loader/Link... > > File Source/core/loader/LinkHeader.cpp (right): > > > > > https://codereview.chromium.org/1322543003/diff/60001/Source/core/loader/Link... > > Source/core/loader/LinkHeader.cpp:36: return false; > > This requires way too much knowledge of ASCII. Like, what the heck is between > > '(' and '*'? :/ > > I'll try to add comments that would ease the pain... > > > > > 1. Would you mind filing a bug to extract out all the character-categorizing > > methods (from here, and platform/network/ContentSecurityPolicyParsers.cpp, and > > various other places) into a single file somewhere in platform/ so that we can > > more easily determine that we _really_ need yet another method? > > > > 2. Would you mind adding tests with some of these characters? > > Will do x 2 *or* I'll create a build/init time seperators/CTL array. That would be both more readable and faster than the gazillion ifs I got going here. WDYT?
On 2015/09/01 at 14:28:12, yoav wrote: > *or* I'll create a build/init time seperators/CTL array. > That would be both more readable and faster than the gazillion ifs I got going here. > > WDYT? I'd be fine with that. Whatever you like. :)
On 2015/09/02 06:40:27, Mike West (buried) wrote: > On 2015/09/01 at 14:28:12, yoav wrote: > > *or* I'll create a build/init time seperators/CTL array. > > That would be both more readable and faster than the gazillion ifs I got going > here. > > > > WDYT? > > I'd be fine with that. Whatever you like. :) When thinking about it some more, even if the spec allows all these chars in an extension parameter, we don't really parse anything that not 100% alpha, so I'll just trim that down to that.
On 2015/09/02 07:01:08, Yoav Weiss wrote: > On 2015/09/02 06:40:27, Mike West (buried) wrote: > > On 2015/09/01 at 14:28:12, yoav wrote: > > > *or* I'll create a build/init time seperators/CTL array. > > > That would be both more readable and faster than the gazillion ifs I got > going > > here. > > > > > > WDYT? > > > > I'd be fine with that. Whatever you like. :) > > When thinking about it some more, even if the spec allows all these chars in an > extension parameter, we don't really parse anything that not 100% alpha, so I'll > just trim that down to that. On third thought, that would result in valid headers being parsed as invalid... :/
Mike - I've added comments tests and aligned the parsing with the spec when it comes to parameters vs. extension parameters (extension parameters don't have to have values while parameters do). Can you please take a look?
LGTM % nit. https://codereview.chromium.org/1322543003/diff/80001/Source/core/loader/Link... File Source/core/loader/LinkHeader.cpp (right): https://codereview.chromium.org/1322543003/diff/80001/Source/core/loader/Link... Source/core/loader/LinkHeader.cpp:35: // So any of: |{}[]/\:;<=>?@,()*'"% TODO link to the bug you created?
https://codereview.chromium.org/1322543003/diff/80001/Source/core/loader/Link... File Source/core/loader/LinkHeader.cpp (right): https://codereview.chromium.org/1322543003/diff/80001/Source/core/loader/Link... Source/core/loader/LinkHeader.cpp:35: // So any of: |{}[]/\:;<=>?@,()*'"% On 2015/09/02 12:30:52, Mike West (buried) wrote: > TODO link to the bug you created? I did :D (a few lines up)
The CQ bit was checked by yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1322543003/#ps80001 (title: "Added comments, tests and aligned with the spec")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1322543003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322543003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201642 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
