feat(cloud): add ENGRAM_CLOUD_EXTRA_HEADERS support#645
Open
4DRIAN0RTIZ wants to merge 1 commit into
Open
Conversation
Adds opt-in support for injecting arbitrary HTTP headers into every outgoing cloud sync request via ENGRAM_CLOUD_EXTRA_HEADERS env var. Comma-separated Key: Value pairs are parsed once at construction and stored on both RemoteTransport and MutationTransport. Authorization key is rejected at parse time to prevent token shadowing. Malformed pairs are skipped with a warning log. Header values are never logged. Enables compatibility with Cloudflare tunnels, reverse proxies, and access systems that require custom headers beyond Authorization.
📝 WalkthroughWalkthroughAdds ChangesCloud extra headers
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SyncCaller
participant RemoteTransport
participant CloudEndpoint
SyncCaller->>RemoteTransport: Start cloud sync request
RemoteTransport->>RemoteTransport: Set bearer Authorization and apply extra headers
RemoteTransport->>CloudEndpoint: Send cloud request
CloudEndpoint-->>RemoteTransport: Return response
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds opt-in support for injecting additional HTTP headers into all outgoing cloud sync requests via ENGRAM_CLOUD_EXTRA_HEADERS, intended for environments like Cloudflare Access / zero-trust proxies, while preventing Authorization header shadowing.
Changes:
- Parse
ENGRAM_CLOUD_EXTRA_HEADERSonce duringRemoteTransport/MutationTransportconstruction and apply headers to all relevant requests. - Add unit tests covering header parsing edge cases and request header propagation.
- Document the new env var in CLI help text and
DOCS.mdautosync documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cloud/remote/transport.go | Adds parsing + application of extra headers, with Authorization guardrail, and wires header application into transports. |
| internal/cloud/remote/transport_headers_test.go | Adds parsing-focused tests (including malformed input + Authorization rejection). |
| internal/cloud/remote/transport_extra_test.go | Adds integration-style tests verifying headers are present on outgoing HTTP requests from both transports. |
| cmd/engram/main.go | Updates CLI environment/help text to include ENGRAM_CLOUD_EXTRA_HEADERS. |
| DOCS.md | Documents the new env var format/rules for autosync deployments. |
Comments suppressed due to low confidence (1)
internal/cloud/remote/transport_headers_test.go:229
- The custom
contains/containsStrhelpers plus theos.Stderrsentinel add noise and an inaccurate comment about “other tests”. Usingstrings.Containsis simpler and removes the need for the unused import workaround.
// contains is a helper to avoid importing strings in the test name collision.
func contains(s, substr string) bool {
return len(s) >= len(substr) && (s == substr || len(substr) == 0 || containsStr(s, substr))
}
func containsStr(s, substr string) bool {
for i := 0; i <= len(s)-len(substr); i++ {
if s[i:i+len(substr)] == substr {
return true
}
}
return false
}
// Suppress unused import warning - os is used by other tests that may reference it.
var _ = os.Stderr
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pair = strings.TrimSpace(pair) | ||
| idx := strings.IndexByte(pair, ':') | ||
| if idx < 0 { | ||
| log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: skipping malformed pair %q (missing colon)", pair) |
Comment on lines
+83
to
+86
| // The log should reference the malformed pair "badpair" | ||
| if !contains(logOut, "badpair") { | ||
| t.Fatalf("expected log to mention 'badpair', got: %q", logOut) | ||
| } |
Comment on lines
+3
to
+8
| import ( | ||
| "bytes" | ||
| "log" | ||
| "os" | ||
| "testing" | ||
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cloud/remote/transport_headers_test.go`:
- Around line 61-64: In internal/cloud/remote/transport_headers_test.go at lines
61-64, add a mutex-protected safeBuffer implementing Write and String, importing
sync as needed; replace the local bytes.Buffer with safeBuffer at lines 61-64,
91-94, 105-108, 122-125, 139-142, and 186-189, preserving the existing global
log interception and assertions.
- Around line 213-228: Replace the custom contains and containsStr helpers in
the test file with the standard strings.Contains function, importing strings or
an alias if needed for any naming collision. Update all call sites accordingly,
then remove the unused os import and the var _ = os.Stderr suppression.
In `@internal/cloud/remote/transport.go`:
- Around line 37-60: Update the extra-header parsing loop in transport.go to
accumulate comma-separated segments without a colon into the current header
value instead of discarding them or logging malformed-pair warnings, while
preserving key canonicalization and Authorization protection. Add an edge-case
test in internal/cloud/remote/transport_headers_test.go covering a header value
containing commas and asserting the complete value is retained.
- Around line 77-84: Replace the call to sortStrings with sort.Strings, add the
standard library sort import, and remove the custom sortStrings insertion-sort
function. Preserve the existing in-place sorting behavior for the string slice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 973ab9ca-b969-41e9-ba8b-2a8e825fbcc5
📒 Files selected for processing (5)
DOCS.mdcmd/engram/main.gointernal/cloud/remote/transport.gointernal/cloud/remote/transport_extra_test.gointernal/cloud/remote/transport_headers_test.go
Comment on lines
+61
to
+64
| orig := log.Writer() | ||
| var buf bytes.Buffer | ||
| log.SetOutput(&buf) | ||
| defer log.SetOutput(orig) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Data race hazard: bytes.Buffer is not thread-safe for global log interception.
These tests intercept the global log output using log.SetOutput(&buf) with a standard bytes.Buffer. If any background goroutine (such as those spun up by httptest.NewServer in other tests) or a concurrent test logs a message, it will race on the unprotected bytes.Buffer.Write, causing the test suite to panic.
internal/cloud/remote/transport_headers_test.go#L61-L64: Wrap thebytes.Bufferin a thread-safe struct with async.Mutexto safely intercept the global logger.internal/cloud/remote/transport_headers_test.go#L91-L94: Use the thread-safe buffer here.internal/cloud/remote/transport_headers_test.go#L105-L108: Use the thread-safe buffer here.internal/cloud/remote/transport_headers_test.go#L122-L125: Use the thread-safe buffer here.internal/cloud/remote/transport_headers_test.go#L139-L142: Use the thread-safe buffer here.internal/cloud/remote/transport_headers_test.go#L186-L189: Use the thread-safe buffer here.
🛠️ Proposed thread-safe buffer helper
Add this helper to transport_headers_test.go and replace var buf bytes.Buffer with var buf safeBuffer in the affected tests:
type safeBuffer struct {
mu sync.Mutex
b bytes.Buffer
}
func (s *safeBuffer) Write(p []byte) (n int, err error) {
s.mu.Lock()
defer s.mu.Unlock()
return s.b.Write(p)
}
func (s *safeBuffer) String() string {
s.mu.Lock()
defer s.mu.Unlock()
return s.b.String()
}📍 Affects 1 file
internal/cloud/remote/transport_headers_test.go#L61-L64(this comment)internal/cloud/remote/transport_headers_test.go#L91-L94internal/cloud/remote/transport_headers_test.go#L105-L108internal/cloud/remote/transport_headers_test.go#L122-L125internal/cloud/remote/transport_headers_test.go#L139-L142internal/cloud/remote/transport_headers_test.go#L186-L189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cloud/remote/transport_headers_test.go` around lines 61 - 64, In
internal/cloud/remote/transport_headers_test.go at lines 61-64, add a
mutex-protected safeBuffer implementing Write and String, importing sync as
needed; replace the local bytes.Buffer with safeBuffer at lines 61-64, 91-94,
105-108, 122-125, 139-142, and 186-189, preserving the existing global log
interception and assertions.
Comment on lines
+213
to
+228
| // contains is a helper to avoid importing strings in the test name collision. | ||
| func contains(s, substr string) bool { | ||
| return len(s) >= len(substr) && (s == substr || len(substr) == 0 || containsStr(s, substr)) | ||
| } | ||
|
|
||
| func containsStr(s, substr string) bool { | ||
| for i := 0; i <= len(s)-len(substr); i++ { | ||
| if s[i:i+len(substr)] == substr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // Suppress unused import warning - os is used by other tests that may reference it. | ||
| var _ = os.Stderr |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove custom contains helpers and unused import suppressions.
Reinventing strings.Contains to avoid an import is an anti-pattern. If there were an actual naming collision with a local strings variable, the import could be aliased (e.g., import stdstrings "strings"). Additionally, bypassing the Go compiler's unused import checks with a dummy variable (var _ = os.Stderr) obfuscates dependencies.
Please import "strings", use strings.Contains throughout the test file, and remove the unused os import entirely.
♻️ Proposed refactor
-// contains is a helper to avoid importing strings in the test name collision.
-func contains(s, substr string) bool {
- return len(s) >= len(substr) && (s == substr || len(substr) == 0 || containsStr(s, substr))
-}
-
-func containsStr(s, substr string) bool {
- for i := 0; i <= len(s)-len(substr); i++ {
- if s[i:i+len(substr)] == substr {
- return true
- }
- }
- return false
-}
-
-// Suppress unused import warning - os is used by other tests that may reference it.
-var _ = os.Stderr🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cloud/remote/transport_headers_test.go` around lines 213 - 228,
Replace the custom contains and containsStr helpers in the test file with the
standard strings.Contains function, importing strings or an alias if needed for
any naming collision. Update all call sites accordingly, then remove the unused
os import and the var _ = os.Stderr suppression.
Comment on lines
+37
to
+60
| pairs := strings.Split(raw, ",") | ||
| result := make(map[string]string, len(pairs)) | ||
|
|
||
| for _, pair := range pairs { | ||
| pair = strings.TrimSpace(pair) | ||
| idx := strings.IndexByte(pair, ':') | ||
| if idx < 0 { | ||
| log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: skipping malformed pair %q (missing colon)", pair) | ||
| continue | ||
| } | ||
| key := strings.TrimSpace(pair[:idx]) | ||
| value := strings.TrimSpace(pair[idx+1:]) | ||
| if key == "" { | ||
| continue | ||
| } | ||
| // Canonicalize the key (e.g. x-trace-id → X-Trace-Id). | ||
| canonical := http.CanonicalHeaderKey(key) | ||
| // Reject Authorization regardless of original casing. | ||
| if canonical == "Authorization" { | ||
| log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: refusing to override Authorization header") | ||
| continue | ||
| } | ||
| result[canonical] = value | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parsing logic breaks on valid header values containing commas.
The implementation uses strings.Split(raw, ","), which unintentionally truncates common HTTP header values that naturally contain commas (e.g., Accept-Encoding: gzip, deflate or X-Forwarded-For: 192.168.1.1, 10.0.0.1). The trailing segments will lack a colon, causing them to be discarded and triggering a spurious warning log.
internal/cloud/remote/transport.go#L37-L60: Update the loop to gracefully accumulate segments that lack colons into thecurrentKeyinstead of instantly discarding them.internal/cloud/remote/transport_headers_test.go#L168-L181: As per path instructions for**/*_test.go, add an edge-case test asserting that a header value containing a comma is parsed correctly.
🛠️ Proposed logic fix for transport.go
- pairs := strings.Split(raw, ",")
- result := make(map[string]string, len(pairs))
-
- for _, pair := range pairs {
- pair = strings.TrimSpace(pair)
- idx := strings.IndexByte(pair, ':')
- if idx < 0 {
- log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: skipping malformed pair %q (missing colon)", pair)
- continue
- }
- key := strings.TrimSpace(pair[:idx])
- value := strings.TrimSpace(pair[idx+1:])
- if key == "" {
- continue
- }
- // Canonicalize the key (e.g. x-trace-id → X-Trace-Id).
- canonical := http.CanonicalHeaderKey(key)
- // Reject Authorization regardless of original casing.
- if canonical == "Authorization" {
- log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: refusing to override Authorization header")
- continue
- }
- result[canonical] = value
- }
+ parts := strings.Split(raw, ",")
+ result := make(map[string]string)
+ var currentKey string
+
+ for _, part := range parts {
+ part = strings.TrimSpace(part)
+ if part == "" {
+ continue
+ }
+
+ idx := strings.IndexByte(part, ':')
+ if idx < 0 {
+ if currentKey != "" {
+ result[currentKey] += ", " + part
+ } else {
+ log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: skipping malformed segment %q (missing colon)", part)
+ }
+ continue
+ }
+
+ key := strings.TrimSpace(part[:idx])
+ if key == "" {
+ currentKey = ""
+ continue
+ }
+
+ canonical := http.CanonicalHeaderKey(key)
+ if canonical == "Authorization" {
+ log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: refusing to override Authorization header")
+ currentKey = "" // Prevent appending trailing segments to a rejected header
+ continue
+ }
+
+ currentKey = canonical
+ result[currentKey] = strings.TrimSpace(part[idx+1:])
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pairs := strings.Split(raw, ",") | |
| result := make(map[string]string, len(pairs)) | |
| for _, pair := range pairs { | |
| pair = strings.TrimSpace(pair) | |
| idx := strings.IndexByte(pair, ':') | |
| if idx < 0 { | |
| log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: skipping malformed pair %q (missing colon)", pair) | |
| continue | |
| } | |
| key := strings.TrimSpace(pair[:idx]) | |
| value := strings.TrimSpace(pair[idx+1:]) | |
| if key == "" { | |
| continue | |
| } | |
| // Canonicalize the key (e.g. x-trace-id → X-Trace-Id). | |
| canonical := http.CanonicalHeaderKey(key) | |
| // Reject Authorization regardless of original casing. | |
| if canonical == "Authorization" { | |
| log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: refusing to override Authorization header") | |
| continue | |
| } | |
| result[canonical] = value | |
| } | |
| parts := strings.Split(raw, ",") | |
| result := make(map[string]string) | |
| var currentKey string | |
| for _, part := range parts { | |
| part = strings.TrimSpace(part) | |
| if part == "" { | |
| continue | |
| } | |
| idx := strings.IndexByte(part, ':') | |
| if idx < 0 { | |
| if currentKey != "" { | |
| result[currentKey] += ", " + part | |
| } else { | |
| log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: skipping malformed segment %q (missing colon)", part) | |
| } | |
| continue | |
| } | |
| key := strings.TrimSpace(part[:idx]) | |
| if key == "" { | |
| currentKey = "" | |
| continue | |
| } | |
| canonical := http.CanonicalHeaderKey(key) | |
| if canonical == "Authorization" { | |
| log.Printf("[cloud] WARN ENGRAM_CLOUD_EXTRA_HEADERS: refusing to override Authorization header") | |
| currentKey = "" // Prevent appending trailing segments to a rejected header | |
| continue | |
| } | |
| currentKey = canonical | |
| result[currentKey] = strings.TrimSpace(part[idx+1:]) | |
| } |
📍 Affects 2 files
internal/cloud/remote/transport.go#L37-L60(this comment)internal/cloud/remote/transport_headers_test.go#L168-L181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cloud/remote/transport.go` around lines 37 - 60, Update the
extra-header parsing loop in transport.go to accumulate comma-separated segments
without a colon into the current header value instead of discarding them or
logging malformed-pair warnings, while preserving key canonicalization and
Authorization protection. Add an edge-case test in
internal/cloud/remote/transport_headers_test.go covering a header value
containing commas and asserting the complete value is retained.
Source: Path instructions
Comment on lines
+77
to
+84
| // sortStrings sorts a slice of strings in-place (insertion sort — small N, avoids importing sort). | ||
| func sortStrings(ss []string) { | ||
| for i := 1; i < len(ss); i++ { | ||
| for j := i; j > 0 && ss[j] < ss[j-1]; j-- { | ||
| ss[j], ss[j-1] = ss[j-1], ss[j] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer standard library sorting.
The net/http package already transitively imports sort (and slices), so avoiding the standard sort package provides no binary size benefit. Please use sort.Strings(keys) or slices.Sort(keys) (Go 1.21+) instead of maintaining a custom sorting implementation.
♻️ Proposed refactor
Replace sortStrings(keys) on line 71 with sort.Strings(keys), remove the sortStrings function entirely, and ensure sort is imported.
-// sortStrings sorts a slice of strings in-place (insertion sort — small N, avoids importing sort).
-func sortStrings(ss []string) {
- for i := 1; i < len(ss); i++ {
- for j := i; j > 0 && ss[j] < ss[j-1]; j-- {
- ss[j], ss[j-1] = ss[j-1], ss[j]
- }
- }
-}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cloud/remote/transport.go` around lines 77 - 84, Replace the call to
sortStrings with sort.Strings, add the standard library sort import, and remove
the custom sortStrings insertion-sort function. Preserve the existing in-place
sorting behavior for the string slice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked Issue
Closes #355
🏷️ PR Type
type:feature— New feature📝 Summary
ENGRAM_CLOUD_EXTRA_HEADERSenv var to inject custom HTTP headers into every outgoing cloud sync request (Cloudflare Access, reverse proxies, zero-trust gateways)Authorizationis rejected at parse time regardless of casing, preventing bearer-token shadowing; malformed pairs are skipped with a warning, values are never loggedRemoteTransportandMutationTransport; zero per-request overhead when unset📂 Changes
internal/cloud/remote/transport.goparseExtraHeaders/applyExtraHeaders,extraHeadersfield on both transport structs, wired intoReadManifest,WriteChunk,ReadChunk,PullMutationsinternal/cloud/remote/transport_extra_test.gointernal/cloud/remote/transport_headers_test.gocmd/engram/main.goDOCS.mdENGRAM_CLOUD_EXTRA_HEADERSin the autosync env var table🧪 Test Plan
go test ./...go test -tags e2e ./internal/server/...Design was reviewed and approved by @Alan-TheGentleman on the issue thread, including the
strings.TrimSpacecondition on both key and value after splitting (closes a leading-space bypass of theAuthorizationguardrail) — confirmed present inparseExtraHeaders.✅ Contributor Checklist
Closes #355)type:*label to this PRgo test ./...go test -tags e2e ./internal/server/...Co-Authored-Bytrailers in commits💬 Notes for Reviewers
Rebased onto latest
mainbefore opening (fork'smainwas ~65 commits behind upstream); resolved one trivial table-formatting conflict inDOCS.md.Summary by CodeRabbit
New Features
ENGRAM_CLOUD_EXTRA_HEADERS.Key: Valuepairs across all cloud sync operations.Documentation