Repository navigation
Mobile web interface and durable live drives - #4
Conversation
8e6f4b0 to
1c26dcf
Compare
52797db to
3a25a98
Compare
|
Independent adversarial review resolutions (latest head
|
3a25a98 to
8fcd548
Compare
ahernsean
left a comment
There was a problem hiding this comment.
Review — Phase 3 web interface and durable live drives
The live-drive state machine (active/ending/completed/cancelled, single-open-drive partial index, deterministic completed-drive UUID) is the strongest part of this stack, and fresh-session recovery works. But the security posture described in the PR body is no longer what the branch ships, and several of the claimed guarantees don't hold at the HTTP layer.
1. Every mutation endpoint is unauthenticated and CSRF-able — and the PR body says otherwise
The description says this PR adds "strict Host/Origin/content-type checks, signed expiring form tokens." Commit 0b5a72b ("remove browser mutation security layer") later in the same branch deletes driving_log/security.py, the form_for helper, all form_token inputs, and the 89 lines of tests that covered them, replacing the whole thing with:
async def read_form(request: Request) -> FormData:
return await request.form()Reproduced against the branch tip:
POST /drives Origin: https://evil.example Host: evil.example
→ 303, drive created
Any page loaded in a browser on a tailnet-connected device can create, edit, and delete drives, upload a CSV, and trigger archive creation. DESIGN.md §Security/Access lists application login, Secure/HttpOnly/SameSite=Strict session cookies, per-session CSRF tokens, and Origin/Host validation as required controls, and states that "network membership alone is not sufficient authorization." deploy/README.md in #5 now asserts the opposite: "Tailscale Serve is the network authorization boundary; there is deliberately no application login."
That may well be the right call for a household app — but right now the merged design doc, the PR description, and the code all disagree with each other. Please either restore the controls or amend DESIGN.md and this PR body in the same stack so the record is coherent.
2. Idempotent finalization is unreachable from the web (driving_log/web.py:363-365)
LiveDriveService.finalize implements the replay correctly: status == 'completed' plus a matching payload hash returns the existing drive. The route short-circuits before it can:
current = live.current() # only returns status in ('active','ending')
if not current or current["id"] != live_id:
raise ConflictError("live drive is no longer awaiting finalization")Reproduced — identical request_id and identical form body:
first POST /live/{id}/finalize → 303 /drives/c8aad7c3-...
replay POST /live/{id}/finalize → 409 Conflict
So a double-tap on "Save completed drive", a retried fetch after a flaky tailnet moment, or a back-and-resubmit shows a Conflict error page instead of landing on the saved drive. This is precisely the failure mode phones produce, and it contradicts "atomic idempotent finalization" in the description. The guard should fall through to live.finalize (which already distinguishes replay from conflict) rather than pre-empting it.
3. Any malformed form field returns a 500 (driving_log/web.py:190-212, :245-283, :285-293)
Only ConflictError and NotFoundError have handlers. Everything else escapes:
POST /drives started_at_local=not-a-date
→ ValueError: Invalid isoformat string: 'not-a-date' → 500
Same for a missing started_at_local (KeyError), a non-numeric version (ValueError at int(...)), an unrecognised road_type (validate_road_type), and — reachable from the real UI — an end time inside the spring-forward gap (NonexistentLocalTimeError). error.html already exists; these should route to it with a 400.
4. DST-ambiguous times are silently resolved to fold 0 (driving_log/web.py:28-31)
fold = int(fold_text) if fold_text in ("0", "1") else 0No template ever emits a start_fold/end_fold field (commit 336fd8a removed the repeated-time controls), so the fallback always wins. resolve_local receives an explicit fold, which suppresses the AmbiguousLocalTimeError that exists specifically to force a choice — every drive in the repeated 1:00–2:00 AM hour is recorded as EDT, silently, with a day/night split that may be wrong by an hour. At minimum this should be flagged as a warning on the affected drive.
5. The weekly-cap flag never reaches the user (driving_log/records.py:372-373, templates/dashboard.html:53-54)
totals() computes first_overage_drive_id — the drive that pushes a week past 10 hours, exactly what DESIGN.md asks to be "clearly flagged." It is never read: not by web.py, not by any template, not by any test. The dashboard renders only
<p>{{ format_minutes(week.overage_minutes) }} over the 10-hour advisory.</p>with no week date, so two over-cap weeks render as two identical anonymous lines, and there's no path from the warning to the drive that caused it. warnings_for has no weekly-cap code either, so the drive detail page shows nothing.
6. /drives is N+1 with a connection per row (driving_log/web.py:165-166)
enriched = [{"row": row, "warnings": records.warnings_for(row["id"])} for row in drives]Each warnings_for opens a fresh read/write connection (running PRAGMA journal_mode = WAL + synchronous = FULL), then issues three more queries — one of which is records.get, opening yet another connection. With the 74 seeded drives that's roughly 150 connections and 300 queries per page load, on the phone-first view. Fetching warnings for all drives in one query, over a read-only connection, would collapse this.
7. Client duration math ignores DST (static/app.js:101-122)
localInputMilliseconds parses the datetime-local value with Date.UTC and formatLocalInput formats with getUTC*. That deliberately avoids the browser's timezone, but it also makes the duration a pure wall-clock difference, while the server stores true elapsed time. A drive spanning a DST change shows one duration in the editor and stores another an hour apart, with no indication which won.
8. Supervisor DL number and state cannot be entered anywhere
The forms deliberately omit both fields (asserted in tests/test_web.py), and drive_update carries the current values forward, so once a drive is created with NULL there is no UI path to ever supply them. DL-4A requires both per entry; DESIGN.md says to "keep nullable until supplied and warn before DMV-facing export." No such warning exists yet. Fine to defer with the export view, but it should be tracked rather than silently absent.
Generated by Claude Code
a0cd51f to
9375aa6
Compare
336fd8a to
d187416
Compare
|
Review resolution at
Added replay, malformed-form, weekly attribution, repeated-time, resume, and cancel regressions. No merge performed. |
Phase 3 of the supervised driving log stack.
Adds the no-login, tailnet-authorized dashboard and mobile UI; manual/edit/delete/import/archive interfaces; solar themes; weekly-cap attribution; and the persistent active/ending/completed/cancelled live-drive state machine with atomic idempotent finalization and fresh-session recovery. Mutations use POST, stable request IDs, and optimistic state/version checks for page correctness and retry safety. Per the household deployment decision, there is no application authentication, Host/Origin allowlist, or CSRF layer.
Local mobile Chromium Playwright and all HTTP tests pass. WebKit remains the CI browser target.