fix(auth): require admin for adapter config and binding mutations#69
fix(auth): require admin for adapter config and binding mutations#69Micsi wants to merge 129 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: 9978aadc40
ℹ️ 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".
| dp_id: uuid.UUID, | ||
| body: AdapterBindingCreate, | ||
| _user: str = Depends(get_current_user), | ||
| _user: str = Depends(get_admin_user), |
There was a problem hiding this comment.
Switching this endpoint to Depends(get_admin_user) does not actually enforce admin rights for API-key clients, because get_current_user returns api_keys.name (not owner) for X-API-Key requests (obs/api/auth.py), while get_admin_user checks users.username. In practice this both breaks legitimate admin automation keys whose key name is not a username and allows a bypass if a non-admin creates a key named after an admin account, so the new admin requirement here is ineffective for API-key authentication.
Useful? React with 👍 / 👎.
| adapter_type: str, | ||
| body: ConfigPatch, | ||
| _user: str = Depends(get_current_user), | ||
| _user: str = Depends(get_admin_user), |
There was a problem hiding this comment.
The new Depends(get_admin_user) gate on adapter config writes/reads inherits the same API-key identity bug in get_current_user: X-API-Key auth returns api_keys.name, but get_admin_user verifies users.username. This means non-admin users can satisfy admin checks by creating a key named like an admin, and valid admin keys with non-username names can be rejected, so these new admin checks are unreliable for API-key clients.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 277d0909df
ℹ️ 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 not owner: | ||
| raise HTTPException( | ||
| status.HTTP_401_UNAUTHORIZED, | ||
| "Legacy API key without owner is not supported; recreate the key", | ||
| ) |
There was a problem hiding this comment.
This change makes every API key row with an empty owner fail authentication (401) because get_current_user now hard-rejects blank owners, but migration V11 (obs/db/database.py) added owner with DEFAULT '' and did not backfill existing keys. In upgraded deployments, any key created before owner was populated will stop working immediately (including non-admin automation paths), which is a production-breaking regression unless all clients rotate keys in lockstep.
Useful? React with 👍 / 👎.
| row = await db.fetchone("SELECT owner FROM api_keys WHERE key_hash=?", (key_hash,)) | ||
| if not row: | ||
| raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Invalid API key") | ||
| owner = (row["owner"] or "").strip() |
There was a problem hiding this comment.
Trimming owner before returning it changes the authenticated principal and can grant the wrong privileges: if a non-admin account exists as "admin " (allowed today because usernames are not normalized on create/update), an API key owned by that user is converted to "admin" here and then passes get_admin_user lookup as the real admin account. Even without escalation, any username that intentionally includes leading/trailing spaces cannot authenticate consistently via API key anymore.
Useful? React with 👍 / 👎.
| now = datetime.now(UTC).isoformat() | ||
| await db.execute_and_commit("UPDATE api_keys SET last_used_at=? WHERE key_hash=?", (now, key_hash)) | ||
| return row["name"] | ||
| return owner |
There was a problem hiding this comment.
Returning owner as the principal without verifying the backing user still exists lets stale keys survive account deletion and then inherit privileges if the username is reused. Concretely, delete_user removes only the users row, while create_user can later recreate the same username (including as admin), so an old key can authenticate as that new account and pass get_admin_user unexpectedly.
Useful? React with 👍 / 👎.
Micsi
added
promoted upstream
…g-injection-vulnerability [Codex] fix(toggle): sanitize SVG markup before v-html rendering
…-in-pushover [Codex] fix(security): harden pushover image_url fetch against SSRF
…nitized-svg-xss [Codex] security: reject active/scriptable SVGs on icon import (prevent stored XSS)
…ed-xss-in-svg-icons [Codex] fix(frontend): sanitize inline SVG icon content before v-html render
…-vulnerability [Codex] fix(logic): block private-network SSRF in api_client node
…icon-rendering-issue [Codex] fix(security): sanitize uploaded SVG icons to prevent stored XSS
# Conflicts: # RELEASENOTES.md # obs/logic/manager.py
# Conflicts: # frontend/src/composables/useIcons.ts
…ufenschalter-widget [Codex] fix(visu): sanitize Stufenschalter step colors before SVG rendering
…ty-in-ical-logic-node [Codex] fix(logic): harden iCal fetch path against SSRF and data exfiltration
…ability-after-token-removal [Codex] fix: WebSocket-Authentifizierung wiederherstellen
…xt-widget [Codex] fix(frontend): sanitize markdown HTML rendering in Text widget
…c-execution-vulnerability [Codex] fix: harden AST sandboxing in logic executor to prevent sandbox escapes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c70dd2bd38
ℹ️ 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 (socket && (socket.readyState === WebSocket.OPEN || socket.readyState === WebSocket.CONNECTING)) { | ||
| if (socketContextKey === nextContextKey) return | ||
| socket.close() | ||
| return |
There was a problem hiding this comment.
When an unauthenticated visu websocket is already open, later calls such as VisuViewer.load() for a different pageId or login calling ws.connect() with a JWT hit this early return and keep the old URL/session. Because the server fixes allowed_dp_ids at connection time, subscriptions for the new page or newly authenticated user are sent over the previous page-scoped connection and live updates are filtered out until a full disconnect/reload.
Useful? React with 👍 / 👎.
| if isinstance(node, ast.Attribute): | ||
| if not (isinstance(node.value, ast.Name) and node.value.id == "math" and not node.attr.startswith("_")): | ||
| raise ExecutionError("Script attribute access is not allowed") |
There was a problem hiding this comment.
This new attribute ban rejects common existing python_script nodes that read optional values with dict methods such as inputs.get('temperature', 0): that AST contains an Attribute on inputs, so the script now raises ExecutionError and the node returns no output. Since inputs is the script's provided data namespace and these methods worked before under the restricted builtins, this breaks normal automation scripts that use defensive input access.
Useful? React with 👍 / 👎.
| activeIcon, | ||
| async (icon) => { | ||
| const token = ++iconLoadToken | ||
| resetSvgBlobUrl() | ||
| if (!isSvgIcon(icon)) return | ||
| const svg = await getSvg(svgIconName(icon)) | ||
| if (token !== iconLoadToken || !svg) return | ||
| svgBlobUrl.value = URL.createObjectURL(new Blob([svg], { type: 'image/svg+xml' })) | ||
| if (!isSvgIcon(icon)) { svgContent.value = ''; return } | ||
| svgContent.value = await getSvg(svgIconName(icon)) |
There was a problem hiding this comment.
Because this watcher no longer uses the previous load token, a slower getSvg() for the old on/off icon can resolve after the state has already changed and overwrite svgContent. This shows the wrong SVG for the current toggle state until the next value change, which is visible when icons are fetched from the list endpoint or the user toggles quickly between states.
Useful? React with 👍 / 👎.
| decoded_probe = content.decode("utf-8", errors="ignore") | ||
| if _SVG_DOCTYPE_RE.search(decoded_probe): |
There was a problem hiding this comment.
Decoding the probe as UTF-8 with errors='ignore' misses non-UTF-8 XML declarations, so a UTF-16 SVG containing <!DOCTYPE ...> is not rejected by this check and ElementTree will parse and expand its internal entities before the sanitized SVG is returned. This reopens the exact DOCTYPE/entity class the sanitizer is trying to block for uploaded or imported icons encoded as UTF-16.
Useful? React with 👍 / 👎.
|
will fix with abeggled#583 upstream |
4 participants
Upstream Tracking
Motivation
Description
get_admin_userfor legacy adapter config mutations and reads (PATCH /{adapter_type}/configandGET /{adapter_type}/config) by updatingobs/api/v1/adapters.pyimports and dependencies.get_admin_userfor binding mutation endpoints (create/update/delete) inobs/api/v1/bindings.py, while keeping read-only listing (GET /{dp_id}/bindings) accessible to authenticated users.Testing
python -m compileall obs/api/v1/adapters.py obs/api/v1/bindings.pyand the compilation succeeded.get_admin_userand that read-only endpoints remain protected byget_current_user.Codex Task