Also impacts ParseHSTSHeader, which uses the same function.

Given the number of bugs stemming from the use of base::StringToInt() in //net (issue 596523) I expect we want a more holistic fix than to just this one callsite.

We can discuss solutions on the parent bug.

Another subtle bug with the implementation is that values with trailing/leading whitespace [1] will be accepted when the numeric portion is equivalent to 9223372036854775807 (MAX_INT64).

  if (!base::StringToInt64(s, &i) && i != std::numeric_limits<int64_t>::max())
    return false;

That is because StringToInt64() makes magical mutations to the output on failure, not just on the overflow case, but also when the numerical portion is valid but it was surrounded by whitespace :(

[1] "whitespace" in this case is using isspace(), so even worse it is locale dependent.

I'm not sure I understand comment #8. 

Is the idea that we want to reject a value of "max-age = 9223372036854775807 ;"?

Or that we'd only want to reject the value if it was "max-age = 9223372036854775807GARBAGE_HERE;"

My reading of https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_util.cc&sq=package:chromium&type=cs&l=973&rcl=1458819216 is that our name/value pair iterator already trims leading and trailing whitespace around the value.

Right, the latter example.
The problem is we are mixing different concepts of "whitespace".

Header parsing uses HTTP notion of linear whitespace [1], so will parse along spaces and horizontal tabs. Cool, good.

Whereas base::StringToInt64() uses the *system-locale* notion of whitespace (isspace()).

In the C locale on Linux that means it will additionally recognize form feeds and vertical tabs: characters which I believe the HTTP response parser will happily pass along to the value portion.

Even if that were not the case, we still have an abstraction problem. The system locale could be anything and who is to say it doesn't consider consider ':' as whitespace. Or even '-'. It is easier to see how those would cause us to now accept something we shouldn't.

These don't strike me as serious compatibility problems, or exploitable bugs.

But it does point to problems with using base::StringToInt*() correctly, and that is what I am working on fixing :)

Returning max-int from base::StringToInt*() on failure is basically useless when there are multiple paths than can do so. Since testing for max-int can mean 1 of 2 things. In fact I didn't realize this until looking at the code more carefully either.

I am working on some easier-to-use-correctly parsing primitives we can use in //net for numbers, and am doing my due diligence to understand the existing use-cases as well as challenges. After looking at this and a few other (incorrect) callsites, it is clear that we need an easier way to do saturated saturated conversions, not just here.

Cheers.

[1] https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_util.h&sq=package:chromium&type=cs&l=23&rcl=1458819216

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8

commit e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8
Author: eroman <eroman@chromium.org>
Date: Tue Apr 12 06:02:06 2016

Fix number parsing for max-age for HSTS/HPKP.

 * Don't allow leading '+'
 * Dont' allow trailing garbage (for overflowed numbers)

BUG=596523,  596561 

Review URL: https://codereview.chromium.org/1831963002

Cr-Commit-Position: refs/heads/master@{#386596}

[modify] https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8/net/http/http_security_headers.cc
[modify] https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8/net/http/http_security_headers.h
[modify] https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8/net/http/http_security_headers_unittest.cc
[modify] https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8/net/http/transport_security_state.h