mirror of
https://github.com/ruvnet/RuView
synced 2026-07-23 17:33:20 +00:00
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
This commit is contained in:
@@ -324,14 +324,37 @@ to trip over:
|
||||
the right time for the guard to already exist, but it is not evidence that
|
||||
the control is exercised today.
|
||||
|
||||
If browser-side admin is wanted, the coherent design is **escalate on demand**:
|
||||
keep `sensing:read` as the default, and have the RFC 6750 challenge send the
|
||||
user back through `/oauth/start` with `sensing:admin` requested, returning a
|
||||
session that holds admin AND a fresh `auth_time`. That preserves least privilege
|
||||
by default, makes the challenge meaningful, and is the only option that does not
|
||||
ask every user to consent to delete capability just to watch a stream. It needs
|
||||
a scope parameter on `/oauth/start` and a UI affordance, so it is deliberately
|
||||
not bundled into this change.
|
||||
### Decision, 2026-07-23: the browser is read-only, permanently
|
||||
|
||||
**Browser-side admin is not wanted.** `BROWSER_SIGNIN_SCOPE` stays
|
||||
`sensing:read`, and the escalate-on-demand design sketched while this was still
|
||||
open is **not** being built.
|
||||
|
||||
The reasoning holds up on its own terms rather than being a concession to
|
||||
scope: the destructive operations — training, model delete, recording delete —
|
||||
already have a home in the CLI, where `--admin` is explicit, typed by a person,
|
||||
and scoped to the session that needed it. 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. Neither is worth it
|
||||
for operations that are administrative by nature and rare by frequency.
|
||||
|
||||
What this settles:
|
||||
|
||||
- **The UI's admin controls are unreachable from a Cognitum browser session**
|
||||
and that is now intended, not a gap. `model.service.js` issuing
|
||||
`DELETE /api/v1/models/{id}` returns 401. The manual token-paste field still
|
||||
works and carries whatever authority the pasted token has, so nothing that
|
||||
worked before this change stops working.
|
||||
- **The client-side step-up redirect has been removed** from
|
||||
`ui/services/api.service.js`. It caught a challenge that can never be issued,
|
||||
and it ended in a promise that never settles — so had any other 401 ever grown
|
||||
that header, every caller would have hung forever. Dead code with a trap in it
|
||||
is worse than no code.
|
||||
- **`ADMIN_REVERIFY_SECS` stays as a server-side backstop.** It is fail-closed
|
||||
and costs nothing, so if the requested scope is ever widened the freshness
|
||||
requirement is already there rather than something to remember. It is
|
||||
documented at its definition as a backstop, so nobody mistakes its passing
|
||||
tests for evidence that it is exercised.
|
||||
|
||||
**Three options, with the tradeoff each carries:**
|
||||
|
||||
@@ -341,13 +364,22 @@ not bundled into this change.
|
||||
| **B. Server-side session store** with the refresh token, revalidated periodically | Real revocation: a disabled grant fails at the next refresh | The server now stores refresh tokens — a new and higher-value secret at rest — and refresh rotates with reuse detection, so a bug logs users out. |
|
||||
| **C. Re-verify on privileged operations only** | `sensing:admin` requires a fresh token; reads keep the long session | Best blast-radius-per-unit-cost, but needs a UI affordance for step-up auth that does not exist. |
|
||||
|
||||
**Recommendation: A now, C next.** A is a one-line change that bounds the
|
||||
window immediately; C is the design that actually matches the risk, since the
|
||||
damage a stale session can do is concentrated in the mutating routes. B is only
|
||||
worth it if RuView later needs true cross-device sign-out.
|
||||
**Chosen: A, at one hour** — `SESSION_TTL_SECS` is 3600, down from 12 hours.
|
||||
|
||||
Whichever is chosen, `SESSION_TTL_SECS` should be pinned by a test asserting the
|
||||
issued cookie's `Max-Age` matches the session's `exp`, so the two cannot drift.
|
||||
C was implemented too, and then the browser-read-only decision above made it a
|
||||
backstop rather than an active control: with no browser session holding
|
||||
`sensing:admin`, there is no privileged operation to re-verify. It is kept
|
||||
because it is fail-closed and free, not because it is doing work today.
|
||||
|
||||
B is not built. It is only worth its cost — storing refresh tokens at rest,
|
||||
against an authorization server that rotates them with reuse detection — if
|
||||
RuView later needs true cross-device sign-out. Shortening the window addresses
|
||||
the same risk for a fraction of the exposure.
|
||||
|
||||
That leaves a residual this ADR should not pretend away: **within one hour, a
|
||||
revoked Cognitum grant still reads sensing data through an existing browser
|
||||
session.** Cognitum publishes no introspection endpoint, so nothing short of B
|
||||
closes that, and one hour is the size of the hole we accepted.
|
||||
|
||||
### P3 — dropping `__Host-` costs cookie origin-integrity, not just `Secure` — **FIXED**
|
||||
|
||||
|
||||
+12
-23
@@ -86,31 +86,20 @@ export class ApiService {
|
||||
// Process response through interceptors
|
||||
const processedResponse = await this.processResponse(response, url);
|
||||
|
||||
// Step-up re-authentication (ADR-271 P2).
|
||||
// NOTE: there is deliberately no step-up re-authentication branch here.
|
||||
//
|
||||
// A browser session outlives the ~15-minute access token that created it,
|
||||
// and Cognitum publishes no introspection endpoint, so the server refuses
|
||||
// PRIVILEGED actions from a session older than a few minutes. That is a
|
||||
// 401, but it means something different from "you are not signed in" —
|
||||
// the user IS signed in, and the fix is to prove it again. The server
|
||||
// marks it with an RFC 6750 error code so the two are distinguishable.
|
||||
// An earlier revision caught the server's RFC 6750 "reauthentication
|
||||
// required" challenge and redirected to /oauth/start. That challenge can
|
||||
// never be issued to a browser: browser sign-in requests `sensing:read`
|
||||
// only and always will (see BROWSER_SIGNIN_SCOPE), so no browser session
|
||||
// holds `sensing:admin`, so the freshness gate the challenge announces is
|
||||
// never reached. Admin work goes through the CLI or a pasted bearer.
|
||||
//
|
||||
// Without this branch a stale-session delete surfaces as a generic
|
||||
// "Request failed" and the user has no way to know that signing in again
|
||||
// resolves it.
|
||||
if (processedResponse.status === 401) {
|
||||
const challenge = processedResponse.headers.get('WWW-Authenticate') || '';
|
||||
if (challenge.includes('reauthentication required')) {
|
||||
// Full-page redirect: the flow ends by setting a cookie, which an
|
||||
// XHR cannot do usefully. Returns here with a fresh auth_time and,
|
||||
// while the Cognitum session is alive, no prompt.
|
||||
window.location.href = '/oauth/start';
|
||||
// Never settles — the navigation is already underway, and resolving
|
||||
// would let the caller render an error for an operation that is
|
||||
// simply being retried after sign-in.
|
||||
return new Promise(() => {});
|
||||
}
|
||||
}
|
||||
// Removed rather than left inert, because it was not merely dead — it
|
||||
// ended in a promise that never settles. If any other 401 ever grew that
|
||||
// header, every caller awaiting this would hang forever with no error.
|
||||
// The server-side guard stays as a fail-closed backstop; the client has
|
||||
// nothing to do about a flow that does not exist.
|
||||
|
||||
// Handle errors
|
||||
if (!processedResponse.ok) {
|
||||
|
||||
@@ -76,9 +76,14 @@ pub const SESSION_TTL_SECS: i64 = 3600;
|
||||
/// rather than making every user re-authenticate hourly for a dashboard whose
|
||||
/// primary use is watching a live stream.
|
||||
///
|
||||
/// When a session is too old for an admin action the request is refused, and
|
||||
/// the UI sends the user back through `/oauth/start` — which returns with a
|
||||
/// fresh `auth_time` and, if their Cognitum session is live, no prompt.
|
||||
/// **This is a backstop, not an active control.** Browser sign-in requests
|
||||
/// `sensing:read` only and always will ([`BROWSER_SIGNIN_SCOPE`]), so no browser
|
||||
/// session holds `sensing:admin` and this branch is never reached in production.
|
||||
/// It is kept because it is cheap and fail-closed: if the requested scope is
|
||||
/// ever widened, the freshness requirement is already in place rather than
|
||||
/// something someone has to remember to add. Its tests exercise it through a
|
||||
/// crate-internal seam that mints an admin cookie the real flow does not
|
||||
/// produce — do not read them as evidence the control is exercised.
|
||||
pub const ADMIN_REVERIFY_SECS: i64 = 300;
|
||||
|
||||
/// The scope `/oauth/start` requests. Read-only, deliberately.
|
||||
@@ -96,10 +101,10 @@ pub const ADMIN_REVERIFY_SECS: i64 = 300;
|
||||
/// constant grows, which is the right ordering — but do not mistake its
|
||||
/// passing tests for evidence that the control is exercised.
|
||||
///
|
||||
/// Widening this to include `sensing:admin` would make every browser sign-in
|
||||
/// consent to delete capability just to watch a stream. The coherent way to add
|
||||
/// browser-side admin is escalate-on-demand: keep this read-only and let the
|
||||
/// RFC 6750 challenge send the user back through `/oauth/start` asking for more.
|
||||
/// **Decided 2026-07-23: browser-side admin is not wanted.** This stays
|
||||
/// read-only. Widening it would make every browser sign-in consent to delete
|
||||
/// capability just to watch a stream, and the destructive operations have a
|
||||
/// deliberate home — the CLI, where `--admin` is explicit and typed.
|
||||
pub const BROWSER_SIGNIN_SCOPE: &str = ruview_auth::scope::SENSING_READ;
|
||||
|
||||
fn now() -> i64 {
|
||||
@@ -806,6 +811,30 @@ mod tests {
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_cookie_max_age_matches_the_session_expiry() {
|
||||
// Two independent expressions of the same lifetime: the cookie's
|
||||
// Max-Age (when the browser stops sending it) and the payload's `exp`
|
||||
// (when we stop accepting it). If they drift, one silently wins —
|
||||
// a longer Max-Age means the browser keeps presenting a session we
|
||||
// reject, a shorter one means we hold authority the browser discards.
|
||||
init_secret_for_tests();
|
||||
let raw = test_cookie_value("s", "a", "sensing:read", SESSION_TTL_SECS);
|
||||
let session = from_cookie_header(&format!("{SESSION_COOKIE}={raw}")).unwrap();
|
||||
|
||||
let set_cookie = cookie(SESSION_COOKIE, &raw, SESSION_TTL_SECS, false);
|
||||
assert!(
|
||||
set_cookie.contains(&format!("Max-Age={SESSION_TTL_SECS}")),
|
||||
"{set_cookie}"
|
||||
);
|
||||
// Same lifetime, allowing a second for the clock ticking between them.
|
||||
assert!(
|
||||
(session.exp - now() - SESSION_TTL_SECS).abs() <= 1,
|
||||
"cookie Max-Age and session exp disagree: exp-now={}, Max-Age={SESSION_TTL_SECS}",
|
||||
session.exp - now()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn browser_sign_in_stays_read_only_until_someone_decides_otherwise() {
|
||||
// Pins the decision documented on BROWSER_SIGNIN_SCOPE. Widening it is
|
||||
|
||||
Reference in New Issue
Block a user