Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1578793002:
Add experimental framework token generation tool (Closed)
DescriptionAdd experimental framework token generation tool
This adds a command line tool for Chromium developers to generate their
own tokens for experimental APIs. The key included matches the one used
by unit and layout tests, and so can be used to generate additional
test documents. Any other key file can also be used, so this tool can
generate keys for any implementation of the EF.
BUG=576278
Committed: https://crrev.com/65322b8dfef6a449c5884891031d28a4d2843c61
Cr-Commit-Position: refs/heads/master@{#378285}
Patch Set 1 #Patch Set 2 : Better adhere to guidelines for third_party dirs #Patch Set 3 : Add OWNERS file for tool directory #
Total comments: 9
Patch Set 4 : Addressing review comments #
Total comments: 6
Patch Set 5 : Rename all the things! #
Total comments: 2
Patch Set 6 : Move third_party code; fix bug from last patch's rename #
Total comments: 23
Patch Set 7 : Addressing feedback from PS#6 #Patch Set 8 : Addressing remaining feedback from PS#6 #Patch Set 9 : Update origin handling; add unit tests #Patch Set 10 : Add version number to token format #Patch Set 11 : Add a header to satisfy licencecheck.pl, which can't check the LICENCE file. #
Messages
Total messages: 35 (11 generated)
Description was changed from ========== Add experimental framework token generation tool This adds a command line tool for Chromium developers to generate their own tokens for experimental APIs. The key included matches the one used by unit and layout tests, and so can be used to generate additional test documents. Any other key file can also be used, so this tool can generate keys for any implementation of the EF. BUG=576278 ========== to ========== Add experimental framework token generation tool This adds a command line tool for Chromium developers to generate their own tokens for experimental APIs. The key included matches the one used by unit and layout tests, and so can be used to generate additional test documents. Any other key file can also be used, so this tool can generate keys for any implementation of the EF. BUG=576278 ==========
iclelland@chromium.org changed reviewers: + chasej@chromium.org, davidben@chromium.org, miket@chromium.org
+r chasej, PTAL, thanks +r davidben - this implements the signing side of the verification in https://codereview.chromium.org/1563903002 - It's crypto, though it's not shipped in the browser, so I figured I should get you to give it a quick scan at least. miket - FYI, this is a command-line counterpart to the EF console; I thought you'd be interested.
Between it being in Python and third_party stuff, I'm not sure if I'm the right person to be reviewing this. https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/third... File tools/experiments/third_party/README.chromium (right): https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/third... tools/experiments/third_party/README.chromium:1: Name: Ed25519: high-speed high-security signatures I suspect you need to get this reviewed by some other folks. I'm not sure how that works. https://www.chromium.org/developers/adding-3rd-party-libraries (Another option is to write a C++ tool and link it against BoringSSL.)
On 2016/01/11 20:42:02, davidben wrote: > Between it being in Python and third_party stuff, I'm not sure if I'm the right > person to be reviewing this. > > https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/third... > File tools/experiments/third_party/README.chromium (right): > > https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/third... > tools/experiments/third_party/README.chromium:1: Name: Ed25519: high-speed > high-security signatures > I suspect you need to get this reviewed by some other folks. I'm not sure how > that works. > > https://www.chromium.org/developers/adding-3rd-party-libraries > > (Another option is to write a C++ tool and link it against BoringSSL.) No problem; I figured you might want to see how the ed25519 signatures were being generated; easy to remove you :)
On 2016/01/11 21:01:27, iclelland wrote: > On 2016/01/11 20:42:02, davidben wrote: > > Between it being in Python and third_party stuff, I'm not sure if I'm the > right > > person to be reviewing this. > > > > > https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/third... > > File tools/experiments/third_party/README.chromium (right): > > > > > https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/third... > > tools/experiments/third_party/README.chromium:1: Name: Ed25519: high-speed > > high-security signatures > > I suspect you need to get this reviewed by some other folks. I'm not sure how > > that works. > > > > https://www.chromium.org/developers/adding-3rd-party-libraries > > > > (Another option is to write a C++ tool and link it against BoringSSL.) > > No problem; I figured you might want to see how the ed25519 signatures were > being generated; easy to remove you :) Sorry for the delay! LGTM
Whoops, pressed wrong button. See comments, then LGTM https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/OWNERS File tools/experiments/OWNERS (left): https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/OWNER... tools/experiments/OWNERS:2: jkarlin@chromium.org suggest dhnishi and mek for more resiliency https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/gener... File tools/experiments/generate_token.py (right): https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/gener... tools/experiments/generate_token.py:30: UTC) when the token should exipire This is up to you, but if this is just a rote copy of --help, then it might not serve a useful purpose beyond explaining the argparse block below. Consider omitting. https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/gener... tools/experiments/generate_token.py:49: if re.match('^[a-z0-9.-]*$', arg): Make sure we (this tool and dev-console server code) agree on what a hostname is. https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/gener... tools/experiments/generate_token.py:56: return urlparse.urlunparse(origin[:2]+('',)*4) whitespace after comma and around binary operator +
LGTM, with naming suggestions. The revised naming has been sorted out (experiment -> origin trial, API -> feature, API key -> token). I'd suggest that things be moved/renamed as appropriate in this CL. Most obvious change is to store it under tools/origin_trials. That said, I'm fine if renaming happens in a separate CL. https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/gener... File tools/experiments/generate_token.py (right): https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/gener... tools/experiments/generate_token.py:11: hostname api_name Should we use something other than "hostname" for this parameter? I know the help describes possible values for this parameter. Hostname has a very specific meaning in some places (e.g. in the definition of an URL or href). Perhaps "origin" would be suitable, as that labels the general concept. It allows for different formats to be supported (i.e. hostname only vs URL). https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/gener... tools/experiments/generate_token.py:11: hostname api_name I would suggest "trial_name" to replace "api_name". I think that's more accurate/precise than "feature_name" (e.g. could be multiple trials, with different names, for the same general feature). https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/third... File tools/experiments/third_party/README.chromium (right): https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/third... tools/experiments/third_party/README.chromium:11: tokens for experimental API usage, and to verify the signatures on those tokens. This would also need to be changed for new naming. That is, "tools/origin_trials" and "for experimental feature usage".
iclelland@chromium.org changed reviewers: - davidben@chromium.org
https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/OWNERS File tools/experiments/OWNERS (left): https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/OWNER... tools/experiments/OWNERS:2: jkarlin@chromium.org On 2016/01/21 21:54:55, miket_OOO wrote: > suggest dhnishi and mek for more resiliency Done. I wasn't going to just volunteer anyone, but I'll let you do it ;) https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/gener... File tools/experiments/generate_token.py (right): https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/gener... tools/experiments/generate_token.py:30: UTC) when the token should exipire On 2016/01/21 21:54:56, miket_OOO wrote: > This is up to you, but if this is just a rote copy of --help, then it might not > serve a useful purpose beyond explaining the argparse block below. Consider > omitting. It is a rote copy, just for anyone reading the code, but I'll omit it, and replace with a note to call -h for more info. https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/gener... tools/experiments/generate_token.py:49: if re.match('^[a-z0-9.-]*$', arg): On 2016/01/21 21:54:56, miket_OOO wrote: > Make sure we (this tool and dev-console server code) agree on what a hostname > is. It looks like your regex disallows a '-' as a component, which makes sense. Updated. https://codereview.chromium.org/1578793002/diff/40001/tools/experiments/gener... tools/experiments/generate_token.py:56: return urlparse.urlunparse(origin[:2]+('',)*4) On 2016/01/21 21:54:56, miket_OOO wrote: > whitespace after comma and around binary operator + Done. https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/gener... File tools/experiments/generate_token.py (right): https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/gener... tools/experiments/generate_token.py:11: hostname api_name On 2016/01/22 18:14:00, chasej wrote: > Should we use something other than "hostname" for this parameter? I know the > help describes possible values for this parameter. Hostname has a very specific > meaning in some places (e.g. in the definition of an URL or href). > > Perhaps "origin" would be suitable, as that labels the general concept. It > allows for different formats to be supported (i.e. hostname only vs URL). Done. https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/gener... tools/experiments/generate_token.py:11: hostname api_name On 2016/01/22 18:14:00, chasej wrote: > I would suggest "trial_name" to replace "api_name". I think that's more > accurate/precise than "feature_name" (e.g. could be multiple trials, with > different names, for the same general feature). Done. https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/third... File tools/experiments/third_party/README.chromium (right): https://codereview.chromium.org/1578793002/diff/60001/tools/experiments/third... tools/experiments/third_party/README.chromium:11: tokens for experimental API usage, and to verify the signatures on those tokens. On 2016/01/22 18:14:00, chasej wrote: > This would also need to be changed for new naming. That is, > "tools/origin_trials" and "for experimental feature usage". Done.
iclelland@chromium.org changed reviewers: + thakis@chromium.org
+r thakis -- Can you take a look, as an OWNER of /tools/? Thanks!
https://codereview.chromium.org/1578793002/diff/80001/tools/origin_trials/thi... File tools/origin_trials/third_party/README.chromium (right): https://codereview.chromium.org/1578793002/diff/80001/tools/origin_trials/thi... tools/origin_trials/third_party/README.chromium:3: URL: http://ed25519.cr.yp.to/software.html third-party code should be in a subfolder of third_party (i.e. tools/origin_trials/third_party/ed25519/...)
https://codereview.chromium.org/1578793002/diff/80001/tools/origin_trials/thi... File tools/origin_trials/third_party/README.chromium (right): https://codereview.chromium.org/1578793002/diff/80001/tools/origin_trials/thi... tools/origin_trials/third_party/README.chromium:3: URL: http://ed25519.cr.yp.to/software.html On 2016/01/26 15:33:39, Nico wrote: > third-party code should be in a subfolder of third_party (i.e. > tools/origin_trials/third_party/ed25519/...) Thanks, done.
lgtm, but: https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... File tools/origin_trials/generate_token.py (right): https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:23: from third_party.ed25519 import ed25519 With a snippet like e.g. https://code.google.com/p/chromium/codesearch#chromium/src/v8/build/gyp_v8&l=46 you don't need all these __init__.py files I think. Can you give that a try? Having random stuff in a "top-level" third_party dir seems unfortunate
palmer@chromium.org changed reviewers: + palmer@chromium.org
https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... File tools/origin_trials/generate_token.py (right): https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:25: HOSTNAME_REGEX = re.compile( http://stackoverflow.com/a/2532344 looks more correct. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:38: def OriginFromArgs(arg): Nit: The function name suggests that the argument is a sequence, but the name |arg| and the code use it as a single value. I see from reading further that it's a convention with argparse. Maybe rename |arg| to |args|? Not important, just a nit anyway. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:52: return urlparse.urlunparse(origin[:2] + ('', )*4) Nit: consistently use spaces around operators. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:52: return urlparse.urlunparse(origin[:2] + ('', )*4) It's a bit mysterious (to me, at least) what urlunparse really does. Additionally, the origin[:2] + ('', )*4 is obscure. It might be better to return an explicit construction, including one that always includes an explicit port. E.g.: if origin.scheme not in ('http', 'https'): raise argparse.ArgumentTypeError('%s does not use a recognized URL scheme' % arg) port = origin.port if not port: port = {'http': 80, 'https': 443}[origin.scheme] return origin.scheme + '://' + origin.hostname + ':' + port https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:62: def Sign(private_key, data): Nit: Consistently use private_key or privateKey style throughout. (I don't know which is Chromium-standard; use whichever one is.) https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:66: return base64.b64encode(signature) + "|" + data Nit: Use "..." or '...' strings throughout; whatever the standard says. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:72: help="Origin for which to enable the API. This can be a " Nit: Punctuation and conciseness. How about: Origin for which to enable the API. This can be either a hostname (default scheme HTTPS, default port 443) or a URL. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:98: private_key = key_file.read() Maybe just read the 1st 64 bytes from the file, and/or trim whitespace, to be more tolerant of non-fatal stuff in the file? People are likely to be copying and pasting, perhaps? I suppose if the key contains whitespace bytes, perhaps the only thing you can do and still be correct is to read the 1st 64 bytes. Just a thought. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:115: print "There was an error generating the signature." Maybe print the exception in case it has anything useful? (Maybe it doesn't.) https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/th... File tools/origin_trials/third_party/ed25519/README.chromium (right): https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/th... tools/origin_trials/third_party/ed25519/README.chromium:7: Security Critical: no I think I agree, but it looks a bit strange. :) Can you add a comment about why we don't consider it security-critical, even though it's about cryptographic signatures? And, if this library gets used for new purposes, how that No could turn into a Yes.
https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... File tools/origin_trials/generate_token.py (right): https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:23: from third_party.ed25519 import ed25519 On 2016/01/26 17:27:46, Nico wrote: > With a snippet like e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/v8/build/gyp_v8&l=46 > you don't need all these __init__.py files I think. Can you give that a try? > Having random stuff in a "top-level" third_party dir seems unfortunate I hate to hack the import path; Python has a well-defined and understood module import mechanism. I'll do it to try to cut down on the number of useless files in the codebase, though. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:38: def OriginFromArgs(arg): On 2016/02/11 22:03:20, palmer wrote: > Nit: The function name suggests that the argument is a sequence, but the name > |arg| and the code use it as a single value. > > I see from reading further that it's a convention with argparse. Maybe rename > |arg| to |args|? > > Not important, just a nit anyway. It is a convention, but you're right that it's a misleading one. It's actually a custom type function; it takes a single command-line argument and constructs an origin from it. I've renamed it to |OriginFromArg| to be clearer. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:52: return urlparse.urlunparse(origin[:2] + ('', )*4) On 2016/02/11 22:03:20, palmer wrote: > Nit: consistently use spaces around operators. I would, but PEP8 says, "If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, and always have the same amount of whitespace on both sides of a binary operator." and calls out specifically Yes: c = (a+b) * (a-b) No: c = (a + b) * (a - b) and I don't think that either Google or Chromium style overrides it. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:62: def Sign(private_key, data): On 2016/02/11 22:03:20, palmer wrote: > Nit: Consistently use private_key or privateKey style throughout. (I don't know > which is Chromium-standard; use whichever one is.) Thanks. I missed apiName on my first pass. PEP8 standard uses private_key for variables; Chromium style overrides that and prescribes MixedCaseNames for functions and methods; apiName was just wrong. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:66: return base64.b64encode(signature) + "|" + data On 2016/02/11 22:03:20, palmer wrote: > Nit: Use "..." or '...' strings throughout; whatever the standard says. Done. The standard explicitly makes no recommendation, other than to be consistent. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:72: help="Origin for which to enable the API. This can be a " On 2016/02/11 22:03:20, palmer wrote: > Nit: Punctuation and conciseness. How about: > > Origin for which to enable the API. This can be either a hostname (default > scheme HTTPS, default port 443) or a URL. Done. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:98: private_key = key_file.read() On 2016/02/11 22:03:20, palmer wrote: > Maybe just read the 1st 64 bytes from the file, and/or trim whitespace, to be > more tolerant of non-fatal stuff in the file? People are likely to be copying > and pasting, perhaps? > > I suppose if the key contains whitespace bytes, perhaps the only thing you can > do and still be correct is to read the 1st 64 bytes. > > Just a thought. Yeah, the key is a binary file; it wouldn't make sense to wrap it in ASCII text without encoding it somehow anyway -- not least because the first or last bytes could very easily be 0x20, 0x0d, 0x0a, etc.; trying to extract that from an arbitrary text file seems out-of-scope for a tool like this anyway; if you really have to, you should run it through uuencode/uudecode or something like that before using it here. I'm finding it hard to imagine a case where someone manages to mangle the file through cut-and-paste in a way that we can reliably recover from. Maybe we scan the entire file looking for a 64-byte section that represents a legitimate private key? In any case, we still have to deal with the possibility that the key file is < 64 bytes long, so I don't see the code getting any shorter here. If you think it avoids some realistic failure case, then I'll allow longer files, and truncate to 64 bytes on read. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:115: print "There was an error generating the signature." On 2016/02/11 22:03:20, palmer wrote: > Maybe print the exception in case it has anything useful? (Maybe it doesn't.) For diagnostics, sure. The exceptions are mostly not going to be helpful to an end user, they really indicate that something wrong happened with our code. I'll print it out anyway, as extra information. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/th... File tools/origin_trials/third_party/ed25519/README.chromium (right): https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/th... tools/origin_trials/third_party/ed25519/README.chromium:7: Security Critical: no On 2016/02/11 22:03:20, palmer wrote: > I think I agree, but it looks a bit strange. :) Can you add a comment about why > we don't consider it security-critical, even though it's about cryptographic > signatures? And, if this library gets used for new purposes, how that No could > turn into a Yes. I'll do that, certainly. Is there a standard for free-form comments in this file?
> > I think I agree, but it looks a bit strange. :) Can you add a comment about > why > > we don't consider it security-critical, even though it's about cryptographic > > signatures? And, if this library gets used for new purposes, how that No could > > turn into a Yes. > > I'll do that, certainly. Is there a standard for free-form comments in this > file? I don't know. Add it to the Description field? Also, what about the hostname lexing and origin return suggestions?
Sorry, Chris -- my workflow got interrupted by the long weekend. I've addressed the remaining issues that were brought up now. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... File tools/origin_trials/generate_token.py (right): https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:25: HOSTNAME_REGEX = re.compile( On 2016/02/11 22:03:20, palmer wrote: > http://stackoverflow.com/a/2532344 looks more correct. Certainly. It handles the 255 character limit, as well as the 63 character per-label limit. I used this regex to bring it exactly in line with the validation in the developer console, but I've now updated it to use essentially that logic. https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/ge... tools/origin_trials/generate_token.py:52: return urlparse.urlunparse(origin[:2] + ('', )*4) On 2016/02/11 22:03:20, palmer wrote: > It's a bit mysterious (to me, at least) what urlunparse really does. Unparse is just the opposite of parse -- parse breaks a string into a tuple; unparse creates a string, given a tuple. Obviously .build() or .construct() would have been more descriptive, but that's just how it's spelled in the standard library. > Additionally, the origin[:2] + ('', )*4 is obscure. I can easily break that up; you're right that it's terser than it needs to be. > > It might be better to return an explicit construction, including one that always > includes an explicit port. E.g.: I'd like to avoid the explicit port for now; I don't have sufficient evidence on the chrome side to say that it will always match the origin when we do the comparison in the renderer. Also, the output of this tool should really match that of the developer console which is being written concurrently, as well as the public docs (which currently say that 443 is assumed for https) > > if origin.scheme not in ('http', 'https'): > raise argparse.ArgumentTypeError('%s does not use a recognized URL scheme' % > arg) Done. > > port = origin.port > if not port: > port = {'http': 80, 'https': 443}[origin.scheme] > > return origin.scheme + '://' + origin.hostname + ':' + port https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/th... File tools/origin_trials/third_party/ed25519/README.chromium (right): https://codereview.chromium.org/1578793002/diff/100001/tools/origin_trials/th... tools/origin_trials/third_party/ed25519/README.chromium:7: Security Critical: no On 2016/02/11 22:03:20, palmer wrote: > I think I agree, but it looks a bit strange. :) Can you add a comment about why > we don't consider it security-critical, even though it's about cryptographic > signatures? And, if this library gets used for new purposes, how that No could > turn into a Yes. Done.
> I'd like to avoid the explicit port for now; I don't have sufficient evidence on > the chrome side to say that it will always match the origin when we do the > comparison in the renderer. Also, the output of this tool should really match > that of the developer console which is being written concurrently, as well as > the public docs (which currently say that 443 is assumed for https) Chrome's origin checks should always be using the url::Origin class, which always fills in the port for origins that have ports. (After all, origins with different ports are different origins.) Being strict and explicit about it is likely to turn up bugs in Chrome/Blink, which is a good thing. :)
On 2016/02/17 00:39:47, palmer wrote: > > I'd like to avoid the explicit port for now; I don't have sufficient evidence > on > > the chrome side to say that it will always match the origin when we do the > > comparison in the renderer. Also, the output of this tool should really match > > that of the developer console which is being written concurrently, as well as > > the public docs (which currently say that 443 is assumed for https) > > Chrome's origin checks should always be using the url::Origin class, which > always fills in the port for origins that have ports. (After all, origins with > different ports are different origins.) Being strict and explicit about it is > likely to turn up bugs in Chrome/Blink, which is a good thing. :) Sounds good; I'll update the public docs and patch the developer console as well to match. At thing point, I think I've added enough code to these methods now that I'm not comfortable assuming correctness by inspection, so I've uploaded a test suite to validate the hostname and origin handling.
On 2016/02/17 15:22:21, iclelland wrote: > On 2016/02/17 00:39:47, palmer wrote: > > > I'd like to avoid the explicit port for now; I don't have sufficient > evidence > > on > > > the chrome side to say that it will always match the origin when we do the > > > comparison in the renderer. Also, the output of this tool should really > match > > > that of the developer console which is being written concurrently, as well > as > > > the public docs (which currently say that 443 is assumed for https) > > > > Chrome's origin checks should always be using the url::Origin class, which > > always fills in the port for origins that have ports. (After all, origins with > > different ports are different origins.) Being strict and explicit about it is > > likely to turn up bugs in Chrome/Blink, which is a good thing. :) > > Sounds good; I'll update the public docs and patch the developer console as well > to match. > > At thing point, I think I've added enough code to these methods now that I'm not > comfortable assuming correctness by inspection, so I've uploaded a test suite to > validate the hostname and origin handling. palmer -- Do you have any other concerns with this CL? I have all of the signoffs required for the third-party addition now, so if you're okay with this, I can land it. Thanks.
Oops, sorry for being slow. LGTM.
No problem -- thanks for the feedback!
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miket@chromium.org, chasej@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1578793002/#ps180001 (title: "Add version number to token format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578793002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578793002/180001
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miket@chromium.org, palmer@chromium.org, chasej@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1578793002/#ps200001 (title: "Add a header to satisfy licencecheck.pl, which can't check the LICENCE file.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578793002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578793002/200001
Message was sent while issue was closed.
Description was changed from ========== Add experimental framework token generation tool This adds a command line tool for Chromium developers to generate their own tokens for experimental APIs. The key included matches the one used by unit and layout tests, and so can be used to generate additional test documents. Any other key file can also be used, so this tool can generate keys for any implementation of the EF. BUG=576278 ========== to ========== Add experimental framework token generation tool This adds a command line tool for Chromium developers to generate their own tokens for experimental APIs. The key included matches the one used by unit and layout tests, and so can be used to generate additional test documents. Any other key file can also be used, so this tool can generate keys for any implementation of the EF. BUG=576278 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add experimental framework token generation tool This adds a command line tool for Chromium developers to generate their own tokens for experimental APIs. The key included matches the one used by unit and layout tests, and so can be used to generate additional test documents. Any other key file can also be used, so this tool can generate keys for any implementation of the EF. BUG=576278 ========== to ========== Add experimental framework token generation tool This adds a command line tool for Chromium developers to generate their own tokens for experimental APIs. The key included matches the one used by unit and layout tests, and so can be used to generate additional test documents. Any other key file can also be used, so this tool can generate keys for any implementation of the EF. BUG=576278 Committed: https://crrev.com/65322b8dfef6a449c5884891031d28a4d2843c61 Cr-Commit-Position: refs/heads/master@{#378285} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/65322b8dfef6a449c5884891031d28a4d2843c61 Cr-Commit-Position: refs/heads/master@{#378285} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
