Add support for customizing HTTP headers#1196
Conversation
There was a problem hiding this comment.
Thanks for the contribution! 🎉
I have a few primary concerns here:
- Although rare, it is valid to send a specific header more than once in a request. When there are headers with the same name their order may also matter.
- How does your implementation handle "reserved" headers that users should not override like
OriginorUser-Agent? - It seems less than ideal to send the same headers to all hosts, eg. if host A requires auth but host B does not, Pkl should not send a configured
Authorizationheader to both hosts. I think a prefix matching scheme like the one used for rewrites is necessary here to ensure the proper security controls are in place.
To this end, I think the Pkl API for this needs to be something along these lines:
headers: Mapping<HttpRewrite, Listing<Pair<String, String>>>This complicates the CLI a bit, of course, but maybe something like this is reasonable?
--http-header <prefix>='<header name>: <header value>'
|
Just a heads up, we want reviews from the entire core team because this feature affects the core language and has security implications. One member of the team is currently out on leave so it's going to be a little bit before we can fully review this, but we haven't forgotten about it! |
stdlib/EvaluatorSettings.pkl
Outdated
| /// HTTP headers to add to outbound requests targeting specified URLs. | ||
| headers: Mapping<HttpRewrite, Listing<Pair<String, String>>>? | ||
| } |
There was a problem hiding this comment.
We need the @Since annotation here for documentation purposes. It would also be good to add specificity and examples to the doc comment, similar to rewrites above.
| /// HTTP headers to add to outbound requests targeting specified URLs. | |
| headers: Mapping<HttpRewrite, Listing<Pair<String, String>>>? | |
| } | |
| /// HTTP headers to add to outbound requests targeting specified URLs. | |
| @Since { version = "0.30.0" } | |
| headers: Mapping<HttpRewrite, Listing<Pair<String, String>>>? | |
| } |
There was a problem hiding this comment.
I'm also curious if it makes sense to rename HttpRewrite to HttpPrefix (with typealias marked @Deprecated for backwards compatibility) and refactoring the corresponding Java APIs. Interested in input from @bioball and @stackoverflow on this.
| val headerParts = header.split(":", limit = 2) | ||
| require(headerParts.size == 2) { "Header '$header' is not in 'name:value' format. " } | ||
| PPair(headerParts[0], headerParts[1]) |
There was a problem hiding this comment.
curl accepts this same syntax, but also accepts <name>:[ \t]+<value> and appears to strip the leading (but not trailing) whitespace from header values before sending the request. To avoid surprise for users of Pkl, it might be good to do the same. In theory, HTTP servers should be ignoring this leading (and trailing) whitespace when parsing headers so it shouldn't matter, but it might be best to avoid taking chances here.
I'm also wondering if it makes sense to do some validation of characters in headers here (and in the EvaluatorSettings API) to eagerly report invalid characters instead of lazily doing so when a matching request is made for the first time.
70161b3 to
c2632cc
Compare
bioball
left a comment
There was a problem hiding this comment.
Hey, thanks for the PR! I agree with @stackoverflow; it'd be good to have this discussed in a SPICE.
If you'd like to drive this feature, can you write one up? It should be submitted as PR to pkl-evolution.
Your SPICE should start by copy/pasting the template. Your SPICE number would be the next number after the latest proposed SPICE see (https://github.com/apple/pkl-evolution/pulls)
| } | ||
| } | ||
| } | ||
| return underlying.send(wrappedRequestBuilder.build(), responseBodyHandler); |
There was a problem hiding this comment.
The logic of adding headers should be handled in RequestRewritingClient
|
I will resume working on this PR after getting the SPICE merged! |
|
As a heads up: the SPICE can only be merged once this pr is merged and released on a new version. So you only need the SPICE to be approved. |
94962c3 to
ecf2d8b
Compare
|
@kyokuping was the close intentional? |
|
Sorry, accidentally closed the PR. 😢 |
|
Gotcha, feel free to re-open a new PR! |
|
Happy to see this, just went looking for http(s) auth support :D |
e8ea9f6 to
ffbf0cf
Compare
|
When will this be merged? Really looking forward to access private repos 🙏🏻 |
HT154
left a comment
There was a problem hiding this comment.
Hey @kyokuping would you mind rebasing this to resolve the conflict(s)?
I have some pretty minor feedback, but from a technical standpoint this looks pretty good. I'd like to try and ship this in 0.32 in a few months, so it'd be great if this was ready for another set of eyes by the time the core team's all back together in a few weeks!
pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt
Outdated
Show resolved
Hide resolved
pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Jen Basch <jbasch94@gmail.com>
- Introduce pattern-based URL matches - Both accept a single and multiple values for header values - Move header append logic to RequestRewritingClient
pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt
Outdated
Show resolved
Hide resolved
| if (!headerName.equals(headerName.toLowerCase())) { | ||
| throw new IllegalArgumentException( | ||
| "HTTP header '%s' should be all lowercase".formatted(headerName)); | ||
| } |
There was a problem hiding this comment.
Is this validation actually necessary? Why would this be preferable to normalizing header names during CLI parsing?
There was a problem hiding this comment.
Based on the previous review on the SPICE, I updated the settings to enforce lowercase values and therefore expected the CLI to match this behavior. If we want to change this in the CLI, we should probably align the EvaluatorSettings implementation too. Which way do you prefer to go with?
There was a problem hiding this comment.
In the evaluator settings, "pre"-normalization is required to ensure the Mapping has unique keys. On the CLI, each instance of the option is parsed linearly, so the CLI could perform its own normalization without issue.
In general, I think it's best that we accept the broadest input possible for each input method (even if those are not consistent). It's very possible the other core folks could disagree, so I'm happy to put a pin in this until they're back over the next few weeks.
Co-authored-by: Jen Basch <jbasch94@gmail.com>
This PR adds support for custom HTTP headers, introducing a
--http-headerCLI flag to acceptkey=valuepairs. These headers can also be specified within thesetting.pklfile.Closes #633
SPICE: apple/pkl-evolution#24