Fluksio/app
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bound what the API accepts, and close the holes the audit found

Browse Source 
**SQLite is the database, and now says so.** `metric_minute` and every run
table are written with `sqlalchemy.dialects.sqlite.insert(...)
.on_conflict_do_update` and with `max(a, b)`, neither of which another
dialect has — so pointing `DATABASE_URL` at Postgres migrated cleanly,
served, logged in, and then lost every observability flush into the
collector's hold buffer and failed every run. It refuses at startup
instead. (The Postgres in the compose stack is Umami's; the engine's own
database has been a file beside the flows since 2026-08-21.)

**Every integer query parameter is bounded.** The caps were written as
`min(limit, 500)`, which a negative walks straight through — `?limit=-1`
compiles to `LIMIT -1` and SQLite returns the whole table. Ten signatures,
now `Query(ge=…, le=…)`. `hours=0` still means an hour, which
`_window_hours` was already deliberate about.

**Exports are capped at 10 000 runs** and say so with `X-Truncated`. The
filters bounded a sensible request and nothing bounded an unfiltered one,
which read every row into memory before a byte was streamed. `_series`
resolves cached curves in two queries rather than a `Run` lookup and a
`RunMetric` query per restored node — a comparison of twenty runs was
calling that twenty times over.

**`PUT /artifacts` has a size limit** (`MAX_ARTIFACT_BYTES`, 2 GiB, 0 to
disable), checked against `Content-Length` and again against the stream for
a chunked body, and its writes moved off the event loop.

**`/observability/timeseries` takes `since`/`until`**, the same window
`/runs` and `/events` take, capped at 2000 points — `hours=720&bucket_s=60`
was 43 200 of them in one array. It is also what a dragged chart needs to
re-fetch at its own resolution rather than magnifying buckets it has.

**Composite indexes** for the three list screens: `run(flow, created_at)`
and `(status, created_at)`, `flow_run(flow, started_at)`,
`engine_event(type, ts)`. Every index was single-column, so SQLite picked
one and sorted the rest by hand. Verified against a copy of a live database
(250k `flow_run` rows): the planner takes all four.

**Redis clients have socket timeouts.** A Redis that stops answering
without closing the connection hung the caller until the kernel gave up —
including `/utils/health/`, whose job is to notice.

**The panels file is written under one lock.** `save_panels` and
`unpair_panel` are both read-modify-write, and a save that read before an
unpair wrote put the old nonce back — silently un-revoking a screen that
had just been unpaired. The nonce carry-forward was written to make that
impossible; the gap between its read and its write is where it happened.

**Startup releases what it acquired.** Everything past `event_bus.bind`
registers how to close itself and the `finally` walks that list backwards;
a failure part-way through used to reach none of the shutdown steps and
leave the worker pool's subprocesses and every background task behind —
under `--reload`, once per bad edit. `modules.reconcile` moved into the
background: `uv` gets five minutes twice over, the healthcheck allows
eighty seconds, and the autoheal restarted the container before it could
finish installing.

`delete_run` takes SQLite's write lock up front (`core.db.writing`) rather
than upgrading a deferred transaction and losing to whichever flush
committed in between. `modules.sync` is serialised — two applies mutated
one venv at once. The proxied-call and stream dicts are bounded, and a
reused id cancels its predecessor instead of dropping the reference.

Security, found in passing and small enough to fix here:

- **`/secrets/` required only a signed-in user.** The names alone say what
  this installation talks to, and `PUT /{name}` takes any name, so any
  account could overwrite the credential a flow authenticates with.
  Superuser now — which `/search` already assumed and said so.
- **`POST /login/access-token` had no rate limit.** Argon2 is deliberately
  expensive and the route is unauthenticated and runs in the shared
  threadpool. Ten *failed* attempts per address per five minutes; a
  successful sign-in spends nothing.
- **a password reset link worked repeatedly for 48 hours.** The token now
  carries a digest of the password hash it was minted against, so it stops
  verifying once it has set one. No table of spent tokens needed.
- **enrolment accepted `http://`**, sending the claim code and then this
  installation's credential in clear. https, or a local address.
- the rate limiter read `request.client.host`, which behind Traefik is the
  proxy — so every per-address limit was one global bucket and one caller
  could lock out everyone. It reads the forwarded address, and its
  bucket table is capped rather than growing one key per address forever.
- SMTP has a timeout and sends after the response, so an unreachable mail
  host cannot pin a threadpool worker, and a reply's timing no longer says
  whether the address exists.

Test suite: engine-written rows are cleared between modules. A `FlowRun`
left `running` by one module turned up in another's query. Per-test
rollback is not available here — the module-scoped `client` runs the real
lifespan and its collector and run service write through sessions of their
own — so this bounds it where the writes come from. Three consecutive
green runs, orders randomised.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M6hPWS6YEbT1P8LxhhFb2T
This commit is contained in:
stroblme
2026-08-29 20:40:05 +02:00
co-authored by Claude Opus 5
parent 1069247085
commit 57eace2226
24 changed files with 821 additions and 260 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/fluksio/api/routes/runs.py
+104 -52
View File
@@ -12,9 +12,9 @@ import time
from collections.abc import Iterator
from datetime import UTC, datetime
from itertools import groupby
from typing import Any, Literal
from typing import Annotated, Any, Literal
from fastapi import APIRouter, Depends, HTTPException, Request, Response
from fastapi import APIRouter, Depends, HTTPException, Query, Request, Response
from fastapi.concurrency import run_in_threadpool
from fastapi.responses import StreamingResponse
from pydantic import BaseModel, Field, model_validator
@@ -23,6 +23,7 @@ from sqlalchemy import select as sa_select
from sqlmodel import Session, col, select
from fluksio.api.deps import CurrentUser, SessionDep, get_current_user
from fluksio.core.db import writing
from fluksio.flow.events import event_bus
from fluksio.flow.messages import requalify
from fluksio.flow.runs import RunRejected, RunService, new_run_id
@@ -271,8 +272,8 @@ def read_runs(
digest: str | None = None,
since: datetime | None = None,
before: datetime | None = None,
limit: int = 50,
offset: int = 0,
limit: Annotated[int, Query(ge=1, le=500)] = 50,
offset: Annotated[int, Query(ge=0)] = 0,
) -> Any:
"""Runs, newest first. The queryable table an experiment log needs.
@@ -306,6 +307,11 @@ def read_overview(session: SessionDep) -> Any:
The list caps at 500 newest runs, so counting flows on the client goes
wrong the moment a history outgrows one page. The database counts instead.
"""
# Deliberately unwindowed. It is a group-by over the whole `run` table,
# which is never pruned — but a run kept for years is the point of the
# table, and a window here would drop a finished experiment off the rail
# rather than making it cheaper to find. A rollup is the answer if the
# scan ever measures.
statement = sa_select(
col(Run.flow),
col(Run.status),
@@ -346,6 +352,12 @@ METRIC_COLUMNS = ("run", "name", "step", "ts", "value")
#: A run's own columns in the wide table. Its inputs and its final numbers
#: follow, prefixed, so an input named "status" cannot collide with the run's.
#: The most runs one export covers. High enough that a real sweep fits and low
#: enough that an unfiltered request cannot read the whole table into memory —
#: which it did, before a byte was streamed, while holding one of the fifteen
#: pooled database connections for the length of the download.
EXPORT_CAP = 10_000
RUN_COLUMNS = (
"id",
"flow",
@@ -370,7 +382,12 @@ def _selected(
since: datetime | None,
until: datetime | None,
) -> list[Run]:
"""The runs an export covers, newest first — the filters the list takes."""
"""The runs an export covers, newest first — the filters the list takes.
Capped: the filters bound a *sensible* request and nothing bounded an
unfiltered one, so asking for everything read every row of the table into
memory before a byte was sent. A truncated export says so in a header.
"""
statement = select(Run).order_by(col(Run.created_at).desc())
if flow:
statement = statement.where(col(Run.flow) == flow)
@@ -385,7 +402,7 @@ def _selected(
statement = statement.where(col(Run.created_at) >= _aware(since))
if until:
statement = statement.where(col(Run.created_at) < _aware(until))
return list(session.exec(statement))
return list(session.exec(statement.limit(EXPORT_CAP + 1)))
def _cell(value: Any) -> Any:
@@ -458,7 +475,11 @@ def _csv(columns: list[str], chunks: Iterator[list[dict[str, Any]]]) -> Iterator
def _stream(
fmt: str, name: str, columns: list[str], chunks: Iterator[list[dict[str, Any]]]
fmt: str,
name: str,
columns: list[str],
chunks: Iterator[list[dict[str, Any]]],
truncated: bool = False,
) -> StreamingResponse:
"""The rows out, a chunk at a time rather than a list.
@@ -473,11 +494,13 @@ def _stream(
else:
body = _csv(columns, chunks)
media = "text/csv; charset=utf-8"
return StreamingResponse(
body,
media_type=media,
headers={"Content-Disposition": f'attachment; filename="{name}.{fmt}"'},
)
headers = {"Content-Disposition": f'attachment; filename="{name}.{fmt}"'}
if truncated:
# The body is otherwise indistinguishable from a complete one, and an
# export quietly missing its oldest runs is the wrong thing to hand
# somebody who is about to plot it.
headers["X-Truncated"] = str(EXPORT_CAP)
return StreamingResponse(body, media_type=media, headers=headers)
@router.get("/export/metrics")
@@ -490,7 +513,7 @@ def export_metrics(
since: datetime | None = None,
until: datetime | None = None,
name: str = "",
stride: int = 1,
stride: Annotated[int, Query(ge=1, le=100_000)] = 1,
format: ExportFormat = "csv",
) -> Any:
"""Every selected run's series as one long table: run, name, step, ts, value.
@@ -500,6 +523,8 @@ def export_metrics(
for every tenth point of two metrics gives every tenth point of both.
"""
runs = _selected(session, flow, status, group, ids, since, until)
truncated = len(runs) > EXPORT_CAP
runs = runs[:EXPORT_CAP]
wanted = [part for part in name.split(",") if part]
step = max(1, stride)
@@ -523,7 +548,7 @@ def export_metrics(
)
yield rows
return _stream(format, "metrics", list(METRIC_COLUMNS), chunks())
return _stream(format, "metrics", list(METRIC_COLUMNS), chunks(), truncated)
@router.get("/export/runs")
@@ -552,6 +577,8 @@ def export_runs(
depth.
"""
runs = _selected(session, flow, status, group, ids, since, until)
truncated = len(runs) > EXPORT_CAP
runs = runs[:EXPORT_CAP]
inputs = [part for part in params.split(",") if part] or sorted(
{path for run in runs for path, _ in _leaves(run.params)}
)
@@ -569,7 +596,7 @@ def export_runs(
row.update((f"metric.{k}", _cell(_dig(run.result, k))) for k in scores)
yield [row]
return _stream(format, "runs", columns, chunks())
return _stream(format, "runs", columns, chunks(), truncated)
@router.get("/{run_id}", response_model=RunDetail)
@@ -623,23 +650,27 @@ def delete_run(run_id: str, session: SessionDep, user: CurrentUser) -> Response:
keeps whatever a ``run_artifact`` row or a live message still names, so
dropping the rows is enough and the hourly sweep reclaims the blobs.
"""
run = session.get(Run, run_id)
if run is None:
raise HTTPException(status_code=404, detail="No such run")
if run.status in ("running", "queued"):
raise HTTPException(
status_code=409,
detail=(
f"Run {run_id} is {run.status}. Cancel it, or wait for it to "
"finish, before deleting it."
),
)
flow = run.flow
session.execute(delete(RunNode).where(col(RunNode.run_id) == run_id))
session.execute(delete(RunMetric).where(col(RunMetric.run_id) == run_id))
session.execute(delete(RunArtifact).where(col(RunArtifact.run_id) == run_id))
session.execute(delete(Run).where(col(Run.id) == run_id))
session.commit()
# Reads the run, then decides whether to delete it — so it takes the write
# lock up front rather than upgrading and losing to whichever flush
# committed in between.
with writing(session):
run = session.get(Run, run_id)
if run is None:
raise HTTPException(status_code=404, detail="No such run")
if run.status in ("running", "queued"):
raise HTTPException(
status_code=409,
detail=(
f"Run {run_id} is {run.status}. Cancel it, or wait for it to "
"finish, before deleting it."
),
)
flow = run.flow
session.execute(delete(RunNode).where(col(RunNode.run_id) == run_id))
session.execute(delete(RunMetric).where(col(RunMetric.run_id) == run_id))
session.execute(delete(RunArtifact).where(col(RunArtifact.run_id) == run_id))
session.execute(delete(Run).where(col(Run.id) == run_id))
session.commit()
event_bus.publish(
{
"type": "audit",
@@ -674,32 +705,50 @@ def _series(session: Session, run_id: str, name: str = "") -> list[RunMetric]:
if restored:
run = session.get(Run, run_id)
flow = run.flow if run is not None else ""
# Two queries for the lot, rather than a `Run` lookup and a `RunMetric`
# query per restored node. A comparison of twenty runs calls this
# twenty times, so the per-node round trips multiplied.
sources = {
row.id: row.flow
for row in session.exec(
select(Run).where(col(Run.id).in_({n.cached_from for n in restored}))
)
}
wanted: dict[tuple[str, str], list[RunNode]] = {}
for node_row in restored:
source = session.get(Run, node_row.cached_from)
if source is None:
source_flow = sources.get(node_row.cached_from)
if source_flow is None:
# The run it came from is gone — deleted with its flow. The
# outputs are still on this run; the curve is not recoverable.
continue
source_node = requalify(node_row.node, flow, source.flow)
for row in session.exec(
key = (
node_row.cached_from,
requalify(node_row.node, flow, source_flow),
)
wanted.setdefault(key, []).append(node_row)
if wanted:
cached = session.exec(
select(RunMetric).where(
col(RunMetric.run_id) == node_row.cached_from,
col(RunMetric.node) == source_node,
col(RunMetric.run_id).in_({run_id for run_id, _ in wanted}),
col(RunMetric.node).in_({node for _, node in wanted}),
)
):
renamed = requalify(row.name, source.flow, flow)
if name and renamed != name:
continue
rows.append(
RunMetric(
run_id=run_id,
name=renamed,
step=row.step,
node=node_row.node,
ts=row.ts,
value=row.value,
)
for row in cached:
for node_row in wanted.get((row.run_id, row.node), ()):
source_flow = sources[node_row.cached_from]
renamed = requalify(row.name, source_flow, flow)
if name and renamed != name:
continue
rows.append(
RunMetric(
run_id=run_id,
name=renamed,
step=row.step,
node=node_row.node,
ts=row.ts,
value=row.value,
)
)
)
rows.sort(key=lambda row: (row.name, row.step))
return rows
@@ -707,7 +756,10 @@ def _series(session: Session, run_id: str, name: str = "") -> list[RunMetric]:
@router.get("/{run_id}/metrics", response_model=list[MetricPoint])
def read_metrics(
run_id: str, session: SessionDep, name: str = "", stride: int = 1
run_id: str,
session: SessionDep,
name: str = "",
stride: Annotated[int, Query(ge=1, le=100_000)] = 1,
) -> Any:
"""One metric's series, in step order — or every one of them, unnamed.