-
Notifications
You must be signed in to change notification settings - Fork 8
SPICE-0022: Custom HTTP Headers #24
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
base: main
Are you sure you want to change the base?
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.
Thanks for writing this! Some initial feedback.
http { | ||
headers { | ||
["https://my.private.repo/"] { | ||
Pair("Authorization", "Bearer my-secret-token") | ||
} | ||
} | ||
} |
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.
Not sure about using Pair
here. I think this should just be:
headers: Mapping<String, Mapping<String, String>>
To address @HT154 's concern from apple/pkl#1196 (review)
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.
This is quite rare indeed; I don't know if it's worth supporting. In most cases, a comma separator semantically represents "multiple values" (e.g. Accept: text/plain,application/json
). Seems like the one case where this is not true is the Set-Cookie
header (see https://stackoverflow.com/a/6375214).
I don't know if Pkl users will ever need the Set-Cookie
header; I think just Mapping<String, String>
very likely will address all user needs.
If we did want to support multiple of the same header, I think the type should instead be:
headers: Mapping<String, Mapping<String, *Listing<String>|String>>>
Where Listing
means: repeat this header multiple times.
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.
Some other notes:
- Header names are case-insensitive. We should enforce that you cannot repeat the same header twice with different casings.
- Some headers make no sense to allow users to set (e.g.
Host
,Content-Length
). I think we should also enforce that these headers cannot be set.
We can validate this in Pkl. Perhaps we can require that header names are lower-case to begin with:
typealias ReservedHeaderName =
"host"
| "content-length"
| "// etc"
typealias HeaderName = String(this == toLowerCase(), !(this is ReservedHeaderName))
headers: Mapping<String, Mapping<HeaderName, String>>
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.
Thinking about this some more, I'm on board with matching hosts based on glob patterns. This enables use-cases like "use this header for all subdomains", which matches how auth works in many cases.
I think we should also enforce that the pattern ends with /
. This is the same validation that we apply on HTTP rewrites, and the purpose is to avoid accidental misconfiguration like https://example.com
matching https://example.company.com
, and mistakenly sending credentials to the incorrect host.
|
||
=== Global HTTP headers | ||
|
||
The initial proposal involved a simpler approach of having a single set of custom headers that would be applied to all outbound HTTP requests. This was rejected due to security concerns. Sending the same set of headers, which might include authentication tokens, to all hosts is a significant security risk. The per-URL-prefix approach provides a more secure way to manage custom headers. |
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.
It's valid to want headers to apply to all requests; for example, you might want all of Pkl's requests to use your own user-agent header.
We can perhaps have *
be a special case that represents all hosts?
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 don't have any strong opinion on this. Curious what the rest of the team thinks about this.
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.
Instead of special-casing *
, what about applying a matching system similar to URLPattern
? This would allow users to create flexible rules.
For example, in MSA, users could apply an internal auth token exclusively to the *://*.service.internal
pattern. It will allow users to safely target a much broader scope than a single host.
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.
That's possible, although, it differs from how HTTP rewrites work. Theoretically, we can change HTTP rewrites to follow this logic too, but it would be a breaking change:
- We determine precedence by longest prefix. This makes sense right now because these prefixes are verbatim, therefore, the longest is the most specific. However, this doesn't hold if we use wildcards (e.g.
[f][o][o]*
is less specific thanfoob*
but is longer). - Wildcards are also URL characters, so existing valid prefixes would then be re-interpreted as a wildcard.
If we accept patterns, glob patterns would probably be better than regexes (glob patterns are designed to work well with paths, /
and .
are not special characters). Also, we have our own specification for glob patterns: https://pkl-lang.org/main/current/language-reference/index.html#glob-patterns
Perhaps we can have HTTP headers and HTTP rewrites just have different rules too (one is a verbatim match, one is a wildcard match), because these address different problems in the first place that require different solutions (mirroring vs. authentication).
spices/SPICE-0022-http-headers.adoc
Outdated
} | ||
---- | ||
|
||
This will add the `Authorization` header to all requests sent to `https://my.private.repo/` and its sub-paths. |
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.
Many auth methods require some sort of extra logic to form a token (for example, some process to fetch a short-lived cookie).
It'd be good to think about how this should work in Pkl.
Possibly, users can use a custom resource reader:
headers {
["https://my.private.repo/"] {
["Cookie"] = read("auth:get-my-cookie")
}
}
One issue here is that custom resource readers are not available when evaluating ~/.pkl/settings.pkl
and PklProject
. This is a chicken-and-egg problem; custom readers can possibly be configured here in the first place.
This isn't necessarily blocking this feature, but something to think about as we design t his.
This proposes a new option for HTTP headers .
Reference implementation: apple/pkl#1196