Skip to content

Follow-ups from reviewing today's security fixes - #46

Merged
bricke merged 1 commit into
mainfrom
fix/review-followups
Oct 10, 2026
Merged

bricke merged 1 commit into
mainfrom
fix/review-followups

Conversation

@bricke

@bricke bricke commented Oct 10, 2026

Copy link
Copy Markdown
Owner

Three small gaps found while reviewing #25, #42, #43, #44 and #45 together.

Deactivated proxy user saw the sign-in screen (from #43). /api/session reported user: null and nothing else. The app therefore told a deactivated proxy user to sign in through their portal, when they were already signed in there.

  • The session now includes deactivated: true.
  • The app shows "Your account is deactivated … Ask them to reactivate it."

Requests still running at deletion (from #44). A request a user started before being deleted, and that finished after, was written under their name once their usage had been relabelled. A later user given that name would see it.

  • The ledger now remembers recent deletions (one hour) by user ID.
  • A request that started before the deletion is filed under the deleted user's label.
  • A new user with the same name, or the same reused ID, starts requests after the deletion and keeps their own.
  • The map is only touched by the ledger's writer goroutine, so it needs no lock.

"Too many failed logins" with no failures (from #42). Logins still being checked count against the throttle. Many people signing in at once from one address, such as an office NAT or built-in sign-in behind a reverse proxy, could get "too many failed logins" without a single failure.

  • That case is now ErrBusy: "too many sign-ins at once from your network, try again in a moment". It is still a 429.
  • The limits themselves are unchanged, so the race fix keeps its guarantee.

Tests:

  • TestDeactivatedProxyUserIsShutOut checks the session.
  • TestRelabelUserMovesAllTheirUsage adds an in-flight request and a new user with the reused ID.
  • The throttle tests expect ErrBusy for in-flight refusals.
  • The session and in-flight checks fail without the fix; the full suite passes with -race.

A deactivated proxy user opening the UI was asked to sign in through
their portal, though they were signed in: /api/session reported no user
and nothing else. It now reports the deactivation, and the app says
their access was turned off.

A request a user started before being deleted, and that finished after,
was written under their name once the usage had been relabelled, so a
later user given that name would see it. The ledger now remembers
recent deletions by user ID and files requests that started before the
deletion under the label. A new user with the same name, or the same
reused ID, starts requests after it and keeps their own.

Logins still being checked count against the throttle, so many people
signing in at once from one address could be told "too many failed
logins" without any failure. That case is now ErrBusy, "too many
sign-ins at once from your network, try again in a moment", still a
429; the limits themselves are unchanged.
@bricke
bricke merged commit eb19e42 into main Oct 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant