Repository navigation
Verify email addresses, fail closed on hotdata 404s, drop the shared ingest credential - #47
Conversation
…g the shared ingest token as a credential Review of tenant isolation found no cross-org data path, but three ways an outsider could get in or stay in: - Team links restricted to a domain were cosmetic: addresses were never verified, so anyone holding the link could type any @Domain address and read the org. Every new account now has to open a mailed link while signed in as that account before it can see or report anything. An unverified address can be registered over (new password, squatter's sessions revoked), which also closes pre-registration squatting. Accounts older than the cutoff are trusted; with no mail key configured the gate is off and the server says so at boot. - rows() read a hotdata 404 as an empty table, so a transient 404 during a join made the joiner admin of an established org, and one during delete-org removed an org with members. Only a 400 is empty now. - The shared HOTUSAGE_INGEST_TOKEN could report usage as anyone in any org. It is now only the switch that requires a token; the value is accepted as a credential only with HOTUSAGE_ALLOW_SHARED_INGEST=1. Also: an org admin can no longer delete a platform operator's account by removing them, and database ids no longer appear in any response.
220e6cf to
f51ea8d
Compare
| if user and self.verification and not self.is_verified(email): | ||
| # an unverified row is a claim on the address, not an account (see | ||
| # create_account): the acceptor sets the password and the mailbox | ||
| # decides who really joined | ||
| if not password: | ||
| raise ValueError("choose a password") | ||
| self.take_over(email, password) | ||
| self.join_org(email, inv["org_slug"]) |
There was a problem hiding this comment.
Take the existing-account join path when as_user already equals email. A session as that account already proves the password, so no takeover is needed. Also compute the invite page's MODE from "exists AND verified", not from get_user alone (server.py:2842).
Failure scenario: bob@acme.com registers at /register, so the row exists and is unverified. An admin then sends bob a single-use invite. GET /invite/<token> reaches server.py:2842, finds a user row, and sets MODE to join (bob signed in) or login (bob not signed in). invite.html:64-79 hides the password field in join mode and hides the whole form in login mode. Bob posts with no password, this branch raises choose a password, and the redirect lands back on the same page with the field still hidden. Bob can never accept the invite. The takeover branch added here is unreachable through the UI for single-use invites.
There was a problem hiding this comment.
Fixed in a74f229. accept_invite takes the join path first whenever as_user equals the invited address, verified or not; the takeover branch only runs for a stranger posting a password. The invite page now sets MODE to join for a matching session, login only for an existing verified account, and new otherwise, so an unverified claim gets the password field. Test: test_a_signed_in_unverified_account_joins_through_a_single_use_invite.
| # A verified answer never reverts, so it is kept for the life of the | ||
| # process; an unverified one is re-read often, since the next request | ||
| # after the click should see the change. | ||
| VERIFIED_NEG_TTL = 15 | ||
| # Accounts created before this predate verification and are trusted as | ||
| # they were. Everyone after it proves the mailbox before seeing or | ||
| # reporting anything. The instant the change shipped, not a round date: | ||
| # a later cutoff would grandfather whoever registered in the gap. | ||
| VERIFICATION_SINCE = datetime(2026, 9, 17, 19, 0, tzinfo=timezone.utc) | ||
|
|
||
| @staticmethod | ||
| def _as_datetime(value): | ||
| if isinstance(value, str): | ||
| try: | ||
| value = datetime.fromisoformat(value.replace("Z", "+00:00")) | ||
| except ValueError: | ||
| return None | ||
| if not isinstance(value, datetime): | ||
| return None | ||
| if value.tzinfo is None: | ||
| value = value.replace(tzinfo=timezone.utc) | ||
| return value | ||
|
|
||
| def is_verified(self, email): | ||
| """Has this address been proved -- or does it predate the requirement? | ||
| Always True with verification off.""" | ||
| if not self.verification: | ||
| return True | ||
| email = email.strip().lower() | ||
| now = time.time() | ||
| with self.lock: | ||
| hit = self.verified_cache.get(email) | ||
| if hit and (hit[0] or now - hit[1] < self.VERIFIED_NEG_TTL): | ||
| return hit[0] |
There was a problem hiding this comment.
Give the positive cache entry a TTL as well. A verified answer never reverts for one address, but the row can be deleted and the address re-registered by someone else.
| # A verified answer never reverts, so it is kept for the life of the | |
| # process; an unverified one is re-read often, since the next request | |
| # after the click should see the change. | |
| VERIFIED_NEG_TTL = 15 | |
| # Accounts created before this predate verification and are trusted as | |
| # they were. Everyone after it proves the mailbox before seeing or | |
| # reporting anything. The instant the change shipped, not a round date: | |
| # a later cutoff would grandfather whoever registered in the gap. | |
| VERIFICATION_SINCE = datetime(2026, 9, 17, 19, 0, tzinfo=timezone.utc) | |
| @staticmethod | |
| def _as_datetime(value): | |
| if isinstance(value, str): | |
| try: | |
| value = datetime.fromisoformat(value.replace("Z", "+00:00")) | |
| except ValueError: | |
| return None | |
| if not isinstance(value, datetime): | |
| return None | |
| if value.tzinfo is None: | |
| value = value.replace(tzinfo=timezone.utc) | |
| return value | |
| def is_verified(self, email): | |
| """Has this address been proved -- or does it predate the requirement? | |
| Always True with verification off.""" | |
| if not self.verification: | |
| return True | |
| email = email.strip().lower() | |
| now = time.time() | |
| with self.lock: | |
| hit = self.verified_cache.get(email) | |
| if hit and (hit[0] or now - hit[1] < self.VERIFIED_NEG_TTL): | |
| return hit[0] | |
| # Both answers expire. A verified answer never reverts for one address, | |
| # but remove_user deletes the row and the address may be registered again | |
| # by someone else -- and a delete is seen only by the instance that served | |
| # it, so a positive entry cached forever on another instance would let the | |
| # next registrant inherit it. An unverified answer is re-read often, since | |
| # the next request after the click should see the change. | |
| VERIFIED_TTL = 60 | |
| VERIFIED_NEG_TTL = 15 | |
| # Accounts created before this predate verification and are trusted as | |
| # they were. Everyone after it proves the mailbox before seeing or | |
| # reporting anything. The instant the change shipped, not a round date: | |
| # a later cutoff would grandfather whoever registered in the gap. | |
| VERIFICATION_SINCE = datetime(2026, 9, 17, 19, 0, tzinfo=timezone.utc) | |
| @staticmethod | |
| def _as_datetime(value): | |
| if isinstance(value, str): | |
| try: | |
| value = datetime.fromisoformat(value.replace("Z", "+00:00")) | |
| except ValueError: | |
| return None | |
| if not isinstance(value, datetime): | |
| return None | |
| if value.tzinfo is None: | |
| value = value.replace(tzinfo=timezone.utc) | |
| return value | |
| def is_verified(self, email): | |
| """Has this address been proved -- or does it predate the requirement? | |
| Always True with verification off.""" | |
| if not self.verification: | |
| return True | |
| email = email.strip().lower() | |
| now = time.time() | |
| with self.lock: | |
| hit = self.verified_cache.get(email) | |
| ttl = self.VERIFIED_TTL if hit and hit[0] else self.VERIFIED_NEG_TTL | |
| if hit and now - hit[1] < ttl: | |
| return hit[0] |
Failure scenario: bob@x.dev is verified, and instances A and B both cache True with no expiry. An org admin removes bob, whose only org that was, so remove_from_org calls remove_user (server.py:1012) and deletes the verified_emails row. remove_user pops verified_cache only on the instance that served the request. An attacker registers bob@x.dev, and every request routed to instance B reads the stale True and passes _gated_on_verify. The attacker reads and reports without ever holding the mailbox. TOKEN_CACHE_TTL (server.py:623-628) caps the session cache for exactly this reason: App Runner runs several instances and a row delete is seen by one of them.
There was a problem hiding this comment.
Fixed in a74f229: VERIFIED_TTL = 60 for positive answers, matching TOKEN_CACHE_TTL, with the suggested comment. Test: test_a_verified_answer_expires_from_the_cache deletes the row behind the cache and checks the stale True ages out.
| the previous claimant held is revoked, so they cannot ride along once | ||
| the real owner verifies. Callers check is_verified first.""" | ||
| email = email.strip().lower() | ||
| self.set_password(email, password) |
There was a problem hiding this comment.
Drop the address's org memberships and admin grants inside take_over, and clear the account's active org. A membership the previous claimant acquired must not pass to the real owner. remove_user already does exactly this at server.py:1047-1050, for the reason its own comment gives at server.py:1051: whoever re-registers an address must not inherit a grant. accept_invite and accept_team_invite call join_org right after take_over, so an invite-driven join still happens.
Failure scenario: mallory holds a verified account and an org evil, and creates a team link with no domain restriction. Mallory posts /invite/<token> with email=victim@corp.com and any password. accept_team_invite reaches the final else and calls create_user, so victim@corp.com becomes an unverified member of evil with org_slug=evil. Victim later registers at /register. create_account (server.py:690) calls take_over, which changes only the password and the sessions. Victim opens the mailed link, verifies, and lands on evil's dashboard. Every collector report from victim routes into evil's database, and mallory reads it as an admin of evil.
Before this change the same pre-registration only locked victim out ("that email is already registered"). The takeover turns that lockout into a silent capture, so the fix belongs in the same commit.
There was a problem hiding this comment.
Fixed in a74f229. take_over now drops every membership and admin grant and writes the users row with org_slug empty, then revokes sessions and clears route_cache. accept_invite and accept_team_invite join their org right after, as before. Test: test_a_takeover_inherits_nothing_the_squatter_acquired runs the exact team-link squat and checks the victim lands in no org.
There was a problem hiding this comment.
Review
Blocking Issues
-
server/server.py:777-784 — unchanged from cycle 1 (thread 4040902695). A single-use invite for an unverified address cannot be accepted through the UI. The invite page computes
MODEfromget_useralone at server.py:2842, so the password field is hidden, and the takeover branch then demands a password. -
server/server.py:1587 — unchanged from cycle 1 (thread 4040904642). A positive
is_verifiedanswer is cached with no expiry.remove_userpops the cache only on the instance that served the delete, so another instance keeps servingTruefor an address that someone else has since registered. -
server/server.py:1608-1621 — new (thread on the
take_overbody).take_overkeeps the previous claimant's org memberships and active org. A pre-registered address therefore hands the real owner a membership in the pre-registrant's org.
Action Required
- Take the existing-account join path in
accept_invitewhenas_useralready equalsemail. Compute the invite page'sMODEfrom exists AND verified. - Give the positive
verified_cacheentry a TTL, asTOKEN_CACHE_TTLdoes at server.py:623-628. - Delete the address's
org_membershipsrows and admin grants intake_over, and clearusers.org_slug. - Add a test for each of the three paths.
No new commits landed since cycle 1, so findings 1 and 2 stand on the code as written.
A takeover now drops every org membership, admin grant and the active org the previous claimant acquired, so pre-registering a victim's address through a team link no longer routes the victim's reports into the squatter's org. Invite-driven callers join their org right after, as before. A positive is_verified answer expires after a minute like the session cache: remove_user's row delete is seen by one instance, and the next registrant of that address must not inherit a verified answer cached forever on another. A single-use invite for an unverified address is acceptable again: signed in as that address it is a plain join (the session already proved the password), and the invite page only hides the password field for a real account or a matching session, so an unverified claim gets the field the takeover branch needs.
| def established(addr): | ||
| # an unverified row is a claim, not an account: the acceptor | ||
| # sets a password and takes it over (see AuthStore.take_over) | ||
| addr = (addr or "").strip().lower() | ||
| return bool(addr and self.auth.get_user(addr) | ||
| and self.auth.is_verified(addr)) | ||
| joining_existing = established(inv["email"]) if inv \ | ||
| else established(form.get("email", "")) |
There was a problem hiding this comment.
Treat the address as established when the signed-in viewer is that address. accept_invite now takes the plain-join path in that case, but the password check here still runs first.
| def established(addr): | |
| # an unverified row is a claim, not an account: the acceptor | |
| # sets a password and takes it over (see AuthStore.take_over) | |
| addr = (addr or "").strip().lower() | |
| return bool(addr and self.auth.get_user(addr) | |
| and self.auth.is_verified(addr)) | |
| joining_existing = established(inv["email"]) if inv \ | |
| else established(form.get("email", "")) | |
| def established(addr): | |
| # an unverified row is a claim, not an account: the acceptor | |
| # sets a password and takes it over (see AuthStore.take_over). | |
| # Signed in as the address, the session is the proof instead. | |
| addr = (addr or "").strip().lower() | |
| return bool(addr and self.auth.get_user(addr) | |
| and (addr == (as_user or "").strip().lower() | |
| or self.auth.is_verified(addr))) | |
| joining_existing = established(inv["email"]) if inv \ | |
| else established(form.get("email", "")) |
Failure scenario: bob@acme.com registers and stays unverified. An admin sends bob a single-use invite. Bob, signed in, opens /invite/<token>. The GET handler sets MODE to join, so invite.html hides the password field. Bob posts with an empty password. established("bob@acme.com") returns False because is_verified is False. Line 2388 raises "password must be at least 8 characters" before accept_invite runs. The redirect shows the same page with the field still hidden, so bob cannot accept the invite. If bob gets past that check, line 2407 calls login(email, ""), which also fails.
The new test test_a_signed_in_unverified_account_joins_through_a_single_use_invite calls AuthStore.accept_invite directly, so it does not cover this handler. With the fix, lines 2400–2402 skip the resend and redirect to /, which is correct for a plain join.
There was a problem hiding this comment.
Fixed in fa0f55e with the suggested established(). HTTP-level test test_an_unverified_invitee_accepts_their_own_invite_without_a_password in test_install_gate.py: signed in as the unverified invitee, GET shows join mode, POST with no password is a 302 to / and bob is a member and still signed in; a stranger gets the password field and is refused without one.
There was a problem hiding this comment.
Review — Blocking Issues
- server/server.py:2380-2388 —
_handle_invite_acceptstill requires a password of 8+ characters when a signed-in, unverified account accepts its own single-use invite.established()returns False for an unverified address, so the check at line 2388 fails beforeaccept_inviteruns. The invite page is injoinmode and hides the password field, so the invite cannot be accepted through the UI. The prior finding (thread 4040902695) is fixed inAuthStore.accept_inviteand the GET handler, but not in the POST handler.
Action Required
- In
established(), return True when the address exists and equals the signed-in viewer's email. - Add an HTTP-level test that posts
/invite/<token>with no password while signed in as an unverified invitee.
The other two prior findings (the positive cache TTL and take_over dropping memberships) are fixed.
…password The POST handler's established() check treated an unverified row as a claim needing a password, even when the viewer was signed in as that address, so the join-mode page (which hides the field) could never submit. A matching session is the proof instead. An HTTP-level test drives the page and the POST as a browser, for the invitee and for a stranger.
| raise ValueError(f"this link only accepts @{inv['domain']} addresses") | ||
| if self.get_user(email): | ||
| user = self.get_user(email) | ||
| if user and self.verification and not self.is_verified(email): |
There was a problem hiding this comment.
In accept_team_invite, take the plain-join path first when as_user equals email. This mirrors accept_invite. Since fa0f55e, the handler's established() treats a session as that address as proof, but this method still runs the takeover first.
| if user and self.verification and not self.is_verified(email): | |
| signed_in_as = (as_user or "").strip().lower() == email | |
| if user and self.verification and not self.is_verified(email) \ | |
| and not signed_in_as: |
The existing elif user: branch then handles the signed-in case. That branch already checks for a duplicate membership and calls join_org.
Failure scenario: bob@acme.com registers and stays unverified. Bob, signed in, opens an acme team link and types his own address. invite.html:51-55 tells him to leave the password blank. established() returns True, so server.py:2390 skips the length check. This method raises "choose a password", so bob cannot join with a blank password. If bob types a password such as "a", take_over sets a 1-character password, because the 8-character check was skipped. take_over also revokes bob's session. Then server.py:2404 redirects to / without a new cookie, so bob is signed out. Before fa0f55e the length check ran and line 2409 issued a fresh session. Add an HTTP-level test for the team-link path next to test_an_unverified_invitee_accepts_their_own_invite_without_a_password.
There was a problem hiding this comment.
Fixed in 596c9e2 with the suggested signed_in_as guard. HTTP-level test test_an_unverified_member_joins_through_a_team_link_without_a_password next to the single-use one: signed in and unverified, bob posts the team link with his address and a blank password, gets a 302 to /, is a member, keeps his session and password; a 1-char password typed afterwards is not installed.
There was a problem hiding this comment.
Review — Blocking Issues: server/server.py:1260 — accept_team_invite still runs the takeover for a signed-in unverified user who uses a team link with their own address. fa0f55e makes established() return True in that case, so _handle_invite_accept skips the 8-character password check and the post-accept login. A blank password is refused with 'choose a password'. A typed password of any length takes over the account, revokes the user's own session, and redirects to / with no new cookie. Action Required: In accept_team_invite, take the plain-join branch when as_user equals email, as accept_invite does. Add an HTTP-level test for a signed-in unverified user accepting a team link with a blank password.
… own account accept_team_invite ran the takeover whenever the row was unverified, even for the person signed in as it. Since the handler now treats a matching session as proof and skips the password check, that path would have set an unchecked password and revoked the session. Mirror accept_invite: a matching session is a plain join. HTTP-level test drives the team link as that browser.
Review of tenant isolation found no cross-org data path, but three ways an
outsider could get in or stay in:
verified, so anyone holding the link could type any @Domain address and
read the org. Every new account now has to open a mailed link while
signed in as that account before it can see or report anything. An
unverified address can be registered over (new password, squatter's
sessions revoked), which also closes pre-registration squatting.
Accounts older than the cutoff are trusted; with no mail key configured
the gate is off and the server says so at boot.
join made the joiner admin of an established org, and one during
delete-org removed an org with members. Only a 400 is empty now.
It is now only the switch that requires a token; the value is accepted
as a credential only with HOTUSAGE_ALLOW_SHARED_INGEST=1.
Also: an org admin can no longer delete a platform operator's account by
removing them, and database ids no longer appear in any response.
Breaking:
/ingestwith the shared token returns 401 unlessHOTUSAGE_ALLOW_SHARED_INGEST=1. Per-user tokens (every signed-in client) are unaffected.Email verification turns on only when
RESEND_API_KEYandHOTUSAGE_EMAIL_FROMare set; without them the server logs that verification is off and behaves as before.Tests: all suites pass, with new coverage for the gate, takeover, rows() 400-vs-404, the shared token, and the sysadmin removal guard.