Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jub0bs opened this issue Apr 25, 2024 · 3 comments
Open

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

jub0bs opened this issue Apr 25, 2024 · 3 comments
Labels
Milestone

Comments

@jub0bs
Copy link

jub0bs commented Apr 25, 2024

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-).

@gopherbot gopherbot added this to the Proposal milestone Apr 25, 2024
@ianlancetaylor
Copy link
Contributor

CC @neild @bradfitz

@neild
Copy link
Contributor

neild commented Jun 10, 2024

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
Copy link
Author

jub0bs commented Jun 10, 2024

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants