Open
Conversation
📝 WalkthroughWalkthroughAdds a first-login enforcement flow: the server detects the factory-reset admin token and returns Changes
Sequence DiagramsequenceDiagram
participant User as User
participant App as Web App
participant API as Auth API
participant DB as Database
participant Config as Runtime Config
User->>App: Log in with token
App->>API: GET /api/settings/auth/info
API->>Config: Check current auth token vs factory default
API-->>App: { requirePasswordChange: true }
App->>User: Render ForceChangeTokenPage
User->>App: Submit oldToken, newToken
App->>App: Trim & validate (>=12, not default)
App->>API: POST /api/settings/auth/change
API->>API: Trim & validate inputs
API->>Config: Verify oldToken matches runtime token
API->>DB: Persist trimmed new token in settings
API->>Config: Update runtime authToken
API-->>App: 200 { success: true, requirePasswordChange: false }
App->>App: Clear session / redirect to login
User->>App: Log in with new token
App->>API: GET /api/settings/auth/info
API->>Config: Confirm default token no longer in use
API-->>App: { requirePasswordChange: false }
App->>User: Show normal app UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/web/App.tsx (1)
772-785:⚠️ Potential issue | 🟠 MajorKeep the user-menu trigger as a real button.
Replacing the trigger with a
<div>on Lines 772-785 removes keyboard focus and Enter/Space activation, so keyboard users can no longer open the profile menu. This needs button semantics here.Suggested fix
- <div + <button + type="button" className="topbar-avatar" aria-label={displayName} + aria-haspopup="menu" + aria-expanded={showUserMenu} onClick={() => { setShowUserMenu(!showUserMenu); setShowThemeMenu(false); }} > @@ - </div> + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/App.tsx` around lines 772 - 785, The avatar trigger currently uses a non-interactive <div>, breaking keyboard accessibility; replace it with a real <button> element that preserves existing behavior: toggle setShowUserMenu(!showUserMenu) and call setShowThemeMenu(false) on click, keep aria-label={displayName}, and render the <img> (avatarUrl/ displayName) inside the button with the same styles; ensure the button has type="button" and retains the "topbar-avatar" class so styling and JS references remain unchanged.src/web/components/ChangeKeyModal.tsx (1)
42-48:⚠️ Potential issue | 🟠 MajorAdd a dedicated success callback for token changes.
Lines 43-48 reuse
onClose()for the success path, so the parent never learns that the credential changed. The old auth session stays in storage and later API calls keep sending a stale bearer token even though the toast tells the user to log in again. Please surface a distinct success callback that clears the session and returns the user to login.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/components/ChangeKeyModal.tsx` around lines 42 - 48, The modal currently calls onClose() on a successful api.changeAuthToken which prevents the parent from clearing the auth session; add a dedicated success callback prop (e.g., onTokenChangeSuccess) to the ChangeKeyModal component, update its prop types, and invoke onTokenChangeSuccess() in the success branch instead of onClose() after showing the toast and resetting local state (setOldToken, setNewToken, setConfirmToken); leave onClose() for cancel/close behavior so the parent can implement session clearing and redirect-to-login when onTokenChangeSuccess is called.
🧹 Nitpick comments (1)
src/web/App.tsx (1)
130-167: Extract the client token policy into one shared helper.Lines 130-167 now duplicate rules that also exist in
src/web/components/ChangeKeyModal.tsx, and the two flows have already diverged: this page blocks the factory-reset token client-side, while the modal leaves that check to the server. A shared validator/constant would keep the settings modal and first-login flow aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/App.tsx` around lines 130 - 167, Extract the client-side admin token policy into a single shared helper (e.g., export a FACTORY_RESET_ADMIN_TOKEN constant and a validateAdminToken or getAdminTokenValidationError function) and import it into both ForceChangeTokenPage and the ChangeKeyModal component; move the rules currently in ForceChangeTokenPage (non-empty, min length 12, not equal FACTORY_RESET_ADMIN_TOKEN) into that helper, replace the local FACTORY_RESET_ADMIN_TOKEN and inline checks in ForceChangeTokenPage with calls to the helper, and update ChangeKeyModal to use the same helper so both flows share identical client-side validation while server-side enforcement remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/routes/api/auth.first-login.test.ts`:
- Around line 15-17: The test overwrites process.env.DATA_DIR in beforeAll
without restoring it, causing order-dependent failures; modify the
setup/teardown to capture the original value (e.g., const previousDataDir =
process.env.DATA_DIR) before assigning process.env.DATA_DIR =
mkdtempSync(join(tmpdir(), 'metapi-auth-first-login-')) in beforeAll, and in
afterAll restore it (process.env.DATA_DIR = previousDataDir or delete
process.env.DATA_DIR if previousDataDir was undefined) while still performing
the existing temp-directory cleanup so the environment is exactly as it was
before the test; update the beforeAll/afterAll blocks in
auth.first-login.test.ts accordingly and reference the same captured variable
name in both hooks.
In `@src/server/routes/api/auth.ts`:
- Around line 11-13: Normalize (trim) both the configured token and any
incoming/submitted token before comparing them: update
isDefaultAdminTokenInUse() to trim config.authToken consistently, and change the
token-equality checks in the handlers for /api/settings/auth/info and
/api/settings/auth/change (the code that compares the incoming token to
config.authToken) to compare trimmed versions of both values (e.g., use
(config.authToken || '').trim() === (incomingToken || '').trim()). Ensure every
place that checks equality against FACTORY_RESET_ADMIN_TOKEN or config.authToken
uses the same trimmed normalization.
In `@src/web/App.first-login.test.tsx`:
- Around line 56-106: The test overwrites global DOM bindings in beforeEach
(localStorage, window, document, Node, Element, HTMLElement, Text,
MutationObserver) but never restores them; save the original global
descriptors/values before you replace them (e.g., origLocalStorage, origWindow,
origDocument, origNode, origElement, origHTMLElement, origText,
origMutationObserver) and then in afterEach restore each original via
Object.defineProperty or direct assignment and then call vi.clearAllMocks();
update the tests around the existing beforeEach/afterEach and the mocked helpers
(apiMock.getEvents) so restored globals are re-used by later tests.
In `@src/web/App.tsx`:
- Around line 581-593: In loadAuthInfo's catch branch that detects error.message
=== 'Session expired', don't leave the app in an authenticated state; instead
clear the stored session and force the app back to the login screen by invoking
the app's logout/clear-auth routine (e.g., remove stored token/session, call the
existing logout function or setAuthed(false)), and ensure any related state like
requirePasswordChange is not used to render protected UI; keep the existing
setAuthInfoLoaded(true) behavior but make sure authed is set false so the shell
doesn't render with a stale token.
---
Outside diff comments:
In `@src/web/App.tsx`:
- Around line 772-785: The avatar trigger currently uses a non-interactive
<div>, breaking keyboard accessibility; replace it with a real <button> element
that preserves existing behavior: toggle setShowUserMenu(!showUserMenu) and call
setShowThemeMenu(false) on click, keep aria-label={displayName}, and render the
<img> (avatarUrl/ displayName) inside the button with the same styles; ensure
the button has type="button" and retains the "topbar-avatar" class so styling
and JS references remain unchanged.
In `@src/web/components/ChangeKeyModal.tsx`:
- Around line 42-48: The modal currently calls onClose() on a successful
api.changeAuthToken which prevents the parent from clearing the auth session;
add a dedicated success callback prop (e.g., onTokenChangeSuccess) to the
ChangeKeyModal component, update its prop types, and invoke
onTokenChangeSuccess() in the success branch instead of onClose() after showing
the toast and resetting local state (setOldToken, setNewToken, setConfirmToken);
leave onClose() for cancel/close behavior so the parent can implement session
clearing and redirect-to-login when onTokenChangeSuccess is called.
---
Nitpick comments:
In `@src/web/App.tsx`:
- Around line 130-167: Extract the client-side admin token policy into a single
shared helper (e.g., export a FACTORY_RESET_ADMIN_TOKEN constant and a
validateAdminToken or getAdminTokenValidationError function) and import it into
both ForceChangeTokenPage and the ChangeKeyModal component; move the rules
currently in ForceChangeTokenPage (non-empty, min length 12, not equal
FACTORY_RESET_ADMIN_TOKEN) into that helper, replace the local
FACTORY_RESET_ADMIN_TOKEN and inline checks in ForceChangeTokenPage with calls
to the helper, and update ChangeKeyModal to use the same helper so both flows
share identical client-side validation while server-side enforcement remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ca7c504-93b9-4f77-84a1-d57e71740e64
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
src/server/routes/api/auth.first-login.test.tssrc/server/routes/api/auth.tssrc/web/App.first-login.test.tsxsrc/web/App.tsxsrc/web/components/ChangeKeyModal.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/web/App.first-login.test.tsx (2)
129-159: Consider adding a test for successful token change completion.The
apiMock.changeAuthTokenmock is defined (line 10) but never exercised. Given the PR objective states "on successful change the session is cleared and re-login is required," consider adding a test that:
- Sets
requirePasswordChange: true- Simulates the token change submission
- Verifies
changeAuthTokenwas called with expected parameters- Confirms auth tokens are cleared and login UI is shown
This would increase coverage of the complete first-login flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/App.first-login.test.tsx` around lines 129 - 159, Add a new test that completes the first-login token change flow: mock apiMock.getAuthInfo to return { requirePasswordChange: true }, render <App /> (same pattern using create/act/flush), simulate submitting the token-change form so that apiMock.changeAuthToken is invoked (assert apiMock.changeAuthToken was called with the expected payload), then assert storage.getItem('auth_token') and storage.getItem('auth_token_expires_at') are null and the UI shows the sign-in elements (e.g., contains 'Admin Token' and 'Sign In'); ensure the test follows the same setup/teardown pattern as the existing tests and references apiMock.changeAuthToken, requirePasswordChange, and storage.getItem for clarity.
37-45: Unuseddump()method.The
dump()method on the storage helper is defined but never used in the tests. Consider removing it to keep the helper minimal, or add a comment if it's intended for debugging purposes.♻️ Proposed fix
function createStorage(initial: Record<string, string> = {}) { const store = new Map(Object.entries(initial)); return { getItem: (key: string) => store.has(key) ? store.get(key)! : null, setItem: (key: string, value: string) => void store.set(key, value), removeItem: (key: string) => void store.delete(key), - dump: () => Object.fromEntries(store.entries()), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/App.first-login.test.tsx` around lines 37 - 45, The helper function createStorage exposes an unused dump() method; remove dump from the returned object (i.e., stop returning dump in createStorage) to keep the helper minimal, and ensure no tests rely on it (if any do, either update those tests or instead replace dump with a short comment noting it was intentionally omitted for brevity). Reference: createStorage and dump.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/web/App.first-login.test.tsx`:
- Around line 129-159: Add a new test that completes the first-login token
change flow: mock apiMock.getAuthInfo to return { requirePasswordChange: true },
render <App /> (same pattern using create/act/flush), simulate submitting the
token-change form so that apiMock.changeAuthToken is invoked (assert
apiMock.changeAuthToken was called with the expected payload), then assert
storage.getItem('auth_token') and storage.getItem('auth_token_expires_at') are
null and the UI shows the sign-in elements (e.g., contains 'Admin Token' and
'Sign In'); ensure the test follows the same setup/teardown pattern as the
existing tests and references apiMock.changeAuthToken, requirePasswordChange,
and storage.getItem for clarity.
- Around line 37-45: The helper function createStorage exposes an unused dump()
method; remove dump from the returned object (i.e., stop returning dump in
createStorage) to keep the helper minimal, and ensure no tests rely on it (if
any do, either update those tests or instead replace dump with a short comment
noting it was intentionally omitted for brevity). Reference: createStorage and
dump.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a31df01-cb7d-42f8-99ad-20e1e9fa37a9
📒 Files selected for processing (4)
src/server/routes/api/auth.first-login.test.tssrc/server/routes/api/auth.tssrc/web/App.first-login.test.tsxsrc/web/App.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/server/routes/api/auth.ts
- src/server/routes/api/auth.first-login.test.ts
- src/web/App.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
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.
变更内容
requirePasswordChange，供前端判断是否必须首登改密。change-me-admin-token。requirePasswordChange=true，直接进入强制改密页，改密成功后清理会话并要求重新登录。验证步骤
npx vitest run --root . src/server/routes/api/auth.first-login.test.ts src/server/middleware/auth.test.ts src/web/App.first-login.test.tsxrequirePasswordChange=true风险说明
requirePasswordChange门禁。回归点
Summary by CodeRabbit
New Features
Improvements
Removals
Tests