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

validation glitches in TraceState.validateKey #1027

Closed
codefromthecrypt opened this issue Mar 18, 2020 · 4 comments · Fixed by #1561
Closed

validation glitches in TraceState.validateKey #1027

codefromthecrypt opened this issue Mar 18, 2020 · 4 comments · Fixed by #1561
Assignees
Labels
priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release

Comments

@codefromthecrypt
Copy link
Contributor

I noticed that TraceState.validateKey is loose compared to the ABNF in the spec. There are a couple missing tests:

  • when tenant format is used (@), the right-side can be 1-14 characters. this is not validated
  • only when tenant format is used (@), the first character can be a digit. Otherwise, it must be a letter

There might be some other problems, but these I noticed. As the spec text doesn't elaborate on the above points, I tried to correct it FWIW w3c/trace-context#386

@codefromthecrypt
Copy link
Contributor Author

ps there are a number of cases around @ syntax.. you can look at brave's tests if it helps. openzipkin/brave#693

@thisthat
Copy link
Member

Based on the latest version (https:/w3c/trace-context/blob/master/spec/20-http_request_header_format.md#list-members), I think the current implementation correctly validates keys and values.

@jkwatson
Copy link
Contributor

@adriancole do you concur? Can we close this?

@codefromthecrypt
Copy link
Contributor Author

I had pushed this to be errata so the spec would change. It hasn't changed as far as I can tell, rather scheduled to change in some unknown version. https://www.w3.org/TR/trace-context/#list-members

Since many people on the spec are also here, I would get someone to ask someone to push errata or say what version of the spec will have this, so that folks know that this is fixed per pending spec errata 2 or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release
Projects
None yet
4 participants