Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a centralized CORS module and applies dynamic origin validation to HTTP and WebSocket endpoints; also enhances frontend sign-in and server-config network error handling and user-facing error messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (browser / mobile)
participant Server as Backend HTTP Server
participant Socket as Socket.IO adapter
participant CORS as corsOriginCallback
participant URLH as buildCorsAllowedOrigins(URLHelper)
participant Logger as AFFiNELogger
Client->>Server: HTTP request / WebSocket handshake (Origin header)
Server->>URLH: build allowedOrigins set
Server->>CORS: corsOriginCallback(origin, allowedOrigins, onBlocked, cb)
CORS->>CORS: isCorsOriginAllowed(origin, allowedOrigins)
alt origin blocked
CORS->>Logger: onBlocked(origin) (log blocked origin)
CORS-->>Server: cb(null, false)
Server-->>Client: Respond with CORS denied / handshake rejected
else origin allowed
CORS-->>Server: cb(null, true)
Server-->>Client: Proceed with request / establish connection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 1
🧹 Nitpick comments (4)
packages/backend/server/src/base/cors.ts (1)
66-83: Permissive behavior for missingorigin— document the security rationale.When
originisundefinedornull,isCorsOriginAllowedreturnstrue(lines 70-72). This is standard practice (server-to-server calls, same-origin requests, and non-browser clients don't sendOrigin), but it means CORS alone doesn't protect against non-browser access. A brief inline comment explaining why missing origins are allowed would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/base/cors.ts` around lines 66 - 83, Add a brief inline comment inside isCorsOriginAllowed explaining why a missing origin (origin === undefined || null) returns true: note that browsers omit Origin for same-origin requests and non-browser or server-to-server clients don't send it, so CORS cannot and should not block those requests; clarify that this is intentional and that other server-side authentication/authorization must be relied on for security. Place the comment near the existing origin check in isCorsOriginAllowed and mention the related dev/testing branch (env.dev / env.testing and isDevLoopbackOrigin) to make the security rationale clear to future maintainers.packages/backend/server/src/server.ts (1)
43-59: Minor inconsistency: HTTP CORS includesexposedHeadersandmaxAgebut WebSocket CORS does not.The HTTP CORS config (here) specifies
exposedHeaders: CORS_EXPOSED_HEADERSandmaxAge: 86400, while the Socket.IO adapter (adapter.tslines 35-53) omits both. WhilemaxAgeis less relevant for WebSocket upgrades (no preflight),exposedHeadersmay matter if your WebSocket transport falls back to HTTP long-polling. Consider aligning them for consistency, or add a comment explaining why they differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/server.ts` around lines 43 - 59, The HTTP CORS setup (app.enableCors) includes exposedHeaders (CORS_EXPOSED_HEADERS) and maxAge (86400) but the Socket.IO adapter CORS config in adapter.ts omits them; update the Socket.IO CORS options used in the adapter (or the function that constructs them, e.g., the Socket.IO server/adapter initialization) to include exposedHeaders: CORS_EXPOSED_HEADERS and maxAge: 86400 for consistency (or, if you intentionally omit them, add a brief comment in adapter.ts explaining why exposedHeaders and maxAge are not needed for WebSocket/long-poll fallback).packages/frontend/core/src/components/sign-in/add-selfhosted.tsx (1)
82-104: Duplicate error → message mapping with an inconsistent fallback.The error-to-message logic is implemented twice — once for
setErrorHintand again fornotify.error— with subtly different fallbacks: thesetErrorHintelsebranch shows a generic string, while thenotify.errorelsebranch uses a dynamicerror.${userFriendlyError.name}key. Adding a new handled error type in the future requires updating both blocks in sync, and the two fallbacks already diverge.Extracting a single helper keeps the mapping in one place:
♻️ Proposed refactor
+ // Derive a single user-visible message from the error + function toErrorMessage(err: UserFriendlyError, t: ReturnType<typeof useI18n>): string { + if (err.is('TOO_MANY_REQUEST')) return t['error.TOO_MANY_REQUEST'](); + if (err.is('NETWORK_ERROR') || err.is('REQUEST_ABORTED')) return t['error.NETWORK_ERROR'](); + return t[`error.${err.name}`](err.data); + } } catch (err) { console.error(err); const userFriendlyError = UserFriendlyError.fromAny(err); setError(true); - if (userFriendlyError.is('TOO_MANY_REQUEST')) { - setErrorHint(t['error.TOO_MANY_REQUEST']()); - } else if ( - userFriendlyError.is('NETWORK_ERROR') || - userFriendlyError.is('REQUEST_ABORTED') - ) { - setErrorHint(t['error.NETWORK_ERROR']()); - } else { - setErrorHint(t['com.affine.auth.sign.add-selfhosted.error']()); - } - - notify.error({ - title: t['com.affine.auth.toast.title.failed'](), - message: - userFriendlyError.is('REQUEST_ABORTED') || - userFriendlyError.is('NETWORK_ERROR') - ? t['error.NETWORK_ERROR']() - : userFriendlyError.is('TOO_MANY_REQUEST') - ? t['error.TOO_MANY_REQUEST']() - : t[`error.${userFriendlyError.name}`](userFriendlyError.data), - }); + const message = toErrorMessage(userFriendlyError, t); + setErrorHint(message); + notify.error({ title: t['com.affine.auth.toast.title.failed'](), message }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/components/sign-in/add-selfhosted.tsx` around lines 82 - 104, The error mapping is duplicated and the fallbacks diverge; create a single helper (e.g., getErrorMessageKey or mapUserFriendlyError) that accepts a UserFriendlyError and returns the canonical translation key (and optionally the data payload) for that error (handle 'TOO_MANY_REQUEST' => 'error.TOO_MANY_REQUEST', 'NETWORK_ERROR'/'REQUEST_ABORTED' => 'error.NETWORK_ERROR', otherwise => `error.${userFriendlyError.name}`). Replace the two branches in setErrorHint and notify.error to call that helper: use t[key]() for the hint and t[key](data) for the toast message, and keep setError(true) as-is; update references to UserFriendlyError, setErrorHint, and notify.error accordingly.packages/frontend/core/src/components/sign-in/sign-in-with-password.tsx (1)
50-50:passwordErrorHintis a derived value, not state — simplify to aconstEvery code path assigns it the exact same expression:
t['com.affine.auth.password.error'](). TheuseEffect+useStatepair exists only to keep that derived value in sync witht, which is exactly what a plain variable already does. This also eliminates the confusingsetPasswordErrorHintcall in theonChangehandler (Line 156), which is visually a no-op becausepasswordErroris simultaneously cleared.♻️ Proposed simplification
- const [passwordErrorHint, setPasswordErrorHint] = useState(''); + const passwordErrorHint = t['com.affine.auth.password.error']();- useEffect(() => { - setPasswordErrorHint(t['com.affine.auth.password.error']()); - }, [t]); -if ( error.is('WRONG_SIGN_IN_CREDENTIALS') || error.is('PASSWORD_REQUIRED') ) { setPasswordError(true); - setPasswordErrorHint(t['com.affine.auth.password.error']()); } else {if (passwordError) { setPasswordError(false); - setPasswordErrorHint(t['com.affine.auth.password.error']()); }Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/components/sign-in/sign-in-with-password.tsx` at line 50, Replace the derived state passwordErrorHint with a plain constant: remove the useState hook and the related useEffect that sets setPasswordErrorHint and instead declare const passwordErrorHint = t['com.affine.auth.password.error'](); also remove any calls to setPasswordErrorHint (e.g., in the onChange handler) and keep the existing logic that clears passwordError; update references to passwordErrorHint elsewhere in this component (including the input onChange and any rendering code) to use the new constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/core/src/components/sign-in/add-selfhosted.tsx`:
- Around line 43-49: The current code unconditionally overwrites a possibly
specific error hint on every locale change and does a two-phase init; change the
useState('') to a lazy initializer that calls
t['com.affine.auth.sign.add-selfhosted.error']() so errorHint starts with the
correct localized value, remove or stop using the useEffect([t]) that blindly
calls setErrorHint on every t change, and when resetting the hint in
onBaseURLChange continue to reset it by calling
setErrorHint(t['com.affine.auth.sign.add-selfhosted.error']()) (not a blank
string) so specific errors like error.TOO_MANY_REQUEST are preserved until
explicitly changed.
---
Nitpick comments:
In `@packages/backend/server/src/base/cors.ts`:
- Around line 66-83: Add a brief inline comment inside isCorsOriginAllowed
explaining why a missing origin (origin === undefined || null) returns true:
note that browsers omit Origin for same-origin requests and non-browser or
server-to-server clients don't send it, so CORS cannot and should not block
those requests; clarify that this is intentional and that other server-side
authentication/authorization must be relied on for security. Place the comment
near the existing origin check in isCorsOriginAllowed and mention the related
dev/testing branch (env.dev / env.testing and isDevLoopbackOrigin) to make the
security rationale clear to future maintainers.
In `@packages/backend/server/src/server.ts`:
- Around line 43-59: The HTTP CORS setup (app.enableCors) includes
exposedHeaders (CORS_EXPOSED_HEADERS) and maxAge (86400) but the Socket.IO
adapter CORS config in adapter.ts omits them; update the Socket.IO CORS options
used in the adapter (or the function that constructs them, e.g., the Socket.IO
server/adapter initialization) to include exposedHeaders: CORS_EXPOSED_HEADERS
and maxAge: 86400 for consistency (or, if you intentionally omit them, add a
brief comment in adapter.ts explaining why exposedHeaders and maxAge are not
needed for WebSocket/long-poll fallback).
In `@packages/frontend/core/src/components/sign-in/add-selfhosted.tsx`:
- Around line 82-104: The error mapping is duplicated and the fallbacks diverge;
create a single helper (e.g., getErrorMessageKey or mapUserFriendlyError) that
accepts a UserFriendlyError and returns the canonical translation key (and
optionally the data payload) for that error (handle 'TOO_MANY_REQUEST' =>
'error.TOO_MANY_REQUEST', 'NETWORK_ERROR'/'REQUEST_ABORTED' =>
'error.NETWORK_ERROR', otherwise => `error.${userFriendlyError.name}`). Replace
the two branches in setErrorHint and notify.error to call that helper: use
t[key]() for the hint and t[key](data) for the toast message, and keep
setError(true) as-is; update references to UserFriendlyError, setErrorHint, and
notify.error accordingly.
In `@packages/frontend/core/src/components/sign-in/sign-in-with-password.tsx`:
- Line 50: Replace the derived state passwordErrorHint with a plain constant:
remove the useState hook and the related useEffect that sets
setPasswordErrorHint and instead declare const passwordErrorHint =
t['com.affine.auth.password.error'](); also remove any calls to
setPasswordErrorHint (e.g., in the onChange handler) and keep the existing logic
that clears passwordError; update references to passwordErrorHint elsewhere in
this component (including the input onChange and any rendering code) to use the
new constant.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
packages/backend/server/src/base/cors.tspackages/backend/server/src/base/index.tspackages/backend/server/src/base/websocket/adapter.tspackages/backend/server/src/server.tspackages/frontend/core/src/components/sign-in/add-selfhosted.tsxpackages/frontend/core/src/components/sign-in/sign-in-with-password.tsxpackages/frontend/core/src/modules/cloud/stores/server-config.ts
packages/frontend/core/src/components/sign-in/add-selfhosted.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #14502 +/- ##
==========================================
- Coverage 57.46% 57.06% -0.40%
==========================================
Files 2860 2861 +1
Lines 154583 154708 +125
Branches 23368 23246 -122
==========================================
- Hits 88829 88291 -538
- Misses 62845 63481 +636
- Partials 2909 2936 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/frontend/core/src/components/sign-in/add-selfhosted.tsx (1)
77-105: Duplicated error-type conditional logic betweenerrorHintandnotify.error.The same three-way branch (
TOO_MANY_REQUEST/NETWORK_ERROR|REQUEST_ABORTED/ fallback) is evaluated twice — once forsetErrorHint(lines 81–90) and again inline in thenotify.errorcall (lines 94–100) — with slightly different ordering. Extracting a shared helper eliminates the duplication and, as a bonus, makes it straightforward to apply the safe-fallback fix from above in one place.♻️ Proposed refactor
+ const getErrorMessage = () => { + if (userFriendlyError.is('TOO_MANY_REQUEST')) return t['error.TOO_MANY_REQUEST'](); + if (userFriendlyError.is('NETWORK_ERROR') || userFriendlyError.is('REQUEST_ABORTED')) + return t['error.NETWORK_ERROR'](); + return ( + (t[`error.${userFriendlyError.name}`] as ((...args: unknown[]) => string) | undefined)?.( + userFriendlyError.data + ) ?? t['com.affine.auth.sign.add-selfhosted.error']() + ); + }; + const errorMessage = getErrorMessage(); + setError(true); - if (userFriendlyError.is('TOO_MANY_REQUEST')) { - setErrorHint(t['error.TOO_MANY_REQUEST']()); - } else if ( - userFriendlyError.is('NETWORK_ERROR') || - userFriendlyError.is('REQUEST_ABORTED') - ) { - setErrorHint(t['error.NETWORK_ERROR']()); - } else { - setErrorHint(t['com.affine.auth.sign.add-selfhosted.error']()); - } + setErrorHint(errorMessage); notify.error({ title: t['com.affine.auth.toast.title.failed'](), - message: - userFriendlyError.is('REQUEST_ABORTED') || - userFriendlyError.is('NETWORK_ERROR') - ? t['error.NETWORK_ERROR']() - : userFriendlyError.is('TOO_MANY_REQUEST') - ? t['error.TOO_MANY_REQUEST']() - : t[`error.${userFriendlyError.name}`](userFriendlyError.data), + message: errorMessage, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/components/sign-in/add-selfhosted.tsx` around lines 77 - 105, The code duplicates the same error-type branching to setErrorHint and to build notify.error message in the catch block of the add-selfhosted component; create a single helper (e.g., mapUserFriendlyError or getErrorResponse) that accepts the UserFriendlyError instance and returns both the hint string and the toast message string (or an object with {hint, toastMessage}) and use it in the catch handler to call setErrorHint(...) and notify.error({...}) so the branching logic for UserFriendlyError.is('TOO_MANY_REQUEST'), .is('NETWORK_ERROR' / 'REQUEST_ABORTED'), and the fallback is centralized; update references to UserFriendlyError, setErrorHint, and notify.error in the catch block to use this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/frontend/core/src/components/sign-in/add-selfhosted.tsx`:
- Around line 77-105: The code duplicates the same error-type branching to
setErrorHint and to build notify.error message in the catch block of the
add-selfhosted component; create a single helper (e.g., mapUserFriendlyError or
getErrorResponse) that accepts the UserFriendlyError instance and returns both
the hint string and the toast message string (or an object with {hint,
toastMessage}) and use it in the catch handler to call setErrorHint(...) and
notify.error({...}) so the branching logic for
UserFriendlyError.is('TOO_MANY_REQUEST'), .is('NETWORK_ERROR' /
'REQUEST_ABORTED'), and the fallback is centralized; update references to
UserFriendlyError, setErrorHint, and notify.error in the catch block to use this
helper.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/frontend/core/src/components/sign-in/add-selfhosted.tsx
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.
fix #13397
fix #14011
PR Dependency Tree
This tree was auto-generated by Charcoal
Summary by CodeRabbit
New Features
Bug Fixes