diff --git a/docsgpt/alembic/versions/0034_auth_events_actor_target.py b/docsgpt/alembic/versions/0034_auth_events_actor_target.py index ffe10aac..0854f730 100644 --- a/docsgpt/alembic/versions/0034_auth_events_actor_target.py +++ b/docsgpt/alembic/versions/0034_auth_events_actor_target.py @@ -20,8 +20,9 @@ Backfill rules, applied only to rows that predate the columns: * ``actor_id`` = the first present of ``metadata->>'by'``, ``metadata->>'granted_by'``, ``metadata->>'revoked_by'``, else ``user_id``. -* ``target_id`` = ``user_id``, except for ``team.*`` events, which were always - filed under the actor and have no single user target. +* ``target_id`` = ``user_id``, except for events that were always filed under + the actor and have no single user target: ``team.*``, and instance- or + team-scoped ``quota_policy_*`` changes. Also adds the indexes the global admin feed needs. Before this the only index was ``(user_id, created_at DESC)``, so the cross-user feed — which orders by @@ -62,6 +63,11 @@ def upgrade() -> None: ), target_id = CASE WHEN event LIKE 'team.%' THEN NULL + -- An instance or team quota policy is filed under the acting + -- admin, not a target user (see api/admin/quotas.py::_audit); + -- only a user-scoped policy has one. + WHEN event IN ('quota_policy_set', 'quota_policy_deleted') + AND COALESCE(metadata->>'scope', '') <> 'user' THEN NULL ELSE user_id END WHERE actor_id IS NULL; diff --git a/docsgpt/api/admin/activity.py b/docsgpt/api/admin/activity.py index 0f2d4ae5..ec6add8f 100644 --- a/docsgpt/api/admin/activity.py +++ b/docsgpt/api/admin/activity.py @@ -209,6 +209,24 @@ def _export_rows(filters: dict, limit: int) -> Iterator[dict]: ) +# Characters a spreadsheet reads as the start of a formula. +_FORMULA_TRIGGERS = ("=", "+", "-", "@", "\t", "\r") + + +def _csv_safe(value: object) -> object: + """Neutralize a leading formula trigger before the value becomes a CSV cell. + + The feed carries attacker-controlled text: ``user_agent`` is the raw header + of whoever made the request, and a denied login records one without ever + authenticating. An export is opened in a spreadsheet by an admin, so a cell + beginning ``=`` would be evaluated there. Prefixing an apostrophe keeps the + value readable and inert. + """ + if isinstance(value, str) and value.startswith(_FORMULA_TRIGGERS): + return f"'{value}" + return value + + def _csv_rows(filters: dict, limit: int) -> Iterator[str]: buffer = io.StringIO() writer = csv.DictWriter(buffer, fieldnames=list(ACTIVITY_COLUMNS)) @@ -220,7 +238,7 @@ def _csv_rows(filters: dict, limit: int) -> Iterator[str]: serialized["detail"] = json.dumps( serialized.get("detail") or {}, default=str ) - writer.writerow(serialized) + writer.writerow({key: _csv_safe(value) for key, value in serialized.items()}) yield _drain(buffer) diff --git a/frontend/src/admin/Activity.tsx b/frontend/src/admin/Activity.tsx index 65c0bdb7..fc2f8ee3 100644 --- a/frontend/src/admin/Activity.tsx +++ b/frontend/src/admin/Activity.tsx @@ -1,5 +1,5 @@ import { ChevronDown, ChevronRight, Download } from 'lucide-react'; -import { useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useSelector } from 'react-redux'; import { useSearchParams } from 'react-router-dom'; @@ -173,7 +173,14 @@ export default function Activity() { }; }, [token]); + // Monotonic request id: a response only lands if no newer request was + // issued meanwhile, so an out-of-order reply cannot leave the table showing + // a different filter combination than the controls. Mirrors the guard in + // settings/Analytics. + const requestId = useRef(0); + const load = useCallback(async () => { + const id = ++requestId.current; setLoading(true); try { const res = await adminService.getActivity( @@ -181,10 +188,11 @@ export default function Activity() { token, ); const json = await res.json().catch(() => ({})); + if (id !== requestId.current) return; setRows(json.activity ?? []); setTotal(json.total ?? 0); } finally { - setLoading(false); + if (id === requestId.current) setLoading(false); } }, [token, page, filters]); @@ -338,17 +346,25 @@ export default function Activity() { const repeat = idx > 0 && rows[idx - 1].actor_id === row.actor_id; return [ - setExpanded(isOpen ? null : key)} - > + - {isOpen ? ( - - ) : ( - - )} +
diff --git a/frontend/src/admin/Usage.tsx b/frontend/src/admin/Usage.tsx index 85e85704..20de3855 100644 --- a/frontend/src/admin/Usage.tsx +++ b/frontend/src/admin/Usage.tsx @@ -221,13 +221,15 @@ export default function Usage() { {topUsers.map((user) => ( - setDrilldown(user.user_id)} - > + - {user.user_id} + = { fallback: 'Provider fallback', }; -function SplitTable({ title, rows }: { title: string; rows: Split[] }) { +function SplitTable({ + title, + rows, + labels, +}: { + title: string; + rows: Split[]; + /** Only the flow table maps keys; a model named `fallback` is a model. */ + labels?: Record; +}) { return (

{title}

@@ -61,7 +70,7 @@ function SplitTable({ title, rows }: { title: string; rows: Split[] }) { {rows.map((row) => ( - {FLOW_LABELS[row.key] ?? row.key} + {labels?.[row.key] ?? row.key} {fmtNumber(row.tokens)} @@ -104,6 +113,9 @@ export default function UserUsageModal({ } let cancelled = false; setLoading(true); + // Drop the previous range's numbers rather than showing them under the + // newly selected one. + setData(null); adminService .getUserUsage(userId, { days }, token) .then((res) => res.json()) @@ -161,7 +173,7 @@ export default function UserUsageModal({ ))}
- {loading && data === null ? ( + {loading ? ( ) : data && !data.success ? ( @@ -193,7 +205,11 @@ export default function UserUsageModal({
- + )} diff --git a/tests/api/test_admin_activity.py b/tests/api/test_admin_activity.py index 38ba0c79..c0929190 100644 --- a/tests/api/test_admin_activity.py +++ b/tests/api/test_admin_activity.py @@ -193,6 +193,29 @@ class TestExport: lines = body.strip().split("\n") assert [json.loads(line)["id"] for line in lines] == ["1", "2"] + def test_csv_cells_cannot_smuggle_a_spreadsheet_formula(self, client): + """``user_agent`` is an unauthenticated attacker's raw header.""" + repo = _repo( + rows=[_row(user_agent="=cmd|'/c calc'!A0", actor_id="+1", ip="-2")] + ) + with _admin(repo): + body = client.get( + "/api/admin/activity/export?format=csv" + ).get_data(as_text=True) + row = next(csv.DictReader(io.StringIO(body))) + assert row["user_agent"].startswith("'=") + assert row["actor_id"].startswith("'+") + assert row["ip"].startswith("'-") + + def test_csv_leaves_ordinary_cells_alone(self, client): + with _admin(_repo()): + body = client.get( + "/api/admin/activity/export?format=csv" + ).get_data(as_text=True) + row = next(csv.DictReader(io.StringIO(body))) + assert row["user_agent"] == "curl/8" + assert row["event"] == "oidc_login" + def test_empty_csv_export_still_has_its_header(self, client): with _admin(_repo(rows=[])): body = client.get("/api/admin/activity/export").get_data(as_text=True) diff --git a/tests/storage/db/test_migration_0034.py b/tests/storage/db/test_migration_0034.py index 0bcd5e36..7abf4d11 100644 --- a/tests/storage/db/test_migration_0034.py +++ b/tests/storage/db/test_migration_0034.py @@ -125,3 +125,38 @@ class TestMigration0034RoundTrip: assert rows["oidc_login"] == "someone|someone" # Team events were always filed under the actor; they have no user target. assert rows["team.create"] == "actor-3|-" + + def test_backfill_scopes_quota_policy_targets(self, pg_engine): + """Only a user-scoped quota policy has a user target. + + Instance and team policies are filed under the acting admin, so + backfilling ``target_id = user_id`` would claim the admin was acted on. + """ + url = pg_engine.url.render_as_string(hide_password=False) + _run_alembic(url, "downgrade", _0033) + with pg_engine.begin() as conn: + conn.execute( + text( + "INSERT INTO auth_events (user_id, event, metadata) VALUES " + "('admin-q', 'quota_policy_set', " + " '{\"by\": \"admin-q\", \"scope\": \"instance\"}'::jsonb), " + "('admin-q2', 'quota_policy_deleted', " + " '{\"by\": \"admin-q2\", \"scope\": \"team\"}'::jsonb), " + "('capped-user', 'quota_policy_set', " + " '{\"by\": \"admin-q3\", \"scope\": \"user\"}'::jsonb)" + ) + ) + _run_alembic(url, "upgrade", "head") + with pg_engine.connect() as conn: + rows = dict( + conn.execute( + text( + "SELECT user_id, actor_id || '|' || COALESCE(target_id, '-') " + "FROM auth_events WHERE user_id IN " + "('admin-q', 'admin-q2', 'capped-user')" + ) + ).fetchall() + ) + assert rows["admin-q"] == "admin-q|-" + assert rows["admin-q2"] == "admin-q2|-" + assert rows["capped-user"] == "admin-q3|capped-user"