Skip to content

Commit 273203e

Browse files
fix: resolve remaining P2/P3 issues in WS notification PR
- **Task 1 (auth-disabled installs):** Check instead of so explicit auth-disabled mode is honored even when users exist in the DB. Normalize empty/falsy owner to in note dict and comparison. - **Task 2 (closed-WS cleanup):** Replace in the send loop with so any send failure exits the stream loop to the block, which properly unsubscribes the stale queue. - **Task 3 (token collision):** Extract shared that iterates prefix-matched rows. Both and now use it, fixing the bug where only checked and could wrongly reject a valid colliding token.
1 parent fd0d5c6 commit 273203e

2 files changed

Lines changed: 48 additions & 42 deletions

File tree

routes/ws_routes.py

Lines changed: 39 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,19 @@ def _validate_api_token(raw_token: str) -> str | None:
6363
return None
6464
if len(raw_token) < 12 or len(raw_token) > 100:
6565
return None
66+
match = _find_matching_token(raw_token)
67+
if match is None:
68+
return None
69+
row, _ = match
70+
scopes = [s.strip() for s in (row.scopes or "").split(",") if s.strip()]
71+
if "notifications:read" not in scopes:
72+
return None
73+
return row.owner
74+
75+
76+
def _find_matching_token(raw_token: str) -> tuple | None:
77+
"""Find the ApiToken row matching *raw_token* among active rows with the
78+
same 8-char prefix. Returns ``(row, raw_token)`` on match or ``None``."""
6679
prefix = raw_token[:8]
6780
try:
6881
with get_db_session() as db:
@@ -73,35 +86,26 @@ def _validate_api_token(raw_token: str) -> str | None:
7386
)
7487
for row in rows:
7588
if bcrypt.checkpw(raw_token.encode(), row.token_hash.encode()):
76-
scopes = [s.strip() for s in (row.scopes or "").split(",") if s.strip()]
77-
if "notifications:read" not in scopes:
78-
return None
79-
return row.owner
89+
return row, raw_token
8090
except Exception:
81-
logger.warning("API token validation failed", exc_info=True)
91+
logger.warning("API token lookup failed", exc_info=True)
8292
return None
8393

8494

8595
def _revalidate_api_token(raw_token: str, expected_owner: str) -> bool:
86-
"""Re-validate an API token is still active and maps to *expected_owner*
87-
and still has the ``notifications:read`` scope."""
88-
prefix = raw_token[:8]
89-
try:
90-
with get_db_session() as db:
91-
row = (
92-
db.query(ApiToken)
93-
.filter(
94-
ApiToken.token_prefix == prefix,
95-
ApiToken.is_active == True,
96-
)
97-
.first()
98-
)
99-
if row and bcrypt.checkpw(raw_token.encode(), row.token_hash.encode()):
100-
scopes = [s.strip() for s in (row.scopes or "").split(",") if s.strip()]
101-
return row.owner == expected_owner and "notifications:read" in scopes
102-
except Exception:
103-
logger.warning("API token re-validation failed", exc_info=True)
104-
return False
96+
"""Re-validate an API token is still active, maps to *expected_owner*,
97+
and still has the ``notifications:read`` scope.
98+
99+
Iterates *all* prefix-matched rows (same as ``_validate_api_token``) so
100+
that a valid token whose 8-char prefix collides with another active token
101+
is not wrongly rejected on the first per-delivery check.
102+
"""
103+
match = _find_matching_token(raw_token)
104+
if match is None:
105+
return False
106+
row, _ = match
107+
scopes = [s.strip() for s in (row.scopes or "").split(",") if s.strip()]
108+
return row.owner == expected_owner and "notifications:read" in scopes
105109

106110

107111
def setup_ws_routes():
@@ -158,13 +162,14 @@ async def ws_notifications(websocket: WebSocket):
158162
owner = auth_mgr.get_username_for_token(session_id)
159163
used_credential = session_id
160164

161-
# Fallback: if auth is disabled, allow anonymous
162-
if not owner and not (auth_mgr and auth_mgr.is_configured):
163-
owner = _ANONYMOUS_OWNER
164-
165-
if owner is None:
166-
await websocket.close(code=4001)
167-
return
165+
# Fallback: if auth is explicitly disabled at the app level, allow anonymous
166+
if not owner:
167+
_auth_disabled = not getattr(websocket.app.state, "auth_enabled", True)
168+
if _auth_disabled:
169+
owner = _ANONYMOUS_OWNER
170+
else:
171+
await websocket.close(code=4001)
172+
return
168173

169174
# ── Subscribe and stream ─────────────────────────────────────────
170175
q = _subscribe(owner)
@@ -188,7 +193,9 @@ async def ws_notifications(websocket: WebSocket):
188193
except WebSocketDisconnect:
189194
raise
190195
except Exception:
191-
pass
196+
# Any send failure means the client is gone; exit the loop
197+
# so the finally block removes the subscriber queue.
198+
return
192199
except WebSocketDisconnect:
193200
pass
194201
finally:

src/task_scheduler.py

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -403,11 +403,13 @@ def add_notification(self, task_name: str, status: str, task_id: str = None, own
403403
notifications and prevent cross-tenant drain. `body` is the result
404404
text — populated when output_target='notification' so the client can
405405
show a rich browser Notification, not just a toast."""
406+
from routes.ws_routes import _ANONYMOUS_OWNER
407+
note_owner = _ANONYMOUS_OWNER if (owner is not None and not owner) else owner
406408
note = {
407409
"task_name": task_name,
408410
"status": status,
409411
"task_id": task_id,
410-
"owner": owner,
412+
"owner": note_owner,
411413
"body": (body[:500] + "…") if body and len(body) > 500 else body,
412414
"timestamp": _utcnow().isoformat() + "Z",
413415
}
@@ -417,13 +419,8 @@ def add_notification(self, task_name: str, status: str, task_id: str = None, own
417419
self._pending_notifications = self._pending_notifications[-50:]
418420
# Push to WebSocket subscribers if the channel is available
419421
try:
420-
from routes.ws_routes import push_notification, _ANONYMOUS_OWNER
421-
if owner is not None:
422-
push_notification(owner, note)
423-
else:
424-
# Auth-disabled path: push to the anonymous channel so
425-
# the WebSocket subscriber receives the notification.
426-
push_notification(_ANONYMOUS_OWNER, note)
422+
from routes.ws_routes import push_notification
423+
push_notification(note_owner, note)
427424
except Exception:
428425
pass
429426

@@ -436,7 +433,9 @@ def pop_notifications(self, owner: str = None) -> list:
436433
or when no owner filter is given — preserves backward behaviour
437434
for the legacy single-user deploy.
438435
"""
439-
if owner is None:
436+
from routes.ws_routes import _ANONYMOUS_OWNER
437+
_owner = _ANONYMOUS_OWNER if (owner is not None and not owner) else owner
438+
if _owner is None:
440439
notes = self._pending_notifications[:]
441440
self._pending_notifications.clear()
442441
return notes
@@ -445,7 +444,7 @@ def pop_notifications(self, owner: str = None) -> list:
445444
# any authenticated user once a second account existed.
446445
keep, take = [], []
447446
for n in self._pending_notifications:
448-
if n.get("owner") == owner:
447+
if n.get("owner") == _owner:
449448
take.append(n)
450449
else:
451450
keep.append(n)

0 commit comments

Comments
 (0)