Support http.AllowQuerySemicolons #2076
Conversation
| @@ -269,6 +271,11 @@ func (p *OAuthProxy) setupServer(opts *options.Options) error { | |||
| TLS: opts.Server.TLS, | |||
| } | |||
|
|
|||
| // Option: AllowQuerySemicolons | |||
| if opts.AllowQuerySemicolons { | |||
| serverOpts.Handler = http.AllowQuerySemicolons(p) | |||
There was a problem hiding this comment.
To make this a little more futureproof/obvious you are just adding a wrapper, perhaps you should pass in serverOpts.Handler instead of p?
Is this the right place to do this anyway? We have a number of HTTP handler chains being set up anyway, have you considered adding it into that instead?
There was a problem hiding this comment.
It can possibly be done there as well, depends on your preference - If you prefer it to be contained in a chain, let me know and I can re-submit the pull.
There was a problem hiding this comment.
Yeah I think lets change it to chain and then in theory we don't need to update it later if someone injects additional handlers into this chain
There was a problem hiding this comment.
@JoelSpeed I'm not sure @MickMake is going to get back to this as he was working on it for us at the time. While I can make the initial change suggestion (passing serverOpts.Handler instead of p), I'm not sure where the chain changes should be made. Can you point me to where the chain of HTTP Handlers are so I can try and update this PR in the desired style?
|
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
|
@JoelSpeed the current behavior causes issues with software that uses |
|
Hi @JoelSpeed, |
| @@ -269,6 +271,11 @@ func (p *OAuthProxy) setupServer(opts *options.Options) error { | |||
| TLS: opts.Server.TLS, | |||
| } | |||
|
|
|||
| // Option: AllowQuerySemicolons | |||
| if opts.AllowQuerySemicolons { | |||
| serverOpts.Handler = http.AllowQuerySemicolons(p) | |||
There was a problem hiding this comment.
Yeah I think lets change it to chain and then in theory we don't need to update it later if someone injects additional handlers into this chain
Description
Provide support for using semicolons in query strings. This adds a new bool flag
--allow-query-semicolons.Motivation and Context
See Issue #1841
How Has This Been Tested?
Tested against OTRS, (the original requester was using this ticket system).
Checklist: