mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-11 03:12:55 +00:00
fix(admin): address review findings
- 0034's backfill set target_id to the acting admin for instance- and team-scoped quota policy changes, which are filed under the actor and have no user target. It now returns NULL for those, matching what quotas.py records going forward, with a test covering all three scopes. - The CSV export wrote every cell verbatim. user_agent is an attacker's raw header, recorded without authenticating on a denied login, and the export is opened in a spreadsheet by an admin -- a cell starting "=" would be evaluated there. Every cell now has a leading formula trigger neutralized. - The activity feed applied every response it received, so a slow reply for an old filter could overwrite the current one. Guarded by a request id, the same way settings/Analytics already does. - The activity detail expander and the top-user drill-down were row onClick handlers, unreachable without a mouse. Both are real buttons now, the expander carrying aria-expanded and a label naming its event. - FLOW_LABELS was applied to both breakdowns in the spend modal, so a model named "fallback" or "workflow" rendered as a flow description. Only the flow table maps keys now. - Changing the range in that modal left the previous range's totals and chart on screen while the new request was in flight.
This commit is contained in:
1 parent
64200a7f23
commit
2ea9d66789
7 files changed
+141
-25
No files matched your search
@@ -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;
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
|
||||
@@ -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 [
|
||||
<TableRow
|
||||
key={key}
|
||||
className="hover:bg-muted/40 cursor-pointer"
|
||||
onClick={() => setExpanded(isOpen ? null : key)}
|
||||
>
|
||||
<TableRow key={key} className="hover:bg-muted/40">
|
||||
<TableCell>
|
||||
{isOpen ? (
|
||||
<ChevronDown className="text-muted-foreground size-4" />
|
||||
) : (
|
||||
<ChevronRight className="text-muted-foreground size-4" />
|
||||
)}
|
||||
<button
|
||||
type="button"
|
||||
aria-expanded={isOpen}
|
||||
aria-label={
|
||||
isOpen
|
||||
? `Hide details for ${eventLabel(row.event)}`
|
||||
: `Show details for ${eventLabel(row.event)}`
|
||||
}
|
||||
className="text-muted-foreground hover:text-foreground focus-visible:ring-ring cursor-pointer rounded focus-visible:ring-2 focus-visible:outline-none"
|
||||
onClick={() => setExpanded(isOpen ? null : key)}
|
||||
>
|
||||
{isOpen ? (
|
||||
<ChevronDown className="size-4" />
|
||||
) : (
|
||||
<ChevronRight className="size-4" />
|
||||
)}
|
||||
</button>
|
||||
</TableCell>
|
||||
<TableCell>
|
||||
<div className="flex items-center gap-2">
|
||||
|
||||
@@ -221,13 +221,15 @@ export default function Usage() {
|
||||
</TableHead>
|
||||
<TableBody>
|
||||
{topUsers.map((user) => (
|
||||
<TableRow
|
||||
key={user.user_id}
|
||||
className="hover:bg-muted/40 cursor-pointer"
|
||||
onClick={() => setDrilldown(user.user_id)}
|
||||
>
|
||||
<TableRow key={user.user_id} className="hover:bg-muted/40">
|
||||
<TableCell className="font-mono text-[13px] break-all">
|
||||
{user.user_id}
|
||||
<button
|
||||
type="button"
|
||||
className="hover:text-foreground focus-visible:ring-ring cursor-pointer rounded text-left underline-offset-2 hover:underline focus-visible:ring-2 focus-visible:outline-none"
|
||||
onClick={() => setDrilldown(user.user_id)}
|
||||
>
|
||||
{user.user_id}
|
||||
</button>
|
||||
</TableCell>
|
||||
<TableCell
|
||||
align="right"
|
||||
|
||||
@@ -41,7 +41,16 @@ const FLOW_LABELS: Record<string, string> = {
|
||||
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<string, string>;
|
||||
}) {
|
||||
return (
|
||||
<div>
|
||||
<p className="text-muted-foreground mb-2 text-sm font-medium">{title}</p>
|
||||
@@ -61,7 +70,7 @@ function SplitTable({ title, rows }: { title: string; rows: Split[] }) {
|
||||
{rows.map((row) => (
|
||||
<TableRow key={row.key}>
|
||||
<TableCell className="text-[13px] break-all">
|
||||
{FLOW_LABELS[row.key] ?? row.key}
|
||||
{labels?.[row.key] ?? row.key}
|
||||
</TableCell>
|
||||
<TableCell align="right" className="tabular-nums">
|
||||
{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({
|
||||
))}
|
||||
</div>
|
||||
|
||||
{loading && data === null ? (
|
||||
{loading ? (
|
||||
<Loading />
|
||||
) : data && !data.success ? (
|
||||
<LoadError message="Failed to load usage." />
|
||||
@@ -193,7 +205,11 @@ export default function UserUsageModal({
|
||||
</div>
|
||||
|
||||
<SplitTable title="Model" rows={data?.by_model ?? []} />
|
||||
<SplitTable title="Flow" rows={data?.by_source ?? []} />
|
||||
<SplitTable
|
||||
title="Flow"
|
||||
rows={data?.by_source ?? []}
|
||||
labels={FLOW_LABELS}
|
||||
/>
|
||||
</div>
|
||||
)}
|
||||
</Modal>
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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"
|
||||
Reference in new issue
Block a user