fix: prevent infinite recursion and fix nanosecond vs second bug in session manager

[magic-peach]

Summary

Fixed multiple bugs in util/session/sessionmanager.go related to login failure tracking.

Fixes #28870

Changes

• Prevent infinite recursion: Replaced recursive pickRandomNonAdminLoginFailure with iterative approach to prevent stack overflow when all entries are admin/username
• Prevent panic: Added bounds check for len(failures) <= 1 to prevent panic from rand.Intn(0)
• Fix duration units: Fixed getLoginFailureWindow returning nanoseconds instead of seconds by multiplying by time.Second
• Fix comparison logic: Fixed expireOldFailedAttempts to use duration comparison directly instead of multiplying by time.Second again
• Fix comparison logic: Fixed exceededFailedLoginAttempts to compare durations directly instead of converting to float64 seconds

[bunnyshell]

❗ Preview Environment deployment failed on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

• 🚀 /bns:deploy to redeploy the environment
• ❌ /bns:delete to remove the environment

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[qodo-code-review]

PR Summary by Qodo

Fix session-manager login failure tracking (duration units + safe eviction)

🐞 Bug fix 🕐 10-20 Minutes

---

AI Description

• Fix login failure window to use seconds (not raw nanoseconds)
• Prevent stack overflow/panic when evicting non-admin login failures
• Correct time-window comparisons to use time.Duration directly

Diagram

graph TD
  Env["Env var config"] --> Win("getLoginFailureWindow()")
  Mgr["SessionManager"] --> Exp("expireOldFailedAttempts()") --> Cache[("Login failures cache")]
  Mgr --> Pick("pickRandomNonAdminLoginFailure()") --> Cache
  Cache --> Ex("exceededFailedLoginAttempts()") --> Win
  subgraph Legend
    direction LR
    _svc["Component"] ~~~ _fn("Function") ~~~ _db[("Cache")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Prefilter eligible keys then pick one

• ➕ Deterministic termination without retry loops
• ➕ Simpler reasoning: choose from an explicit eligible set
• ➕ Avoids repeated full-map scans when many keys are ineligible
• ➖ Allocates a slice of keys (small overhead) unless reusing a buffer
• ➖ Slightly more code if optimizing allocations

2. Reservoir sampling over eligible entries (single pass)

• ➕ Single pass with no extra allocations
• ➕ Uniform random selection among eligible keys
• ➖ More complex to implement/review correctly than prefiltering
• ➖ Harder to unit test for randomness properties

Recommendation: The PR’s iterative retry approach is a clear improvement over recursion and eliminates stack overflow risk. If this path becomes hot or eviction happens frequently under attack load, consider switching to a prefiltered eligible-key slice (or reservoir sampling) to avoid repeated scans; otherwise the current change is acceptable for simplicity.

Files changed (1) +16 / -11

Bug fix (1) +16 / -11

sessionmanager.goFix failure-window units and harden login-failure eviction logic +16/-11

Fix failure-window units and harden login-failure eviction logic

• Corrects the configured login failure window to be interpreted in seconds by scaling env values with time.Second. Fixes comparisons in expiration and threshold checks to compare time.Duration values directly. Reworks random non-admin eviction to be non-recursive and adds a guard for small maps to prevent rand.Intn panics.

util/session/sessionmanager.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 15 rules

1. Eviction can miss eligible user ✓ Resolved 🐞 Bug ⛨ Security

Description

pickRandomNonAdminLoginFailure retries only len(failures) random picks and can return nil even when
a non-admin/non-current user exists, so updateFailureCount may skip eviction and allow the failures
map to exceed the configured max size. This also contradicts existing tests that dereference the
returned pointer and expect the only eligible user to always be returned.

Code

util/session/sessionmanager.go[R340-356]

+	if len(failures) <= 1 {
+		return nil
+	}
+	for range len(failures) {
+		idx := rand.Intn(len(failures))
+		i := 0
+		for key := range failures {
+			if i == idx {
+				if key != common.ArgoCDAdminUsername && key != username {
+					return &key
+				}
+				break
			}
-			return &key
+			i++
		}
-		i++
	}
	return nil

Relevance

⭐⭐⭐ High

Team routinely accepts correctness/security fixes around nil/edge cases; SessionManager fixes were
actively reviewed in #26388/#28656.

PR-#26388
PR-#28656

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new selection loop can repeatedly land on protected keys and exhaust its limited retries,
returning nil despite the presence of an eligible key; updateFailureCount only evicts when a non-nil
key is returned, so a nil result means no eviction even at capacity. The unit tests explicitly
dereference the returned pointer and assert it is always the eligible key, which will fail if nil is
returned.

util/session/sessionmanager.go[338-357]
util/session/sessionmanager.go[374-385]
util/session/sessionmanager_test.go[1530-1566]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pickRandomNonAdminLoginFailure` currently samples random indices **with replacement** for only `len(failures)` attempts, which does not guarantee selecting an eligible key (non-admin and not the current username) even when one exists. When it returns `nil`, `updateFailureCount` can proceed without eviction while already at the max cache size.

### Issue Context
This eviction is part of a DoS/memory-exhaustion mitigation. Returning `nil` in cases where eligible keys exist undermines the cache-size constraint and also breaks existing tests that assume a non-nil return when an eligible key is present.

### Fix Focus Areas
- util/session/sessionmanager.go[338-357]
- util/session/sessionmanager.go[374-385]
- util/session/sessionmanager_test.go[1530-1566]

### Proposed fix
1. Build a slice of eligible keys:
  - Iterate once over `failures`.
  - Append keys where `key != common.ArgoCDAdminUsername && key != username`.
2. If the eligible slice is empty, return `nil`.
3. Otherwise, select uniformly: `k := eligible[rand.Intn(len(eligible))]`.
4. Return the chosen key (preferably change signature to `(string, bool)` to avoid returning `&key` of a loop variable; update callers accordingly).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. No tests for login failures ✓ Resolved 📘 Rule violation ▣ Testability

Description

Login failure tracking behavior was changed (time window units, expiration logic, and random
eviction behavior), but this PR does not add or update any automated tests to cover the
new/bug-fixed behavior. This increases the risk of regressions (e.g., duration unit mistakes or
eviction edge-cases reappearing) without detection.

Code

util/session/sessionmanager.go[R125-130]

// Returns the number of maximum seconds the login is allowed to delay for
func getLoginFailureWindow() time.Duration {
-	return time.Duration(env.ParseNumFromEnv(envLoginFailureWindowSeconds, defaultFailureWindow, 0, math.MaxInt32))
+	return time.Duration(env.ParseNumFromEnv(envLoginFailureWindowSeconds, defaultFailureWindow, 0, math.MaxInt32)) * time.Second
}

// NewSessionManager creates a new session manager from Argo CD settings

Relevance

⭐⭐⭐ High

SessionManager behavior changes typically come with new tests (added in PRs #26388, #28656).

PR-#26388
PR-#28656

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2230598 requires tests for behavior-changing code changes. The PR modifies the
login failure window duration calculation and multiple core login-failure behaviors (expiration
comparison and non-admin eviction selection), but no test changes are included in the PR diff.

Rule 2230598: Require tests for behavior-changing code changes
util/session/sessionmanager.go[125-128]
util/session/sessionmanager.go[327-357]
util/session/sessionmanager.go[422-440]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Behavior-changing fixes in `util/session/sessionmanager.go` (duration unit corrections and random eviction logic changes) were made without adding/updating tests to lock in the expected behavior.

## Issue Context
The compliance rule requires at least one new or updated automated test for behavior-changing code. Existing tests do not explicitly assert the corrected duration units (seconds vs nanoseconds) or cover the brute-force cache overflow edge-cases addressed by the new iterative non-admin selection logic.

## Fix Focus Areas
- util/session/sessionmanager.go[125-357]
- util/session/sessionmanager_test.go[583-719]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[codecov]

Bundle Report

Bundle size has no change ✅

[dudinea]

@magic-peach thank you for your submission!

Please could you open issue(s) for the bugs you found in the session manager and link your PR to the issue(s) created?

Please could you write/modify unit tests for the changes you made?

Please could you make your PR draft and not submit it to the review until it passes the tests and complies with other requirements to PRs according to the Developer's Guide?

[nitishfy]

Honestly, this looks very AI generated and so is the issue. I'm not closing this PR but please fix the failing checks before moving forward. Also, please incorporate the feedback that @dudinea has left.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.38%. Comparing base (4891d4f) to head (39aca3a).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #28860      +/-   ##
==========================================
+ Coverage   65.33%   65.38%   +0.04%     
==========================================
  Files         426      426              
  Lines       60031    60166     +135     
==========================================
+ Hits        39221    39339     +118     
+ Misses      17211    17208       -3     
- Partials     3599     3619      +20     

Flag	Coverage Δ
e2e	26.74% <0.00%> (-0.11%)	⬇️
unit-tests	60.83% <100.00%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.