-
Notifications
You must be signed in to change notification settings - Fork 10
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| = Custom HTTP Headers | ||
|
|
||
| * Proposal: link:./SPICE-0022-http-headers.adoc[SPICE-0022] | ||
| * Author: link:https://github.com/kyokuping[kyokuping] | ||
| * Status: Proposed | ||
| * Category: Language, Standard Library, Tooling | ||
|
|
||
| == Introduction | ||
|
|
||
| This SPICE proposes a new evaluator option to allow adding custom HTTP headers to outbound requests, with the ability to target specific URL prefixes. | ||
|
|
||
| == Motivation | ||
|
|
||
| When Pkl interacts with HTTP resources, it may need to provide authentication tokens or other custom headers to access them. For example, a Pkl module might be hosted in a private repository that requires an `Authorization` header for access. | ||
|
|
||
| Currently, there is no way to add custom headers to Pkl's HTTP requests. This limits Pkl's ability to interact with a wide range of HTTP-based resources. | ||
|
|
||
| == Proposed Solution | ||
|
|
||
| A new HTTP setting will be added, called `headers`. This setting allows users to specify a list of headers to be added to outbound HTTP requests. To avoid leaking credentials, headers can be configured on a per-URL-pattern basis. | ||
|
|
||
| For example, to provide an authentication token for specific subdomains, the configuration would look like this: | ||
|
|
||
| .~/.pkl/settings.pkl | ||
| [source,pkl] | ||
| ---- | ||
| amends "pkl:settings" | ||
|
|
||
| http { | ||
| headers { | ||
| ["https://*.my.private.repo/*"] { | ||
| ["authorization"] = "Bearer my-secret-token" | ||
| } | ||
| } | ||
| } | ||
| ---- | ||
|
|
||
| This will add the `authorization` header to all requests sent to `https://*.my.private.repo/` (subdomains of `my.private.repo`) and its subpaths. | ||
|
|
||
| == Detailed design | ||
|
|
||
| The `pkl.EvaluatorSettings.Http` class will be extended with a new `headers` property. | ||
|
|
||
| .pkl:EvaluatorSettings | ||
| [source,pkl] | ||
| ---- | ||
| class Http { | ||
| // ... existing properties | ||
|
|
||
| /// HTTP headers to add to outbound requests targeting specified URLs. | ||
| headers: Mapping<URLPattern, Mapping<HttpHeaderName, *Listing<String>|String>>? | ||
| } | ||
| ---- | ||
|
|
||
| The details for `URLPattern` and `HttpHeaderName` will be provided in the following sections. | ||
|
|
||
| A new CLI flag, `--http-header`, will be introduced to allow specifying headers from the command line. The flag will accept key-value pairs in the format `<url-pattern>=<header-name>:<header-value>[,<header-name>:<header-value>...]`. | ||
|
|
||
| Example: | ||
| [source,shell] | ||
| ---- | ||
| pkl eval \ | ||
| --http-header "https://*.my.private.repo/*:authorization=Bearer my-secret-token" \ | ||
| myModule.pkl | ||
| ---- | ||
|
|
||
| The HTTP client will be modified to check for matching header configurations for each outgoing request and add the corresponding headers. | ||
|
|
||
| === The `URLPattern` typealias | ||
|
|
||
| The `URLPattern` typealias defines glob patterns that are used to match the URLs of HTTP requests and determine which HTTP headers to apply. | ||
|
|
||
| To prevent accidental matches, the pattern should end with a slash (`/`) or a wildcard (`*`). | ||
|
|
||
| If several patterns match a given request URL, all matching patterns will be considered and their headers will be applied. | ||
|
|
||
| === The `HttpHeaderName` typealias | ||
|
|
||
| The `HttpHeaderName` typealias is used to represent the name of an HTTP header, defined as follows: | ||
|
|
||
| .pkl:EvaluatorSettings | ||
| [source,pkl] | ||
| ---- | ||
| // Reserved HTTP header names that are inappropriate to be set by the user | ||
| typealias ReservedHttpHeaderName = | ||
| "accept-charset" | ||
| | "accept-encoding" | ||
| | "access-control-request-headers" | ||
| | "access-control-request-method" | ||
| | "connection" | ||
| | "content-length" | ||
| | "cookie" | ||
| | "date" | ||
| | "dnt" | ||
| | "expect" | ||
| | "host" | ||
| | "keep-alive" | ||
| | "origin" | ||
| | "permissions-policy" | ||
| | "referer" | ||
| | "te" | ||
| | "trailer" | ||
| | "transfer-encoding" | ||
| | "upgrade" | ||
| | "via" | ||
|
|
||
| // Reserved HTTP header prefixes that are inappropriate to be set by the user | ||
| const local ReservedHttpHeaderPrefix = new Listing { | ||
| "proxy-" | ||
| "sec-" | ||
| "access-control-" | ||
| } | ||
| const local hasReservedHttpHeaderPrefix = (header: String) -> | ||
| ReservedHttpHeaderPrefix.any((it) -> header.startsWith(it)) | ||
|
|
||
| // Regex for validating HTTP header names based on RFC 7230 | ||
| const local httpHeaderNameRegex = Regex("^[a-zA-Z0-9!#\\$%&'*+-.^_`|~]+$") | ||
| const local hasValidHttpHeaderName = (header: String) -> | ||
| httpHeaderNameRegex.findMatchesIn(header) | ||
|
|
||
| typealias HttpHeaderName = String( | ||
| this == toLowerCase(), // Only accept lowercase header names | ||
| !(this is ReservedHttpHeaderName), | ||
| !hasReservedHttpHeaderPrefix.apply(this), | ||
| hasValidHttpHeaderName | ||
| ) | ||
| ---- | ||
|
|
||
| == Compatibility | ||
|
|
||
| This change is strictly backward-compatible. Existing Pkl programs will continue to work as-is. | ||
|
|
||
| == Future directions | ||
|
|
||
| Future enhancements could include support for more complex header manipulation, such as removing default headers. | ||
|
|
||
| == Alternatives considered | ||
|
|
||
| === 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of special-casing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
If we accept patterns, glob patterns would probably be better than regexes (glob patterns are designed to work well with paths, 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). |
||
|
|
||
| === Auth-specific support | ||
|
|
||
| Someone could argue it'd make more sense to specifically support auth handling rather than exposing the full ability to add custom HTTP headers. While this can be considered more future-proof in terms of exposing minimal API surface to maintain compatibility, this would limit the potential of supporting many different use cases outside of auth. That being said, adding separate auth support can also be considered later, mostly for supporting complex auth flows like OAuth. | ||
|
|
||
| === Plain Text Patterns | ||
|
|
||
| The initial proposal used plain text prefixes for matching URLs. After considering use cases that require more flexible matching, the design was updated to use glob patterns. | ||
|
|
||
| Glob patterns allow for more powerful and concise rules. For example, a single pattern like `*.service.internal` can apply a common authentication header to all internal services. Similarly, a pattern like `*.my-company.com` can apply a rule to all subdomains of a company. | ||
|
|
||
| == Acknowledgements | ||
|
|
||
| Thanks to the Pkl team for their feedback on the initial proposal, which helped shape the more secure and flexible design presented in this SPICE. | ||
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
Pairhere. I think this should just be:To address @HT154 's concern from apple/pkl#1196 (review)
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 theSet-Cookieheader (see https://stackoverflow.com/a/6375214).I don't know if Pkl users will ever need the
Set-Cookieheader; I think justMapping<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:
Where
Listingmeans: 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:
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:
Uh oh!
There was an error while loading. Please reload this page.
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 likehttps://example.commatchinghttps://example.company.com, and mistakenly sending credentials to the incorrect host.