golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.92k stars 17.65k forks source link

proposal: x/net/http/httpguts: add IsToken #67031

Open jub0bs opened 6 months ago

jub0bs commented 6 months ago

Proposal Details

According to RFC 9110 and RFC 6265, header-field names, methods, and cookie names all share the same production: token. However, the logic for validating tokens is duplicated across x/net/http/httpguts and net/http:

  1. net/http relies on httpguts.ValidHeaderFieldName for validating header names in transport.go.
  2. net/http relies on a combination of strings.IndexFunc and httpguts.IsTokenRune for validating HTTP methods in method.go; and
  3. net/http relies on that same combination for validating cookie names in cookie.go.

/cc @neild

I propose the addition, in x/net/http/httpguts, of a function unifying the logic for validating tokens:

func IsToken(string) bool

Its implementation would be identical to httpguts.ValidHeaderFieldName's implementation at tip.

Then, both method validation and cookie-name validation could simply call httpguts.IsToken.

httpguts.ValidHeaderFieldName would simply delegate to httpguts.IsToken. The former would then become redundant; perhaps it could be marked for deprecation, but I'm not sure what the best course of action is in this regard.

(Of course, a simple alternative to all this would be to add no function to httpguts and instead rely on httpguts.ValidHeaderFieldName for all three types of validation; but such an approach feels semantically dishonest.)


Beside the argument for reducing logic duplication, benchmarks indicate that the validation logic for methods and cookie names is not as fast as it could be (due in part to unnecessary UTF-8 decoding) and would benefit (at no extra cost) from httpguts.ValidHeaderFieldName's recent speedup:

goos: darwin
goarch: amd64
pkg: REDACTED
cpu: Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
                       │     std      │               jub0bs                │
                       │    sec/op    │   sec/op     vs base                │
IsCookieNameValid-8      1665.0n ± 1%   377.0n ± 0%  -77.36% (p=0.000 n=20)

                       │     std      │               jub0bs                │
                       │     B/op     │    B/op     vs base                 │
IsCookieNameValid-8      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=20)

                       │     std      │               jub0bs                │
                       │  allocs/op   │ allocs/op   vs base                 │
IsCookieNameValid-8      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=20)

Although a speedup of method validation is unlikely to be noticeable in practice, a speedup of cookie-name validation may be noticeable in cases of requests containing many cookies and/or cookies with long names (esp. those that use cookie name prefixes like __Secure- and __Host-).

ianlancetaylor commented 6 months ago

CC @neild @bradfitz

neild commented 4 months ago

I'd be fine with adding httpguts.IsToken, but the simple interim approach that doesn't require this proposal to be accepted is to add an internal isToken function in net/http:

// isToken reports whether v is a valid token (https://www.rfc-editor.org/rfc/rfc2616#section-2.2).
func isToken(v string) bool {
  // For historical reasons, this function is called ValidHeaderFieldName (see issue #67031).
  return httpguts.ValidHeaderFieldName(v)
}
jub0bs commented 4 months ago

@neild

the simple interim approach that doesn't require this proposal to be accepted is to add an internal isToken function in net/http

I'd be fine with that!