fix(headless): merge extra headers#6376
Conversation
WalkthroughAdds per-browser default headers to Browser, initializes them from options.CustomHeaders (excluding User-Agent), and applies them to pages via a new Browser.applyDefaultHeaders method. The page creation flow now calls applyDefaultHeaders and removes the previous hardcoded Accept-Language SetExtraHeaders call. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Engine
participant Browser
participant Page
Client->>Engine: request new page
Engine->>Browser: ensure Browser created with defaultHeaders
Browser->>Page: create rod.Page
Browser->>Page: applyDefaultHeaders(Page)\n(SetExtraHeaders: Accept-Language + defaultHeaders)
Browser->>Page: apply User-Agent if provided
Client->>Page: proceed with cookies/navigation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)pkg/protocols/headless/engine/engine.go (2)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ysokolovsky
force-pushed
the
fix/headless-headers-merge
branch
from
38843c3 to
3fc2cc2
Compare
![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
🔭 Outside diff range comments (2)
pkg/protocols/headless/engine/engine.go (2)
101-116: Trim User-Agent value when parsing CLI headersAvoid leading/trailing whitespace in UA; currently it may retain a leading space after the colon. Trim it like other headers.
- if strings.EqualFold(parts[0], "User-Agent") { - customAgent = parts[1] + if strings.EqualFold(parts[0], "User-Agent") { + customAgent = strings.TrimSpace(parts[1]) } else { k := strings.TrimSpace(parts[0]) v := strings.TrimSpace(parts[1]) if k == "" || v == "" { continue } defaultHeaders[k] = v }
23-34: getHTTPClient can return (nil, nil) after first failure due to sync.Once capturing error in a local varIf newHttpClient fails the first time, Once prevents retry, the local err is lost on subsequent calls, and callers may receive a nil client with a nil error. Persist the initialization error on the Browser struct and return it consistently.
type Browser struct { customAgent string defaultHeaders map[string]string tempDir string previousPIDs map[int32]struct{} // track already running PIDs engine *rod.Browser options *types.Options launcher *launcher.Launcher // use getHTTPClient to get the http client - httpClient *http.Client - httpClientOnce *sync.Once + httpClient *http.Client + httpClientErr error + httpClientOnce *sync.Once }func (b *Browser) getHTTPClient() (*http.Client, error) { - var err error - b.httpClientOnce.Do(func() { - b.httpClient, err = newHttpClient(b.options) - }) - return b.httpClient, err + b.httpClientOnce.Do(func() { + b.httpClient, b.httpClientErr = newHttpClient(b.options) + }) + return b.httpClient, b.httpClientErr }Alternative approach: remove sync.Once and retry construction on subsequent calls until success using a mutex; but the above is the minimal, safe change.
Also applies to: 161-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/protocols/headless/engine/engine.go(4 hunks)pkg/protocols/headless/engine/page.go(1 hunks)
🔇 Additional comments (1)
pkg/protocols/headless/engine/page.go (1)
64-67: Apply default headers early — good; ensure Accept-Language behavior remains intentionalApplying headers once per page before UA override is correct. With the engine-side fix to always set Accept-Language unless overridden, this preserves previous behavior while allowing user-specified headers.
If you keep the current engine implementation, Accept-Language will not be set when no custom headers are provided. Confirm intended behavior or adopt the proposed engine change.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
pkg/protocols/headless/engine/engine.go (1)
101-115: Normalize and harden header parsing; trim UA; handle “User-Agent” detection after trimming.
- Trim key/value before checking for “User-Agent” so “ User-Agent” is recognized.
- Trim the UA value to avoid leading spaces.
- Basic hardening: skip headers containing CR/LF to prevent header injection into CDP.
- Optional: use strings.Cut for simpler split.
- for _, option := range options.CustomHeaders { - parts := strings.SplitN(option, ":", 2) - if len(parts) != 2 { - continue - } - if strings.EqualFold(parts[0], "User-Agent") { - customAgent = parts[1] - } else { - k := strings.TrimSpace(parts[0]) - v := strings.TrimSpace(parts[1]) - if k == "" || v == "" { - continue - } - defaultHeaders[k] = v - } - } + for _, h := range options.CustomHeaders { + parts := strings.SplitN(h, ":", 2) + if len(parts) != 2 { + continue + } + k := strings.TrimSpace(parts[0]) + v := strings.TrimSpace(parts[1]) + if k == "" || v == "" { + continue + } + // basic hardening against CR/LF in header k/v + if strings.ContainsAny(k, "\r\n") || strings.ContainsAny(v, "\r\n") { + continue + } + if strings.EqualFold(k, "User-Agent") { + customAgent = v + continue + } + defaultHeaders[k] = v + }Alternative (optional):
- parts := strings.SplitN(h, ":", 2) - if len(parts) != 2 { - continue - } - k := strings.TrimSpace(parts[0]) - v := strings.TrimSpace(parts[1]) + key, val, ok := strings.Cut(h, ":") + if !ok { + continue + } + k := strings.TrimSpace(key) + v := strings.TrimSpace(val)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/protocols/headless/engine/engine.go(4 hunks)pkg/protocols/headless/engine/page.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/protocols/headless/engine/engine.go (2)
pkg/protocols/headless/engine/page.go (2)
Options(50-54)Page(26-41)pkg/types/types.go (1)
Options(32-464)
| if err = i.browser.applyDefaultHeaders(page); err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good placement; ensure Accept-Language still applies with no custom headers.
Calling applyDefaultHeaders right after setting Timeout is correct. With the current engine implementation, Accept-Language won’t be set if defaultHeaders is empty (regression). Please merge the engine.go fix to always apply Accept-Language unless the user provided one.
To confirm coverage and avoid missed call sites:
🏁 Script executed:
#!/bin/bash
# Find all Page creations and check that default headers are applied.
rg -n 'Page\s*\(\s*proto\.TargetCreateTarget' -A2
rg -n 'SetExtraHeaders\s*\(' -A2Length of output: 487
All page creation sites covered; merge Accept-Language fix
The placement of applyDefaultHeaders immediately after setting Timeout in page.go is correct and covers all page creation calls. However, until the engine patch is merged, Accept-Language won’t be applied when defaultHeaders is empty.
Key locations checked:
- Page creation in
pkg/protocols/headless/engine/page.go:58 SetExtraHeaderscall inpkg/protocols/headless/engine/engine.go:157–159
Action required:
- Merge the fix in
engine.goto ensureAccept-Languageis always set unless explicitly overridden by the user.
🤖 Prompt for AI Agents
In pkg/protocols/headless/engine/page.go around lines 64 to 67,
applyDefaultHeaders is correctly placed but currently Accept-Language is not
applied when defaultHeaders is empty; merge the fix from
pkg/protocols/headless/engine/engine.go (around lines 157–159) into the codebase
so that SetExtraHeaders ensures Accept-Language is always added unless the user
explicitly sets it—update SetExtraHeaders logic to insert Accept-Language into
headers when missing (or when defaultHeaders is empty) before it’s passed to
applyDefaultHeaders and ensure the merged change is used by page creation paths.
|
@coderabbitai create and link github issue with this PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 823 @ehsandeep I've successfully created GitHub issue #6377 to track this PR's implementation of headless browser header merging. The issue has been assigned to you and includes all relevant links. Issue URL: #6377 |
dwisiswant0
changed the title
4 participants
Proposed changes
New field: Browser.defaultHeaders map[string]string.
Parse CLI headers: copy non-UA headers into defaultHeaders; keep User-Agent in customAgent .
New helper: applyDefaultHeaders(*rod.Page) error builds alternating []string{k,v,...} and calls SetExtraHeaders once.
Invoke helper: At the start of Instance.Run(...) after page creation and timeout.
fixes issue 6352
Steps to reproduce
X-DemoinlineChecklist
Summary by CodeRabbit
New Features
Refactor