feat(admin): entitlements, roles, and admin console - #341
Merged
FlyM1ss merged 15 commits intoAug 14, 2026
Conversation
Add per-user max_concurrent_backtests and credits, admin list/patch/bootstrap APIs, and an Admin page with site stats. Signup stays role=user. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
@Allan-Feng is attempting to deploy a commit to the allan-feng's projects Team on Vercel. A member of the Team first needs to authorize it. |
3 tasks
Stop ADMIN_BOOTSTRAP_SECRET from remaining a standing privilege-escalation path after the first admin exists, and keep a wrong-length guess as 403 instead of a 500. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The frozen app route contract and fast-boot cache-buster assertions still described main; CI failed on the new admin surface and the app.js/styles revisions this PR already shipped. Co-authored-by: Cursor <cursoragent@cursor.com>
- app.js cache-buster 88 -> 98: main already ships v=88, so merging left new app.js content behind a URL browsers had cached (blank Admin page). - Drop avatars from the admin projection (200k-char data URIs x 100 rows). - Paginate the users table; the list response now reports total/limit/offset. - Bootstrap guessing is now capped per client key and server-wide, not only per user (signup is open, so a new account bought a fresh budget). - Bootstrap's post-promotion quota seed no longer 500s a committed promotion. - Role + entitlements apply in one transaction (apply_admin_patch). - Entitlement upsert uses COALESCE so a partial patch cannot clobber the field it omitted; ::integer casts on the Postgres side. - /api/auth/me reads entitlements off the session join, not a 2nd query. - Blank quota inputs are refused instead of silently no-op'ing. - A 403 from an admin route re-reads /me so a demoted admin loses the menu. - secrets_equal docstring/test/CLAUDE.md: compare_digest does not raise on a length mismatch; it raises TypeError on a non-ASCII str. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KfoctuqF6C5V787BL6wqmK
# Conflicts: # dashboard/backend/tests/test_frontend_fast_boot.py # dashboard/frontend/app.html # dashboard/frontend/app.js
Collaborator
|
Follow-ups from this review, filed so they don't die in a comment:
Nothing here blocks this PR. |
This was referenced Aug 13, 2026
…nd 2) Fixes the 15 verified round-2 review findings plus 7 more confirmed by adversarial verification of the fix diff itself. Highlights: - Enforce max_concurrent_backtests: agents owned by one account share its active-run budget on both /api/v1 and /api/v2 create paths (429 too_many_active_runs_for_account). Remote entitlement/ownership reads are prefetched OFF the shared create lock and fail open on store outage, so the cap never adds Neon latency or a hard DB dependency to run creation. - apply_admin_patch: role + entitlements in one transaction on both store twins, with in-transaction read-back; drop set_user_role. - Admin routes: shared require_admin dependency, shared 429 builder, bootstrap rate-limit keyed on client_ip (header-rotation-proof budgets), explicit-null PATCH bodies rejected instead of silently no-oping. - Admin console (app.js): request-seq guard owns every paint incl. the denial branch, PATCH only changed fields, quota bounds shared with the server, boot gate stops the pre-hydration admin-shell flash. - Tests: per-account cap covered on v1 AND v2, @pg_only twins for the entitlements upsert/FK mapping/atomic patch, admin_users pinned in the event-loop guard, app-level tests isolated from the shared user store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
CodeQL py/clear-text-logging-sensitive-data (high): the repr of a store exception can carry connection details, and the users store handles password material. The outage marker keeps the exception class name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
py/clear-text-logging-sensitive-data re-minted on the same print: the agent dict (api_key material) taints everything read from it, so any dynamic payload here re-fires the alert. The marker only needs to exist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
Collaborator
|
Round-2 review fixes pushed ( Highlights:
CI green on |
Security review fixes for Open-Finance-Lab#341. The admin routes themselves held up — one correct `require_admin` gate, a transactional PATCH, a constant-time compare, and both twins serializing the one-shot promotion. What did not hold up was the plane around them: the entitlement was built as a control and wired where it could not bind, and bootstrap's own budgets could be turned on the operator. - Close the two cap bypasses. `resolve_owner_cap_context` returned None the instant `owner_user_id` was falsy, and agent creation needs no login at all (`_require_owner_context` accepts a self-chosen X-Session-Id), so signing in was strictly worse than staying anonymous: one shared account budget versus the per-agent cap times however many agents you registered. Unclaimed agents are now billed to the browser session that created them, resolved inside the store (`list_owner_scope_agent_ids`, both twins) because `_public_agent` withholds `owner_browser_session` on purpose — `_owner_context` accepts that value AS an ownership credential. - Cap the legacy `/api/v1/backtest/*` surface. It authenticates nothing and writes no `protocol_runs` row, so all three protocol caps were blind to it: a bare POST spawned a thread and pinned a bar window, unbounded, beside a quota system holding every authenticated agent to its limit. Per-session and global budgets, counted and inserted under one lock. Opt-in, so the protocol path (already capped three ways) is not charged twice. - Raise the default quota to 5 and make it env-overridable. Nothing seeds `user_entitlements` at signup and nothing backfills, so `1` was not a default but every existing account's live limit on deploy day — a silent demotion from 5x(agents), with no remedy short of an admin. - Bootstrap: check the global budget AFTER the compare. Checked first, 20 wrong guesses a window — re-spent every window, from any account, and signup is open — refused the correct secret indefinitely, and the window it blocked is exactly the fresh-deploy window the route exists for. Require >= 32 chars (refused as if unset), and answer 403 for unset/weak/wrong alike so a caller cannot learn whether a deployment is bootstrappable. - Lock FixedWindowRateLimiter. The module had no `threading` import at all, and the buckets are mutated on every call — `_pruned` popleft()s from inside `check`, not only `record`. `allow()` is now atomic. - Reject control characters in emails at the validator. strip() only touched the ends, so an interior newline validated, was stored, and forged lines in the admin console's role-change confirm() — a native dialog with no markup to escape. One fix at the source beats one guard per renderer. - Get the store off the event loop in signup/login. This PR added a sync `get_entitlements()` to `_auth_json`, called from both async handlers, which the threadpool guard cannot see (it pins plain-`def` routes, and those two are exempt for already awaiting). Bundled with `create_session` into one `asyncio.to_thread` hop, plus the guard that can see it. Also: router-level `require_admin` so a route added later is gated by default, a 403 case for all four routes enumerated from the router, an audit line on every role/quota change (ids only — no email reaches a print sink), quota floor 0 so a quota can actually suspend, `credits` labelled not-enforced in the console it is editable from, and the first Postgres tests for `promote_first_admin` — prod's copy of the one-shot predicate had none. Cache busters to 100/100: Open-Finance-Lab#342 also ships styles.css?v=97 and Open-Finance-Lab#344 ships 99/98, so 98/97 would have collided with no git conflict (Open-Finance-Lab#347/Open-Finance-Lab#348). Backend suite: 2797 passed, 76 skipped. SDK suite: 171 passed, 2 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
py/clear-text-logging-sensitive-data (alert #1249) pinned its source at the literal `32` on the `_MIN_BOOTSTRAP_SECRET_LEN = 32` line and its sink at the length-only warning that interpolates it — i.e. the query classified a plain int as a secret purely because the constant's NAME matched its sensitive-data regex, then reported the warning that says "your secret is too short" as the secret being logged in clear text. Nothing sensitive ever flowed: `_bootstrap_secret()` prints a length threshold and an env var name, never the value or a slice of it. Renamed to `_BOOTSTRAP_MIN_LENGTH`, which is also the better name — it is a length, not a secret. The rule stays useful for the cases it exists for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
styles.css never defines that token, so the rule was relying on its var() fallback. Inherit the th's own colour and step back with opacity instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
It still said "the two reads here" and "per-account", from before unclaimed agents got a browser-session budget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
…dgets Two things the previous comment left implicit and a later reader would have to rediscover: - POST /api/v1/backtest/start is the shipping SDK's path (packaging/ agentictrading/client.py:204), not only an unauthenticated abuse surface. That is why the per-session budget is 5, matching MAX_ACTIVE_RUNS_PER_AGENT, rather than something punitive. - MAX_LEGACY_ACTIVE_GLOBAL counts every resident session in _sessions, including the ones the v1/v2 protocol surfaces open through run_service.create_run — they share this registry. Deliberate: the scarce resource is resident bar windows regardless of who opened them, and when it is scarce the surface refused first should be the one with no account, agent or key behind it. So this ceiling can refuse a legacy start while protocol creates continue; never the reverse. Comments only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
0 is the suspend value, and 0 is exactly what a truthiness check drops. The console has three places it can silently vanish -- the input's min, the diff that decides what to PATCH, and the repaint that writes the server's answer back -- and in each the unsafe version reads perfectly natural (`if (value)`, `value || fallback`). All three are zero-safe today; nothing said so. Also pins the "not enforced yet" label on credits (an operator zeroing a phantom control sees a 200 and believes they acted) and _adminConfirmEmail, since confirm() is plain text with no markup to escape. Backend suite: 2800 passed, 76 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
Collaborator
|
Security review round pushed as Fixed
Prod was checked over the Render API: Filed rather than fixed — #350 (the browser-session cap is an incentive fix, not a bound against a caller who rotates the header; only |
Closes Open-Finance-Lab#351. `user_entitlements.credits` was stored, capped, admin-editable and returned to clients with nothing anywhere reading it. One credit now buys one LLM-driven dashboard backtest. - Metered path is `/backtest/run` with `decision_source='llm'` only -- the one place the operator's own key pays for the model. Rule-based runs make no model call; the protocol surfaces (`/api/v1/runs`, `/api/v2/runs`) return a decision the agent's own LLM client produced, so charging them would bill a user for a resource nobody bought. Those keep `max_concurrent_backtests`, which is a concurrency control rather than a budget. - Debit at accept, AFTER the single-flight check: a caller turned away because someone else's backtest is running never got a run. Refunded when the run made no LLM call at all -- `agent_runs.llm_calls` is the witness because it is the billing counter, ticking even on a truncated response that cost real money (unlike `llm_decisions`, the H6 coverage counter). The endpoint also unwinds a thread that fails to start, the one case the worker's own finally block cannot reach. - Strict opt-in via `CREDITS_METERING_ENABLED`, so merging changes nothing until an operator arms it. Arming makes LLM runs sign-in-only: a signed-out session has no balance, and a free anonymous allowance would make signing out the cheaper option -- the incentive inversion `resolve_owner_cap_context` already had to correct for the concurrency cap, with money as the resource. - `DEFAULT_CREDITS` (100, env-overridable) is deliberately not 0. Nothing seeds `user_entitlements` and nothing backfills, so a zero default would turn one env var into a site-wide lockout found via support tickets. - Ledger is three statements, not an upsert: a conditional UPDATE is the whole guard, and none of them touch `updated_at`/`updated_by_admin_id`, which are admin-edit provenance. The seed is skipped when the default cannot cover the spend, so a refusal never freezes today's `DEFAULT_CREDITS` into a row a later, larger value could not reach. Every Postgres parameter is cast `::integer`: `INSERT ... SELECT $1` resolves parameter types without the insert target's columns, so an uncast one is a hard 42P08 that no SQLite test can see. - Console label is server-driven (`/api/admin/stats` now reports `credits_metering_enabled`), replacing a hardcoded "(not enforced yet)" that would have gone on saying that after an operator armed metering. Backend suite: 2826 passed, 78 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCiPGs3ZYVjyFYVLa6E7bZ
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
3 participants
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.
Summary
max_concurrent_backtestsandcredits, both enforced./api/admin/*), all behind one router-levelrequire_admin.Closes #351.
Notes
role=user; admin is assigned via bootstrap/SQL/admin UI.max_concurrent_backtestsbinds on both run surfaces (v1 protocol + v2), one ledger.creditsmeters LLM-driven dashboard backtests, 1 per run. Strict opt-in — off unlessCREDITS_METERING_ENABLEDis set, so merging changes no behaviour. Arming it makes/backtest/runsign-in-only for LLM runs. See CLAUDE.md.ADMIN_BOOTSTRAP_SECRET,USERS_DATABASE_URL.Test plan
pytest dashboard/backend/tests/ -q→ 2826 passed, 78 skippedtest_credit_metering.py,test_admin_users.py,test_admin_console_frontend.py@pg_only) runs on CI only — verify the credit-ledger cases in the job logMade with Cursor