feat(api): add interactive browser login for connection authentication#2928
Merged
Conversation
Implements browser-based authentication flow to capture cookies, session storage, and bearer tokens. Users can log in interactively and save auth state to HTTP connections. Supports Azure portal login with MSAL token extraction and includes browser connection testing functionality.
Contributor
WalkthroughAdds browser-based connection login/testing (Chromedp flows to capture cookies/session storage/JWTs and persist connections), DB-backed connection test/load paths, Playwright session and JWT models/formatters, API client methods, context management, and a CGO-disabled fast Makefile build target. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Command
participant Browser as Browser (Chrome DevTools)
participant Azure as Azure Identity
participant DB as Database
User->>CLI: connection login azure --url <URL>
CLI->>Browser: Launch non-headless browser with profile dir
Browser->>Azure: Navigate to auth URL
User->>Azure: Authenticate (enter credentials)
Azure-->>Browser: Redirect/complete with session storage & cookies
Browser->>Browser: Extract cookies, sessionStorage
Browser->>CLI: Return captured state (cookies, storage)
CLI->>DB: Save connection with storageState, headers, bearer tokens
DB-->>CLI: Persisted connection
CLI-->>User: Login complete
sequenceDiagram
participant CLI as CLI Command
participant DB as Database
participant Browser as Browser (Chrome DevTools)
participant Graph as Microsoft Graph
CLI->>DB: Load connection by name/namespace
DB-->>CLI: Return connection (cookies, sessionStorage, tokens)
CLI->>Browser: Launch fresh browser session
CLI->>Browser: Inject cookies (network.SetCookie)
CLI->>Browser: Inject sessionStorage via JS eval
Browser->>Graph: Navigate to screenshot/test URL
Graph-->>Browser: Page content loads
Browser->>Browser: detectLoginPage -> fail if login detected
Browser->>Browser: Capture screenshot
Browser-->>CLI: Return PNG
CLI->>Filesystem: Write screenshot to --output
CLI-->>User: Test result / screenshot path
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
Makefile (1)
157-158: Avoid version metadata drift between build targets.Line 158 omits the
-ldflagsused bybuild(Line 149), sobuild-slimbinaries can carry different/empty version metadata. Consider sharing one build-flag variable across both targets.♻️ Suggested diff
+.PHONY: build-flags +BUILD_LDFLAGS = -ldflags "-X \"main.version=$(VERSION_TAG) built at $(DATE)\"" + .PHONY: build build: static - go build -o ./.bin/$(NAME) -ldflags "-X \"main.version=$(VERSION_TAG) built at $(DATE)\"" main.go + go build -o ./.bin/$(NAME) $(BUILD_LDFLAGS) main.go .PHONY: build-slim build-slim: $(TAILWIND_JS) ## Fast go build only (no codegen or fmt) - CGO_ENABLED=0 go build -o ./.bin/$(NAME) main.go + CGO_ENABLED=0 go build -o ./.bin/$(NAME) $(BUILD_LDFLAGS) main.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 157 - 158, The build-slim target omits the -ldflags used by the build target, causing version metadata drift; update the Makefile so both targets share a single ldflags variable (e.g., LDFLAGS or BUILD_LDFLAGS) and have build-slim use that variable when invoking go build (referencing targets "build" and "build-slim" and the ldflags variable) so the produced binaries carry identical version metadata.connection/browser_state.go (1)
100-105:truncateStrmay split multi-byte UTF-8 characters.Slicing a string by byte index can produce invalid UTF-8 if the string contains multi-byte characters (e.g., emojis or non-ASCII text in cookie values).
♻️ Proposed fix using rune-aware truncation
func truncateStr(s string, maxLen int) string { - if len(s) <= maxLen { + runes := []rune(s) + if len(runes) <= maxLen { return s } - return s[:maxLen] + "..." + return string(runes[:maxLen]) + "..." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/browser_state.go` around lines 100 - 105, truncateStr currently slices the string by bytes which can split multi-byte UTF-8 characters; update truncateStr to be rune-aware (e.g., convert s to a []rune or iterate runes) and truncate by rune count so you never cut in the middle of a UTF-8 codepoint, then reconstruct the string and append "..." only when truncation occurred; reference the truncateStr function to locate and replace the byte-slicing implementation.cmd/connection_browser.go (2)
426-436: Confusing loop logic: always overwritesprops["bearer"]with the last token.The loop at lines 430-436 first checks for a Graph token and breaks, but if no Graph token is found, it unconditionally sets
props["bearer"]to each token, ending with the last one. Thebreakinside theifmakes the outer assignment unreachable for non-Graph tokens on that iteration.♻️ Clearer logic for selecting the default bearer token
if len(data.BearerTokens) > 0 { for aud, token := range data.BearerTokens { props["bearer_"+aud] = token } + // Prefer Graph API token as the default bearer + var defaultBearer string for aud, token := range data.BearerTokens { if strings.Contains(aud, "graph.microsoft.com") { - props["bearer"] = token + defaultBearer = token break } - props["bearer"] = token + if defaultBearer == "" { + defaultBearer = token + } + } + if defaultBearer != "" { + props["bearer"] = defaultBearer } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/connection_browser.go` around lines 426 - 436, The current loops over data.BearerTokens incorrectly keep overwriting props["bearer"] and end up with the last token; change the logic to pick a single default token: iterate the map once, record the first token as a fallback and if you encounter an audience containing "graph.microsoft.com" set that as the default and break, then after the loop set props["bearer"] to the chosen token. Use the existing data.BearerTokens and props["bearer"] symbols (and strings.Contains(aud, "graph.microsoft.com")) to locate and implement this single-pass selection.
550-551: Avoid fixedtime.Sleepfor page load synchronization.Using
time.Sleep(5 * time.Second)is brittle and may be insufficient for slow networks or excessive for fast ones. Consider using chromedp's built-in wait functions.♻️ Use chromedp wait functions instead
- // Wait for the page to settle - time.Sleep(5 * time.Second) + // Wait for the page to settle using network idle detection + if err := chromedp.Run(browserCtx, chromedp.WaitReady("body")); err != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "Warning: page may not be fully loaded: %v\n", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/connection_browser.go` around lines 550 - 551, Replace the brittle time.Sleep(5 * time.Second) call with chromedp's wait primitives: inside the same block where time.Sleep(5 * time.Second) is called (use the existing ctx passed to chromedp.Run), call chromedp.Run(ctx, chromedp.WaitVisible("<relevant CSS or XPath selector>"), chromedp.WaitReady("<same selector>")) or use chromedp.Poll/ chromedp.WaitNotPresent as appropriate to wait for the specific page element or condition you need; remove the fixed sleep after confirming the selector/condition is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/connection_browser.go`:
- Around line 379-387: The code registers stop (returned from duty.Start) as a
shutdown hook via shutdown.AddHookWithPriority and also defers stop(), causing
duplicate calls; in saveConnection remove the deferred call to stop() (or
alternatively make the stop function idempotent) so stop is only invoked by the
shutdown hook, referencing the duty.Start call and shutdown.AddHookWithPriority
registration to locate the change.
- Around line 252-314: The doneCh in waitForLoginComplete can block goroutines
(stdin reader, the URL watcher goroutine using chromedp.Location, and the bearer
watcher using extractSessionStorage) because it is only buffered with size 1
while up to 3 goroutines may send; change the signaling to be non-blocking by
either making doneCh buffered to accommodate all senders (e.g., capacity 3) or
replace each blocking send (doneCh <- struct{}{}) with a non-blocking send using
select { case doneCh <- struct{}{}: default: } so the goroutines won’t hang when
the channel is already full.
In `@cmd/connection_test_cmd.go`:
- Around line 64-96: The runConnectionTestFromDB function registers stop as a
shutdown hook and also defers stop(), which can cause double cleanup; either
remove the deferred call or remove the shutdown hook registration so stop is
only invoked once, or make stop() idempotent. Update the code around duty.Start
and shutdown.AddHookWithPriority in runConnectionTestFromDB to either: (a) keep
shutdown.AddHookWithPriority("database", shutdown.PriorityCritical, stop) and
delete defer stop(), or (b) keep defer stop() and remove the
shutdown.AddHookWithPriority registration, ensuring the chosen approach matches
the pattern used in connection_browser.go; reference the stop variable returned
from duty.Start and the shutdown.AddHookWithPriority call when making the
change.
In `@connection/check.go`:
- Around line 49-53: The call to ctx.WithHARCollector doesn't exist on
duty/context.Context; remove the HAR collector setup (the har.NewCollector(...)
and ctx.WithHARCollector(collector) lines) from connection/check.go, or if HAR
collection is required replace them by creating the collector with
har.NewCollector and attaching it via the correct HAR-compatible context type or
API (e.g., convert/use the HAR context wrapper or a function that accepts a
collector) instead of calling ctx.WithHARCollector; update any references to the
local variable collector accordingly.
In `@connection/jwt.go`:
- Around line 86-88: The code only handles aud as a string; update the
claims["aud"] handling to also accept an audience array by checking for
[]interface{} and []string forms: when claims["aud"] is an array, extract the
first string element (or join elements if you prefer a combined value) and
assign it to j.Audience; keep the existing string branch intact. Target the
claims["aud"] inspection near where j.Audience is set to ensure both string and
array JWT aud formats (Azure AD) are supported.
---
Nitpick comments:
In `@cmd/connection_browser.go`:
- Around line 426-436: The current loops over data.BearerTokens incorrectly keep
overwriting props["bearer"] and end up with the last token; change the logic to
pick a single default token: iterate the map once, record the first token as a
fallback and if you encounter an audience containing "graph.microsoft.com" set
that as the default and break, then after the loop set props["bearer"] to the
chosen token. Use the existing data.BearerTokens and props["bearer"] symbols
(and strings.Contains(aud, "graph.microsoft.com")) to locate and implement this
single-pass selection.
- Around line 550-551: Replace the brittle time.Sleep(5 * time.Second) call with
chromedp's wait primitives: inside the same block where time.Sleep(5 *
time.Second) is called (use the existing ctx passed to chromedp.Run), call
chromedp.Run(ctx, chromedp.WaitVisible("<relevant CSS or XPath selector>"),
chromedp.WaitReady("<same selector>")) or use chromedp.Poll/
chromedp.WaitNotPresent as appropriate to wait for the specific page element or
condition you need; remove the fixed sleep after confirming the
selector/condition is satisfied.
In `@connection/browser_state.go`:
- Around line 100-105: truncateStr currently slices the string by bytes which
can split multi-byte UTF-8 characters; update truncateStr to be rune-aware
(e.g., convert s to a []rune or iterate runes) and truncate by rune count so you
never cut in the middle of a UTF-8 codepoint, then reconstruct the string and
append "..." only when truncation occurred; reference the truncateStr function
to locate and replace the byte-slicing implementation.
In `@Makefile`:
- Around line 157-158: The build-slim target omits the -ldflags used by the
build target, causing version metadata drift; update the Makefile so both
targets share a single ldflags variable (e.g., LDFLAGS or BUILD_LDFLAGS) and
have build-slim use that variable when invoking go build (referencing targets
"build" and "build-slim" and the ldflags variable) so the produced binaries
carry identical version metadata.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9afdd94-10ef-49e4-99b6-f8b7ec6d2d77
📒 Files selected for processing (8)
Makefilecmd/connection_browser.gocmd/connection_test_cmd.goconnection/browser_state.goconnection/check.goconnection/jwt.goconnection/print.gogo.mod
💤 Files with no reviewable changes (1)
- connection/print.go
Comment on lines
+252
to
+314
| func waitForLoginComplete(browserCtx gocontext.Context, flags browserLoginFlags) { | ||
| doneCh := make(chan struct{}, 1) | ||
|
|
||
| go func() { | ||
| reader := bufio.NewReader(os.Stdin) | ||
| _, _ = reader.ReadString('\n') | ||
| doneCh <- struct{}{} | ||
| }() | ||
|
|
||
| if flags.WaitForURL != "" { | ||
| go func() { | ||
| ticker := time.NewTicker(time.Second) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| var currentURL string | ||
| if err := chromedp.Run(browserCtx, chromedp.Location(¤tURL)); err == nil { | ||
| if strings.Contains(currentURL, flags.WaitForURL) { | ||
| time.Sleep(2 * time.Second) | ||
| doneCh <- struct{}{} | ||
| return | ||
| } | ||
| } | ||
| case <-browserCtx.Done(): | ||
| return | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| if flags.Bearer { | ||
| go func() { | ||
| ticker := time.NewTicker(2 * time.Second) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| session, err := extractSessionStorage(browserCtx) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| for aud, token := range extractBearerTokens(session) { | ||
| jwt := connection.DecodeJWT(token) | ||
| if jwt != nil && time.Until(jwt.ExpiresAt) > 0 { | ||
| fmt.Fprintf(os.Stderr, "Found valid token for %s (expires in %s)\n", aud, time.Until(jwt.ExpiresAt).Round(time.Second)) | ||
| doneCh <- struct{}{} | ||
| return | ||
| } | ||
| } | ||
| case <-browserCtx.Done(): | ||
| return | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| select { | ||
| case <-doneCh: | ||
| case <-time.After(flags.Timeout): | ||
| fmt.Fprintln(os.Stderr, "Login timed out") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Potential goroutine leak when multiple completion conditions trigger.
The doneCh channel has buffer size 1, but up to 3 goroutines (stdin reader, URL watcher, bearer token watcher) may try to send. If multiple conditions complete, goroutines will block forever trying to send to a full channel.
🔧 Proposed fix - use buffered channel or select with default
func waitForLoginComplete(browserCtx gocontext.Context, flags browserLoginFlags) {
- doneCh := make(chan struct{}, 1)
+ doneCh := make(chan struct{}, 3) // Buffer for all possible senders
go func() {
reader := bufio.NewReader(os.Stdin)
_, _ = reader.ReadString('\n')
- doneCh <- struct{}{}
+ select {
+ case doneCh <- struct{}{}:
+ default:
+ }
}()Apply similar non-blocking sends to the other goroutines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/connection_browser.go` around lines 252 - 314, The doneCh in
waitForLoginComplete can block goroutines (stdin reader, the URL watcher
goroutine using chromedp.Location, and the bearer watcher using
extractSessionStorage) because it is only buffered with size 1 while up to 3
goroutines may send; change the signaling to be non-blocking by either making
doneCh buffered to accommodate all senders (e.g., capacity 3) or replace each
blocking send (doneCh <- struct{}{}) with a non-blocking send using select {
case doneCh <- struct{}{}: default: } so the goroutines won’t hang when the
channel is already full.
Comment on lines
+379
to
+387
| func saveConnection(cmd *cobra.Command, flags browserLoginFlags, data *browserSessionData) error { | ||
| ctx, stop, err := duty.Start("mission-control", duty.ClientOnly) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| shutdown.AddHookWithPriority("database", shutdown.PriorityCritical, stop) | ||
| defer stop() | ||
|
|
||
| props := make(map[string]string) |
Contributor
There was a problem hiding this comment.
Redundant cleanup: both shutdown hook and defer call stop().
Line 384 registers stop as a shutdown hook and line 385 defers it. This causes stop() to be called twice on normal exit.
🔧 Proposed fix - remove redundant defer
ctx, stop, err := duty.Start("mission-control", duty.ClientOnly)
if err != nil {
return err
}
shutdown.AddHookWithPriority("database", shutdown.PriorityCritical, stop)
- defer stop()Note: Keep the defer if the shutdown hook is intended for signal handling only and normal function exit should also clean up. In that case, ensure stop() is idempotent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/connection_browser.go` around lines 379 - 387, The code registers stop
(returned from duty.Start) as a shutdown hook via shutdown.AddHookWithPriority
and also defers stop(), causing duplicate calls; in saveConnection remove the
deferred call to stop() (or alternatively make the stop function idempotent) so
stop is only invoked by the shutdown hook, referencing the duty.Start call and
shutdown.AddHookWithPriority registration to locate the change.
Comment on lines
+49
to
+53
| collector := har.NewCollector(har.HARConfig{ | ||
| MaxBodySize: 64 * 1024, | ||
| CaptureContentTypes: []string{""}, | ||
| }) | ||
| ctx = ctx.WithHARCollector(collector) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if WithHARCollector exists in the duty package
ast-grep --pattern 'func ($_ Context) WithHARCollector($$$) $$$'
# Also search for any HAR-related methods on Context
rg -n 'func.*Context.*HAR' --type goRepository: flanksource/mission-control
Length of output: 1783
🌐 Web query:
Does github.com/flanksource/duty/context provide a WithHARCollector method?
💡 Result:
No, github.com/flanksource/duty/context does not provide a WithHARCollector method.
Citations:
- 1: https://github.com/flanksource/duty
- 2: https://github.com/flanksource/canary-checker/blob/master/checks/http.go
- 3: https://pkg.go.dev/github.com/flanksource/duty/rbac
- 4: https://www.debbiedoescasino.com
Remove ctx.WithHARCollector() call — method does not exist on duty/context.Context.
The duty context package does not provide a WithHARCollector method. Either remove the HAR collector setup (lines 49-53) or use an alternative context type if HAR collection is required.
🧰 Tools
🪛 GitHub Actions: Build
[error] 53-53: Go build failed: ctx.WithHARCollector undefined (type "github.com/flanksource/duty/context".Context has no field or method WithHARCollector)
🪛 GitHub Actions: CodeQL
[error] 53-53: CodeQL Go extractor failed to extract package github.com/flanksource/incident-commander/connection: ctx.WithHARCollector undefined (type "github.com/flanksource/duty/context".Context has no field or method WithHARCollector)
🪛 GitHub Actions: Lint
[error] 53-53: golangci-lint error: ctx.WithHARCollector undefined (type "github.com/flanksource/duty/context".Context has no field or method WithHARCollector)
🪛 GitHub Actions: Test
[error] 53-53: Compilation error: ctx.WithHARCollector undefined (type "github.com/flanksource/duty/context".Context has no field or method WithHARCollector).
🪛 GitHub Check: e2e
[failure] 53-53:
ctx.WithHARCollector undefined (type "github.com/flanksource/duty/context".Context has no field or method WithHARCollector)
🪛 GitHub Check: lint
[failure] 53-53:
ctx.WithHARCollector undefined (type "github.com/flanksource/duty/context".Context has no field or method WithHARCollector)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/check.go` around lines 49 - 53, The call to ctx.WithHARCollector
doesn't exist on duty/context.Context; remove the HAR collector setup (the
har.NewCollector(...) and ctx.WithHARCollector(collector) lines) from
connection/check.go, or if HAR collection is required replace them by creating
the collector with har.NewCollector and attaching it via the correct
HAR-compatible context type or API (e.g., convert/use the HAR context wrapper or
a function that accepts a collector) instead of calling ctx.WithHARCollector;
update any references to the local variable collector accordingly.
Comment on lines
+86
to
+88
| if v, ok := claims["aud"].(string); ok { | ||
| j.Audience = v | ||
| } |
Contributor
There was a problem hiding this comment.
The aud claim can be an array in Azure AD tokens.
Per JWT/OAuth2 specs, the aud claim can be either a string or an array of strings. Azure AD tokens commonly use array format. The current code only handles the string case.
🔧 Proposed fix to handle array audiences
if v, ok := claims["aud"].(string); ok {
j.Audience = v
+ } else if arr, ok := claims["aud"].([]any); ok && len(arr) > 0 {
+ if s, ok := arr[0].(string); ok {
+ j.Audience = s
+ }
}📝 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
| if v, ok := claims["aud"].(string); ok { | |
| j.Audience = v | |
| } | |
| if v, ok := claims["aud"].(string); ok { | |
| j.Audience = v | |
| } else if arr, ok := claims["aud"].([]any); ok && len(arr) > 0 { | |
| if s, ok := arr[0].(string); ok { | |
| j.Audience = s | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/jwt.go` around lines 86 - 88, The code only handles aud as a
string; update the claims["aud"] handling to also accept an audience array by
checking for []interface{} and []string forms: when claims["aud"] is an array,
extract the first string element (or join elements if you prefer a combined
value) and assign it to j.Audience; keep the existing string branch intact.
Target the claims["aud"] inspection near where j.Audience is set to ensure both
string and array JWT aud formats (Azure AD) are supported.
Adds a new context system for managing multiple Mission Control configurations. Contexts store server URL, access token, and database connection. Auto-saves context on login and applies saved context settings on startup. Includes CLI commands to list, switch, and view contexts.
Enables CLI to use Mission Control API when available instead of direct database access. Adds SDK client with GetConnection, SaveConnection, and TestConnection methods. Connection commands now prefer API when authenticated context exists.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
13-47:⚠️ Potential issue | 🔴 CriticalFix incompatibility between
chromedpandcdprotoversions.The pinned
cdprotoversion (2026-03-20) is incompatible withchromedpv0.15.0 (released 2026-03-21), which expectscdprotofrom 2026-03-21. This mismatch causes build failures per GitHub issue#1623. Either upgradecdprototo match the expected 2026-03-21 version or downgradechromedpto a compatible release such as v0.15.1 (which includes the correctcdprotoalignment).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 13 - 47, The build fails due to a version mismatch between github.com/chromedp/cdproto (currently pinned to the 2026-03-20 pseudo-version) and the chromedp client; fix by aligning versions in go.mod: either bump github.com/chromedp/cdproto to the 2026-03-21 pseudo-version that chromedp v0.15.0 expects, or change the chromedp module to a release that bundles the older cdproto (e.g., chromedp v0.15.1); after updating the module version(s) in go.mod, run `go get`/`go mod tidy` and rebuild to verify the incompatibility is resolved.
♻️ Duplicate comments (2)
cmd/connection_browser.go (2)
370-376:⚠️ Potential issue | 🟡 MinorRedundant cleanup: both shutdown hook and defer call
stop().Line 375 registers
stopas a shutdown hook and line 376 defers it. This causesstop()to be called twice on normal exit.🔧 Proposed fix - remove redundant defer
ctx, stop, err := duty.Start("mission-control", duty.ClientOnly) if err != nil { return err } shutdown.AddHookWithPriority("database", shutdown.PriorityCritical, stop) - defer stop()Note: Keep the defer if the shutdown hook is intended for signal handling only and normal function exit should also clean up. In that case, ensure
stop()is idempotent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/connection_browser.go` around lines 370 - 376, In saveConnection, you're registering stop via shutdown.AddHookWithPriority and also deferring stop(), causing double invocation; either remove the defer stop() call or make the shutdown hook only for signal handling and ensure the stop() implementation (the duty.Stop function) is idempotent; update code around saveConnection, the stop variable and the shutdown.AddHookWithPriority usage so stop() is invoked exactly once on normal exit (or safely multiple times if you choose idempotence).
243-305:⚠️ Potential issue | 🟠 MajorPotential goroutine leak when multiple completion conditions trigger.
The
doneChchannel has buffer size 1, but up to 3 goroutines (stdin reader, URL watcher, bearer token watcher) may try to send. If multiple conditions complete, goroutines will block forever trying to send to a full channel.🔧 Proposed fix - use buffered channel or non-blocking sends
func waitForLoginComplete(browserCtx gocontext.Context, flags browserLoginFlags) { - doneCh := make(chan struct{}, 1) + doneCh := make(chan struct{}, 3) // Buffer for all possible senders go func() { reader := bufio.NewReader(os.Stdin) _, _ = reader.ReadString('\n') - doneCh <- struct{}{} + select { + case doneCh <- struct{}{}: + default: + } }()Apply similar non-blocking sends to the other goroutines at lines 263 and 289.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/connection_browser.go` around lines 243 - 305, The doneCh in waitForLoginComplete can block because three goroutines (the stdin reader, the URL watcher goroutine inside the flags.WaitForURL block, and the bearer token watcher inside the flags.Bearer block) may all attempt to send to a buffered channel of size 1; modify each send to doneCh to be non-blocking (use a select { case doneCh <- struct{}{}: default: } or equivalent) or increase the channel capacity to the number of potential senders so none of the goroutines can block on the send (update the sends in the anonymous reader goroutine, the URL watcher goroutine, and the bearer watcher goroutine).
🧹 Nitpick comments (4)
cmd/connection_browser.go (3)
416-428: Bearer token selection logic could be simplified.The two loops at lines 418-428 can be combined. The current logic also has a subtle issue: it iterates all tokens setting
props["bearer"]to each one, then breaks only when finding graph.microsoft.com.♻️ Proposed simplification
// Store bearer tokens if len(data.BearerTokens) > 0 { + var defaultBearer string for aud, token := range data.BearerTokens { props["bearer_"+aud] = token - } - for aud, token := range data.BearerTokens { if strings.Contains(aud, "graph.microsoft.com") { props["bearer"] = token - break + } else if defaultBearer == "" { + defaultBearer = token } - props["bearer"] = token + } + if props["bearer"] == "" && defaultBearer != "" { + props["bearer"] = defaultBearer } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/connection_browser.go` around lines 416 - 428, The loop logic storing bearer tokens is redundant and incorrectly overrides props["bearer"] multiple times; update the block that uses data.BearerTokens to do a single pass: for each aud, set props["bearer_"+aud]=token, capture the first token (firstToken) and if aud contains "graph.microsoft.com" capture graphToken, then after the loop set props["bearer"] to graphToken if present otherwise to firstToken; refer to variables props and data.BearerTokens to locate and replace the two loops with this single-pass logic.
217-225: Duplicated cookie conversion logic.The same conversion from
*network.Cookietoconnection.Cookieappears twice. Consider extracting to a helper function.♻️ Proposed refactor
func convertCookies(cdpCookies []*network.Cookie) connection.Cookies { var cookies connection.Cookies for _, c := range cdpCookies { cookies = append(cookies, connection.Cookie{ Name: c.Name, Value: c.Value, Domain: c.Domain, Path: c.Path, Expires: float64(c.Expires), HTTPOnly: c.HTTPOnly, Secure: c.Secure, SameSite: string(c.SameSite), }) } return cookies }Then use
convertCookies(data.Cookies)at both locations.Also applies to: 381-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/connection_browser.go` around lines 217 - 225, Extract the duplicated conversion logic from []*network.Cookie to connection.Cookies into a helper function (e.g., convertCookies(cdpCookies []*network.Cookie) connection.Cookies) that iterates cdpCookies and builds connection.Cookie entries (preserving Name, Value, Domain, Path, Expires as float64, HTTPOnly, Secure, SameSite), then replace both inline loops (the block building cookies at the first occurrence and the similar block around lines 381-393) with calls to convertCookies(data.Cookies) to avoid duplication.
541-542: Hardcoded sleep is fragile for page load detection.A 5-second sleep may be insufficient for slow networks or too long for fast ones. Consider using
chromedp.WaitReadyor similar.💡 Optional: Use page readiness detection
- // Wait for the page to settle - time.Sleep(5 * time.Second) + // Wait for the page to be ready + if err := chromedp.Run(browserCtx, chromedp.WaitReady("body", chromedp.ByQuery)); err != nil { + // Fallback to sleep if wait fails + time.Sleep(3 * time.Second) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/connection_browser.go` around lines 541 - 542, Replace the fragile hardcoded time.Sleep(5 * time.Second) in cmd/connection_browser.go with an active page-readiness wait: instead create/derive a context with timeout and use chromedp.WaitReady (or chromedp.Poll / chromedp.Run with a query) to wait for a reliable DOM signal (e.g., "body" or a specific element/id that indicates the page has loaded) before proceeding; update the code around the existing time.Sleep call to call chromedp.WaitReady("body", chromedp.ByQuery) (or a more specific selector) with the context timeout so slow networks time out cleanly and fast loads proceed immediately.cmd/context.go (1)
110-117: Consider logging config load failures for debugging.
contextHasAPIsilently ignoresLoadConfigerrors. While this is safe behavior, a debug-level log could help troubleshoot issues.💡 Optional: Add debug logging
func contextHasAPI() (*MCContext, bool) { - cfg, _ := LoadConfig() + cfg, err := LoadConfig() + if err != nil { + // Could add debug logging here if logger is available + return nil, false + } if cfg == nil { return nil, false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/context.go` around lines 110 - 117, The function contextHasAPI currently swallows the error from LoadConfig; update it to capture the returned error (err := LoadConfig()) and, if err != nil, emit a debug-level log using the repo's existing logger (e.g., logger.Debugf or similar) with a clear message including the error (while preserving the current return behavior). Modify the signature usage in contextHasAPI to use the captured err, leave returns and nil checks for cfg/ctx as-is, and reference LoadConfig and contextHasAPI in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/connection_browser.go`:
- Around line 472-478: In runBrowserTest, you register the shutdown hook by
calling shutdown.AddHookWithPriority(...) after starting duty.Start, but you
also call defer stop(), which causes double cleanup; remove the redundant defer
stop() and rely on the registered shutdown hook to call stop (leave duty.Start,
stop variable and the AddHookWithPriority call intact) so stop is only invoked
via the shutdown hook.
- Around line 525-533: The code reads conn.Properties["sessionStorage"] in
runBrowserTest but saveConnection stores the session under
props["storageState"], so injectSessionStorage never gets data; update the
retrieval to use the same key or persist sessionStorage separately: either
change the lookup in runBrowserTest to conn.Properties["storageState"] and
deserialize into the PlaywrightSessionState and/or extract its sessionStorage
map, or modify saveConnection to also save the raw sessionStorage string under
"sessionStorage"; adjust the logic around injectSessionStorage,
PlaywrightSessionState, and the serialize/deserialize paths so the stored value
matches what injectSessionStorage expects.
---
Outside diff comments:
In `@go.mod`:
- Around line 13-47: The build fails due to a version mismatch between
github.com/chromedp/cdproto (currently pinned to the 2026-03-20 pseudo-version)
and the chromedp client; fix by aligning versions in go.mod: either bump
github.com/chromedp/cdproto to the 2026-03-21 pseudo-version that chromedp
v0.15.0 expects, or change the chromedp module to a release that bundles the
older cdproto (e.g., chromedp v0.15.1); after updating the module version(s) in
go.mod, run `go get`/`go mod tidy` and rebuild to verify the incompatibility is
resolved.
---
Duplicate comments:
In `@cmd/connection_browser.go`:
- Around line 370-376: In saveConnection, you're registering stop via
shutdown.AddHookWithPriority and also deferring stop(), causing double
invocation; either remove the defer stop() call or make the shutdown hook only
for signal handling and ensure the stop() implementation (the duty.Stop
function) is idempotent; update code around saveConnection, the stop variable
and the shutdown.AddHookWithPriority usage so stop() is invoked exactly once on
normal exit (or safely multiple times if you choose idempotence).
- Around line 243-305: The doneCh in waitForLoginComplete can block because
three goroutines (the stdin reader, the URL watcher goroutine inside the
flags.WaitForURL block, and the bearer token watcher inside the flags.Bearer
block) may all attempt to send to a buffered channel of size 1; modify each send
to doneCh to be non-blocking (use a select { case doneCh <- struct{}{}: default:
} or equivalent) or increase the channel capacity to the number of potential
senders so none of the goroutines can block on the send (update the sends in the
anonymous reader goroutine, the URL watcher goroutine, and the bearer watcher
goroutine).
---
Nitpick comments:
In `@cmd/connection_browser.go`:
- Around line 416-428: The loop logic storing bearer tokens is redundant and
incorrectly overrides props["bearer"] multiple times; update the block that uses
data.BearerTokens to do a single pass: for each aud, set
props["bearer_"+aud]=token, capture the first token (firstToken) and if aud
contains "graph.microsoft.com" capture graphToken, then after the loop set
props["bearer"] to graphToken if present otherwise to firstToken; refer to
variables props and data.BearerTokens to locate and replace the two loops with
this single-pass logic.
- Around line 217-225: Extract the duplicated conversion logic from
[]*network.Cookie to connection.Cookies into a helper function (e.g.,
convertCookies(cdpCookies []*network.Cookie) connection.Cookies) that iterates
cdpCookies and builds connection.Cookie entries (preserving Name, Value, Domain,
Path, Expires as float64, HTTPOnly, Secure, SameSite), then replace both inline
loops (the block building cookies at the first occurrence and the similar block
around lines 381-393) with calls to convertCookies(data.Cookies) to avoid
duplication.
- Around line 541-542: Replace the fragile hardcoded time.Sleep(5 * time.Second)
in cmd/connection_browser.go with an active page-readiness wait: instead
create/derive a context with timeout and use chromedp.WaitReady (or
chromedp.Poll / chromedp.Run with a query) to wait for a reliable DOM signal
(e.g., "body" or a specific element/id that indicates the page has loaded)
before proceeding; update the code around the existing time.Sleep call to call
chromedp.WaitReady("body", chromedp.ByQuery) (or a more specific selector) with
the context timeout so slow networks time out cleanly and fast loads proceed
immediately.
In `@cmd/context.go`:
- Around line 110-117: The function contextHasAPI currently swallows the error
from LoadConfig; update it to capture the returned error (err := LoadConfig())
and, if err != nil, emit a debug-level log using the repo's existing logger
(e.g., logger.Debugf or similar) with a clear message including the error (while
preserving the current return behavior). Modify the signature usage in
contextHasAPI to use the captured err, leave returns and nil checks for cfg/ctx
as-is, and reference LoadConfig and contextHasAPI in your change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 760b10cb-1ac5-4c5d-b3fa-b8e7f1505959
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
cmd/auth_login.gocmd/connection_browser.gocmd/connection_test_cmd.gocmd/connections.gocmd/context.gocmd/root.gogo.modsdk/client.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/connection_test_cmd.go
Comment on lines
+472
to
+478
| func runBrowserTest(cmd *cobra.Command, args []string) error { | ||
| ctx, stop, err := duty.Start("mission-control", duty.ClientOnly) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| shutdown.AddHookWithPriority("database", shutdown.PriorityCritical, stop) | ||
| defer stop() |
Contributor
There was a problem hiding this comment.
Same redundant defer issue in runBrowserTest.
Lines 477-478 have the same pattern of shutdown hook + defer that causes double cleanup.
🔧 Proposed fix
ctx, stop, err := duty.Start("mission-control", duty.ClientOnly)
if err != nil {
return err
}
shutdown.AddHookWithPriority("database", shutdown.PriorityCritical, stop)
- defer stop()📝 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
| func runBrowserTest(cmd *cobra.Command, args []string) error { | |
| ctx, stop, err := duty.Start("mission-control", duty.ClientOnly) | |
| if err != nil { | |
| return err | |
| } | |
| shutdown.AddHookWithPriority("database", shutdown.PriorityCritical, stop) | |
| defer stop() | |
| func runBrowserTest(cmd *cobra.Command, args []string) error { | |
| ctx, stop, err := duty.Start("mission-control", duty.ClientOnly) | |
| if err != nil { | |
| return err | |
| } | |
| shutdown.AddHookWithPriority("database", shutdown.PriorityCritical, stop) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/connection_browser.go` around lines 472 - 478, In runBrowserTest, you
register the shutdown hook by calling shutdown.AddHookWithPriority(...) after
starting duty.Start, but you also call defer stop(), which causes double
cleanup; remove the redundant defer stop() and rely on the registered shutdown
hook to call stop (leave duty.Start, stop variable and the AddHookWithPriority
call intact) so stop is only invoked via the shutdown hook.
Comment on lines
+525
to
+533
| // Inject MSAL session storage | ||
| if sessionJSON := conn.Properties["sessionStorage"]; sessionJSON != "" { | ||
| var session map[string]string | ||
| if err := json.Unmarshal([]byte(sessionJSON), &session); err == nil { | ||
| if err := injectSessionStorage(browserCtx, screenshotURL, session); err != nil { | ||
| fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to inject sessionStorage: %v\n", err) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Property key mismatch: reading "sessionStorage" but saving "storageState".
saveConnection stores the session state under props["storageState"] (line 401), but runBrowserTest reads from conn.Properties["sessionStorage"] (line 526). This will always be empty, so session storage injection will never work.
🔧 Proposed fix
// Inject MSAL session storage
- if sessionJSON := conn.Properties["sessionStorage"]; sessionJSON != "" {
- var session map[string]string
- if err := json.Unmarshal([]byte(sessionJSON), &session); err == nil {
+ if storageJSON := conn.Properties["storageState"]; storageJSON != "" {
+ var state connection.PlaywrightSessionState
+ if err := json.Unmarshal([]byte(storageJSON), &state); err == nil {
+ // Extract session storage from origins if available, or use tokens
if err := injectSessionStorage(browserCtx, screenshotURL, session); err != nil {Note: The PlaywrightSessionState struct stores session data differently. You may need to restructure how session storage is persisted and retrieved, or store the raw sessionStorage map separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/connection_browser.go` around lines 525 - 533, The code reads
conn.Properties["sessionStorage"] in runBrowserTest but saveConnection stores
the session under props["storageState"], so injectSessionStorage never gets
data; update the retrieval to use the same key or persist sessionStorage
separately: either change the lookup in runBrowserTest to
conn.Properties["storageState"] and deserialize into the PlaywrightSessionState
and/or extract its sessionStorage map, or modify saveConnection to also save the
raw sessionStorage string under "sessionStorage"; adjust the logic around
injectSessionStorage, PlaywrightSessionState, and the serialize/deserialize
paths so the stored value matches what injectSessionStorage expects.
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
1 participant
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.
Implements browser-based authentication flow to capture cookies, session storage, and bearer tokens. Users can log in interactively and save auth state to HTTP connections. Supports Azure portal login with MSAL token extraction and includes browser connection testing functionality.
Summary by CodeRabbit
New Features
Build
build-slimmake target.Chores