fix(auth): require authentication for datapoint value reads and websocket connections#59
fix(auth): require authentication for datapoint value reads and websocket connections#59Micsi wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40a5f7e520
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async def get_value( | ||
| dp_id: uuid.UUID, | ||
| _user: str | None = Depends(optional_current_user), | ||
| _user: str = Depends(get_current_user), |
There was a problem hiding this comment.
Requiring get_current_user here makes GET /api/v1/datapoints/{id}/value reject all non-JWT/API-key requests, but the viewer explicitly supports public/protected page access without JWT and bootstraps widget values via datapoints.getValue(..., silent401=true) (frontend/src/views/VisuViewer.vue, frontend/src/stores/datapoints.ts). After this change, those pages return 401 for initial value fetches even when page access is otherwise valid, so widgets render without current values for anonymous/PIN-only sessions.
Useful? React with 👍 / 👎.
| else: | ||
| resolved_token = None | ||
| await ws.close(code=4001, reason="Authentication required") | ||
| return |
There was a problem hiding this comment.
Closing the socket when no token is provided breaks live updates for public/protected viewer pages because the browser client only appends ?token= when a JWT exists (frontend/src/composables/useWebSocket.ts), while VisuViewer still calls ws.connect() for accessible pages without requiring JWT (frontend/src/views/VisuViewer.vue). In that scenario the server now rejects every handshake, so non-JWT users lose realtime datapoint/ringbuffer updates and the client repeatedly reconnects.
Useful? React with 👍 / 👎.
|
Konflikte mit Umgesetzte Punkte aus den Kommentaren:
Gates im Worktree gelaufen:
Zusätzlich ergänzt:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5aa31cedf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ctx.pageId) params.set('page_id', ctx.pageId) | ||
| if (ctx.sessionToken) params.set('session_token', ctx.sessionToken) | ||
| const query = params.toString() | ||
| return `${proto}://${location.host}/api/v1/ws${query ? `?${query}` : ''}` |
There was a problem hiding this comment.
By adding page_id/session_token to the WebSocket URL, the backend now binds anonymous connections to a page-scoped datapoint allowlist at handshake time (obs/api/v1/websocket.py). However, this client still treats connect() as idempotent for any existing OPEN/CONNECTING socket, so navigating from one public/protected page to another reuses the old connection instead of opening a new URL with the new page_id; the server then filters new subscriptions and live updates stop for the second page (widgets only keep their initial HTTP snapshot until a full reconnect).
Useful? React with 👍 / 👎.
| for value in widget.config.values(): | ||
| if isinstance(value, str): | ||
| ids.add(value) |
There was a problem hiding this comment.
The new anonymous page-scoping logic only adds top-level string values from widget.config, so datapoint IDs stored in nested structures are silently excluded from allowed_dp_ids; examples already used by the frontend include Chart (series[].dp_id), HorizontalBar (bars[].dp_id), and Info (extra_datapoints[].id). For public/protected viewer sessions this causes subscribe to drop those IDs, so those widgets no longer receive live updates even though they are part of the page configuration.
Useful? React with 👍 / 👎.
| try: | ||
| decode_token(resolved_token) | ||
| user = decode_token(resolved_token) | ||
| except Exception: | ||
| await ws.close(code=4001, reason="Invalid token") | ||
| return |
There was a problem hiding this comment.
The new handshake flow closes the socket immediately on any invalid token query parameter before evaluating page_id/session_token access, which breaks public/protected viewer sessions whenever a stale JWT remains in browser storage. In this repo the viewer URL builder always appends token when getJwt() is non-empty, so an expired JWT can now block realtime updates even though the same user still has valid page-scoped access (and HTTP endpoints treat invalid optional auth as anonymous).
Useful? React with 👍 / 👎.
| const token = localStorage.getItem('access_token') | ||
| const url = `${proto}://${window.location.host}/api/v1/ws${token ? `?token=${encodeURIComponent(token)}` : ''}` |
There was a problem hiding this comment.
This change reintroduces JWTs in the WebSocket URL (?token=...) for the admin GUI, which exposes bearer tokens to URL logging surfaces (browser history/session restore, reverse-proxy access logs, monitoring tooling) and regresses the repo’s prior hardening goal of avoiding token-in-query transport. Because these are long-lived admin tokens from localStorage, leakage here is sufficient for account takeover until token expiry/rotation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 341e21ee37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if widget.datapoint_id: | ||
| ids.add(widget.datapoint_id) | ||
| if widget.status_datapoint_id: | ||
| ids.add(widget.status_datapoint_id) | ||
| _collect_strings(widget.config, ids) |
There was a problem hiding this comment.
Expand the page-scoped allowlist to include datapoints referenced by WidgetRef widgets; right now it only adds datapoints that are directly present on the current page config. WidgetRef resolves its target widget at runtime (frontend/src/widgets/WidgetRef/Widget.vue) and then subscribes to that widget’s datapoint IDs, but those IDs are not in allowed_dp_ids, so WebSocketManager.subscribe silently drops them for anonymous page-scoped connections. This regresses live updates for referenced widgets on public/protected viewer pages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5b90de035
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| source_page = await _load_page_config(source_page_id) | ||
| if source_page is None: |
There was a problem hiding this comment.
When building allowed_dp_ids for anonymous page-scoped sockets, the new widget_ref recursion loads the referenced page config unconditionally and includes its datapoints (obs/api/v1/websocket.py), but it never applies the source page access checks that GET /visu/widget-ref/{page_id} enforces (obs/api/v1/visu.py lines 539-556). If a public page contains a widget_ref pointing to a protected/user page, anonymous clients can still receive live ringbuffer_entry updates for that restricted page’s datapoints because those IDs are now in the WS allowlist. This is an authorization bypass introduced by the recursion path.
Useful? React with 👍 / 👎.
1 participant
Upstream Tracking
Motivation
Description
get_current_userinobs/api/v1/datapoints.pyforGET /{dp_id}/value.obs/api/v1/websocket.pyby rejecting connections without a token with4001 Authentication requiredand continuing to reject invalid tokens with4001 Invalid token.obs/api/v1/datapoints.pyandobs/api/v1/websocket.pyand are intentionally minimal to restore prior access controls.Testing
pytest -q obs/api/v1/test_datapoints.py obs/api/v1/test_websocket.pywhich failed because those targeted test files are not present in this repo layout (no tests collected).pytest -qwhich aborted collection due to a missing test dependency (ModuleNotFoundError: No module named 'pytest_asyncio'), so full test suite execution could not be completed in this environment.Codex Task