Skip to content

Conversation

samhvw8
Copy link
Contributor

@samhvw8 samhvw8 commented Jun 10, 2025

Describe Your Changes

  • Fix CORS preflight validation to use Host header for target validation
  • Use Origin header correctly for CORS response headers
  • Improve host validation to support both host:port and host-only formats
  • Filter upstream CORS headers to prevent duplicate Access-Control-Allow-Origin
  • Add CORS headers to all error responses for consistent behavior
  • Fix host matching logic to handle trusted hosts with and without ports
  • Ensure single Access-Control-Allow-Origin header per response

This resolves CORS preflight failures that were blocking cross-origin requests to the local API server, enabling proper network access from web applications and external tools.

Fixes Issues

  • Closes #
  • Closes #

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

Important

Enhance CORS handling in proxy_request() by improving validation, response consistency, and host matching logic.

  • CORS Handling:
    • Fix CORS preflight validation to use Host header for target validation in proxy_request().
    • Use Origin header correctly for CORS response headers in proxy_request().
    • Ensure single Access-Control-Allow-Origin header per response in proxy_request().
    • Add CORS headers to all error responses for consistent behavior in proxy_request().
    • Filter upstream CORS headers to prevent duplicate Access-Control-Allow-Origin in proxy_request().
  • Host Validation:
    • Improve host validation to support both host:port and host-only formats in is_valid_host().
    • Fix host matching logic to handle trusted hosts with and without ports in is_valid_host().
  • Misc:
    • Add helper functions is_cors_header() and add_cors_headers_with_host_and_origin() to manage CORS headers.

This description was created by Ellipsis for d0044a1. You can customize this summary. It will automatically update as commits are pushed.

- Fix CORS preflight validation to use Host header for target validation
- Use Origin header correctly for CORS response headers
- Improve host validation to support both host:port and host-only formats
- Filter upstream CORS headers to prevent duplicate Access-Control-Allow-Origin
- Add CORS headers to all error responses for consistent behavior
- Fix host matching logic to handle trusted hosts with and without ports
- Ensure single Access-Control-Allow-Origin header per response

This resolves CORS preflight failures that were blocking cross-origin
requests to the local API server, enabling proper network access from
web applications and external tools.

Fixes: OPTIONS requests being rejected due to incorrect host validation
Resolves: "access control allow origin cannot contain more than one origin" error
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to 914d9b8 in 2 minutes and 30 seconds. Click for details.
  • Reviewed 427 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/src/core/server.rs:422
  • Draft comment:
    Echoing the Origin header without strict host validation could be risky. Consider allowing the Origin header only when the host is trusted to avoid potential CORS vulnerabilities.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. src-tauri/src/core/server.rs:153
  • Draft comment:
    The allowed headers for CORS are defined in multiple places. Consider consolidating them into a shared constant to avoid inconsistencies.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
3. src-tauri/src/core/server.rs:265
  • Draft comment:
    Error messages like 'Invalid host header 1' and 'Missing host header 2' are inconsistent. Consider standardizing these messages for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
4. src-tauri/src/core/server.rs:73
  • Draft comment:
    The detailed debug logging of request headers (including Host and Origin) might expose sensitive information. Revisit logging levels in production environments.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
5. src-tauri/src/core/server.rs:166
  • Draft comment:
    There is a duplicate entry for 'user-agent' in the allowed headers array. Please remove the redundant entry.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_oLmR4pBPRGaCgkDc

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

"keep-alive",
"origin",
"user-agent",
"user-agent",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate 'user-agent' in allowed_headers array. Consider removing the duplicate.

Suggested change
"user-agent",

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed ad4015c in 2 minutes and 49 seconds. Click for details.
  • Reviewed 112 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/src/core/server.rs:121
  • Draft comment:
    Added root path bypass for host validation in CORS preflight. Ensure the security rationale is documented.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that the security rationale is documented. This falls under the category of asking the author to ensure something is done, which is against the rules. The comment does not provide a specific code suggestion or ask for a specific test to be written.
2. src-tauri/src/core/server.rs:135
  • Draft comment:
    Improved logging now includes the request path when warning about untrusted hosts.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. src-tauri/src/core/server.rs:163
  • Draft comment:
    Removed duplicate 'user-agent' header in the allowed headers list.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. src-tauri/src/core/server.rs:251
  • Draft comment:
    Host header verification now bypasses for root paths; confirm this behavior aligns with security requirements.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. src-tauri/src/core/server.rs:306
  • Draft comment:
    Authorization check is skipped for root paths; verify that bypassing auth here is acceptable.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_NYpFsbnLUq7NLQjZ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed d0044a1 in 3 minutes and 12 seconds. Click for details.
  • Reviewed 65 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/src/core/server.rs:125
  • Draft comment:
    Consider extracting the whitelisted paths (["/", "/openapi.json", "/favicon.ico"]) into a shared constant to avoid duplication and potential divergence across OPTIONS and non-OPTIONS branches.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. src-tauri/src/core/server.rs:264
  • Draft comment:
    The whitelisting check uses req.uri().path() in the OPTIONS branch but applies get_destination_path() in the main branch. Ensure this difference is intentional since it may lead to inconsistent bypass behavior when a prefix is used.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_fdr3JqdojXKS52Dh

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@samhvw8 samhvw8 requested a review from louis-menlo June 11, 2025 02:38
@samhvw8 samhvw8 merged commit eef37de into release/v0.5.18 Jun 11, 2025
18 checks passed
@samhvw8 samhvw8 deleted the fix/local-api-access-network branch June 11, 2025 02:44
@github-project-automation github-project-automation bot moved this to QA in Jan Jun 11, 2025
@github-actions github-actions bot added this to the v0.5.18 milestone Jun 11, 2025
@david-menloai david-menloai moved this from QA to Done in Jan Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants