From 0447c64757bd41ec5aba75d12095d994f41a847c Mon Sep 17 00:00:00 2001 From: David O'Keeffe Date: Mon, 18 May 2026 18:35:36 +1000 Subject: [PATCH] =?UTF-8?q?fix(security):=20idempotent=20/api/configure-pa?= =?UTF-8?q?t=20=E2=80=94=20defence-in-depth?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #42 closed the impersonation vector by gating /api/configure-pat on the owner. That's the primary control: only the owner can submit a PAT. But the endpoint is still non-idempotent. Once a PAT is active, the owner (or anything driving the owner's browser — XSS, malicious tab, session-hijacking malware, or just an accidental double-submit) can POST a different PAT and the rotator will swap to it, revoking the previously-active token as a side effect. This adds an idempotency guard immediately after the owner check: if pat_rotator.token and not pat_rotator.is_token_expired: return 409 "PAT already configured. Restart the app to reconfigure." Bootstrap is single-shot by design — the rotator handles all subsequent credential refreshes automatically. There's no legitimate reason for the frontend to call configure-pat twice in one app lifetime (confirmed by grep — the only caller is the PAT-bootstrap path inside the "PAT not valid" branch of createPane()). The expired-token escape hatch is preserved: if the rotator times out because all sessions were reaped (long idle), is_token_expired returns True and the owner can re-bootstrap without an app restart. 409 Conflict (not 403 Forbidden) is the right status here: the issue isn't authorization (the owner DOES have permission) — it's that the operation conflicts with current state. Tests: - test_configure_pat_rejected_when_already_configured: rotator alive → even owner gets 409. Mocks pat_rotator.token + is_token_expired as PropertyMocks on the class (instance properties don't accept patching through mock.patch.object directly). - test_configure_pat_allowed_when_token_expired: rotator timed out → owner can still bootstrap (gets 400 "Token required" with empty body, proving they passed both gates). - All 6 TestConfigurePatAuth cases pass; 233/233 total. Co-authored-by: Isaac --- app.py | 16 ++++++++++ tests/test_auth_enforcement.py | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/app.py b/app.py index ff2ae55..18c3295 100644 --- a/app.py +++ b/app.py @@ -963,6 +963,22 @@ def configure_pat(): logger.warning(f"Rejected configure-pat from non-owner {get_request_user()} (owner: {app_owner})") return jsonify({"error": "Forbidden"}), 403 + # Idempotency / defence-in-depth: bootstrap is single-shot. Once a PAT + # is configured and the rotator is alive, refuse re-submission. Without + # this, an XSS or session-hijack vector inside the owner's browser could + # drive a swap to an attacker-controlled PAT — the owner-gate above + # would let it through because the request truly does come from the + # owner's session. The expired-token escape hatch preserves the legitimate + # re-bootstrap path (rotator timed out while idle, owner needs to refresh). + if pat_rotator.token and not pat_rotator.is_token_expired: + logger.warning( + f"Rejected configure-pat: PAT already active " + f"(user={get_request_user()}, source={request.remote_addr})" + ) + return jsonify({ + "error": "PAT already configured. Restart the app to reconfigure." + }), 409 + data = request.json token = data.get("token", "").strip() if not token: diff --git a/tests/test_auth_enforcement.py b/tests/test_auth_enforcement.py index 42030c3..f4889b6 100644 --- a/tests/test_auth_enforcement.py +++ b/tests/test_auth_enforcement.py @@ -212,6 +212,63 @@ def test_configure_pat_case_insensitive_for_owner(self): finally: app_module.app_owner = original_owner + # ---- Idempotency: defence-in-depth against XSS/session-hijack ---- + + def test_configure_pat_rejected_when_already_configured(self): + """When the rotator is alive with a fresh token, re-submission must + be refused with 409 — even from the legitimate owner. Defence-in-depth + against an attacker driving the owner's browser to swap PATs.""" + app_module = _get_app_module() + original_owner = app_module.app_owner + try: + app_module.app_owner = "owner@databricks.com" + client = _make_client(app_module) + with mock.patch.object(app_module, "_is_databricks_apps", return_value=True), \ + mock.patch.object(type(app_module.pat_rotator), "token", + new_callable=mock.PropertyMock, return_value="dapi-active"), \ + mock.patch.object(type(app_module.pat_rotator), "is_token_expired", + new_callable=mock.PropertyMock, return_value=False): + resp = client.post( + "/api/configure-pat", + json={"token": "dapi-attempt-swap"}, + headers={"X-Forwarded-Email": "owner@databricks.com"}, + ) + assert resp.status_code == 409, ( + f"Re-configure when rotator active should 409, got {resp.status_code}" + ) + body = resp.get_json() + assert "already configured" in body.get("error", "").lower(), ( + f"Error message should mention 'already configured', got: {body}" + ) + finally: + app_module.app_owner = original_owner + + def test_configure_pat_allowed_when_token_expired(self): + """Expired-token escape hatch: if the rotator timed out (long idle), + the owner must be able to re-bootstrap without restarting the app.""" + app_module = _get_app_module() + original_owner = app_module.app_owner + try: + app_module.app_owner = "owner@databricks.com" + client = _make_client(app_module) + with mock.patch.object(app_module, "_is_databricks_apps", return_value=True), \ + mock.patch.object(type(app_module.pat_rotator), "token", + new_callable=mock.PropertyMock, return_value="dapi-stale"), \ + mock.patch.object(type(app_module.pat_rotator), "is_token_expired", + new_callable=mock.PropertyMock, return_value=True): + # Empty body → should reach "Token required" (400), proving + # we passed BOTH the owner gate and the idempotency check. + resp = client.post( + "/api/configure-pat", + json={}, + headers={"X-Forwarded-Email": "owner@databricks.com"}, + ) + assert resp.status_code == 400, ( + f"Expired-PAT path should reach token-required check (400), got {resp.status_code}" + ) + finally: + app_module.app_owner = original_owner + # --------------------------------------------------------------------------- # 2. Case-insensitive email matching