feat: add secret file authentication support (-sf flag) #2371
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds file-based authentication support: new authx strategies (Basic, Bearer, Headers, Cookies, Query), an auth file model and loader (YAML/JSON), file-backed and multi auth providers, CLI flag (--secret-file / -sf), and runner integration applying strategies to outgoing requests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runner
participant MultiAuthProv as AuthProvider
participant FileProv as FileAuthProvider
participant Strategy
participant HTTPReq as retryablehttp.Request
CLI->>Runner: Start with --secret-file
Runner->>MultiAuthProv: NewAuthProvider(path)
MultiAuthProv->>FileProv: NewFileAuthProvider(path)
FileProv->>FileProv: Load & parse YAML/JSON, validate secrets
FileProv-->>MultiAuthProv: provider ready
MultiAuthProv-->>Runner: AuthProvider returned
Runner->>Runner: Prepare request for target URL
Runner->>MultiAuthProv: LookupURLX(targetURL)
MultiAuthProv->>FileProv: LookupAddr(host)
FileProv-->>MultiAuthProv: []AuthStrategy
MultiAuthProv-->>Runner: strategies
loop for each strategy
Runner->>Strategy: ApplyOnRR(HTTPReq)
Strategy->>HTTPReq: set headers / cookies / query / auth
end
HTTPReq-->>Runner: authenticated request ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @common/authprovider/authx/cookies_auth.go:
- Around line 36-59: The loop in CookiesAuthStrategy.ApplyOnRR mutates
existingCookies with slices.Delete while iterating, which can skip or corrupt
items; instead build a new filtered slice (or use a map of names from
s.Data.Cookies) by iterating existingCookies once and appending only those whose
Name is not in s.Data.Cookies, then replace existingCookies with that filtered
slice before clearing headers and calling req.AddCookie; this avoids in-place
deletion and fixes the index-stability bug (refer to existingCookies,
s.Data.Cookies, slices.Delete and req.AddCookie in ApplyOnRR).
In @common/authprovider/authx/file.go:
- Around line 185-202: The Cookie.Parse function mishandles edge cases: remove
the redundant check for len(slice)==0 (strings.Split never returns empty for
non-empty input) and treat tmp as the whole cookie string even if no ';'
present; then parse the name/value using strings.SplitN on the first '=' (e.g.,
strings.SplitN(slice[0], "=", 2)) to preserve '=' characters in the value, trim
spaces for Key and Value, and return a clear error if no '=' is found. Ensure
these changes are applied inside the Cookie.Parse method to correctly populate
c.Key and c.Value.
In @common/authprovider/multi.go:
- Around line 22-49: LookupURL and LookupURLX are inconsistent with LookupAddr:
they stop on a non-nil but empty []authx.AuthStrategy, causing premature
returns. Change the conditional in LookupURL and LookupURLX to check for a
non-empty slice (len(strategy) > 0) instead of strategy != nil so they behave
like LookupAddr and continue iterating when a provider returns an empty slice;
keep the return type and semantics of the functions unchanged.
🧹 Nitpick comments (6)
common/authprovider/authx/bearer_auth.go (1)
24-31: Consider nil/empty token handling.If
s.Datais nil ors.Data.Tokenis empty, this will either panic or set an invalidAuthorization: Bearerheader. While validation may occur at a higher level (e.g., during secret file loading), defensive checks here would prevent unexpected behavior.♻️ Optional defensive check
func (s *BearerTokenAuthStrategy) Apply(req *http.Request) { + if s.Data == nil || s.Data.Token == "" { + return + } req.Header.Set("Authorization", "Bearer "+s.Data.Token) } func (s *BearerTokenAuthStrategy) ApplyOnRR(req *retryablehttp.Request) { + if s.Data == nil || s.Data.Token == "" { + return + } req.Header.Set("Authorization", "Bearer "+s.Data.Token) }common/authprovider/file.go (2)
44-78: Misleading comment and silent regex error handling.
Line 47: The comment "allocate copy of pointer" is misleading.
_secretis aSecretstruct value (copied from the slice), not a pointer. The assignment creates another copy to avoid closure capture issues.Lines 54-56: Regex compilation errors are silently skipped. Although
Secret.Validate()checks regex validity beforehand, this silentcontinuecould mask issues if validation is bypassed or if the implementation changes.♻️ Suggested improvements
func (f *FileAuthProvider) init() { for _, _secret := range f.store.Secrets { - secret := _secret // allocate copy of pointer + secret := _secret // create local copy to avoid closure capture issues if len(secret.DomainsRegex) > 0 { for _, domain := range secret.DomainsRegex { if f.compiled == nil { f.compiled = make(map[*regexp.Regexp][]authx.AuthStrategy) } compiled, err := regexp.Compile(domain) if err != nil { - continue + continue // Already validated in NewFileAuthProvider; skip if somehow invalid }
81-104: Port normalization only handles 80 and 443.
LookupAddrstrips ports 80 and 443 but leaves other ports intact. If a secrets file specifiesapi.example.combut the lookup receivesapi.example.com:8080, matching will fail. Consider documenting this behavior or normalizing consistently.common/authprovider/authx/file.go (3)
66-80:GetStrategy()returns nil for unknown types without indication.If
s.Typedoesn't match any known auth type,nilis returned silently. WhileValidate()should prevent this, consider logging or returning an error for defensive programming.
204-218: File extension check is case-sensitive.Extensions like
.YML,.YAML, or.JSONwon't be recognized. Consider normalizing the extension to lowercase before comparison.♻️ Proposed fix
func GetAuthDataFromFile(file string) (*Authx, error) { ext := filepath.Ext(file) + ext = strings.ToLower(ext) if !generic.EqualsAny(ext, ".yml", ".yaml", ".json") { return nil, fmt.Errorf("invalid file extension: supported extensions are .yml,.yaml and .json got %s", ext) }
98-145: Redundant default case in validation switch.The
defaultcase at lines 142-144 is unreachable because lines 83-85 already return an error for unsupported types. This dead code can be removed.♻️ Proposed cleanup
case strings.EqualFold(s.Type, string(QueryAuth)): // ... validation logic - default: - return fmt.Errorf("invalid type: %s", s.Type) } return nil
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
common/authprovider/authx/basic_auth.gocommon/authprovider/authx/bearer_auth.gocommon/authprovider/authx/cookies_auth.gocommon/authprovider/authx/file.gocommon/authprovider/authx/headers_auth.gocommon/authprovider/authx/query_auth.gocommon/authprovider/authx/strategy.gocommon/authprovider/file.gocommon/authprovider/interface.gocommon/authprovider/multi.gorunner/options.gorunner/runner.go
🧰 Additional context used
🧬 Code graph analysis (10)
common/authprovider/interface.go (3)
common/authprovider/file.go (2)
FileAuthProvider(16-21)NewFileAuthProvider(24-42)common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/multi.go (1)
NewMultiAuthProvider(18-20)
common/authprovider/authx/query_auth.go (2)
common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/authx/file.go (1)
Secret(53-63)
common/authprovider/multi.go (2)
common/authprovider/interface.go (1)
AuthProvider(24-34)common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)
runner/runner.go (1)
common/authprovider/interface.go (3)
AuthProvider(24-34)AuthProviderOptions(37-40)NewAuthProvider(43-53)
common/authprovider/authx/basic_auth.go (2)
common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/authx/file.go (1)
Secret(53-63)
common/authprovider/file.go (3)
common/authprovider/authx/file.go (2)
Authx(39-43)GetAuthDataFromFile(205-218)common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/interface.go (2)
AuthProvider(24-34)ErrNoSecrets(14-14)
common/authprovider/authx/file.go (6)
common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/authx/basic_auth.go (1)
NewBasicAuthStrategy(19-21)common/authprovider/authx/bearer_auth.go (1)
NewBearerTokenAuthStrategy(19-21)common/authprovider/authx/headers_auth.go (1)
NewHeadersAuthStrategy(19-21)common/authprovider/authx/cookies_auth.go (1)
NewCookiesAuthStrategy(20-22)common/authprovider/authx/query_auth.go (1)
NewQueryAuthStrategy(20-22)
common/authprovider/authx/headers_auth.go (2)
common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/authx/file.go (1)
Secret(53-63)
common/authprovider/authx/bearer_auth.go (2)
common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/authx/file.go (1)
Secret(53-63)
common/authprovider/authx/cookies_auth.go (2)
common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/authx/file.go (2)
Secret(53-63)Cookie(163-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test Builds (ubuntu-latest)
- GitHub Check: Test Builds (macOS-latest)
- GitHub Check: Test Builds (windows-latest)
- GitHub Check: Functional Test (windows-latest)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Analyze (go)
- GitHub Check: release-test
🔇 Additional comments (12)
runner/options.go (3)
354-355: LGTM!The
SecretFilefield is properly documented and follows the existing naming conventions in theOptionsstruct.
528-528: LGTM!The CLI flag follows the existing pattern and matches the requested flag names (
-sf,--secret-file) from the linked issue #2358.
683-685: LGTM!Proper validation for the secret file existence. The error message is clear and consistent with other file validation messages in this function.
common/authprovider/authx/strategy.go (1)
1-16: LGTM!Clean interface design that properly abstracts authentication strategies. Supporting both
*http.Requestand*retryablehttp.Requestensures compatibility with the codebase's HTTP client usage patterns.runner/runner.go (3)
39-39: LGTM!The import and field addition follow existing patterns in the codebase. The
authProviderfield is appropriately private.Also applies to: 98-98
417-425: LGTM!Proper initialization with error handling. The error wrapping provides good context for debugging.
1720-1728: Verify header precedence behavior.Auth strategies are applied after
SetCustomHeaders(line 1719). If a user specifies a customAuthorizationheader and also has a secret file with bearer/basic auth for the same domain, the secret file's auth will overwrite the custom header sinceHeader.Setreplaces existing values.Consider whether this is the intended behavior. If custom headers should take precedence, the auth application order should be reversed.
common/authprovider/authx/basic_auth.go (1)
1-31: LGTM!Clean implementation using the standard library's
SetBasicAuthmethod. The compile-time interface check ensures type safety.common/authprovider/authx/query_auth.go (1)
1-42: LGTM!Good use of
urlutil.NewOrderedParams()to preserve query parameter ordering. UsingAdd()rather thanSet()is appropriate here as it allows the auth params to be appended without replacing existing ones.common/authprovider/authx/cookies_auth.go (1)
25-33: LGTM!The
Applymethod correctly adds cookies to the request usingAddCookie.common/authprovider/authx/headers_auth.go (1)
23-39: LGTM!Direct header map assignment correctly preserves exact casing for case-sensitive APIs. The comment documentation clearly explains this design choice.
common/authprovider/interface.go (1)
42-53: No changes required. The concern about emptySecretsFilesdoes not apply in practice. The caller only invokesNewAuthProviderwhenoptions.SecretFileis non-empty (line 416 in runner/runner.go), so an empty providers list will not occur. Additionally, even if it did,MultiAuthProvidergracefully handles empty providers by returningnilfrom lookup methods, and the runner checksif r.authProvider != nilbefore using it. The current implementation is correct.
- Fix slices.Delete while iterating bug in cookies_auth.go - Fix Cookie.Parse to handle '=' chars in values using SplitN - Add case-insensitive file extension handling in GetAuthDataFromFile - Remove unreachable default case in Secret.Validate - Make LookupURL/LookupURLX consistent with LookupAddr (use len>0) - Improve comments for clarity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Test Cookie.Parse edge cases (equals in value, spaces, empty fields) - Test Secret validation for all auth types - Test file loading with different extensions (case-insensitive) - Test all auth strategies (Apply and ApplyOnRR methods) - Test FileAuthProvider domain lookup (exact and regex) - Test MultiAuthProvider delegation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @common/authprovider/authx/file.go:
- Around line 119-132: The validation loop for CookiesAuth iterates s.Cookies by
value so cookie.Parse() updates only the copy; change the loop to iterate by
index (or range over pointers) and operate on &s.Cookies[i] (or
s.Cookies[i].Parse/Validate) so parsed Key/Value are persisted back into the
original slice; ensure you still call Validate() on the modified element and
return the same formatted errors from the CookiesAuth branch.
🧹 Nitpick comments (4)
common/authprovider/provider_test.go (2)
124-138: Consider checking parse errors in tests.The
url.Parseandurlutil.Parseerrors are being silently ignored. While these URLs are hardcoded and valid, explicitly checking errors makes tests more robust and serves as documentation.Proposed fix
t.Run("LookupURL", func(t *testing.T) { u, _ := url.Parse("https://example.com/path") + if u == nil { + t.Fatal("Failed to parse URL") + } strategies := provider.LookupURL(u) if len(strategies) != 1 { t.Errorf("LookupURL() returned %d strategies, want 1", len(strategies)) } }) t.Run("LookupURLX", func(t *testing.T) { u, _ := urlutil.Parse("https://example.com/path") + if u == nil { + t.Fatal("Failed to parse URL") + } strategies := provider.LookupURLX(u)
167-177: Consider reusing the test helper for consistency.The second secrets file creation duplicates the logic in
createTestSecretsFile. While functional, reusing the helper would be more consistent.Proposed fix
- // Create second file in different temp dir - tmpDir2 := t.TempDir() - filePath2 := filepath.Join(tmpDir2, "secrets2.yaml") - err = os.WriteFile(filePath2, []byte(content2), 0644) - if err != nil { - t.Fatalf("Failed to create test secrets file: %v", err) - } + filePath2 := createTestSecretsFile(t, content2)common/authprovider/authx/file_test.go (1)
243-330: Test files share the same temp directory - potential filename collision.All test cases write to the same
tmpDir, and since tests may run in parallel or quickly in sequence, files from previous iterations could cause interference. Each test case should either use a unique filename or clean up after itself.Proposed fix - use unique filenames per test
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Create test file - filePath := filepath.Join(tmpDir, tt.filename) + // Use a subdirectory per test to avoid filename collisions + testDir := filepath.Join(tmpDir, tt.name) + if err := os.MkdirAll(testDir, 0755); err != nil { + t.Fatalf("Failed to create test directory: %v", err) + } + filePath := filepath.Join(testDir, tt.filename) err := os.WriteFile(filePath, []byte(tt.content), 0644)common/authprovider/multi.go (1)
22-50: Consider nil provider validation or guard.If a
nilprovider is passed toNewMultiAuthProvider, callingprovider.LookupAddr(host)will panic. Consider filtering out nil providers in the constructor or adding a nil check in the loop.Option 1: Filter nil providers in constructor
func NewMultiAuthProvider(providers ...AuthProvider) AuthProvider { - return &MultiAuthProvider{Providers: providers} + filtered := make([]AuthProvider, 0, len(providers)) + for _, p := range providers { + if p != nil { + filtered = append(filtered, p) + } + } + return &MultiAuthProvider{Providers: filtered} }Option 2: Guard in lookup methods
func (m *MultiAuthProvider) LookupAddr(host string) []authx.AuthStrategy { for _, provider := range m.Providers { + if provider == nil { + continue + } strategy := provider.LookupAddr(host)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
common/authprovider/authx/cookies_auth.gocommon/authprovider/authx/file.gocommon/authprovider/authx/file_test.gocommon/authprovider/authx/strategy_test.gocommon/authprovider/file.gocommon/authprovider/multi.gocommon/authprovider/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- common/authprovider/authx/cookies_auth.go
- common/authprovider/file.go
🧰 Additional context used
🧬 Code graph analysis (5)
common/authprovider/authx/strategy_test.go (6)
common/authprovider/authx/file.go (4)
Secret(53-63)BasicAuth(20-20)KV(146-149)Cookie(161-165)common/authprovider/authx/basic_auth.go (1)
NewBasicAuthStrategy(19-21)common/authprovider/authx/bearer_auth.go (1)
NewBearerTokenAuthStrategy(19-21)common/authprovider/authx/headers_auth.go (1)
NewHeadersAuthStrategy(19-21)common/authprovider/authx/cookies_auth.go (1)
NewCookiesAuthStrategy(19-21)common/authprovider/authx/query_auth.go (1)
NewQueryAuthStrategy(20-22)
common/authprovider/multi.go (2)
common/authprovider/interface.go (1)
AuthProvider(24-34)common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)
common/authprovider/authx/file_test.go (1)
common/authprovider/authx/file.go (4)
Cookie(161-165)Secret(53-63)KV(146-149)GetAuthDataFromFile(207-220)
common/authprovider/authx/file.go (6)
common/authprovider/authx/strategy.go (1)
AuthStrategy(11-16)common/authprovider/authx/basic_auth.go (1)
NewBasicAuthStrategy(19-21)common/authprovider/authx/bearer_auth.go (1)
NewBearerTokenAuthStrategy(19-21)common/authprovider/authx/headers_auth.go (1)
NewHeadersAuthStrategy(19-21)common/authprovider/authx/cookies_auth.go (1)
NewCookiesAuthStrategy(19-21)common/authprovider/authx/query_auth.go (1)
NewQueryAuthStrategy(20-22)
common/authprovider/provider_test.go (2)
common/authprovider/file.go (1)
NewFileAuthProvider(24-42)common/authprovider/multi.go (1)
NewMultiAuthProvider(18-20)
🪛 GitHub Check: Lint Test
common/authprovider/authx/strategy_test.go
[failure] 107-107:
SA1008: keys in http.Header are canonicalized, "X-API-Key" is not canonical; fix the constant or use http.CanonicalHeaderKey (staticcheck)
[failure] 94-94:
SA1008: keys in http.Header are canonicalized, "X-API-Key" is not canonical; fix the constant or use http.CanonicalHeaderKey (staticcheck)
🪛 Gitleaks (8.30.0)
common/authprovider/authx/file_test.go
[high] 33-33: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Functional Test (windows-latest)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (18)
common/authprovider/provider_test.go (3)
12-21: LGTM - Well-designed test helper.The helper function properly uses
t.Helper()for better stack traces and leveragest.TempDir()for automatic cleanup.
23-105: Good coverage for LookupAddr scenarios.The test covers a comprehensive set of domain matching scenarios including exact match, case insensitivity, port normalization for standard ports (80, 443), subdomain matching, regex matching, and non-matching cases. This aligns well with the PR objectives for domain-based credential application.
213-256: Good error path coverage.Tests appropriately validate error handling for empty secrets, invalid secret types, and missing required fields.
common/authprovider/authx/strategy_test.go (5)
10-48: LGTM - Comprehensive BasicAuth strategy tests.Tests properly verify both
ApplyandApplyOnRRmethods, and correctly usereq.BasicAuth()to validate the encoded credentials.
50-77: LGTM - BearerToken tests cover both request types.Correctly validates the
Authorization: Bearer <token>header format.
116-173: LGTM - Cookies strategy tests are thorough.The
ApplyOnRR replaces existing cookiessubtest specifically validates that existing cookies with matching names are replaced while others are preserved - important behavior for the auth flow.
175-215: LGTM - Query strategy tests verify parameter injection.Properly tests that new query parameters are added while preserving existing ones on both request types.
79-114: No changes needed — implementation correctly preserves exact header casing.The
HeadersAuthStrategy.Applymethod uses direct map assignment (req.Header[header.Key] = []string{header.Value}), which intentionally bypasses HTTP header canonicalization. The implementation comments explicitly document this behavior. The test assertions on exact keys like"X-API-Key"are correct and will pass as written. The static analyzer warning is a false positive.common/authprovider/authx/file_test.go (3)
9-82: LGTM - Comprehensive cookie parsing tests.Good edge case coverage including: cookies with
=in values (common in JWTs/base64), whitespace handling,Set-Cookieprefix stripping, and proper error cases for empty/invalid inputs.
84-241: LGTM - Thorough secret validation test coverage.Tests cover all five auth types (BasicAuth, BearerToken, Header, Cookie, Query) with both valid configurations and various error conditions. Good inclusion of case-insensitive type matching test at line 222-230 and regex validation test at lines 212-220.
332-408: LGTM - Strategy factory tests validate type mapping.Tests correctly verify that
GetStrategy()returns the appropriate strategy implementation for each auth type andnilfor unknown types.common/authprovider/multi.go (1)
10-20: LGTM - Clear documentation and straightforward constructor.The comments clearly explain the first-match semantics, which is important for users configuring multiple providers with potentially overlapping domains.
common/authprovider/authx/file.go (6)
17-36: LGTM - Clear auth type definitions.The constants and
SupportedAuthTypes()helper provide a clean way to enumerate and validate auth types.
38-63: LGTM - Well-structured data models.The structs are properly tagged for both JSON and YAML serialization, and the Secret struct supports all documented auth mechanisms.
65-80: LGTM - Strategy factory pattern.The
GetStrategy()method correctly maps auth types to their strategy implementations using case-insensitive comparison.
146-178: LGTM - Simple validation for KV and Cookie types.Validation correctly allows
Cookieto skip key/value checks whenRawis provided (since Parse will populate them).
180-204: LGTM - Robust cookie parsing.Good handling of edge cases:
Set-Cookie:prefix stripping,SplitNwith limit 2 to preserve=in values, whitespace trimming, and proper error messages.
206-244: LGTM - File loading with proper extension validation.Case-insensitive extension handling and clear separation between YAML and JSON parsing paths. Error wrapping with
errkitprovides good context.
| case strings.EqualFold(s.Type, string(CookiesAuth)): | ||
| if len(s.Cookies) == 0 { | ||
| return fmt.Errorf("cookies cannot be empty in cookies auth") | ||
| } | ||
| for _, cookie := range s.Cookies { | ||
| if cookie.Raw != "" { | ||
| if err := cookie.Parse(); err != nil { | ||
| return fmt.Errorf("invalid raw cookie in cookiesAuth: %s", err) | ||
| } | ||
| } | ||
| if err := cookie.Validate(); err != nil { | ||
| return fmt.Errorf("invalid cookie in cookiesAuth: %s", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Raw cookie parsing doesn't persist to original slice.
The loop iterates over s.Cookies by value, so cookie is a copy. When cookie.Parse() populates Key and Value, only the local copy is modified—the original Cookie in s.Cookies remains unchanged with empty Key/Value fields. This means raw cookies won't work when the strategy later tries to apply them.
Proposed fix - iterate by index
case strings.EqualFold(s.Type, string(CookiesAuth)):
if len(s.Cookies) == 0 {
return fmt.Errorf("cookies cannot be empty in cookies auth")
}
- for _, cookie := range s.Cookies {
- if cookie.Raw != "" {
- if err := cookie.Parse(); err != nil {
+ for i := range s.Cookies {
+ if s.Cookies[i].Raw != "" {
+ if err := s.Cookies[i].Parse(); err != nil {
return fmt.Errorf("invalid raw cookie in cookiesAuth: %s", err)
}
}
- if err := cookie.Validate(); err != nil {
+ if err := s.Cookies[i].Validate(); err != nil {
return fmt.Errorf("invalid cookie in cookiesAuth: %s", err)
}
}📝 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.
| case strings.EqualFold(s.Type, string(CookiesAuth)): | |
| if len(s.Cookies) == 0 { | |
| return fmt.Errorf("cookies cannot be empty in cookies auth") | |
| } | |
| for _, cookie := range s.Cookies { | |
| if cookie.Raw != "" { | |
| if err := cookie.Parse(); err != nil { | |
| return fmt.Errorf("invalid raw cookie in cookiesAuth: %s", err) | |
| } | |
| } | |
| if err := cookie.Validate(); err != nil { | |
| return fmt.Errorf("invalid cookie in cookiesAuth: %s", err) | |
| } | |
| } | |
| case strings.EqualFold(s.Type, string(CookiesAuth)): | |
| if len(s.Cookies) == 0 { | |
| return fmt.Errorf("cookies cannot be empty in cookies auth") | |
| } | |
| for i := range s.Cookies { | |
| if s.Cookies[i].Raw != "" { | |
| if err := s.Cookies[i].Parse(); err != nil { | |
| return fmt.Errorf("invalid raw cookie in cookiesAuth: %s", err) | |
| } | |
| } | |
| if err := s.Cookies[i].Validate(); err != nil { | |
| return fmt.Errorf("invalid cookie in cookiesAuth: %s", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In @common/authprovider/authx/file.go around lines 119 - 132, The validation
loop for CookiesAuth iterates s.Cookies by value so cookie.Parse() updates only
the copy; change the loop to iterate by index (or range over pointers) and
operate on &s.Cookies[i] (or s.Cookies[i].Parse/Validate) so parsed Key/Value
are persisted back into the original slice; ensure you still call Validate() on
the modified element and return the same formatted errors from the CookiesAuth
branch.
Mzack9999
added
the
Type: Enhancement
Summary
-sf, --secret-fileflag for file-based authentication (closes Add authentication file support (-sf, --secret-file) similar to nuclei #2358)Test Cases
1. Create test file
test_secrets.yaml:2. Test commands:
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.