Add auto login#7547
Open
Mzack9999 wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughAdds turnkey HTTP/headless auto-login, manual session capture, recording replay, browser-storage authentication, session reauthentication, provider lifecycle cleanup, and authenticated HTTP/headless protocol integration with extensive tests. ChangesTurnkey authentication
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runner
participant AuthProvider
participant LoginEngine
participant Target
participant HTTPOrHeadless
CLI->>Runner: configure auto-login options
Runner->>AuthProvider: construct dynamic provider
HTTPOrHeadless->>AuthProvider: resolve strategies
AuthProvider->>LoginEngine: authenticate and capture session
LoginEngine->>Target: submit login flow
Target-->>LoginEngine: cookies, token, or browser storage
LoginEngine-->>AuthProvider: apply captured session
AuthProvider-->>HTTPOrHeadless: headers, cookies, and storage
HTTPOrHeadless->>Target: authenticated request
Target-->>HTTPOrHeadless: response status
HTTPOrHeadless->>AuthProvider: notify response status
AuthProvider->>LoginEngine: reauthenticate when configured
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/protocols/http/request.go (1)
1096-1102: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBuild-breaking:
RateLimitTakeis not a field onanalyzers.Options.
pkg/fuzz/analyzers/analyzers.godefinesOptionswithoutRateLimitTake, so this struct literal will not compile. Either add the missing field toanalyzers.Optionsor remove/replace this argument with the intended value.🤖 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 `@pkg/protocols/http/request.go` around lines 1096 - 1102, Fix the `analyzer.Analyze` options construction in the request analysis flow so it matches the fields defined by `analyzers.Options`: either add and properly support `RateLimitTake` in that type, or remove/replace the unsupported struct field with the intended existing value while preserving the rate-limit behavior.Source: Linters/SAST tools
pkg/protocols/headless/engine/page.go (1)
230-237: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReauthentication is signaled too late to affect subsequent actions. Response inspection occurs after the entire action sequence, while auth material is resolved only once before that sequence.
pkg/protocols/headless/engine/page.go#L230-L237: inspect each main navigation response during action execution.pkg/protocols/headless/engine/page.go#L189-L194: re-resolve and apply stale auth material before the following navigation.pkg/protocols/headless/engine/auth_reauth_test.go#L80-L128: add a second navigation and assert refreshed credentials are actually sent.🤖 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 `@pkg/protocols/headless/engine/page.go` around lines 230 - 237, Update page.go lines 230-237 so each main navigation response is inspected during action execution and can trigger reauthentication immediately, rather than only after the full action sequence. In page.go lines 189-194, re-resolve and apply refreshed auth material before the next navigation. Extend auth_reauth_test.go lines 80-128 with a second navigation and assert that it sends the refreshed credentials.pkg/authprovider/authx/dynamic.go (1)
133-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the parsed refresh interval during revalidation.
After a previously configured interval is cleared,
Validateleaves the oldrefreshIntervalactive, so the session continues expiring unexpectedly.Proposed fix
func (d *Dynamic) Validate() error { // NOTE: Validate() must not be called concurrently with Fetch()/GetStrategies(). // Re-validating resets fetch state and allows re-fetching. d.fetchState = &fetchState{} + d.refreshInterval = 0🤖 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 `@pkg/authprovider/authx/dynamic.go` around lines 133 - 136, Update Dynamic.Validate to reset the parsed refreshInterval alongside fetchState when revalidation begins, ensuring a previously configured interval is cleared and cannot remain active after configuration changes.
🧹 Nitpick comments (1)
pkg/protocols/http/build_request.go (1)
146-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared URL-resolution logic between
ApplyAuthandNotifyResponse.Both methods repeat the same
g.request.URL/urlutil.ParseAbsoluteURL(g.rawRequest.FullURL, true)switch.ApplyAuthlogs a warning on parse failure whileNotifyResponsesilently returns — worth unifying via a shared helper (e.g.targetURL() (*urlutil.URL, error)) so both call sites get consistent error visibility and future changes don't drift.♻️ Proposed refactor
+func (g *generatedRequest) targetURL() (*urlutil.URL, error) { + if g.request != nil { + return g.request.URL, nil + } + if g.rawRequest != nil { + return urlutil.ParseAbsoluteURL(g.rawRequest.FullURL, true) + } + return nil, nil +} + func (g *generatedRequest) NotifyResponse(provider authprovider.AuthProvider, resp *http.Response) { if provider == nil || resp == nil { return } - var target *urlutil.URL - switch { - case g.request != nil: - target = g.request.URL - case g.rawRequest != nil: - parsed, err := urlutil.ParseAbsoluteURL(g.rawRequest.FullURL, true) - if err != nil { - return - } - target = parsed - } - if target == nil { + target, err := g.targetURL() + if err != nil || target == nil { return } for _, strategy := range provider.LookupURLX(target) {🤖 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 `@pkg/protocols/http/build_request.go` around lines 146 - 174, Extract the duplicated g.request/g.rawRequest URL resolution from ApplyAuth and NotifyResponse into a shared generatedRequest helper such as targetURL() (*urlutil.URL, error). Have both methods use this helper, preserving ApplyAuth’s warning behavior and ensuring NotifyResponse reports URL parse failures consistently rather than silently returning; retain nil/unsupported-request handling.
🤖 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 `@cmd/nuclei/main.go`:
- Line 521: Update the auto-login password option in the flag initialization
around options.AuthPassword to support a non-argv source such as an environment
variable, file, or stdin, and discourage or deprecate direct -auth-password
usage. Ensure the resolved password still populates options.AuthPassword for
auto-login while avoiding exposure through shell history and process listings.
In `@internal/runner/runner.go`:
- Around line 664-700: The auto-login setup must not run when capture mode is
enabled, since AuthCapture also supplies AuthLoginURL and currently creates
overlapping providers. Update the condition around autoLoginStoreFromOptions and
NewStoreAuthProvider to exclude r.options.AuthCapture, while preserving the
separate capture-once provider flow.
In `@pkg/authprovider/authx/autologin.go`:
- Around line 59-82: The Validate method currently replaces explicitly
configured Steps whenever Recording is set. Update the recording branch in
AutoLoginConfig.Validate to detect and reject or otherwise explicitly handle a
non-empty user-supplied a.Steps before assigning the recording-derived steps,
preserving the explicit configuration instead of silently overwriting it.
In `@pkg/authprovider/authx/dynamic.go`:
- Around line 514-523: Update Dynamic.runFetchLocked so fetchState.fetched is
set only when d.fetchCallback(d) succeeds; preserve an error result as
stale/unfetched so later Fetch(false) calls can retry instead of treating failed
authentication as complete. Keep fetchedAt updates tied to successful fetches,
and ensure ApplyStrategies can continue receiving retryable failures, adding
bounded backoff only if supported by the existing fetch-state flow.
In `@pkg/authprovider/authx/file.go`:
- Around line 70-73: Preserve protocol-native session scope across the auth
model and runner: in pkg/authprovider/authx/file.go lines 70-73, replace
host-only LocalStorage and SessionStorage mappings with origin-scoped entries
containing scheme, host, and port; in internal/runner/lazy.go lines 120-146,
retain each cookie’s domain, path, secure attributes, and captured storage
origin when seeding sessions; in internal/runner/lazy.go lines 225-244, keep the
login endpoint distinct from application hosts so refreshed credentials apply
only to the intended origins.
In `@pkg/authprovider/authx/session_test.go`:
- Around line 220-223: Update the goroutine-based test around strategy.Apply to
avoid calling require.Equal inside the spawned goroutine, since FailNow is
unsafe there. Use a non-terminating assertion such as assert.Equal or t.Errorf,
or collect the comparison failure and report it after wg.Wait(); preserve the
Authorization header expectation.
In `@pkg/authprovider/authx/strategy.go`:
- Around line 22-25: Extend the ResponseInspector contract and its
implementations so OnResponse receives or can identify the strategy/session
generation that authenticated the corresponding request. Record the applied
generation on each authenticated request, and ignore expiry responses such as
401 when their generation is older than the current session; only the current
generation may mark the session stale or trigger re-authentication.
In `@pkg/authprovider/autologin/capture.go`:
- Line 24: Update CaptureOnce and its manual-capture readiness flow so the ready
callback accepts context.Context and is invoked with the current ctx. Propagate
this signature change to all callback implementations and ensure the callback
uses the context to interrupt any blocking terminal read during cancellation.
In `@pkg/authprovider/autologin/headless.go`:
- Around line 223-235: Update sameLoginOrigin to compare the complete normalized
origin: scheme, hostname, and effective port, rather than only Hostname().
Normalize default ports consistently for each scheme, reject unparsable or
incomplete response URLs, and preserve the existing fail-closed behavior for nil
loginURL or empty respURL.
- Around line 217-219: Treat either browser storage area as valid session
material: update the session validation in headless.go to accept non-empty
LocalStorage or SessionStorage alongside cookies and token, and update both
predicates in pkg/authprovider/autologin/realapp_test.go at lines 92-93 and
144-145 to include SessionStorage. Preserve the existing authentication checks
and replay behavior.
- Around line 605-627: Update capturePageCookies to deduplicate cookies using
their combined name, domain, and path rather than name alone. Preserve cookies
with the same name when either Domain or Path differs, while continuing to skip
nil cookies and exact duplicates.
In `@pkg/authprovider/autologin/login.go`:
- Around line 230-233: Update the session cookie handling around collectCookies
and renderCookieHeader to preserve the cookie jar’s domain, path, secure, and
expiry scoping instead of merging cookies by name. Retain the jar or equivalent
scoped cookie metadata in Session, and derive the applicable Cookie header
separately for each outgoing target URL so stale or unrelated login-flow cookies
are not replayed.
- Around line 248-271: Update the success decision in Login so HTTP 401/403 and
other failed response statuses cannot be accepted solely because a pre-login
cookie exists; reject them with ErrLoginFailed before the cookie/token fallback.
For ambiguous successful 2xx landing pages without a token, require an explicit
authentication success probe or condition, reusing the existing login-flow
configuration and symbols rather than treating any cookie as sufficient.
In `@pkg/authprovider/autologin/realapp_test.go`:
- Around line 92-93: Update the captured-session assertions in both locations to
treat non-empty session storage as a valid captured session, alongside cookies,
tokens, and local storage. Use the existing SessionStorage field on session and
preserve the current failure message and other predicates.
- Around line 51-76: Update logSession to stop printing cookie values, token
prefixes, and any captured recording or step values; retain only non-sensitive
names, actions, and counts. Remove value-bearing formatting while preserving the
existing session summary and storage key/count diagnostics.
In `@pkg/authprovider/autologin/recording.go`:
- Around line 65-70: Update the recording importer around the action conversion
logic in recording.go to reject unsupported replay actions instead of silently
transforming or dropping them. Do not map doubleclick to click, substitute
expression waits with fixed waits, or ignore unknown actions; either implement
faithful replay support for each action or return an error identifying the
unsupported step and action.
- Around line 71-80: Update the “change” handling and the corresponding
alternate path to pass all recorded selector groups to parameterizeValue instead
of only the result of pickSelector. Make parameterizeValue treat the value as a
password when any candidate selector identifies a password field, while
retaining pickSelector for choosing the compiled step’s Selector.
In `@pkg/protocols/headless/engine/auth_reauth_test.go`:
- Around line 80-128: Extend TestNotifyAuthResponse_E2E with a rotating auth
provider/inspector that refreshes credentials after the initial 401, then add a
second navigation to an endpoint requiring the refreshed credentials and
returning success. Assert the refresh occurred and the authenticated navigation
succeeded, while preserving the existing assertion that the initial 401 reached
the response inspector.
---
Outside diff comments:
In `@pkg/authprovider/authx/dynamic.go`:
- Around line 133-136: Update Dynamic.Validate to reset the parsed
refreshInterval alongside fetchState when revalidation begins, ensuring a
previously configured interval is cleared and cannot remain active after
configuration changes.
In `@pkg/protocols/headless/engine/page.go`:
- Around line 230-237: Update page.go lines 230-237 so each main navigation
response is inspected during action execution and can trigger reauthentication
immediately, rather than only after the full action sequence. In page.go lines
189-194, re-resolve and apply refreshed auth material before the next
navigation. Extend auth_reauth_test.go lines 80-128 with a second navigation and
assert that it sends the refreshed credentials.
In `@pkg/protocols/http/request.go`:
- Around line 1096-1102: Fix the `analyzer.Analyze` options construction in the
request analysis flow so it matches the fields defined by `analyzers.Options`:
either add and properly support `RateLimitTake` in that type, or remove/replace
the unsupported struct field with the intended existing value while preserving
the rate-limit behavior.
---
Nitpick comments:
In `@pkg/protocols/http/build_request.go`:
- Around line 146-174: Extract the duplicated g.request/g.rawRequest URL
resolution from ApplyAuth and NotifyResponse into a shared generatedRequest
helper such as targetURL() (*urlutil.URL, error). Have both methods use this
helper, preserving ApplyAuth’s warning behavior and ensuring NotifyResponse
reports URL parse failures consistently rather than silently returning; retain
nil/unsupported-request handling.
🪄 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: a0d0259d-3a92-4d84-9eea-72b0b3b3fe9f
⛔ Files ignored due to path filters (1)
pkg/authprovider/autologin/testdata/recordings/spa_login.jsonis excluded by!**/*.json
📒 Files selected for processing (44)
cmd/nuclei/main.gointernal/fuzzplayground/auth.gointernal/fuzzplayground/server.gointernal/runner/lazy.gointernal/runner/lazy_autologin_test.gointernal/runner/runner.gopkg/authprovider/authx/autologin.gopkg/authprovider/authx/autologin_test.gopkg/authprovider/authx/dynamic.gopkg/authprovider/authx/dynamic_test.gopkg/authprovider/authx/file.gopkg/authprovider/authx/session_e2e_test.gopkg/authprovider/authx/session_test.gopkg/authprovider/authx/strategy.gopkg/authprovider/authx/web_storage_auth.gopkg/authprovider/authx/web_storage_auth_test.gopkg/authprovider/autologin/capture.gopkg/authprovider/autologin/capture_test.gopkg/authprovider/autologin/detect.gopkg/authprovider/autologin/detect_test.gopkg/authprovider/autologin/headless.gopkg/authprovider/autologin/headless_test.gopkg/authprovider/autologin/login.gopkg/authprovider/autologin/login_test.gopkg/authprovider/autologin/playground_test.gopkg/authprovider/autologin/realapp_test.gopkg/authprovider/autologin/recording.gopkg/authprovider/autologin/recording_test.gopkg/authprovider/autologin_e2e_test.gopkg/authprovider/file.gopkg/authprovider/file_test.gopkg/authprovider/interface.gopkg/authprovider/multi.gopkg/protocols/headless/engine/auth.gopkg/protocols/headless/engine/auth_e2e_test.gopkg/protocols/headless/engine/auth_reauth_test.gopkg/protocols/headless/engine/auth_storage_test.gopkg/protocols/headless/engine/auth_test.gopkg/protocols/headless/engine/page.gopkg/protocols/headless/request.gopkg/protocols/http/build_request.gopkg/protocols/http/http.gopkg/protocols/http/request.gopkg/types/types.go
| flagSet.BoolVarP(&options.PreFetchSecrets, "prefetch-secrets", "ps", false, "prefetch secrets from the secrets file"), | ||
| flagSet.StringVarP(&options.AuthLoginURL, "auth-login-url", "alu", "", "login page url for turnkey auto-login authenticated scan"), | ||
| flagSet.StringVarP(&options.AuthUsername, "auth-username", "au", "", "username for auto-login (-auth-login-url)"), | ||
| flagSet.StringVarP(&options.AuthPassword, "auth-password", "ap", "", "password for auto-login (-auth-login-url)"), |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Provide a non-argv input for the auto-login password.
-auth-password exposes credentials through shell history and potentially process listings for the scan duration. Prefer an environment-, file-, or stdin-backed option and discourage direct argv usage.
🤖 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 `@cmd/nuclei/main.go` at line 521, Update the auto-login password option in the
flag initialization around options.AuthPassword to support a non-argv source
such as an environment variable, file, or stdin, and discourage or deprecate
direct -auth-password usage. Ensure the resolved password still populates
options.AuthPassword for auto-login while avoiding exposure through shell
history and process listings.
Comment on lines
+664
to
+700
| if r.options.AuthLoginURL != "" || r.options.AuthRecording != "" { | ||
| engine := "http" | ||
| if r.options.AuthHeadless || r.options.AuthRecording != "" { | ||
| engine = "headless" | ||
| } | ||
| target := r.options.AuthLoginURL | ||
| if target == "" { | ||
| target = r.options.AuthRecording | ||
| } | ||
| r.Logger.Info().Msgf("Auto-login enabled (%s engine) for %s", engine, target) | ||
| store, err := autoLoginStoreFromOptions(r.options) | ||
| if err != nil { | ||
| return errors.Wrap(err, "could not build auto-login auth provider") | ||
| } | ||
| cliProvider, err := authprovider.NewStoreAuthProvider(store, nil, autoLoginOpts) | ||
| if err != nil { | ||
| return errors.Wrap(err, "could not create auto-login auth provider") | ||
| } | ||
| providers = append(providers, cliProvider) | ||
| } | ||
|
|
||
| // Capture-once: open a visible browser for a one-time manual login and | ||
| // build a static session store from the captured cookies/token. | ||
| if r.options.AuthCapture { | ||
| if r.options.AuthLoginURL == "" { | ||
| return errors.New("-auth-capture requires -auth-login-url (the page to open for manual login)") | ||
| } | ||
| r.Logger.Info().Msgf("Capture-once login enabled for %s", r.options.AuthLoginURL) | ||
| store, err := captureOnceStoreFromOptions(r.options) | ||
| if err != nil { | ||
| return errors.Wrap(err, "could not capture login session") | ||
| } | ||
| captureProvider, err := authprovider.NewStoreAuthProvider(store, nil, autoLoginOpts) | ||
| if err != nil { | ||
| return errors.Wrap(err, "could not create capture-once auth provider") | ||
| } | ||
| providers = append(providers, captureProvider) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not create a turnkey provider in capture mode.
Because -auth-capture requires -auth-login-url, both branches execute and create overlapping providers. This can perform an unintended automated login and apply conflicting captured sessions. Reject incompatible modes during validation or exclude capture mode here.
Proposed fix
- if r.options.AuthLoginURL != "" || r.options.AuthRecording != "" {
+ if !r.options.AuthCapture && (r.options.AuthLoginURL != "" || r.options.AuthRecording != "") {📝 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 r.options.AuthLoginURL != "" || r.options.AuthRecording != "" { | |
| engine := "http" | |
| if r.options.AuthHeadless || r.options.AuthRecording != "" { | |
| engine = "headless" | |
| } | |
| target := r.options.AuthLoginURL | |
| if target == "" { | |
| target = r.options.AuthRecording | |
| } | |
| r.Logger.Info().Msgf("Auto-login enabled (%s engine) for %s", engine, target) | |
| store, err := autoLoginStoreFromOptions(r.options) | |
| if err != nil { | |
| return errors.Wrap(err, "could not build auto-login auth provider") | |
| } | |
| cliProvider, err := authprovider.NewStoreAuthProvider(store, nil, autoLoginOpts) | |
| if err != nil { | |
| return errors.Wrap(err, "could not create auto-login auth provider") | |
| } | |
| providers = append(providers, cliProvider) | |
| } | |
| // Capture-once: open a visible browser for a one-time manual login and | |
| // build a static session store from the captured cookies/token. | |
| if r.options.AuthCapture { | |
| if r.options.AuthLoginURL == "" { | |
| return errors.New("-auth-capture requires -auth-login-url (the page to open for manual login)") | |
| } | |
| r.Logger.Info().Msgf("Capture-once login enabled for %s", r.options.AuthLoginURL) | |
| store, err := captureOnceStoreFromOptions(r.options) | |
| if err != nil { | |
| return errors.Wrap(err, "could not capture login session") | |
| } | |
| captureProvider, err := authprovider.NewStoreAuthProvider(store, nil, autoLoginOpts) | |
| if err != nil { | |
| return errors.Wrap(err, "could not create capture-once auth provider") | |
| } | |
| providers = append(providers, captureProvider) | |
| if !r.options.AuthCapture && (r.options.AuthLoginURL != "" || r.options.AuthRecording != "") { | |
| engine := "http" | |
| if r.options.AuthHeadless || r.options.AuthRecording != "" { | |
| engine = "headless" | |
| } | |
| target := r.options.AuthLoginURL | |
| if target == "" { | |
| target = r.options.AuthRecording | |
| } | |
| r.Logger.Info().Msgf("Auto-login enabled (%s engine) for %s", engine, target) | |
| store, err := autoLoginStoreFromOptions(r.options) | |
| if err != nil { | |
| return errors.Wrap(err, "could not build auto-login auth provider") | |
| } | |
| cliProvider, err := authprovider.NewStoreAuthProvider(store, nil, autoLoginOpts) | |
| if err != nil { | |
| return errors.Wrap(err, "could not create auto-login auth provider") | |
| } | |
| providers = append(providers, cliProvider) | |
| } | |
| // Capture-once: open a visible browser for a one-time manual login and | |
| // build a static session store from the captured cookies/token. | |
| if r.options.AuthCapture { | |
| if r.options.AuthLoginURL == "" { | |
| return errors.New("-auth-capture requires -auth-login-url (the page to open for manual login)") | |
| } | |
| r.Logger.Info().Msgf("Capture-once login enabled for %s", r.options.AuthLoginURL) | |
| store, err := captureOnceStoreFromOptions(r.options) | |
| if err != nil { | |
| return errors.Wrap(err, "could not capture login session") | |
| } | |
| captureProvider, err := authprovider.NewStoreAuthProvider(store, nil, autoLoginOpts) | |
| if err != nil { | |
| return errors.Wrap(err, "could not create capture-once auth provider") | |
| } | |
| providers = append(providers, captureProvider) |
🤖 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/runner/runner.go` around lines 664 - 700, The auto-login setup must
not run when capture mode is enabled, since AuthCapture also supplies
AuthLoginURL and currently creates overlapping providers. Update the condition
around autoLoginStoreFromOptions and NewStoreAuthProvider to exclude
r.options.AuthCapture, while preserving the separate capture-once provider flow.
Comment on lines
+59
to
+82
| // Validate validates the auto-login configuration. | ||
| func (a *AutoLoginConfig) Validate() error { | ||
| // A recording compiles into Steps and drives a headless login. Load it first | ||
| // so a missing/invalid recording fails fast and so it can supply the login | ||
| // URL when one isn't given explicitly. | ||
| if a.Recording != "" { | ||
| steps, err := autologin.StepsFromRecordingFile(a.Recording, a.Username, a.Password) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| a.Steps = steps | ||
| a.Headless = true | ||
| if a.LoginURL == "" { | ||
| a.LoginURL = autologin.FirstNavigateURL(steps) | ||
| } | ||
| } | ||
| if a.LoginURL == "" { | ||
| return errkit.New("login-url is required for auto-login dynamic secret") | ||
| } | ||
| if a.Password == "" { | ||
| return errkit.New("password is required for auto-login dynamic secret") | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Recording silently overwrites a user-supplied Steps.
If a user sets both Recording and Steps, Validate() unconditionally replaces a.Steps with the recording-derived steps (Line 69) with no warning, discarding the explicit config.
♻️ Proposed fix
if a.Recording != "" {
+ if len(a.Steps) > 0 {
+ return errkit.New("recording and steps are mutually exclusive for auto-login dynamic secret")
+ }
steps, err := autologin.StepsFromRecordingFile(a.Recording, a.Username, a.Password)📝 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
| // Validate validates the auto-login configuration. | |
| func (a *AutoLoginConfig) Validate() error { | |
| // A recording compiles into Steps and drives a headless login. Load it first | |
| // so a missing/invalid recording fails fast and so it can supply the login | |
| // URL when one isn't given explicitly. | |
| if a.Recording != "" { | |
| steps, err := autologin.StepsFromRecordingFile(a.Recording, a.Username, a.Password) | |
| if err != nil { | |
| return err | |
| } | |
| a.Steps = steps | |
| a.Headless = true | |
| if a.LoginURL == "" { | |
| a.LoginURL = autologin.FirstNavigateURL(steps) | |
| } | |
| } | |
| if a.LoginURL == "" { | |
| return errkit.New("login-url is required for auto-login dynamic secret") | |
| } | |
| if a.Password == "" { | |
| return errkit.New("password is required for auto-login dynamic secret") | |
| } | |
| return nil | |
| } | |
| // Validate validates the auto-login configuration. | |
| func (a *AutoLoginConfig) Validate() error { | |
| // A recording compiles into Steps and drives a headless login. Load it first | |
| // so a missing/invalid recording fails fast and so it can supply the login | |
| // URL when one isn't given explicitly. | |
| if a.Recording != "" { | |
| if len(a.Steps) > 0 { | |
| return errkit.New("recording and steps are mutually exclusive for auto-login dynamic secret") | |
| } | |
| steps, err := autologin.StepsFromRecordingFile(a.Recording, a.Username, a.Password) | |
| if err != nil { | |
| return err | |
| } | |
| a.Steps = steps | |
| a.Headless = true | |
| if a.LoginURL == "" { | |
| a.LoginURL = autologin.FirstNavigateURL(steps) | |
| } | |
| } | |
| if a.LoginURL == "" { | |
| return errkit.New("login-url is required for auto-login dynamic secret") | |
| } | |
| if a.Password == "" { | |
| return errkit.New("password is required for auto-login dynamic secret") | |
| } | |
| return nil | |
| } |
🤖 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 `@pkg/authprovider/authx/autologin.go` around lines 59 - 82, The Validate
method currently replaces explicitly configured Steps whenever Recording is set.
Update the recording branch in AutoLoginConfig.Validate to detect and reject or
otherwise explicitly handle a non-empty user-supplied a.Steps before assigning
the recording-derived steps, preserving the explicit configuration instead of
silently overwriting it.
Comment on lines
+514
to
+523
| func (d *Dynamic) runFetchLocked() { | ||
| if d.fetchCallback == nil { | ||
| d.fetchState.err = errkit.New("dynamic secret fetch callback not set: call SetLazyFetchCallback() before Fetch()") | ||
| return | ||
| } | ||
| d.fetchState.err = d.fetchCallback(d) | ||
| d.fetchState.fetched = true | ||
| d.fetchState.stale = false | ||
| d.fetchState.fetchedAt = time.Now() | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not mark failed authentication attempts as successfully fetched.
When the callback fails, fetched=true and stale=false prevent subsequent Fetch(false) calls from retrying. ApplyStrategies then silently omits authentication indefinitely. Track successful fetches separately and retain a retryable stale/unfetched state, preferably with bounded backoff.
🤖 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 `@pkg/authprovider/authx/dynamic.go` around lines 514 - 523, Update
Dynamic.runFetchLocked so fetchState.fetched is set only when d.fetchCallback(d)
succeeds; preserve an error result as stale/unfetched so later Fetch(false)
calls can retry instead of treating failed authentication as complete. Keep
fetchedAt updates tied to successful fetches, and ensure ApplyStrategies can
continue receiving retryable failures, adding bounded backoff only if supported
by the existing fetch-state flow.
Comment on lines
+70
to
+73
| // LocalStorage / SessionStorage carry browser web storage for WebStorageAuth | ||
| // secrets, seeded into headless scan pages before page scripts run. | ||
| LocalStorage map[string]string `json:"local-storage" yaml:"local-storage"` | ||
| SessionStorage map[string]string `json:"session-storage" yaml:"session-storage"` |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Preserve protocol-native session scope instead of collapsing it to the login host.
The current model loses browser-origin and cookie scope, causing both credential overexposure and broken SSO flows.
pkg/authprovider/authx/file.go#L70-L73: represent and enforce web storage using scheme, host, and port.internal/runner/lazy.go#L120-L146: preserve cookie domain/path/secure attributes and the captured storage origin.internal/runner/lazy.go#L225-L244: separate the login endpoint from the application hosts where refreshed credentials apply.
📍 Affects 2 files
pkg/authprovider/authx/file.go#L70-L73(this comment)internal/runner/lazy.go#L120-L146internal/runner/lazy.go#L225-L244
🤖 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 `@pkg/authprovider/authx/file.go` around lines 70 - 73, Preserve
protocol-native session scope across the auth model and runner: in
pkg/authprovider/authx/file.go lines 70-73, replace host-only LocalStorage and
SessionStorage mappings with origin-scoped entries containing scheme, host, and
port; in internal/runner/lazy.go lines 120-146, retain each cookie’s domain,
path, secure attributes, and captured storage origin when seeding sessions; in
internal/runner/lazy.go lines 225-244, keep the login endpoint distinct from
application hosts so refreshed credentials apply only to the intended origins.
Comment on lines
+51
to
+76
| func logSession(t *testing.T, s *Session) { | ||
| t.Helper() | ||
| t.Logf("final url: %s", s.FinalURL) | ||
| t.Logf("cookies (%d):", len(s.Cookies)) | ||
| for _, c := range s.Cookies { | ||
| v := c.Value | ||
| if len(v) > 24 { | ||
| v = v[:24] + "...(truncated)" | ||
| } | ||
| t.Logf(" %s = %s", c.Name, v) | ||
| } | ||
| if s.Token != "" { | ||
| tok := s.Token | ||
| if len(tok) > 32 { | ||
| tok = tok[:32] + "...(truncated)" | ||
| } | ||
| t.Logf("token: %s", tok) | ||
| } | ||
| t.Logf("localStorage keys: %d, sessionStorage keys: %d", len(s.LocalStorage), len(s.SessionStorage)) | ||
| for k := range s.LocalStorage { | ||
| t.Logf(" local[%s]", k) | ||
| } | ||
| for k := range s.SessionStorage { | ||
| t.Logf(" session[%s]", k) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not print captured credentials or recording values.
Cookie/token prefixes remain sensitive, and compiled step values may contain credentials. Log names, actions, and counts only.
Proposed redaction
for _, c := range s.Cookies {
- v := c.Value
- if len(v) > 24 {
- v = v[:24] + "...(truncated)"
- }
- t.Logf(" %s = %s", c.Name, v)
+ t.Logf(" cookie captured: %s", c.Name)
}
if s.Token != "" {
- tok := s.Token
- if len(tok) > 32 {
- tok = tok[:32] + "...(truncated)"
- }
- t.Logf("token: %s", tok)
+ t.Log("token captured")
}
...
- t.Logf(" %s selector=%q value=%q", s.Action, s.Selector, s.Value)
+ t.Logf(" %s selector=%q", s.Action, s.Selector)Also applies to: 127-132
🤖 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 `@pkg/authprovider/autologin/realapp_test.go` around lines 51 - 76, Update
logSession to stop printing cookie values, token prefixes, and any captured
recording or step values; retain only non-sensitive names, actions, and counts.
Remove value-bearing formatting while preserving the existing session summary
and storage key/count diagnostics.
Comment on lines
+92
to
+93
| require.True(t, len(session.Cookies) > 0 || session.Token != "" || len(session.LocalStorage) > 0, | ||
| "expected to capture a session (cookie, token or web storage)") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include SessionStorage in the captured-session predicates.
A session authenticated exclusively through session storage currently fails both real-application harnesses.
- require.True(t, len(session.Cookies) > 0 || session.Token != "" || len(session.LocalStorage) > 0,
+ require.True(t, len(session.Cookies) > 0 || session.Token != "" || len(session.LocalStorage) > 0 || len(session.SessionStorage) > 0,Also applies to: 144-145
🤖 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 `@pkg/authprovider/autologin/realapp_test.go` around lines 92 - 93, Update the
captured-session assertions in both locations to treat non-empty session storage
as a valid captured session, alongside cookies, tokens, and local storage. Use
the existing SessionStorage field on session and preserve the current failure
message and other predicates.
Comment on lines
+65
to
+70
| case "click", "doubleclick": | ||
| sel := pickSelector(rs.Selectors) | ||
| if sel == "" { | ||
| continue | ||
| } | ||
| steps = append(steps, LoginStep{Action: "click", Selector: sel}) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unsupported replay actions instead of silently changing the flow.
A recorded doubleclick becomes one click, expression waits become an unrelated fixed wait, and unknown actions disappear. The importer can therefore report success for a semantically incomplete recording. Support these actions or return an error identifying the unsupported step.
Also applies to: 93-102
🤖 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 `@pkg/authprovider/autologin/recording.go` around lines 65 - 70, Update the
recording importer around the action conversion logic in recording.go to reject
unsupported replay actions instead of silently transforming or dropping them. Do
not map doubleclick to click, substitute expression waits with fixed waits, or
ignore unknown actions; either implement faithful replay support for each action
or return an error identifying the unsupported step and action.
Comment on lines
+71
to
+80
| case "change": | ||
| sel := pickSelector(rs.Selectors) | ||
| if sel == "" { | ||
| continue | ||
| } | ||
| steps = append(steps, LoginStep{ | ||
| Action: "fill", | ||
| Selector: sel, | ||
| Value: parameterizeValue(rs.Value, sel, username, password), | ||
| }) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Inspect every recorded selector before retaining a field value.
Password masking checks only pickSelector(...). With alternatives such as #input-2 and aria/Password, CSS wins and an unmatched recorded password remains in the compiled steps. Pass all selector groups into parameterizeValue and mask when any candidate identifies a password field.
Proposed change
- Value: parameterizeValue(rs.Value, sel, username, password),
+ Value: parameterizeValue(rs.Value, rs.Selectors, username, password),
...
-func parameterizeValue(value, selector, username, password string) string {
+func parameterizeValue(value string, selectors [][]string, username, password string) string {
if password != "" && value == password {
return "{{password}}"
}
if username != "" && value == username {
return "{{username}}"
}
- if looksLikePasswordSelector(selector) {
- return "{{password}}"
+ for _, group := range selectors {
+ for _, selector := range group {
+ if looksLikePasswordSelector(selector) {
+ return "{{password}}"
+ }
+ }
}
return value
}Also applies to: 141-160
🤖 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 `@pkg/authprovider/autologin/recording.go` around lines 71 - 80, Update the
“change” handling and the corresponding alternate path to pass all recorded
selector groups to parameterizeValue instead of only the result of pickSelector.
Make parameterizeValue treat the value as a password when any candidate selector
identifies a password field, while retaining pickSelector for choosing the
compiled step’s Selector.
Comment on lines
+80
to
+128
| // TestNotifyAuthResponse_E2E proves the full chain: a real navigation that | ||
| // returns 401 is forwarded to the auth provider's response inspector, so | ||
| // reauth-status-codes works for headless scans (not just HTTP). | ||
| func TestNotifyAuthResponse_E2E(t *testing.T) { | ||
| if _, ok := launcher.LookPath(); !ok { | ||
| t.Skip("no system chrome/chromium found; skipping headless reauth e2e") | ||
| } | ||
|
|
||
| opts := &types.Options{AllowLocalFileAccess: true} | ||
| require.NoError(t, protocolstate.Init(opts)) | ||
|
|
||
| browser, err := New(&types.Options{ShowBrowser: false, UseInstalledChrome: true}) | ||
| require.NoError(t, err) | ||
| defer browser.Close() | ||
|
|
||
| instance, err := browser.NewInstance() | ||
| require.NoError(t, err) | ||
| defer func() { _ = instance.Close() }() | ||
|
|
||
| ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusUnauthorized) | ||
| _, _ = fmt.Fprintln(w, "<html><body>unauthorized</body></html>") | ||
| })) | ||
| defer ts.Close() | ||
|
|
||
| insp := &recordingInspector{reauthOn: http.StatusUnauthorized} | ||
| input := contextargs.NewWithInput(context.Background(), ts.URL) | ||
| input.CookieJar, err = cookiejar.New(nil) | ||
| require.NoError(t, err) | ||
|
|
||
| actions := []*Action{ | ||
| {ActionType: ActionTypeHolder{ActionType: ActionNavigate}, Data: map[string]string{"url": "{{BaseURL}}"}}, | ||
| {ActionType: ActionTypeHolder{ActionType: ActionWaitLoad}}, | ||
| } | ||
|
|
||
| _, p, err := instance.Run(input, actions, nil, &Options{ | ||
| Timeout: 30 * time.Second, | ||
| Options: opts, | ||
| AuthProvider: &inspectorProvider{insp: insp}, | ||
| }) | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| if p != nil { | ||
| p.Close() | ||
| } | ||
| }() | ||
|
|
||
| require.Contains(t, insp.seen(), http.StatusUnauthorized, "401 navigation must be forwarded to the auth response inspector") | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Exercise an actual refresh followed by another authenticated navigation.
This test only proves eventual status forwarding. Add a rotating provider and a second navigation that succeeds only after refreshed credentials are applied; otherwise the current lifecycle regression still passes.
🤖 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 `@pkg/protocols/headless/engine/auth_reauth_test.go` around lines 80 - 128,
Extend TestNotifyAuthResponse_E2E with a rotating auth provider/inspector that
refreshes credentials after the initial 401, then add a second navigation to an
endpoint requiring the refreshed credentials and returning success. Assert the
refresh occurred and the authenticated navigation succeeded, while preserving
the existing assertion that the initial 401 reached the response inspector.
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.
Turnkey login capture and session refresh for authenticated scans. Related to #4793.
Summary by CodeRabbit
New Features
Bug Fixes