-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Enhance the 'Forwarded' header checker in conformance tests. #4626
Enhance the 'Forwarded' header checker in conformance tests. #4626
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markusthoemmes: 0 warnings.
In response to this:
Proposed Changes
This test uses an intricate regular-expression to verify the returned Forwarded header roughly conforms to the spec. Unfortunately, it does not account for arbitrary additions (known as extensions to the spec) and trailing delimiters. While trailing delimiters are non-standard, they are unfortunately added by some routers.
Release Note
NONE
/assign @vagababov @dgerd
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
expected string | ||
} | ||
|
||
// token as defined in https://tools.ietf.org/html/rfc7230#section-3.2.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var (
...
)
Perhaps?
if pair == "" { | ||
continue | ||
} | ||
parts := strings.Split(strings.TrimSpace(pair), "=") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if it's a=b=c
? SplitN
, may be? Since this will pass token matcher validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it? = is not an allowed character in token. I‘ll habe another look though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, it's a delimeter.
var tokenMatcher = regexp.MustCompile(`^[0-9a-zA-Z!#$%&'*+-.^_|~]+$`) | ||
|
||
// approximation of quoted-string as defined in https://tools.ietf.org/html/rfc7230#section-3.2.6 | ||
var quotedStringMatcher = regexp.MustCompile(`^"[^"]+"$`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I read A sender SHOULD NOT generate a quoted-pair in a quoted-string except where necessary to quote DQUOTE
, it feels that \"
is valid inside quoted text. But, I do not think we have to be that precise. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, hence it‘s an approximation 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold
if you want to collect "vars" together, otherwise just cancel.
This test uses an intricate regular-expression to verify the returned Forwarded header roughly conforms to the spec. Unfortunately, it does not account for arbitrary additions (known as extensions to the spec) and trailing delimiters. While trailing delimiters are non-standard, they are unfortunately added by some routers.
6ee22ee
to
24c8be2
Compare
/hold cancel @vagababov collapsed the var statements. |
/lgtm |
…#4626) * Enhance the 'Forwarded' header checker in conformance tests. This test uses an intricate regular-expression to verify the returned Forwarded header roughly conforms to the spec. Unfortunately, it does not account for arbitrary additions (known as extensions to the spec) and trailing delimiters. While trailing delimiters are non-standard, they are unfortunately added by some routers. * Collapse var statements.
Proposed Changes
This test uses an intricate regular-expression to verify the returned Forwarded header roughly conforms to the spec. Unfortunately, it does not account for arbitrary additions (known as extensions to the spec) and trailing delimiters. While trailing delimiters are non-standard, they are unfortunately added by some routers.
Release Note
/assign @vagababov @dgerd