File: internal/service/auth_service.go (lines 780, 785, 790, 799)
Project: tinyauth
Severity: BUG • Confidence: high • Slug: other-logic-bug
Finding
In ensureOAuthSessionLimit (lines 772-805), the inner loop's first branch unconditionally sets oldestId/oldestTime to the first session it sees (lines 785-789, predicated on oldestTime == 0) BEFORE checking whether that id is already in cleanupIds. So on the second outer iteration onward, if Go's randomized map iteration order happens to start at an already-selected id, that id is re-selected and 'continue'd; the eventual cleanupIds slice contains duplicates and the final delete loop ends up removing fewer than OAuthCleanupCount entries. The map can therefore stay at or above MaxOAuthPendingSessions, making the cleanup ineffective when limit pressure is high — and a higher-than-intended steady state increases the chance that legitimate OAuth-mid-flow sessions are evicted by attacker traffic to /api/oauth/url (an unauthenticated endpoint).
Recommendation
Move the cleanupIds membership check (line 790-792) before the 'oldestTime == 0' bootstrap, or track a parallel set of selected ids. A cleaner rewrite: collect all session ids+expiry once into a slice, sort by expiry, then delete the first OAuthCleanupCount.
Revalidation
Verdict: true-positive
Confirmed. In ensureOAuthSessionLimit (lines 772-805), the inner loop's bootstrap branch (lines 785-789) unconditionally sets oldestId/oldestTime to the first map entry visited when oldestTime == 0, BEFORE checking whether that id is already in cleanupIds (the slices.Contains check at lines 790-792 is on the next branch). Because Go randomizes map iteration order, on outer iteration N+1 the inner loop can bootstrap with an id already chosen on iteration N. If no subsequent inner-iteration session has a smaller ExpiresAt, the loop ends with oldestId equal to that already-selected id, and cleanupIds gets a duplicate. The final delete loop (lines 802-804) deletes that id twice (second is a no-op), so fewer than OAuthCleanupCount sessions are actually evicted. With sustained pressure on the unauthenticated /api/oauth/url endpoint, the pending-sessions map can sit at the cap and start evicting legitimate in-flight sessions. The recommendation (move the membership check above the bootstrap, or sort by expiry once) is correct.
File:
internal/service/auth_service.go(lines 780, 785, 790, 799)Project: tinyauth
Severity: BUG • Confidence: high • Slug:
other-logic-bugFinding
In ensureOAuthSessionLimit (lines 772-805), the inner loop's first branch unconditionally sets oldestId/oldestTime to the first session it sees (lines 785-789, predicated on oldestTime == 0) BEFORE checking whether that id is already in cleanupIds. So on the second outer iteration onward, if Go's randomized map iteration order happens to start at an already-selected id, that id is re-selected and 'continue'd; the eventual cleanupIds slice contains duplicates and the final delete loop ends up removing fewer than OAuthCleanupCount entries. The map can therefore stay at or above MaxOAuthPendingSessions, making the cleanup ineffective when limit pressure is high — and a higher-than-intended steady state increases the chance that legitimate OAuth-mid-flow sessions are evicted by attacker traffic to /api/oauth/url (an unauthenticated endpoint).
Recommendation
Move the cleanupIds membership check (line 790-792) before the 'oldestTime == 0' bootstrap, or track a parallel set of selected ids. A cleaner rewrite: collect all session ids+expiry once into a slice, sort by expiry, then delete the first OAuthCleanupCount.
Revalidation
Verdict: true-positive
Confirmed. In ensureOAuthSessionLimit (lines 772-805), the inner loop's bootstrap branch (lines 785-789) unconditionally sets oldestId/oldestTime to the first map entry visited when oldestTime == 0, BEFORE checking whether that id is already in cleanupIds (the slices.Contains check at lines 790-792 is on the next branch). Because Go randomizes map iteration order, on outer iteration N+1 the inner loop can bootstrap with an id already chosen on iteration N. If no subsequent inner-iteration session has a smaller ExpiresAt, the loop ends with oldestId equal to that already-selected id, and cleanupIds gets a duplicate. The final delete loop (lines 802-804) deletes that id twice (second is a no-op), so fewer than OAuthCleanupCount sessions are actually evicted. With sustained pressure on the unauthenticated /api/oauth/url endpoint, the pending-sessions map can sit at the cap and start evicting legitimate in-flight sessions. The recommendation (move the membership check above the bootstrap, or sort by expiry once) is correct.