mirror of
https://github.com/ruvnet/RuView
synced 2026-08-02 19:11:46 +00:00
fcbed5fbcbfa5c5f756259593a2f7eb29c2bf309
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Dragan Spiridonov
|
56327d0931 |
decide: browser sessions are read-only permanently; drop the dead step-up client
Decision: browser-side admin is not wanted. `BROWSER_SIGNIN_SCOPE` stays
`sensing:read`, and the escalate-on-demand design sketched while this was open
is not being built. Destructive operations — training, model delete, recording
delete — keep their home in the CLI, where `--admin` is explicit and typed by a
person. Routing them through a browser would mean either asking every user to
consent to delete capability in order to watch a stream, or building a second
consent flow to avoid that.
Consequences, now settled rather than open:
- The UI's admin controls are unreachable from a Cognitum browser session, and
that is intended. The manual token-paste field is unchanged and still carries
whatever authority the pasted token has, so nothing that worked before stops
working.
- REMOVED the client-side step-up redirect from ui/services/api.service.js. It
caught an RFC 6750 challenge that can never be issued to a browser, and it
ended in `return new Promise(() => {})` — so if any other 401 had ever grown
that header, every caller awaiting it would have hung forever with no error
and no timeout. Dead code with a trap in it is worse than no code.
- KEPT ADMIN_REVERIFY_SECS as a server-side backstop. Fail-closed and free, so
if the requested scope is ever widened the freshness requirement is already
in place. Documented at its definition as a backstop specifically so nobody
reads its passing tests as evidence the control is exercised — the tests
reach it through a crate-internal seam that mints an admin cookie the real
flow does not produce.
ADR-271 also stops hedging on the session TTL: chosen is A at one hour. Option B
(server-side refresh-token store) is not built, and the ADR now names the
residual instead of implying it is closed — within one hour a revoked Cognitum
grant still reads sensing data through an existing browser session. There is no
introspection endpoint, so nothing short of B closes that, and one hour is the
size of the hole we accepted.
Adds `the_cookie_max_age_matches_the_session_expiry`, which the previous ADR
revision asked for and nobody had written: Max-Age and the payload's `exp` are
two independent expressions of one lifetime, and drift means either the browser
presents a session we reject or we hold authority the browser discarded.
Verified: workspace 176 suites clean, UI 22, api.service.js parses.
Co-Authored-By: Ruflo & AQE
|
||
|
Dragan Spiridonov
|
1ed0bc57ef |
docs(adr): correct a false claim about browser scope; pin the decision in code
A cross-vendor pre-merge sweep found no merge blockers, but did surface an
error in ADR-271 that I wrote — and a coherence gap behind it.
ADR-271 P2 stated that capping the browser session at `sensing:read` was
"considered and rejected, because the dashboard genuinely performs admin
operations". That is wrong. `/oauth/start` (main.rs:9206) already requests
`SENSING_READ` and nothing else, deliberately, with a comment saying so. The
browser session is ALREADY read-only, so the breakage I claimed capping would
cause is simply the current behaviour.
Two consequences, now stated in the ADR instead of left to be discovered:
1. The UI's admin controls do not work from a browser OAuth session.
`model.service.js:136` issues DELETE /api/v1/models/{id}, which 401s. Admin
work needs the CLI (`login --admin`) or a pasted admin bearer. A gap in a new
feature, not a regression — the token-paste path is unchanged.
2. The ADMIN_REVERIFY_SECS step-up control added in the previous commit guards
a case that cannot currently arise. No browser session holds `sensing:admin`,
so the freshness branch never fires in production. Its tests pass because the
crate-internal seam mints an admin cookie the real flow never produces.
That second point is worth being blunt about: it is the same shape as several
defects this branch already fixed — correct code, green tests, unreachable call
site. The difference is that here the guard is deliberately ahead of the need
rather than mistakenly behind it, and saying which one it is matters.
So the constant is now named `BROWSER_SIGNIN_SCOPE` rather than inlined, with
the cost of widening it documented at the definition, and two tests:
`browser_sign_in_stays_read_only_until_someone_decides_otherwise` pins the
value, and `the_authorize_url_actually_carries_that_scope` proves it reaches the
wire — asserting on the constant alone would pass even if `begin` were called
with something else, which is exactly the isolation failure being guarded
against.
The ADR also records the coherent way to add browser-side admin if wanted:
escalate-on-demand via the RFC 6750 challenge, keeping least privilege by
default rather than asking every user to consent to delete capability to watch
a stream. Not bundled here — it needs a scope parameter and a UI affordance.
Sweep verdict: no merge blockers. Two other non-blocking risks it raised are
accurate and unchanged: concurrent JWKS refresh at the stale boundary is not
atomic (a duplicated idempotent GET, already documented as an accepted cost),
and the service worker's SHELL_ASSETS use root-relative paths while the UI
mounts under /ui, so offline shell precaching is incomplete — pre-existing,
unrelated to this branch.
Verified: workspace 176 suites clean.
Co-Authored-By: Ruflo & AQE
|
||
|
Dragan Spiridonov
|
89cceaf835 |
fix(auth): close P1/P2/P3 — JWKS stall, 12h session, cookie shadowing
All three deferred findings from the qe-court round. Each fix is guarded by a test confirmed to FAIL against the old behaviour. P1 — JWKS: self-inflicted stall, and a blocking fetch on a tokio worker. `fetched_at` advances only on SUCCESS, and the only rate limiter sat behind `if fresh`. So once the TTL elapsed after the last successful fetch, `fresh` was permanently false, the limiter was never consulted, and EVERY request performed its own blocking 3s-timeout fetch. A Pi that loses WAN stalled itself 300s later with no attacker present; an attacker could force the same state by flooding tokens with an unknown `kid`. Now: `last_attempt_at` is recorded BEFORE every fetch regardless of outcome, and gates the stale path too; a stale-but-present key is served rather than erroring, which is the offline tolerance this module always claimed. Measured by the new test: 26 outbound fetches before, 1 after. Kept as TWO independent limiters. Merging them looks tidy and is wrong — a routine refetch would then suppress the unknown-`kid` path for 30s and delay pickup of a key rotation inside the TTL. I made that mistake first; two existing tests caught it. The blocking call also now runs in `spawn_blocking` at the verify boundary, matching what `main.rs` already does for the token exchange, where the comment reads "the same mistake this codebase had to fix in jwks.rs". The hot verification path had never been given the same treatment. A panicked task fails closed. P2 — session lifetime, per decision: 1 hour, plus step-up. SESSION_TTL_SECS 12h -> 1h, and privileged (`sensing:admin`) actions now require the user to have authenticated within ADMIN_REVERIFY_SECS (5 min), tracked by a new `auth_time` claim. Reads ride the full session; only the routes where a stale session does damage are re-verified, so a dashboard whose main use is watching a live stream does not re-auth hourly. `auth_time` is `#[serde(default)]`, so a cookie issued before the field existed reads as 0 — infinitely stale. Such a session keeps working for reads and cannot perform privileged actions. Fail-closed and self-healing on next sign-in. The refusal carries an RFC 6750 `WWW-Authenticate` error code, because the client's correct response differs from a plain 401: the user IS signed in and needs to prove it again. `api.service.js` acts on that and redirects through `/oauth/start` — otherwise a stale-session delete surfaces as a generic "Request failed" with no hint that signing in again fixes it. P3 — cookie shadowing. `read_cookie` returned the FIRST match, and RFC 6265 §5.4 sends longer-`Path` cookies first. Cookies are not isolated by port or scheme, so any other service on the host — or a plain-HTTP MITM injecting Set-Cookie — could plant `ruview_session=<their own validly signed session>; Path=/ui`. The victim sent both, the attacker's first, and it verified because it genuinely was signed: silent session takeover, with `/oauth/status` reporting the attacker's account. The signature was doing its job throughout, which is why "it's signed" never answered this. `__Host-` would, but requires `Secure`, and RuView is routinely reached over plain HTTP on a LAN. So both credential paths now accept only when EXACTLY ONE candidate verifies. An attacker can still cause a refusal by planting a second valid cookie — a nuisance — but no longer a takeover. Planting junk changes nothing, so this does not become a trivial DoS. Tests: +1 jwks (26-vs-1 fetch amplification), +4 step-up, +4 shadowing, +1 duplicate-name reader. Mutation-verified: reverting the stale-path guard gives 26 fetches; reverting to first-match cookie reads fails `a_shadowing_cookie_cannot_silently_take_over_the_session`. Verified: workspace 176 suites clean under CI flags, ruview-auth 62+25+2 with --all-features, UI 22. ADR-271: P1/P2/P3 marked RESOLVED with the analysis retained, since it explains why each fix has the shape it does. Co-Authored-By: Ruflo & AQE |
||
|
Dragan Spiridonov
|
6ce50d5158 |
test(auth): cover browser sign-in; ADR-271/272 corrections and remediation plans
Browser sign-in was the newest security surface in this PR and had no
executable evidence behind it: browser_session.rs was 534 lines with 13 tests,
every one of which hit a private helper (sign, unsign, cookie, read_cookie,
is_live, has_scope). No test called issue, from_cookie_header, begin,
verifier_for_callback or is_configured, and no test anywhere presented a session
cookie to the gate.
+10 tests in browser_session, +6 in bearer_auth. Three mutants the adversarial
review named, each now verified dead by actually applying the mutation:
(a) delete the `state` comparison in verifier_for_callback
-> a_callback_whose_state_does_not_match_is_refused FAILED
Without it the callback accepts a code from a flow the user never
started: login CSRF, victim silently lands in the attacker's session.
(b) `session.is_live().then_some(session)` -> `Some(session)`
-> an_expired_session_cookie_does_not_authenticate FAILED
-> an_expired_browser_session_is_refused FAILED
`is_live` was already unit-tested; nothing asserted the CALLER consults
it. Same "tested in isolation, call site untested" shape as the earlier
refresh-never-invoked defect.
(c) `session.has_scope(required)` -> `true`
-> a_read_scoped_browser_session_cannot_delete_or_train FAILED
Without it any browser session could delete models and start training.
Mutant (c) initially appeared to SURVIVE. It did not — there are two
has_scope call sites and the first substitution only hit one. Mutating the
one in `session_or_unauthorized` kills the test. That accident confirmed a
separate finding: the cookie branch inside require_bearer is unreachable when
an Authorization header is present, because the OAuth step returns on both
arms. It fails closed, so it is not a hole, but "try the next credential" is
what the code reads like. Pinned by
a_bad_bearer_beats_a_good_cookie_rather_than_falling_back.
Adds two crate-internal test seams (init_secret_for_tests, test_cookie_value).
test_cookie_value signs through the same path as `issue`, so tests presenting a
cookie exercise real verification rather than a test-only bypass.
ADR-271:
- The "browser cannot obtain an OAuth token" section asserted
`grep -ril "oauth|cognitum|pkce" ui/` returns nothing. It now returns three
files, invalidated by commits in this same PR. Marked superseded, original
retained under a fold, replaced with what actually ships.
- Records the two deferred decisions with designs rather than patches: P1 the
blocking JWKS fetch on a tokio worker (whose rate limiter is bypassed on
exactly the stale path that matters, because fetched_at updates only on
success — so after the TTL every request fetches, and a Pi that loses WAN
stalls itself with no attacker present); P2 the 12-hour session from a
15-minute token, with three costed options. Capping the session to
sensing:read was considered and rejected: the dashboard genuinely issues
DELETE /api/v1/models/{id}.
- P3 records that dropping `__Host-` costs origin-integrity, not just Secure —
read_cookie takes the FIRST match and cookies are not port-scoped, so a
same-host writer can shadow a session. Forgery was never the threat that
prefix addresses.
- Documents redirect_uri's hardcoded default and the unconsumed CLI credential
as known-incomplete, per decision to leave both as-is.
ADR-272: corrected a claim that would mislead users into a 401. It stated the
Python client DOES send Authorization: Bearer on the handshake; ws.py passes no
headers at all (zero occurrences of extra_headers or Authorization), so every
published client 401s once auth is enabled. Server-side decision unchanged.
Verified: sensing-server 566 + 179 + 7 + 5 + 8 + 4 + 16 pass under CI flags,
ruview-auth 61 + 25 + 2 with --all-features, auth_wiring 7.
Co-Authored-By: Ruflo & AQE
|
||
|
Dragan Spiridonov
|
f67a880a1a |
docs: correct three doc-vs-code contradictions found by adversarial review
All three are my own drift — the exact failure this session criticised in other
repos' ADRs and then reproduced.
1. ADR-271 §2 still listed "`iss` matches the configured issuer verbatim" in the
accept-rule. That rule was removed from the code in
|
||
|
Dragan Spiridonov
|
714dae9a2c |
docs(adr): amend ADR-271 — the login flow lives in ruview-auth behind a feature
The ADR said the login flow was "not in this crate". It is now, gated behind a non-default `login` feature. The original line existed to keep the sensing server lean; a feature gate achieves that without forcing the Tauri desktop app to grow a second copy of a PKCE + rotating-refresh implementation — the kind of duplication that drifts and then disagrees about something subtle. Recording the change rather than letting the ADR quietly go stale, which is the exact failure this session hit twice in other repos' ADRs. Co-Authored-By: Ruflo & AQE |
||
|
Dragan Spiridonov
|
92cbeb0c34 |
docs(adr): ADR-271 — RuView as a Cognitum OAuth resource server
The previous commit referenced ADR-271 in five places without the ADR existing. This writes it. Records the decision and, more importantly, the direction — RuView verifies tokens users present, it does not obtain tokens to call Cognitum. Every other Cognitum OAuth integration in the org is the client side of a plane RuView does not have; the sole relevant precedent is meta-llm's oauthBearer.ts. Covers: why offline verification is a requirement rather than an optimisation (Pi-class hosts lose WAN, and no introspection endpoint exists); why the accept-rule is a port and not a design; why long-lived setup/workload credentials are refused (no database to check revocation); why scope — not `aud`, not `client_id` — is the capability boundary; and the alternatives, including the `cog_`-minting approach that was tried org-wide and found broken against production. Co-Authored-By: Ruflo & AQE |