Custom headers #61
Custom headers #61
Conversation
…nfiguration for custom headers in remote requests.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for custom headers to be used in remote HTTP requests.
- Introduces a customHeaders field and a corresponding ClientOption in remote/http.go.
- Updates the configuration to allow specifying custom headers and passes them via cmd/root.go.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| remote/http.go | Adds customHeaders field and WithCustomHeader option, and applies headers in requestOnce. |
| config/config.go | Adds YAML configuration for custom headers. |
| cmd/root.go | Passes custom headers from config into the remote client. |
Comments suppressed due to low confidence (2)
remote/http.go:74
- Consider renaming WithCustomHeader to WithCustomHeaders for consistency with the underlying field name and to accurately reflect that multiple headers may be provided.
func WithCustomHeader(headers map[string]string) ClientOption {
remote/http.go:122
- Please add unit tests to ensure that custom headers are correctly applied to outgoing requests when provided via the ClientOption.
for cHeaderKey, cHeaderValue := range c.customHeaders {
rmartinoscar
left a comment
rmartinoscar
left a comment
There was a problem hiding this comment.
I agree with AI WithCustomHeader should be plural for consistency sake other then that LGTM
|
Been using this for a while and I have no problem with this patch :) |
WalkthroughAdds configurable custom HTTP headers for remote queries: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User / Config
participant CLI as rootCmd
participant Remote as remote.New (client)
participant HTTP as Panel/API
Note over User: config supplies custom_headers map
User->>CLI: read config.RemoteQuery.CustomHeaders
CLI->>Remote: remote.New(..., WithCustomHeaders(headers), ...)
Remote->>Remote: client.customHeaders = headers
Remote->>HTTP: client.requestOnce(...) sets standard headers
Remote->>HTTP: client iterates client.customHeaders and sets allowed headers
HTTP-->>Remote: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
remote/http.go (1)
73-78: Function name should be plural to match its behavior.The function accepts a map of multiple headers but uses a singular name. As already noted in previous review comments, this should be
WithCustomHeaders(plural) for clarity and consistency.
🧹 Nitpick comments (1)
config/config.go (1)
130-134: Fix comment formatting to follow Go conventions.The comment block should have a space after
//on each line for proper Go formatting.Apply this diff to fix the formatting:
- //When using services like Cloudflare Access to manage access to - //a specific system via an external authentication system, - //it is possible to add special headers to bypass authentication. - //The mentioned headers can be appended to queries sent from Wings to the panel. + // When using services like Cloudflare Access to manage access to + // a specific system via an external authentication system, + // it is possible to add special headers to bypass authentication. + // The mentioned headers can be appended to queries sent from Wings to the panel. CustomHeaders map[string]string `yaml:"custom_headers"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/root.go(1 hunks)config/config.go(1 hunks)remote/http.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
remote/http.go (1)
remote/types.go (1)
ClientOption(29-29)
cmd/root.go (2)
remote/http.go (1)
WithCustomHeader(74-78)config/config.go (1)
Get(427-435)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
remote/http.go (1)
45-45: LGTM!The field declaration is appropriate for storing custom HTTP headers.
cmd/root.go (1)
156-156: LGTM!The integration correctly passes custom headers from configuration to the remote client. The implementation handles nil maps gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
remote/http.go (1)
121-124: Prevent custom headers from overriding critical request headersBecause
Header.Setis called after the standard headers, a misconfiguredCustomHeadersmap can overwriteAuthorization(and other important headers), breaking auth or protocol assumptions. It’s safer to explicitly block a small set of protected headers and log that they were ignored.- req.Header.Set("Authorization", fmt.Sprintf("Bearer %s.%s", c.tokenId, c.token)) - - for cHeaderKey, cHeaderValue := range c.customHeaders { - req.Header.Set(cHeaderKey, cHeaderValue) - } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s.%s", c.tokenId, c.token)) + + // Apply custom headers, but do not allow overriding certain critical ones. + protected := map[string]struct{}{ + "Authorization": {}, + "Host": {}, + // Optionally add: "User-Agent", "Accept", "Content-Type" + } + + for key, value := range c.customHeaders { + if _, blocked := protected[http.CanonicalHeaderKey(key)]; blocked { + log.WithField("header", key).Warn("remote: ignoring custom header attempting to override protected header") + continue + } + req.Header.Set(key, value) + }
🧹 Nitpick comments (1)
remote/http.go (1)
39-46: Defensively copyheadersinWithCustomHeadersto avoid shared mutable stateRight now you store the caller‑provided map directly on the client. If that map is later mutated (especially from multiple goroutines), the client will see those changes and you can hit data races. Safer to take a shallow copy when the option is created.
-func WithCustomHeaders(headers map[string]string) ClientOption { - return func(c *client) { - c.customHeaders = headers - } -} +func WithCustomHeaders(headers map[string]string) ClientOption { + // Take a defensive copy so the client has its own immutable view. + var copied map[string]string + if headers != nil { + copied = make(map[string]string, len(headers)) + for k, v := range headers { + copied[k] = v + } + } + + return func(c *client) { + c.customHeaders = copied + } +}Also applies to: 73-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/root.go(1 hunks)remote/http.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
remote/http.go (1)
remote/types.go (1)
ClientOption(29-29)
cmd/root.go (2)
remote/http.go (1)
WithCustomHeaders(74-78)config/config.go (1)
Get(427-435)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cmd/root.go (1)
152-160: Custom headers wiring into remote client looks correctPassing
config.Get().RemoteQuery.CustomHeadersintoremote.WithCustomHeaderscleanly exposes the new configuration without changing existing behavior when the map is empty or omitted.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
remote/http.go (1)
122-133: Header override guard is bypassed by different casing; canonicalize keys before checkingThe
criticalHeadersmap uses canonical names ("Authorization","User-Agent", etc.), but the guard checkscriticalHeaders[key]using the raw user-supplied key. Because HTTP header names are case-insensitive andhttp.Header.Setcanonicalizes keys internally, a user-provided"authorization"key will:
- Not match the guard (since
"authorization"≠"Authorization"in the map), and- Still overwrite the canonical
"Authorization"header whenSetis called.So the protection against overriding critical headers is currently incomplete, and the risk previously noted about custom headers being able to override auth still exists in a slightly different form.
Canonicalizing before the lookup and before
Setcloses this hole:- // Apply custom headers, but prevent overriding critical headers - criticalHeaders := map[string]bool{ - "Authorization": true, - "User-Agent": true, - "Accept": true, - "Content-Type": true, - } - for key, value := range c.customHeaders { - if !criticalHeaders[key] { - req.Header.Set(key, value) - } - } + // Apply custom headers, but prevent overriding critical headers. + criticalHeaders := map[string]bool{ + "Authorization": true, + "User-Agent": true, + "Accept": true, + "Content-Type": true, + // Optionally also protect "Host" if overriding it should be disallowed. + } + for key, value := range c.customHeaders { + canonicalKey := http.CanonicalHeaderKey(key) + if criticalHeaders[canonicalKey] { + continue + } + req.Header.Set(canonicalKey, value) + }If you also want to forbid overriding
Host, add"Host": truetocriticalHeaders.
🧹 Nitpick comments (1)
remote/http.go (1)
39-46: Field wiring forcustomHeaderslooks fine; consider defensively copying the mapThe new field and
WithCustomHeadersoption are straightforward, butWithCustomHeadersstores the caller’s map by reference. If the same map is mutated while the client is in use (especially from another goroutine), it can cause surprising header changes or even a concurrent map read/write panic in Go.A small defensive copy would make the client more robust without affecting callers:
-func WithCustomHeaders(headers map[string]string) ClientOption { - return func(c *client) { - c.customHeaders = headers - } -} +func WithCustomHeaders(headers map[string]string) ClientOption { + // Nil-safe: ranging over a nil map later is fine. + copied := make(map[string]string, len(headers)) + for k, v := range headers { + copied[k] = v + } + + return func(c *client) { + c.customHeaders = copied + } +}Also applies to: 73-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
remote/http.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
remote/http.go (1)
remote/types.go (1)
ClientOption(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Analyze (go)
NaysKutzu
left a comment
NaysKutzu
left a comment
There was a problem hiding this comment.
Still works in my wings (Im testing using cloudflare access and i have it in the header)
5 participants
Changes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.