Require same-origin stream commands (#1355)#1
Merged
Conversation
* Require same-origin stream commands Protect the per-session command relay from browser-originated cross-origin requests while preserving same-origin dashboard access. Co-authored-by: Muhtasham <20128202+Muhtasham@users.noreply.github.com> * Harden stream command origin checks Require command relay requests to come from loopback same-origin metadata and prevent request bodies from spoofing security headers. Co-authored-by: Muhtasham <20128202+Muhtasham@users.noreply.github.com> --------- Co-authored-by: Muhtasham <20128202+Muhtasham@users.noreply.github.com>
Reviewer's GuideEnforces that /api/command stream control requests are only accepted from same-origin loopback browser contexts, tightens origin/host validation, adjusts CORS behavior specifically for command relay, and adds unit/e2e coverage to ensure cross-origin or spoofed requests are rejected before reaching the daemon. Flow diagram for /api/command same-origin enforcementflowchart TD
A[handle_http_request for /api/command] --> B[is_same_origin_command_request]
B -->|false| C[write_json_error_response_no_cors<br/>HTTP 403 Forbidden]
C --> D[Close connection]
B -->|true| E[Read body and relay to daemon]
E --> F[command_cors_headers<br/>reflect same-origin Origin]
F --> G[Send JSON response without wildcard CORS]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
authority_host, the slice&authority[..=bracket_end + 1]uses an index relative to the stripped string, so for bracketed IPv6 with a port (e.g.[::1]:7777) it will incorrectly include the colon; you likely want to offset the index by the leading[(e.g. compute the position of]inauthorityor adjust by+1only once) so that the host portion is just the bracketed address. - The fake-daemon helpers (
spawn_fake_daemoninhttp.rstests andspawn_fake_daemon_socketplus the raw HTTP helpers ine2e_tests.rs) are very similar; consider extracting shared helpers into a common test utility to reduce duplication and keep daemon relay behavior consistent across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `authority_host`, the slice `&authority[..=bracket_end + 1]` uses an index relative to the stripped string, so for bracketed IPv6 with a port (e.g. `[::1]:7777`) it will incorrectly include the colon; you likely want to offset the index by the leading `[` (e.g. compute the position of `]` in `authority` or adjust by `+1` only once) so that the host portion is just the bracketed address.
- The fake-daemon helpers (`spawn_fake_daemon` in `http.rs` tests and `spawn_fake_daemon_socket` plus the raw HTTP helpers in `e2e_tests.rs`) are very similar; consider extracting shared helpers into a common test utility to reduce duplication and keep daemon relay behavior consistent across tests.
## Individual Comments
### Comment 1
<location path="cli/src/native/e2e_tests.rs" line_range="118-127" />
<code_context>
+ String::from_utf8(response).expect("HTTP response should be utf-8")
+}
+
+#[cfg(unix)]
+async fn spawn_fake_daemon_socket(
+ socket_dir: &std::path::Path,
</code_context>
<issue_to_address>
**question (testing):** Clarify or revisit the #[ignore] on the end-to-end same-origin stream command test
Since this test is `#[ignore]`, CI won’t detect regressions in same-origin behavior. If it’s ignored due to flakiness or environment constraints, please either document that reason in a comment or factor out a smaller, reliable e2e check (for example, just the rejection path) that can run in CI while preserving coverage of this behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+118
to
+127
| #[cfg(unix)] | ||
| async fn spawn_fake_daemon_socket( | ||
| socket_dir: &std::path::Path, | ||
| session_name: &str, | ||
| ) -> tokio::sync::oneshot::Receiver<String> { | ||
| use tokio::io::AsyncBufReadExt; | ||
|
|
||
| let socket_path = socket_dir.join(format!("{session_name}.sock")); | ||
| let _ = std::fs::remove_file(&socket_path); | ||
| let listener = |
There was a problem hiding this comment.
question (testing): Clarify or revisit the #[ignore] on the end-to-end same-origin stream command test
Since this test is #[ignore], CI won’t detect regressions in same-origin behavior. If it’s ignored due to flakiness or environment constraints, please either document that reason in a comment or factor out a smaller, reliable e2e check (for example, just the rejection path) that can run in CI while preserving coverage of this behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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
2 participants
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.
Protect the per-session command relay from browser-originated cross-origin requests while preserving same-origin dashboard access.
Require command relay requests to come from loopback same-origin metadata and prevent request bodies from spoofing security headers.
Summary by Sourcery
Enforce strict same-origin and loopback-only validation for stream command HTTP requests and adjust CORS responses accordingly to harden the command relay against cross-origin abuse.
Bug Fixes:
Enhancements:
Tests:
Summary by cubic
Enforce same-origin, loopback-only checks for stream command requests to block cross-origin abuse while keeping same-origin dashboard access working. CORS for
/api/commandnow reflects only allowed origins (no wildcard).Bug Fixes
/api/commandPOST/OPTIONS whenOriginorRefererdon’t match the loopbackHost(prevents DNS rebinding and cross-origin command injection).Origin/Referer./api/commandonly when allowed and same-origin; never*.Refactors
Written for commit 55f38f4. Summary will update on new commits. Review in cubic