security: add loopback origin check to /system/set-env#81
Merged
Conversation
Surfaced during PR #66 security review. Closes a local-LAN credential-overwrite vector — the endpoint mutates os.environ for HF_TOKEN / TRANSLATE_API_KEY and the backend binds 0.0.0.0:3900, so any LAN host or arbitrary local process could overwrite stored tokens. The vector existed pre-PR for in-memory state and was widened by PR #66's prefs.json persistence. Implementation: gate /system/set-env on request.client.host being one of ("127.0.0.1", "::1", "localhost"); raise HTTPException(403, "set-env requires loopback origin") otherwise. The check is the first statement of the handler so the existing allow-list, os.environ mutation, logger, and return-shape behaviour are preserved verbatim for the loopback path. Reads the actual TCP peer address (not any X-Forwarded-For header) so the guard is not spoofable. Tests: three regression tests in tests/test_api.py cover the 403 (default TestClient host == "testclient"), 200 (TestClient with client=("127.0.0.1", 50000)), and 400 (loopback origin still respects the ALLOWED_KEYS allow-list) paths. Notes: other POST endpoints in backend/api/routers/system.py share the same gap and are catalogued in .planning/quick/260518-ivy-add-loopback-origin-check-to-system-set-/260518-ivy-deferred-items.md for follow-up (Task #18). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Surfaced during PR #66 security review. Closes a local-LAN credential- overwrite vector that existed pre-PR for in-memory state and would have been widened by PR #66's prefs.json persistence to disk. Companion to commit e1f08a6 (the code fix). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds a loopback-origin check to the ChangesLoopback origin guard for /system/set-env
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
@.planning/quick/260518-ivy-add-loopback-origin-check-to-system-set-/260518-ivy-PLAN.md:
- Line 239: The markdown table right before the closing </threat_model> tag
isn't terminated properly; insert a blank line between the last STRIDE table row
and the </threat_model> tag so the table is closed correctly (i.e., ensure
there's an empty line after the final STRIDE table row and before
</threat_model> to satisfy markdownlint).
In
@.planning/quick/260518-ivy-add-loopback-origin-check-to-system-set-/260518-ivy-SUMMARY.md:
- Around line 72-83: Add explicit fenced-code languages (e.g., bash) to both
markdown code blocks containing the pytest commands: the block with the line 'uv
run python -m pytest tests/test_api.py -k "set_env" -x -q' and the block with
'uv run python -m pytest tests/test_router_smoke.py -x -q' should start with
```bash instead of ``` so markdownlint MD040 is satisfied.
In @.planning/STATE.md:
- Line 3: Update the footer timestamp in .planning/STATE.md so it matches the
header "Last updated" date (change the footer date from 2026-05-16 to
2026-05-18) to ensure the file uses a single canonical last-updated date; edit
the footer line containing the older date to the new date shown in the header.
In `@tests/test_api.py`:
- Around line 588-600: The test test_set_env_loopback_still_validates_allowlist
assumes DISALLOWED is unset; preserve and restore any pre-existing value by
capturing original = os.environ.get("DISALLOWED") at the start, run the test
(ensuring you import os), and in a finally block restore os.environ: if original
is None remove os.environ["DISALLOWED"] if present else set it back to original;
keep the same assertions against the TestClient POST to "/system/set-env" so the
test is isolated and won't fail due to environment leakage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92a53a4a-fcf5-4be5-89da-46dc9f08426e
📒 Files selected for processing (6)
.planning/STATE.md.planning/quick/260518-ivy-add-loopback-origin-check-to-system-set-/260518-ivy-PLAN.md.planning/quick/260518-ivy-add-loopback-origin-check-to-system-set-/260518-ivy-SUMMARY.md.planning/quick/260518-ivy-add-loopback-origin-check-to-system-set-/260518-ivy-deferred-items.mdbackend/api/routers/system.pytests/test_api.py
| | T-ivy-04 | Tampering | Local non-OmniVoice process on loopback still able to call /system/set-env | accept | Out of scope for this quick fix. OS-level UID/process auth would be required; defer to a future hardening pass. Same-machine processes already share write access to $HF_HOME/token under the local-first model. | | ||
| | T-ivy-05 | Spoofing | `X-Forwarded-For` header tricking the guard | mitigate-by-design | Guard reads `request.client.host` (the actual TCP peer), NOT any header. Documented in test rationale. | | ||
| | T-ivy-SC | Tampering | npm/pip installs in this commit | mitigate | No new package installs — `fastapi.Request` is already present in the pinned dependency set. Slopcheck not required for this commit. | | ||
| </threat_model> |
Contributor
There was a problem hiding this comment.
Fix markdown table termination before </threat_model>.
Add a blank line between the last STRIDE table row and </threat_model> so markdownlint doesn’t treat the tag as a broken table row.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 239-239: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing leading pipe
(MD055, table-pipe-style)
[warning] 239-239: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 239-239: Table column count
Expected: 5; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@.planning/quick/260518-ivy-add-loopback-origin-check-to-system-set-/260518-ivy-PLAN.md
at line 239, The markdown table right before the closing </threat_model> tag
isn't terminated properly; insert a blank line between the last STRIDE table row
and the </threat_model> tag so the table is closed correctly (i.e., ensure
there's an empty line after the final STRIDE table row and before
</threat_model> to satisfy markdownlint).
Comment on lines
+72
to
+83
| ``` | ||
| $ uv run python -m pytest tests/test_api.py -k "set_env" -x -q | ||
| ... [100%] | ||
| 3 passed, 28 deselected in 1.36s | ||
| ``` | ||
|
|
||
| Plus the wider router smoke suite as a non-regression check: | ||
|
|
||
| ``` | ||
| $ uv run python -m pytest tests/test_router_smoke.py -x -q | ||
| 23 passed, 7 warnings in 106.67s | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Both command snippets should use explicit fence languages (e.g., bash) to satisfy markdownlint MD040.
Proposed markdown fix
-```
+```bash
$ uv run python -m pytest tests/test_api.py -k "set_env" -x -q
... [100%]
3 passed, 28 deselected in 1.36s@@
- +bash
$ uv run python -m pytest tests/test_router_smoke.py -x -q
23 passed, 7 warnings in 106.67s
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| $ uv run python -m pytest tests/test_api.py -k "set_env" -x -q | |
| ... [100%] | |
| 3 passed, 28 deselected in 1.36s | |
| ``` | |
| Plus the wider router smoke suite as a non-regression check: | |
| ``` | |
| $ uv run python -m pytest tests/test_router_smoke.py -x -q | |
| 23 passed, 7 warnings in 106.67s | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@.planning/quick/260518-ivy-add-loopback-origin-check-to-system-set-/260518-ivy-SUMMARY.md
around lines 72 - 83, Add explicit fenced-code languages (e.g., bash) to both
markdown code blocks containing the pytest commands: the block with the line 'uv
run python -m pytest tests/test_api.py -k "set_env" -x -q' and the block with
'uv run python -m pytest tests/test_router_smoke.py -x -q' should start with
```bash instead of ``` so markdownlint MD040 is satisfied.
| # STATE: OmniVoice Studio v0.3.x Stabilization | ||
|
|
||
| **Last updated:** 2026-05-16 | ||
| **Last updated:** 2026-05-18 — Completed quick task 260518-ivy: loopback origin check on /system/set-env |
Contributor
There was a problem hiding this comment.
Align the footer timestamp with the new header timestamp.
Line 3 says last updated on 2026-05-18, but the footer still says 2026-05-16. Please keep one canonical date.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/STATE.md at line 3, Update the footer timestamp in
.planning/STATE.md so it matches the header "Last updated" date (change the
footer date from 2026-05-16 to 2026-05-18) to ensure the file uses a single
canonical last-updated date; edit the footer line containing the older date to
the new date shown in the header.
Comment on lines
+588
to
+600
| def test_set_env_loopback_still_validates_allowlist(): | ||
| """Even on the loopback path, keys outside the allow-list must return 400 — | ||
| the new guard must NOT bypass the existing allow-list enforcement.""" | ||
| from fastapi.testclient import TestClient | ||
| from main import app | ||
|
|
||
| loopback_client = TestClient(app, client=("127.0.0.1", 50000)) | ||
| res = loopback_client.post( | ||
| "/system/set-env", | ||
| json={"key": "DISALLOWED", "value": "x"}, | ||
| ) | ||
| assert res.status_code == 400 | ||
| assert "DISALLOWED" not in os.environ |
Contributor
There was a problem hiding this comment.
Preserve pre-existing DISALLOWED env state in the allowlist test.
This test assumes DISALLOWED is absent. If it exists in the runner env, the assertion can fail for the wrong reason. Save/restore it like the HF_TOKEN tests.
Proposed test-isolation fix
def test_set_env_loopback_still_validates_allowlist():
@@
- loopback_client = TestClient(app, client=("127.0.0.1", 50000))
- res = loopback_client.post(
- "/system/set-env",
- json={"key": "DISALLOWED", "value": "x"},
- )
- assert res.status_code == 400
- assert "DISALLOWED" not in os.environ
+ loopback_client = TestClient(app, client=("127.0.0.1", 50000))
+ original = os.environ.get("DISALLOWED")
+ os.environ.pop("DISALLOWED", None)
+ try:
+ res = loopback_client.post(
+ "/system/set-env",
+ json={"key": "DISALLOWED", "value": "x"},
+ )
+ assert res.status_code == 400
+ assert "DISALLOWED" not in os.environ
+ finally:
+ if original is None:
+ os.environ.pop("DISALLOWED", None)
+ else:
+ os.environ["DISALLOWED"] = original🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_api.py` around lines 588 - 600, The test
test_set_env_loopback_still_validates_allowlist assumes DISALLOWED is unset;
preserve and restore any pre-existing value by capturing original =
os.environ.get("DISALLOWED") at the start, run the test (ensuring you import
os), and in a finally block restore os.environ: if original is None remove
os.environ["DISALLOWED"] if present else set it back to original; keep the same
assertions against the TestClient POST to "/system/set-env" so the test is
isolated and won't fail due to environment leakage.
This was referenced May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
request.client.hostallow-list check (127.0.0.1/::1/localhost) toPOST /system/set-envso non-loopback callers receive403instead of being able to mutateos.environforHF_TOKEN/TRANSLATE_API_KEY.PERSISTENT_KEYSallow-list. This PR closes the underlying vulnerability so PR l10n(zh-CN): full Chinese localization + Windows compat + backend fixes #66's revision lands onto a clean base.request.client is Nonebranch handles ASGI middleware that strips client info (rare but documented in Starlette).Scope
backend/api/routers/system.pyRequestimport + signature + 3-line loopback guardtests/test_api.py.planning/quick/260518-ivy-.../.planning/STATE.md3 of 4 commits are pure planning/process artifacts; the one functional change is
e1f08a6. CI matrix from #71 should validate cross-platform.Test plan
uv run python -m pytest tests/test_api.py -k set_env -x -q→ 3 passeduv run python -m pytest tests/test_router_smoke.py→ 23 passedhttp://<lan-ip>:3900/system/set-envreturns403(was previously200+ state mutation)Follow-up
260518-ivy-deferred-items.mdenumerates the 5 siblingPOSTroutes insystem.pythat share the same auth/origin gap pattern. Task tracker has these queued for a broader/system/*audit (separate PR, not bundled here to keep this surface tight).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
/system/set-envendpoint with origin validation to prevent LAN-based credential overwrite attacks. Non-loopback requests now return HTTP 403 and are rejected; loopback requests continue to work as expected.Tests