Skip to content

fix(backend): restrict CORS to configurable origins and explicit methods/headers#1283

Closed
dataCenter430 wants to merge 1 commit intoeigent-ai:mainfrom
dataCenter430:fix-CORS-configuration
Closed

fix(backend): restrict CORS to configurable origins and explicit methods/headers#1283
dataCenter430 wants to merge 1 commit intoeigent-ai:mainfrom
dataCenter430:fix-CORS-configuration

Conversation

@dataCenter430
Copy link
Contributor

Related Issue

CORS configuration: missing or overly permissive CORS can allow unwanted origins.

Closes #1260

Description

Replace permissive CORS (allow_origins=[""] with allow_credentials=True and
allow_methods/allow_headers=["
"]) with a safe, configurable setup:

  • Origins: read from CORS_ORIGINS (comma-separated) in ~/.eigent/.env or env.
    In development, if unset, allow only http://localhost:5173, 127.0.0.1:5173,
    and ports 3000. In non-development, allow no origins until CORS_ORIGINS is set.
  • Methods: GET, POST, PUT, DELETE, OPTIONS only.
  • Headers: Content-Type, Authorization, x-stack-auth only.

Document CORS_ORIGINS in backend README. Ensures credentials work correctly
(no wildcard origin with credentials) and reduces risk of unwanted origins
calling the API from the browser.

Why?

  • Security: Overly permissive CORS lets any website send credentialed requests to your API from the browser; restricting to an explicit origin list and to the methods/headers the app uses reduces that risk.
  • Correctness: Using allow_origins=["*"] with allow_credentials=True is invalid per the CORS spec; browsers may ignore or mishandle it. Using a concrete list of origins with credentials is valid and predictable.
  • Operability: Development keeps working with default localhost origins; production stays strict until CORS_ORIGINS is set, and the README explains how to configure it.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Contribution Guidelines Acknowledgement

@Wendong-Fan
Copy link
Contributor

thanks @dataCenter430 for the PR, could @4pmtong and @nitpicker55555 help reviewing this?

Copy link
Collaborator

@nitpicker55555 nitpicker55555 left a comment

Choose a reason for hiding this comment

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

@Wendong-Fan @dataCenter430 @4pmtong I think this pr is redundent, eigent is desktop application, if the backend only binds to localhost, CORS restrictions provide no real security value for local APIs; hardcoding headers is also fragile.

Suggest to close

@dataCenter430
Copy link
Contributor Author

Thanks for the feedback. @nitpicker55555
This PR isn’t meant to address a remote attacker threat in the current Electron-only, localhost-bound deployment. The intent is defense-in-depth and future-proofing in case the backend is:
• Reused outside Electron
• Exposed via containerization or dev tooling
• Accessed from a browser context (intentionally or accidentally)
CORS here acts as a guardrail, not a hard security boundary. The env-var configuration allows behavior to be adjusted without code changes.
Regarding allow_headers: the explicit list is intentional to keep the API surface strict. If the team prefers flexibility, I’m happy to relax this to *.

If we’re confident the backend will never be used outside the current model, I’m okay but I wanted to add a minimal safety net for likely future scenarios.
@Wendong-Fan @nitpicker55555 Therefore, I want best practicies.
Best regards

@nitpicker55555
Copy link
Collaborator

Thanks for the feedback. @nitpicker55555 This PR isn’t meant to address a remote attacker threat in the current Electron-only, localhost-bound deployment. The intent is defense-in-depth and future-proofing in case the backend is: • Reused outside Electron • Exposed via containerization or dev tooling • Accessed from a browser context (intentionally or accidentally) CORS here acts as a guardrail, not a hard security boundary. The env-var configuration allows behavior to be adjusted without code changes. Regarding allow_headers: the explicit list is intentional to keep the API surface strict. If the team prefers flexibility, I’m happy to relax this to *.

If we’re confident the backend will never be used outside the current model, I’m okay but I wanted to add a minimal safety net for likely future scenarios. @Wendong-Fan @nitpicker55555 Therefore, I want best practicies. Best regards

"defense-in-depth" argument doesn't apply here — defense-in-depth requires an actual threat surface, and a localhost-only API has none. Future-proofing for hypothetical scenarios (containerization, external exposure) is YAGNI — if the architecture changes, CORS requirements will be different anyway, and can be added at that point with the real constraints in mind. Best practice is matching security measures to the actual threat model, not applying web patterns to desktop apps. Suggest to close.

@dataCenter430
Copy link
Contributor Author

Thanks for the thoughtful feedback, I agree that security controls should match the actual threat model, and that a localhost-only API does not present a traditional remote attack surface.
To clarify, my intent wasn’t to position this as protection against external attackers or to apply web security patterns blindly. The change is mainly about explicitly defining the expected browser origins for the local API and avoiding implicit assumptions about runtime context (dev server vs packaged Electron).
Even though the backend binds to localhost, the renderer still runs in a browser context, and CORS continues to define which browser origins are allowed to communicate with the API. In practice, this serves more as explicit contract and isolation of intended usage rather than as a primary security boundary.
That said, I understand the YAGNI concern. If the team prefers to keep the backend completely minimal given the current architecture, I’m fine closing this and reintroducing CORS only if/when the exposure model changes.
Happy to align with the team’s direction here, thx again @nitpicker55555

@nitpicker55555
Copy link
Collaborator

Thanks for the thoughtful feedback, I agree that security controls should match the actual threat model, and that a localhost-only API does not present a traditional remote attack surface. To clarify, my intent wasn’t to position this as protection against external attackers or to apply web security patterns blindly. The change is mainly about explicitly defining the expected browser origins for the local API and avoiding implicit assumptions about runtime context (dev server vs packaged Electron). Even though the backend binds to localhost, the renderer still runs in a browser context, and CORS continues to define which browser origins are allowed to communicate with the API. In practice, this serves more as explicit contract and isolation of intended usage rather than as a primary security boundary. That said, I understand the YAGNI concern. If the team prefers to keep the backend completely minimal given the current architecture, I’m fine closing this and reintroducing CORS only if/when the exposure model changes. Happy to align with the team’s direction here, thx again @nitpicker55555

Electron’s renderer accesses the backend via http://localhost:5173 in development or file:///app:// in production, and the current allow_origins=["*"] already covers all these cases.

The “problem” you’re trying to solve — restricting which origins can access the backend — does not present a realistically exploitable attack surface in a desktop application context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Architecture and UX Improvement

3 participants