-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix: headers validations for local api server #5186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: headers validations for local api server #5186
Conversation
There was a problem hiding this 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 7a5ab16 in 1 minute and 38 seconds. Click for details.
- Reviewed
211
lines of code in5
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/cmd.rs:350
- Draft comment:
The trusted_hosts parameter is now passed correctly to start_server. Consider adding a brief comment documenting the expected format and purpose of this parameter for clarity. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
2. web-app/src/containers/TrustedHostsInput.tsx:20
- Draft comment:
Consider adding client-side validation (such as regex checking) to ensure that the entered trusted hosts conform to a valid host format for better UX and to prevent invalid inputs. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
Workflow ID: wflow_7QtiPQNCclgSWTid
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -38,6 +43,16 @@ export const useLocalApiServer = create<LocalApiServerState>()( | |||
setCorsEnabled: (value) => set({ corsEnabled: value }), | |||
verboseLogs: true, | |||
setVerboseLogs: (value) => set({ verboseLogs: value }), | |||
trustedHosts: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trustedHosts: [], | |
trustedHosts: ["localhost", "127.0.0.1", "0.0.0.0"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also guide users on the format, otherwise add description for the comma separation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
7a5ab16
to
c261ce1
Compare
There was a problem hiding this 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 c261ce1 in 1 minute and 44 seconds. Click for details.
- Reviewed
216
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
1
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. web-app/src/containers/TrustedHostsInput.tsx:19
- Draft comment:
Consider adding client-side validation for the host entries to ensure they match a valid hostname pattern. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_uwSAFs7SDoPaQSCn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
c261ce1
to
1ec7ab8
Compare
There was a problem hiding this 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 1ec7ab8 in 57 seconds. Click for details.
- Reviewed
215
lines of code in5
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/cmd.rs:350
- Draft comment:
The trusted_hosts parameter is correctly integrated. Consider documenting the behavior when an empty list is provided. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
2. src-tauri/src/core/server.rs:74
- Draft comment:
Host header validation is properly added. Consider logging a warning if header conversion fails instead of silently defaulting to an empty string. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
3. src-tauri/src/core/server.rs:150
- Draft comment:
In is_valid_host, default host names are lowercased on each comparison. Precomputing the lowercased defaults could improve efficiency if the list grows. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
4. web-app/src/containers/TrustedHostsInput.tsx:19
- Draft comment:
The handleBlur function splits and trims input correctly; consider adding host format validation (e.g., a simple regex) to ensure entries are well-formed. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. web-app/src/hooks/useLocalApiServer.ts:47
- Draft comment:
The addTrustedHost method appends hosts without checking for duplicates. Although the UI deduplicates on blur, adding a safeguard at the state level could prevent redundant entries. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
Workflow ID: wflow_USUGFyxinO8g6dHu
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
1ec7ab8
to
b674a52
Compare
There was a problem hiding this 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 b674a52 in 49 seconds. Click for details.
- Reviewed
214
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
6
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/cmd.rs:349
- Draft comment:
Using unwrap_or_default for app_token is acceptable if an empty token is allowed. Otherwise consider explicit error handling. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src-tauri/src/core/server.rs:73
- Draft comment:
Consider logging rejected or missing Host headers for improved security auditing. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src-tauri/src/core/server.rs:158
- Draft comment:
Optimize repeated to_lowercase calls by caching the lowercased host string before comparisons. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. web-app/src/containers/TrustedHostsInput.tsx:39
- Draft comment:
Consider adding a placeholder example (e.g., 'localhost, 127.0.0.1') to guide users on the expected format. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. src-tauri/src/core/server.rs:185
- Draft comment:
Ensure that trusted_hosts in ProxyConfig are consistently preprocessed (e.g., trimmed/lowercased) if needed for reliable validation. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. web-app/src/routes/settings/local-api-server.tsx:44
- Draft comment:
The trustedHosts state is now passed to the backend. Verify that the UI input format (comma-separated list) aligns with backend validation expectations. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_CGthzoD9i5CTjeEr
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This pull request introduces a new feature for managing trusted hosts in the API server, enhancing security by allowing only specified hosts to access the server. It includes changes across the backend and frontend to support this functionality.
Backend Changes (Security Enhancements)
trusted_hosts
parameter to thestart_server
function andProxyConfig
struct to store and pass the list of trusted hosts. (src-tauri/src/core/cmd.rs
,src-tauri/src/core/server.rs
) [1] [2] [3]proxy_request
function to block requests from untrusted hosts. (src-tauri/src/core/server.rs
)is_valid_host
to validate host headers against default and user-configured trusted hosts. (src-tauri/src/core/server.rs
)Frontend Changes (User Interface and State Management)
TrustedHostsInput
component for users to manage trusted hosts via the UI. (web-app/src/containers/TrustedHostsInput.tsx
)useLocalApiServer
hook to include state management for trusted hosts, with methods to add, remove, and set hosts. (web-app/src/hooks/useLocalApiServer.ts
) [1] [2]TrustedHostsInput
component into the Local API Server settings page, allowing users to configure trusted hosts. (web-app/src/routes/settings/local-api-server.tsx
) [1] [2] [3] [4]Important
Enhance API server security by adding trusted host management, including backend validation and frontend configuration.
trusted_hosts
parameter tostart_server
function andProxyConfig
struct incmd.rs
andserver.rs
.proxy_request
function inserver.rs
to block untrusted hosts.is_valid_host
function inserver.rs
to validate host headers.TrustedHostsInput
component inTrustedHostsInput.tsx
for managing trusted hosts via UI.useLocalApiServer
hook inuseLocalApiServer.ts
to manage trusted hosts state.TrustedHostsInput
into settings page inlocal-api-server.tsx
for user configuration.This description was created by
for b674a52. You can customize this summary. It will automatically update as commits are pushed.