fix(authx): race condition in Dynamic.Fetch with -secret-file#7025
Open
Bushi-gg wants to merge 4 commits intoprojectdiscovery:devfrom
Open
fix(authx): race condition in Dynamic.Fetch with -secret-file#7025Bushi-gg wants to merge 4 commits intoprojectdiscovery:devfrom
Bushi-gg wants to merge 4 commits intoprojectdiscovery:devfrom
Conversation
…namic Templates could fire before dynamic secret auth completed in two ways: 1. runner.go only prefetched when --prefetch-secrets was set (default: false) 2. Dynamic.Fetch() used CAS atomics that let concurrent callers slip through with a nil error before fetch completed Replace the two atomic.Bool fields with a *fetchState struct (sync.Once + error) so all concurrent callers block until fetch completes. The pointer ensures state is shared across value copies of Dynamic (DynamicAuthStrategy embeds by value). Also fix GetStrategies() to use if err := d.Fetch(true) so the nil-fetchState case is handled without bypassing the error guard. Fixes projectdiscovery#6592
1 task
Neo Security AuditNo security issues found Highlights
Hardening Notes
Comment |
Contributor
WalkthroughRunEnumeration now always prefetches secrets when an AuthProvider is present. The Dynamic auth provider was refactored to use a shared fetchState with sync.Once, exposes ErrNotValidated and new accessors, and adds concurrency tests validating single-run Fetch behavior and shared state across value copies. Changes
Sequence DiagramsequenceDiagram
participant Runner
participant AuthProvider
participant FetchState
participant Once as sync.Once
rect rgba(100,150,200,0.5)
Note over Runner,AuthProvider: Old (conditional prefetch)
Runner->>AuthProvider: RunEnumeration()
alt AuthProvider exists && option enabled
AuthProvider->>AuthProvider: PreFetchSecrets() (conditional)
AuthProvider->>Runner: return (maybe prefetched)
else
AuthProvider->>Runner: return (no prefetch)
end
end
rect rgba(150,200,100,0.5)
Note over Runner,FetchState: New (mandatory prefetch with shared state)
Runner->>AuthProvider: RunEnumeration()
AuthProvider->>FetchState: Validate() / ensure fetchState
AuthProvider->>FetchState: PreFetchSecrets() (unconditional)
FetchState->>Once: Do(fetchCallback)
Once->>FetchState: store result / err
FetchState->>AuthProvider: return result / err
AuthProvider->>Runner: return (secrets ready or error)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/authprovider/authx/dynamic_test.go (1)
160-189: Consider adding a test where the callback returns a non-nil error.This test verifies all waiters receive
nilon success. It would strengthen the suite to also verify that when the callback returns a specific error, all concurrent waiters observe that same error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/authprovider/authx/dynamic_test.go` around lines 160 - 189, Add a companion test that mirrors "all-waiters-get-same-error" but has d.fetchCallback return a specific non-nil error; create many concurrent goroutines that call d.Fetch(false) and assert every returned error equals that same sentinel error. Use the same setup symbols (Dynamic, TemplatePath, Variables, Validate, fetchCallback, Fetch) and ensure you close the ready channel to release waiters, wait for the WaitGroup, and compare each errs[i] to the expected error.pkg/authprovider/authx/dynamic.go (2)
210-230:gologger.Fatal()terminates the process — risky for library consumers.
Fetch()callsgologger.Fatal().Msgf(...)on Lines 213 and 227, which callsos.Exit(1). In a library context (e.g., SDK usage vialib/sdk_private.go), this kills the host process instead of allowing graceful error handling. The error is already returned, so callers can handle it.This pattern appears to predate this PR, but this is a good opportunity to reconsider — especially since the PR explicitly touches these code paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/authprovider/authx/dynamic.go` around lines 210 - 230, The Fetch method in type Dynamic calls gologger.Fatal() (which exits the process) on error paths; remove those fatal exits in Dynamic.Fetch and instead log the error (e.g., gologger.Error().Msgf(...)) and return the error so callers can handle it. Specifically, update the checks around d.fetchState == nil and after d.fetchState.err to avoid calling gologger.Fatal(), ensure d.fetchState.err is returned unchanged, and preserve existing behavior of invoking d.fetchCallback via d.fetchState.once.Do and setting d.fetchState.err when callback is nil.
233-238:Error()returnsnilfor an unvalidatedDynamic— consider returningErrNotValidatedfor consistency.
Fetch()returnsErrNotValidatedwhenfetchStateis nil, butError()returnsnilin the same scenario. This asymmetry could silently mask a "not validated" state when callers useError()instead ofFetch().Proposed fix
func (d *Dynamic) Error() error { if d.fetchState == nil { - return nil + return ErrNotValidated } d.fetchState.once.Do(func() {}) // ensure happens-before for err read return d.fetchState.err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/authprovider/authx/dynamic.go` around lines 233 - 238, Dynamic.Error() currently returns nil when d.fetchState is nil, while Fetch() returns ErrNotValidated, causing inconsistent behavior; update Dynamic.Error() to mirror Fetch() by returning ErrNotValidated when d.fetchState == nil, otherwise keep the existing once.Do(...) barrier and return d.fetchState.err. Reference symbols: Dynamic.Error(), Dynamic.Fetch(), fetchState, fetchState.once.Do, and ErrNotValidated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/authprovider/authx/dynamic.go`:
- Around line 80-82: Validate() currently unconditionally resets d.fetchState
which can reinitialize sync.Once and allow Fetch() callbacks to run again; add a
guard at the start of Dynamic.Validate() such as returning early if d.fetchState
!= nil (i.e. treat Validate() as idempotent) so re-initialization is prevented,
or alternatively add a comment on Dynamic.Validate() stating it must only be
called once; update the method that creates fetchState (Validate) and mention
fetchState, Validate(), Fetch(), and sync.Once in the comment if you choose the
documentation route.
---
Nitpick comments:
In `@pkg/authprovider/authx/dynamic_test.go`:
- Around line 160-189: Add a companion test that mirrors
"all-waiters-get-same-error" but has d.fetchCallback return a specific non-nil
error; create many concurrent goroutines that call d.Fetch(false) and assert
every returned error equals that same sentinel error. Use the same setup symbols
(Dynamic, TemplatePath, Variables, Validate, fetchCallback, Fetch) and ensure
you close the ready channel to release waiters, wait for the WaitGroup, and
compare each errs[i] to the expected error.
In `@pkg/authprovider/authx/dynamic.go`:
- Around line 210-230: The Fetch method in type Dynamic calls gologger.Fatal()
(which exits the process) on error paths; remove those fatal exits in
Dynamic.Fetch and instead log the error (e.g., gologger.Error().Msgf(...)) and
return the error so callers can handle it. Specifically, update the checks
around d.fetchState == nil and after d.fetchState.err to avoid calling
gologger.Fatal(), ensure d.fetchState.err is returned unchanged, and preserve
existing behavior of invoking d.fetchCallback via d.fetchState.once.Do and
setting d.fetchState.err when callback is nil.
- Around line 233-238: Dynamic.Error() currently returns nil when d.fetchState
is nil, while Fetch() returns ErrNotValidated, causing inconsistent behavior;
update Dynamic.Error() to mirror Fetch() by returning ErrNotValidated when
d.fetchState == nil, otherwise keep the existing once.Do(...) barrier and return
d.fetchState.err. Reference symbols: Dynamic.Error(), Dynamic.Fetch(),
fetchState, fetchState.once.Do, and ErrNotValidated.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/runner/runner.golib/sdk_private.gopkg/authprovider/authx/dynamic.gopkg/authprovider/authx/dynamic_test.go
…l with Error, add godoc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/authprovider/authx/dynamic_test.go (2)
131-156: Make waiter-blocking assertions deterministic.
readyis closed immediately after goroutine launch, so the callback may never actually block before release. This weakens the race-regression signal.Suggested test hardening
t.Run("fetch-callback-runs-once", func(t *testing.T) { var callCount atomic.Int32 ready := make(chan struct{}) + entered := make(chan struct{}, 1) d := &Dynamic{ TemplatePath: "test.yaml", Variables: []KV{{Key: "k", Value: "v"}}, } require.NoError(t, d.Validate()) d.fetchCallback = func(_ *Dynamic) error { + select { + case entered <- struct{}{}: + default: + } <-ready callCount.Add(1) return nil } @@ for i := 0; i < n; i++ { go func() { defer wg.Done() _ = d.Fetch(false) }() } + <-entered close(ready) wg.Wait()Apply the same
enteredbarrier pattern in the twoall-waiters-*subtests.Also applies to: 161-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/authprovider/authx/dynamic_test.go` around lines 131 - 156, The test "fetch-callback-runs-once" races because ready is closed immediately after launching goroutines; change to the "entered" barrier pattern used in the other subtests: create an entered channel (buffered to n), have each goroutine send a signal to entered just before calling d.Fetch(false), then have the main test wait to receive n signals from entered before closing ready; update the same pattern in the other subtest covering lines 161-217 so all waiters are guaranteed to be blocked inside d.fetchCallback before releasing ready. Ensure you keep references to the same vars (ready, entered, callCount, d.fetchCallback, wg, n) so the logic remains clear.
230-233: Add coverage forError()afterValidate()and before firstFetch().This file currently checks
Error()only on unvalidated state. Add a regression case forValidate() -> Error() -> Fetch()to protect once-initialization behavior.Suggested regression test
+ t.Run("error-after-validate-does-not-consume-fetch", func(t *testing.T) { + var callCount atomic.Int32 + d := &Dynamic{ + TemplatePath: "test.yaml", + Variables: []KV{{Key: "k", Value: "v"}}, + } + require.NoError(t, d.Validate()) + d.fetchCallback = func(_ *Dynamic) error { + callCount.Add(1) + return nil + } + + require.NoError(t, d.Error()) + require.NoError(t, d.Fetch(false)) + require.Equal(t, int32(1), callCount.Load()) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/authprovider/authx/dynamic_test.go` around lines 230 - 233, Add a regression test that constructs a Dynamic, calls Validate(ctx) on it, then asserts the subsequent call to d.Error() returns the post-validation state (i.e. no longer ErrNotValidated) before any Fetch() is called; implement a t.Run named like "validate-then-error-before-fetch" which creates d := &Dynamic{}, calls require.NoError(t, d.Validate(context.Background())), then checks require.NotErrorIs(t, d.Error(), ErrNotValidated) (or require.NoError(t, d.Error()) if the expected post-Validate state is nil) to protect Validate() -> Error() -> Fetch() initialization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/authprovider/authx/dynamic.go`:
- Around line 236-242: Remove the sync.Once side-effect from Dynamic.Error:
delete the call to d.fetchState.once.Do(func(){}) so Error no longer consumes
the shared once used by Fetch; instead guard reads of d.fetchState.err (e.g., by
using the existing fetchState mutex or an atomic/started flag) to ensure safe
concurrent access and the proper happens-before ordering without preventing
Dynamic.Fetch's callback from running; update Dynamic.Error to check
d.fetchState != nil and then read d.fetchState.err under that synchronization
(references: Dynamic.Error, Dynamic.Fetch, fetchState.once, fetchState.err).
---
Nitpick comments:
In `@pkg/authprovider/authx/dynamic_test.go`:
- Around line 131-156: The test "fetch-callback-runs-once" races because ready
is closed immediately after launching goroutines; change to the "entered"
barrier pattern used in the other subtests: create an entered channel (buffered
to n), have each goroutine send a signal to entered just before calling
d.Fetch(false), then have the main test wait to receive n signals from entered
before closing ready; update the same pattern in the other subtest covering
lines 161-217 so all waiters are guaranteed to be blocked inside d.fetchCallback
before releasing ready. Ensure you keep references to the same vars (ready,
entered, callCount, d.fetchCallback, wg, n) so the logic remains clear.
- Around line 230-233: Add a regression test that constructs a Dynamic, calls
Validate(ctx) on it, then asserts the subsequent call to d.Error() returns the
post-validation state (i.e. no longer ErrNotValidated) before any Fetch() is
called; implement a t.Run named like "validate-then-error-before-fetch" which
creates d := &Dynamic{}, calls require.NoError(t,
d.Validate(context.Background())), then checks require.NotErrorIs(t, d.Error(),
ErrNotValidated) (or require.NoError(t, d.Error()) if the expected post-Validate
state is nil) to protect Validate() -> Error() -> Fetch() initialization
behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/authprovider/authx/dynamic_test.go`:
- Around line 164-196: The test lacks an assertion that non-winning callers
remain blocked while the fetchCallback is still executing; add a concurrency
counter to detect premature completions and assert it is zero while the callback
is still blocked. Specifically, in the "all-waiters-get-same-error" subtest
(symbols: Dynamic, fetchCallback, Fetch, errs, entered, ready), introduce an
atomic counter or a completions channel incremented inside each goroutine when
it returns from d.Fetch(false); after <-entered assert the counter is still zero
(meaning no goroutine finished while the callback is blocked), then close(ready)
to let the callback complete, wait for wg, and finally assert all errs are nil.
This ensures waiters do not return early before the callback finishes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/authprovider/authx/dynamic.go (1)
245-253:Error()afterValidate()but beforeFetch()returnsnil, which is ambiguous.After
Validate()initializesfetchState, but beforeFetch()runs,Error()returnsnil— indistinguishable from a successful fetch. If callers ever need to differentiate "not yet fetched" from "fetched successfully," this would be a gap. The current usage flow (Validate → Fetch → check) avoids this, so it's not a bug today, but a brief doc comment noting the precondition would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/authprovider/authx/dynamic.go` around lines 245 - 253, The Error() method can return nil after Validate() but before Fetch(), making "not yet fetched" indistinguishable from a successful fetch; update the doc comment for func (d *Dynamic) Error() to document this precondition: state must be fetched (i.e., Validate() followed by Fetch()) before interpreting a nil error, and mention that callers who need to detect "not yet fetched" should check fetchState presence or use an explicit sentinel; reference the methods Error(), Validate(), Fetch() and the fetchState field to make the intended usage clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/authprovider/authx/dynamic.go`:
- Around line 245-253: The Error() method can return nil after Validate() but
before Fetch(), making "not yet fetched" indistinguishable from a successful
fetch; update the doc comment for func (d *Dynamic) Error() to document this
precondition: state must be fetched (i.e., Validate() followed by Fetch())
before interpreting a nil error, and mention that callers who need to detect
"not yet fetched" should check fetchState presence or use an explicit sentinel;
reference the methods Error(), Validate(), Fetch() and the fetchState field to
make the intended usage clear.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/authprovider/authx/dynamic.gopkg/authprovider/authx/dynamic_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/authprovider/authx/dynamic_test.go
Author
|
@neo review |
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.
Fixes #6592
/claim #6592
When using
-secret-file, templates can fire unauthenticated due to two issues:runner.goandsdk_private.gogatePreFetchSecrets()behind a flag that defaults to false, so secrets aren't fetched before scanning startsDynamic.Fetch()usesatomic.BoolCAS which lets concurrent callers return immediately with a nil error while the first caller is still fetching — they proceed without authThis replaces the two
atomic.Boolfields with a*fetchStatestruct containingsync.Once+error. All concurrent callers now block insideonce.Dountil the fetch callback completes. The pointer ensures the state is shared whenDynamicis copied by value intoDynamicAuthStrategy.Also fixes
GetStrategies()to checkFetch()return value directly instead of inspectingfetchStatefields, which could bypass the error guard whenfetchStateis nil.Changes:
dynamic.go: replace atomic CAS withsync.Oncevia shared*fetchStaterunner.go,sdk_private.go: always prefetch when AuthProvider is setdynamic_test.go: add concurrent fetch tests (callback-runs-once, waiters-block, value-copy sharing)Summary by CodeRabbit
Bug Fixes
New Features
Tests