Add Funnel login and repo-local runtime

[ahernsean]

Summary

• require a Sean or Jen password-backed session before accessing driving-log data
• assign each new drive to a validated Sean-or-Jen supervisor selected in the form, defaulting to the signed-in parent
• harden login redirects, password rotation, scrypt cost, and failed-login backoff
• add an explicit Dashboard link and move Archives under Data
• keep credentials, SQLite state, archives, and restore requests in git-ignored driving-log-runtime/
• safely copy an existing legacy runtime into that directory on first install

Setup

Run ./driving-log install --public-scheme https. It discovers the machine Tailscale DNS name, creates the runtime directory, migrates legacy state if present, and installs/reloads the user units. Then run ./driving-log --set-password Sean and ./driving-log --set-password Jen; each command prompts twice and writes only a salted password hash to the protected runtime environment. Finally run ./driving-log start and tailscale funnel --bg --https=8443 http://127.0.0.1:8766.

Validation

• Ruff, format check, and mypy passed
• focused local auth/web tests passed
• GitHub CI previously passed all checks on the selector commit; this hardening commit has a fresh run queued

[ahernsean]

Overview

This PR replaces the previous "Tailscale Serve is the auth boundary" model with an application-level login (two hardcoded accounts, scrypt-hashed passwords, HMAC-signed session cookies) so the service can be safely exposed via Tailscale Funnel (i.e. the public internet, not just the tailnet). It also relocates all mutable runtime state from ~/.config/driving-log / ~/.local/state/driving-log into a git-ignored driving-log-runtime/ directory inside the repo checkout, with a one-time safe migration of any pre-existing legacy state, plus small nav/UX cleanups (Dashboard link, Archives moved under Data).

Overall this is a solid, well-tested change (98 tests, new coverage for auth, migration, and CLI password-setting). The findings below are mostly about hardening the login now that this app is reachable from the open internet rather than just the tailnet — worth a look before you flip Funnel on for real.

Findings

1. Open redirect via backslash bypass in safe_next() (Medium)

driving_log/web.py:312

def safe_next(value: str | None) -> str:
    return value if value and value.startswith("/") and not value.startswith("//") else "/"

This only blocks the //host protocol-relative form. Browsers that implement the WHATWG URL spec treat \ as equivalent to / for special schemes (http/https), so a value like /\evil.example.com (or /\/evil.example.com) passes this check (starts with /, not //) but is resolved by the browser as //evil.example.com — i.e. a redirect to an attacker-controlled host. Reachable via GET /login?next=/\evil.example.com and via the hidden next field on POST /login (web.py:331), so a successful sign-in can be steered to an external page right after entering the password — useful for a phishing follow-up. No test currently covers this.

Fix: reject any next value containing a backslash, or better, parse with urllib.parse.urlsplit and require netloc == "" in addition to the existing checks.

2. Rotating a password doesn't invalidate existing sessions (Medium)

driving_log/auth.py:31-58, driving_log/cli.py:324-352
make_session/session_user only check the HMAC signature and exp (30-day lifetime) — the session payload has no binding to the current password hash. So ./driving-log --set-password Sean (documented in deploy/README.md as the credential-rotation path) does not invalidate any session cookie issued before the change. If the motivation for rotating a password is a suspected leak, the old cookie keeps working for up to 30 days regardless.

Fix: mix a short fingerprint of the current password hash (or a persisted session epoch/nonce) into the signed payload, and check it in session_user.

3. No login rate limiting or lockout (Medium, now that this is internet-facing)

driving_log/web.py:326-345
POST /login has no throttling, delay, or lockout after repeated failures. Under the old "Tailscale Serve" model this didn't matter (network-level trust boundary), but Funnel puts this endpoint on the open internet. scrypt(n=2**14) is cheap enough (~tens of ms) that an unthrottled remote attacker can attempt a meaningful number of guesses per second against two well-known account names (sean/jen, visible right on the login page).

Fix: at minimum, add a per-IP/per-account exponential backoff or a fixed delay + counter, or rely on a reverse-proxy rate limit if one is easy to add in front of Funnel.

4. scrypt cost parameters are on the low side for an internet-facing login (Low)

driving_log/auth.py:52 — n=2**14, r=8, p=1. OWASP's current baseline recommendation for interactive login is n=2**17 (with r=8, p=1), assuming your hardware can absorb the latency. Worth bumping now that the login boundary is doing more work than before.

5. PR description slightly overstates the supervisor-assignment change (Informational)

The PR body says drives are assigned "to the signed-in supervising driver, regardless of submitted form values," but selected_supervisor() (web.py:306-310) still honors the submitted supervisor_name as long as it's one of the two valid accounts — a signed-in Jen can still save a drive supervised by "Sean Ahern" (and the new test at tests/test_web.py:~1123 exercises exactly that). The real guarantee added here is "must be a valid account," not "must be the signed-in user." That's a reasonable design for a two-parent household, just worth aligning the PR description with what's implemented so nobody's surprised later.

Recommendations

• Fix #1 (open redirect) before enabling Funnel publicly — it's cheap to fix and this is the one genuinely internet-facing surface being added.
• Consider #2 and #3 together: if you want --set-password to double as an incident-response "kick everyone out" tool, #2 needs the epoch/nonce fix; if not, at least document that rotation doesn't revoke existing sessions.
• #4 is a nice-to-have; not urgent for a two-user personal app, but cheap to bump.
• Nothing else stood out — the runtime-relocation/migration logic (operations.py:_migrate_legacy_runtime) is careful (uses sqlite3 .backup() rather than a raw file copy, leaves the legacy copy in place as a fallback, only migrates once), and the new tests (test_operations.py, test_web.py) cover the login/redirect/supervisor-validation/migration paths well.

Suggested testing methods

Automated (add to the existing suite):

• A test hitting GET /login?next=/\evil.example.com (and POST /login with the same in the next field) asserting the resulting Location header has no host component — this is the regression test that's currently missing for finding #1.
• A test that signs in, captures the session cookie, calls --set-password for that account, and asserts the old cookie is rejected — regression test for #2 once fixed.

Manual verification before enabling Funnel:

1. Run ./driving-log install --public-scheme https, then ./driving-log --set-password Sean and --set-password Jen, then ./driving-log start — confirm the service refuses to start if a password/session-secret env var is missing (kill one and restart to check validate_authentication() actually fails closed).
2. From a private/incognito browser, hit the Funnel URL directly and confirm you're redirected to /login with no data visible (including JSON API paths like /live/state — should get a 401 with Accept: application/json, not silently return data).
3. Log in as each account, confirm current_user/"Signed in as …" renders correctly, confirm you can choose either supervising driver from the dropdown when saving/starting a drive, and confirm sign-out clears the cookie and re-locks the app.
4. Try the open-redirect payload manually: visit https://<funnel-host>/login?next=/%5Cevil.example (URL-encoded backslash), log in, and watch where the browser actually navigates.
5. Confirm the legacy-runtime migration is a no-op on a second install run (no duplicate copy, no error) if you have pre-existing state under ~/.local/state/driving-log.
6. Re-run the documented validation gaps from the PR body — Chromium-only Playwright pass and the coverage-gate command that didn't complete locally — in CI or another host, since those weren't fully exercised.

[ahernsean]

Addressed the review findings in commit 43413b4:

• safe_next now rejects backslashes and requires a hostless, schemeless URL; regression coverage added
• sessions include a fingerprint of the current password hash, so --set-password invalidates prior cookies
• failed logins now use per-client/account exponential backoff, capped at 8 seconds
• scrypt is now n=2^17, r=8, p=1 with an explicit 256 MiB maxmem allowance for this host
• the PR description now accurately says supervisor selection is validated to Sean or Jen and defaults to the signed-in parent; selecting the other parent for an offline drive remains intentional

[ahernsean]

Re-review — commit 43413b4 ("Harden public login sessions and redirects")

Nice turnaround. This commit addresses all four technical findings from the previous review pass, and the PR description was updated to match the actual supervisor-selection behavior (finding #5). Verified each fix by reading the diff and re-checking against the concrete PoCs from the earlier review:

• ✅ Open redirect (#1) — safe_next() (web.py:317-322) now rejects backslashes and parses with urlsplit, requiring empty netloc/scheme. tests/test_web.py adds a regression test posting next=/\evil.example.com and asserting the redirect target is /. Matches the exact payload I suggested manually testing.
• ✅ Session invalidation on password rotation (#2) — make_session/session_user now bind a credential fingerprint (truncated SHA-256 of the current password hash) into the signed payload (auth.py:35-38, 63-71), so a token signed under the old hash is rejected once --set-password changes it. tests/test_foundation.py adds a direct regression test for this.
• ✅ scrypt cost (#4) — bumped n from 2**14 to 2**17, with an explicit maxmem=256 * 1024 * 1024 on both password_hash and verify_password (auth.py:14-15, 90-93, 105). The maxmem bump is necessary and correct here — Python's hashlib.scrypt default maxmem (32 MiB) is too small for n=2**17, r=8 (needs roughly 128 MiB), so without it this would raise rather than just being slow.
• ✅ Supervisor-assignment description (#5) — PR body now says "assign each new drive to a validated Sean-or-Jen supervisor selected in the form, defaulting to the signed-in parent," which matches the implementation.

New finding from this commit

Failed-login backoff has a check-then-act race under concurrent requests (Medium)

driving_log/web.py:340-345

key = (client_host, account)
failures, _ = login_failures.get(key, (0, 0.0))
await asyncio.sleep(min(2 ** min(failures, 3), 8))
login_failures[key] = (failures + 1, time.monotonic())

This read-sleep-write isn't atomic with respect to other concurrent requests for the same key. If an attacker fires a burst of, say, 20 concurrent POST /login requests for the same account, all 20 coroutines call .get() and observe failures=0 before any of them reach the await (event-loop scheduling runs the synchronous prefix of each handler first). All 20 then sleep ~1 second in parallel, and all 20 write back (0 + 1, now) — the writes clobber each other, so login_failures[key] ends up at 1, not 20. Repeating this burst pattern indefinitely keeps the observed failures count pinned near 1, so the exponential backoff (meant to reach 8s after a few failures) never actually escalates — the attacker gets roughly 20 password guesses per second forever, regardless of burst size. Sequential/scripted single-connection brute forcing is correctly slowed by this change; concurrent/parallelized brute forcing is not.

Related, smaller point: since this app is deployed behind tailscale funnel --bg --https=8443 http://127.0.0.1:8766 (per deploy/README.md), Funnel terminates TLS and proxies to the app over loopback, so request.client.host (web.py:338) will be 127.0.0.1 for every request regardless of the real internet client's source IP — there's no X-Forwarded-For (or similar) handling in this codebase. That's not a bug by itself (it still gives you a real, if coarse, per-account throttle key), but it does mean the (client_host, account) tuple only ever has 2 distinct values in production, which makes the race above easier to hit (any burst against either account collides on the same dict entry) and means a single global slider is effectively all you get — worth knowing so the "per-client" framing in the code doesn't create false confidence.

Suggested fix: serialize increments with an asyncio.Lock per key (or a single lock, given there are only ~2 keys), or better, do the increment before the delay instead of after, e.g.:

key = (client_host, account)
failures, _ = login_failures.get(key, (0, 0.0))
login_failures[key] = (failures + 1, time.monotonic())
await asyncio.sleep(min(2 ** min(failures, 3), 8))

This alone doesn't fully close the race (two coroutines can still both read failures=0 before either writes), but combined with a lock around the read-modify-write it would. Given this is a personal 2-account app behind Funnel rather than a high-value target, a simpler fix that's "good enough" is a single asyncio.Lock guarding the whole read-modify-write block — the throttle only needs to be correct, not fast.

Testing methods for the fix

• Add a test that fires several concurrent POST /login failures for the same account (e.g. asyncio.gather over multiple client requests) and asserts login_failures (or observable total delay) reflects the true attempt count, not just the last write.
• Manually: script N parallel curl/httpx POSTs with a wrong password against the same account and time how long it takes to exhaust, say, 50 attempts — compare against what the backoff formula predicts for sequential attempts.

Everything else from the previous pass looks good — no new issues spotted elsewhere in this commit (imports are all used, _credential_fingerprint's 16-hex-char truncation is fine since it's just a freshness tag under an already-HMAC'd payload, not a secret on its own). CI (quality-unit, security, rocky-sqlite, browser-webkit, CI tests) is green on this commit.

[ahernsean]

Re-review — commit d7baa2f ("Serialize login failure backoff")

Confirmed fix for the last open finding.

• ✅ Check-then-act race in the login backoff (from previous review) — the read-modify-write of login_failures[key] is now wrapped in login_failures_lock and the increment happens before the await asyncio.sleep(...), not after (web.py:343-346), which is exactly the pattern I'd suggested. The lock is only held across the two dict operations, not across the sleep itself, so legitimate concurrent requests still don't block on each other's delay — just on the tiny bookkeeping. Correct and minimal.
• ✅ Test coverage — test_concurrent_failed_logins_advance_backoff_atomically (tests/test_web.py) fires 4 concurrent failed logins via asyncio.gather, patches asyncio.sleep to record delays without waiting, and asserts the delays come out as [1, 2, 4, 8] — i.e. each concurrent attempt gets a distinct, correctly-escalating count instead of the four attempts clobbering each other into the same bucket. This is precisely the regression test that was missing, and it demonstrates the race is closed (before this fix, this test would have shown four 1s instead).

I ran the auth/web/login test files locally (tests/test_foundation.py, tests/test_web.py) — all 35 tests pass, including the new concurrency test and the open-redirect regression test from the prior commit. CI (quality-unit, security) is green on this commit; browser-webkit and rocky-sqlite were still running at review time.

Status

All findings from this review thread (open redirect, session invalidation on password rotation, scrypt cost, PR description accuracy, and the login-backoff race) are now addressed and covered by tests. Nothing further to flag — this looks ready to merge from a security/correctness standpoint, pending the two in-flight CI jobs finishing green.