diff --git a/hermes_cli/browser_credential.py b/hermes_cli/browser_credential.py index 70abaa0178..c7580598b1 100644 --- a/hermes_cli/browser_credential.py +++ b/hermes_cli/browser_credential.py @@ -47,7 +47,17 @@ def generation() -> str: def is_current(recorded: Optional[str]) -> bool: """Whether a grant that recorded generation *recorded* was made under the credential in use now.""" - return recorded is not None and hmac.compare_digest(recorded, generation()) + return current_if(recorded) is not None + + +def current_if(recorded: Optional[str]) -> Optional[str]: + """The credential in use now when *recorded* is its generation, else None. For a grant that + hands the credential over or binds a login to it: one read, so a rotation between the check + and the use cannot pass on the credential that replaced the one the grant was made under.""" + credential = current() + if recorded is None or not hmac.compare_digest(recorded, generation_of(credential)): + return None + return credential def on_rotate(name: str, revoker: Callable[[], int]) -> None: diff --git a/hermes_cli/web_routers/webapp.py b/hermes_cli/web_routers/webapp.py index 52e6311562..39dd3b8bfc 100644 --- a/hermes_cli/web_routers/webapp.py +++ b/hermes_cli/web_routers/webapp.py @@ -61,19 +61,20 @@ async def host_terminal_ws(ws: WebSocket) -> None: def _mint_handoff_ticket(state, ttl: float, *, launch: bool) -> str: - """One-use ticket that ``/webapp/window-session`` exchanges for this start's session token.""" - from hermes_cli.web_server import _SESSION_TOKEN + """One-use ticket that ``/webapp/window-session`` exchanges for this start's session token. The + store keeps ``(expiry, browser credential generation, launch)``, never the credential itself.""" + from hermes_cli import browser_credential now = time.monotonic() with _ticket_lock: tickets = getattr(state, "webapp_window_tickets", {}) tickets = {key: value for key, value in tickets.items() - if value[0] > now and value[1] == _SESSION_TOKEN} + if value[0] > now and browser_credential.is_current(value[1])} state.webapp_window_tickets = tickets if len(tickets) >= _MAX_WINDOW_TICKETS: raise HTTPException(429, "Too many pending Webapp windows; try again shortly") ticket = secrets.token_urlsafe(32) - tickets[ticket] = (now + ttl, _SESSION_TOKEN, launch) + tickets[ticket] = (now + ttl, browser_credential.generation(), launch) return ticket @@ -253,7 +254,7 @@ async def issue_audio_ticket(request: Request, socket: str = ""): @router.post("/webapp/window-session") async def redeem_window_ticket(request: Request): - from hermes_cli.web_server import _SESSION_TOKEN + from hermes_cli import browser_credential _require_private_webapp(request) # This endpoint deliberately sits outside /api: the child window or launch @@ -279,9 +280,10 @@ async def redeem_window_ticket(request: Request): entry = None elif entry is not None: del tickets[ticket] - if entry is None or entry[0] <= time.monotonic() or entry[1] != _SESSION_TOKEN: + token = browser_credential.current_if(entry[1]) if entry is not None and entry[0] > time.monotonic() else None + if token is None: raise HTTPException(403, "Webapp window link expired or was already used; open a new window") - return JSONResponse({"token": _SESSION_TOKEN}, headers={"Cache-Control": "no-store"}) + return JSONResponse({"token": token}, headers={"Cache-Control": "no-store"}) _LAUNCH_SCRIPT = r""" diff --git a/hermes_cli/webapp_sign_in.py b/hermes_cli/webapp_sign_in.py index 236ad42e28..0f18f7bb4d 100644 --- a/hermes_cli/webapp_sign_in.py +++ b/hermes_cli/webapp_sign_in.py @@ -104,11 +104,8 @@ def bind(response: Response, request: Request) -> None: def renew(response: Response, request: Request) -> None: """Keep the binding as long-lived as the session cookies a refresh just rotated, unless the credential it was checked against was replaced while the request ran.""" - # One read of the credential: binding the login to a second read could bind it to a credential - # that replaced the one it was checked against. - token = browser_credential.current() - signed_in = sign_in_of(request) - if signed_in is not None and hmac.compare_digest(signed_in, browser_credential.generation_of(token)): + token = browser_credential.current_if(sign_in_of(request)) + if token is not None: _set(response, request, token) diff --git a/tests/hermes_cli/test_browser_credential.py b/tests/hermes_cli/test_browser_credential.py index 8520876d0c..3c2c375903 100644 --- a/tests/hermes_cli/test_browser_credential.py +++ b/tests/hermes_cli/test_browser_credential.py @@ -9,13 +9,16 @@ from hermes_cli import browser_credential, web_server def test_a_generation_is_current_only_while_its_credential_is_in_use(monkeypatch): + """A grant hands over or binds to the credential only while its recorded generation is current.""" monkeypatch.setattr(web_server, "_SESSION_TOKEN", "a" * 43) recorded = browser_credential.generation() assert recorded == browser_credential.generation_of("a" * 43) != "a" * 43 assert browser_credential.is_current(recorded) assert not browser_credential.is_current(None) + assert browser_credential.current_if(recorded) == "a" * 43 monkeypatch.setattr(web_server, "_SESSION_TOKEN", "b" * 43) assert not browser_credential.is_current(recorded) + assert browser_credential.current_if(recorded) is None def test_rotate_replaces_the_credential_then_runs_every_revoker(monkeypatch): diff --git a/tests/hermes_cli/test_webapp_window_handoff.py b/tests/hermes_cli/test_webapp_window_handoff.py index a75d2b40fe..198b0d22f5 100644 --- a/tests/hermes_cli/test_webapp_window_handoff.py +++ b/tests/hermes_cli/test_webapp_window_handoff.py @@ -90,3 +90,19 @@ def test_child_handoff_is_bound_to_local_webapp_start(local_webapp, monkeypatch, headers={"Origin": "http://127.0.0.1", "X-Hermes-Window-Ticket": ticket}, follow_redirects=False) assert denied.status_code in (302, 401, 403, 404) assert server._SESSION_TOKEN not in denied.text + + +def test_pending_handoffs_keep_the_credentials_generation_not_the_credential(local_webapp): + """Window and launch tickets waiting to be spent record the browser credential's generation + (``hermes_cli.browser_credential``), never the credential itself, and still hand it over once.""" + from hermes_cli.web_routers import webapp + + server, client = local_webapp + token = server._SESSION_TOKEN + window = client.post("/api/webapp/window-ticket", headers={"X-Hermes-Session-Token": token}).json()["ticket"] + webapp.mint_launch_ticket(server.app.state) + webapp.mint_printed_launch_ticket(server.app.state) + stored = server.app.state.webapp_window_tickets + assert len(stored) == 3 and token not in repr(stored) + spent = client.post("/webapp/window-session", headers={"Origin": "http://127.0.0.1", "X-Hermes-Window-Ticket": window}) + assert spent.json()["token"] == token