Repository navigation
Follow-ups from reviewing today's security fixes - #46
Merged
Merged
Conversation
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.
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
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.
Three small gaps found while reviewing #25, #42, #43, #44 and #45 together.
Deactivated proxy user saw the sign-in screen (from #43).
/api/sessionreporteduser: nulland nothing else. The app therefore told a deactivated proxy user to sign in through their portal, when they were already signed in there.deactivated: true.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.
"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.
ErrBusy: "too many sign-ins at once from your network, try again in a moment". It is still a 429.Tests:
TestDeactivatedProxyUserIsShutOutchecks the session.TestRelabelUserMovesAllTheirUsageadds an in-flight request and a new user with the reused ID.ErrBusyfor in-flight refusals.-race.