refactor(auth): 共享认证状态机迁移到 app/shared (#1537) - #1609
Conversation
📝 WalkthroughWalkthroughThe PR centralizes Hub OIDC authentication, PKCE, token lifecycle, and session management in ChangesHub authentication
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AuthPage
participant createHubAuthCore
participant PlatformPorts
participant TokenDance
AuthPage->>createHubAuthCore: loginWithTokenDance()
createHubAuthCore->>PlatformPorts: start callback channel
createHubAuthCore->>TokenDance: request authorization
TokenDance-->>createHubAuthCore: authorization URL
createHubAuthCore->>PlatformPorts: open authorization URL
PlatformPorts-->>createHubAuthCore: callback code and state
createHubAuthCore->>TokenDance: exchange code
TokenDance-->>createHubAuthCore: tokens and profile
createHubAuthCore->>PlatformPorts: save tokens and synchronize session
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
0cfa251 to
48f00da
Compare
- 新增 @shared/api/auth：Hub 认证状态机 SSOT（OIDC PKCE、token 生命周期、refresh fallback、logout 清理），web/desktop 只注入平台 Port - web/desktop hubAuth.ts 变薄壳，保留 createHubAuth/HubAuth/HubAuthState/OidcError 表面 - 新增 webPorts/desktopPorts 平台实现；行为与旧实现兼容 - 补充 shared 状态机测试、desktop Tauri 模式 Port 测试、web 登录流与登录 UI 测试
48f00da to
9a94713
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
app/shared/src/api/auth/authStateMachine.ts (2)
64-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate listener failures from the auth flow.
notify()calls each listener synchronously with no error handling. If one subscriber throws, the remaining subscribers never receive the snapshot, and the exception propagates out ofcompleteLogin,markAuthenticated, orclearSession. A UI render error in one subscriber then failstryAutoLoginorlogout.Wrap each callback so one faulty subscriber cannot break the state transition.
♻️ Proposed isolation
function notify() { snapshot = createSnapshot(); - listeners.forEach((fn) => fn(snapshot)); + listeners.forEach((fn) => { + try { + fn(snapshot); + } catch { + // A subscriber failure must not abort the auth state transition. + } + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/shared/src/api/auth/authStateMachine.ts` around lines 64 - 67, Update notify() so each listeners callback invocation is isolated with its own error handling, allowing all subscribers to receive the snapshot even if one throws. Prevent listener exceptions from propagating into completeLogin, markAuthenticated, clearSession, tryAutoLogin, or logout while preserving the existing notification order and snapshot behavior.
258-273: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the authorize response before the redirect.
authorizeRespis only asserted by the type annotation at line 233. The value is a parsed network response. If the Hub returns a body withoutauthorization_urlorstate, line 273 callsredirectOpener.open(undefined)and line 264 stores anundefinedpending state. The Web adapter then navigates to the string"undefined", and the failure does not carry an i18ncode, so the login UI cannot map it.Check both fields and throw an
OidcErrorwith the existingstartFailedcode.♻️ Proposed validation
const { state: serverState, authorization_url: authUrl } = authorizeResp; + if (!serverState || !authUrl) { + throw new OidcError( + 'startFailed', + 'Hub returned an incomplete OIDC authorize response.', + 'missing state or authorization_url', + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/shared/src/api/auth/authStateMachine.ts` around lines 258 - 273, Validate authorizeResp.state and authorizeResp.authorization_url before constructing the BrowserOIDCPending object or calling redirectOpener.open in the authorization flow. If either field is missing or invalid, throw an OidcError using the existing startFailed code so the login UI can map the failure, and preserve the existing redirect and pending-storage behavior for valid responses.app/shared/src/api/auth/authStateMachine.test.ts (2)
420-470: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the pending record is saved before the redirect opens.
authStateMachine.tssaves the pending record at line 270 and opens the authorization URL at line 273. That order is required for browser-redirect mode, because the page unloads duringopen(). A change that saved the pending record afteropen()would break Web login while every test in this block still passed, since the local-callback-server doubles never unload the page.Add an ordering assertion so the invariant is protected.
💚 Proposed assertion
it('completes the full OIDC flow: authorize → callback → exchange → login', async () => { const { ports, memory, sessionSync, resolveCallback } = localServerPorts(); + const pendingSavedBeforeRedirect: boolean[] = []; + const realOpen = ports.redirectOpener.open; + ports.redirectOpener.open = vi.fn(async (url: string) => { + pendingSavedBeforeRedirect.push(ports.pendingStorage.load() !== null); + await realOpen(url); + });Then assert after the login resolves:
+ // The pending record must survive the page unload in browser-redirect mode. + expect(pendingSavedBeforeRedirect).toEqual([true]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/shared/src/api/auth/authStateMachine.test.ts` around lines 420 - 470, Add an ordering assertion to the full OIDC flow test around loginWithTokenDance, verifying pendingStorage.save completes before ports.redirectOpener.open is invoked. Use call-order tracking or equivalent assertions after login resolves, while preserving the existing authorization, callback, and authenticated-state checks.
310-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for single-use pending state.
authStateMachine.tsclears the pending record at line 174, before it validates the state. That makes the browser callback single-use and blocks a replay of a captured callback URL. No test asserts this ordering.A regression that moved
pendingStorage.clear()into the success branch would keep every current test green while allowing replay. Add a test that runstryAutoLogintwice against the same callback URL and asserts the second call returnsfalseand performs no secondoidcCallback.💚 Proposed test
+ it('consumes the pending state once, so a replayed callback is rejected', async () => { + const { ports, memory } = createFakePorts(); + memory.pending = makePending('state-1'); + pushBrowserCallback('code-1', 'state-1'); + + const client = createFakeClient({ + oidcCallback: vi.fn().mockResolvedValue({ + access_token: 'access-oidc', + refresh_token: 'refresh-oidc', + expires_in: 3600, + user: alice, + }), + }); + const { auth } = createAuth(ports, client); + + await expect(auth.tryAutoLogin()).resolves.toBe(true); + expect(memory.pending).toBeNull(); + + // Replay the same callback URL. + pushBrowserCallback('code-1', 'state-1'); + await expect(auth.tryAutoLogin()).resolves.toBe(false); + expect(client.oidcCallback).toHaveBeenCalledTimes(1); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/shared/src/api/auth/authStateMachine.test.ts` around lines 310 - 349, Add a test near the existing successful callback test that invokes tryAutoLogin twice with the same pending state and browser callback URL, asserting the first attempt succeeds, the second returns false, and client.oidcCallback is called only once. Reuse the existing fake ports, pending-state setup, and valid callback response patterns.app/desktop/src/api/auth/desktopPorts.test.ts (2)
92-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd assertions that both listeners are removed after the callback settles.
captureListenersreturns a no-op unlisten function, so these tests cannot detect a leaked subscription. The success path indesktopPorts.tscurrently leaves theoidc-callbacklistener registered, and the error path leavesoidc-callback-errorregistered. Return distinct spies fromlistenMockand assert that both run after each settle path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/desktop/src/api/auth/desktopPorts.test.ts` around lines 92 - 124, Update the two callback-channel tests around createDesktopHubAuthPorts to make listenMock return distinct unlisten spies for the oidc-callback and oidc-callback-error subscriptions instead of relying on captureListeners’ no-op cleanup. After awaiting each callback’s settlement, assert that both unlisten spies have been called, covering cleanup in both success and error paths.
68-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset
windowOpenMockbetween tests.
beforeEachresetsinvokeMock,listenMock, andshellOpenMock, but notwindowOpenMock.vi.restoreAllMocks()targetsvi.spyOndescriptors, so the standalonevi.fn()can keep call history across tests and make the fallback window.open assertion pass early.♻️ Proposed fix
shellOpenMock.mockReset(); + windowOpenMock.mockReset(); sessionStorage.clear();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/desktop/src/api/auth/desktopPorts.test.ts` around lines 68 - 80, Update the beforeEach setup to reset windowOpenMock alongside invokeMock, listenMock, and shellOpenMock, while preserving its existing implementation and window.open spy configuration.app/web/README.md (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the 最后更新 date with this documentation change.
Line 18 adds new architecture documentation, but Line 3 still reads
最后更新：2026-07-26. The header then misstates when the document last changed. The added path../shared/src/api/auth/resolves correctly from this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/web/README.md` at line 3, Update the README header’s 最后更新 date to reflect the current documentation change, leaving the newly added architecture documentation and its valid ../shared/src/api/auth/ path unchanged.app/web/src/api/auth/webPorts.ts (2)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate the stored token source instead of casting.
Line 40-42 casts an arbitrary
sessionStoragestring toHubTokenSource. A stale or unexpected key value then enters the shared auth state as a valid union member. Narrow the value explicitly.♻️ Suggested validation
- return (typeof sessionStorage !== 'undefined' - ? sessionStorage.getItem(TOKEN_SOURCE_KEY) - : null) as HubTokenSource; + const raw = typeof sessionStorage !== 'undefined' + ? sessionStorage.getItem(TOKEN_SOURCE_KEY) + : null; + return raw === 'tokendance' || raw === 'hub' ? raw : null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/web/src/api/auth/webPorts.ts` around lines 35 - 46, Update readTokenSource to validate the value returned by sessionStorage.getItem against the allowed HubTokenSource values before returning it, and return null for missing or unexpected strings. Remove the direct cast so arbitrary stored data cannot enter the shared auth state as a valid token source.
31-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNormalize
OIDC_CALLBACK_PATHbefore the pathname comparison.
readBrowserCallbackcomparesurl.pathnametoOIDC_CALLBACK_PATHwith strict equality (Line 67). If an operator setsVITE_OIDC_CALLBACK_PATHwithout a leading slash, or with a trailing slash, the comparison never matches. The callback then returnsnulland the login silently fails with no diagnostic. Normalize the configured value once at module load.♻️ Suggested normalization
-const OIDC_CALLBACK_PATH = import.meta.env.VITE_OIDC_CALLBACK_PATH - || `${APP_BASE_PATH === '/' ? '' : APP_BASE_PATH.replace(/\/$/, '')}/auth/tokendance/callback`; +function normalizeCallbackPath(path: string): string { + const withLeadingSlash = path.startsWith('/') ? path : `/${path}`; + return withLeadingSlash.length > 1 ? withLeadingSlash.replace(/\/$/, '') : withLeadingSlash; +} + +const OIDC_CALLBACK_PATH = normalizeCallbackPath( + import.meta.env.VITE_OIDC_CALLBACK_PATH + || `${APP_BASE_PATH === '/' ? '' : APP_BASE_PATH.replace(/\/$/, '')}/auth/tokendance/callback`, +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/web/src/api/auth/webPorts.ts` around lines 31 - 33, Normalize the configured OIDC callback path when initializing OIDC_CALLBACK_PATH so it always begins with a single leading slash and has no trailing slash before readBrowserCallback performs its strict pathname comparison. Preserve the existing derived default path behavior while applying the same normalization to VITE_OIDC_CALLBACK_PATH.app/web/src/api/hubAuth.test.ts (1)
366-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the PKCE binding between
code_challengeandcodeVerifier.Line 366 checks the
code_challengeshape, and Line 399 checks thecodeVerifiershape. Neither assertion proves that the challenge is the S256 hash of the persisted verifier. That binding is the property PKCE depends on. Capture the request body, then compare the base64url SHA-256 of the storedcodeVerifieragainst the sentcode_challenge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/web/src/api/hubAuth.test.ts` around lines 366 - 399, Update the browser-redirect test around createHubAuth/loginWithTokenDance to capture the authorization request body, retain the sent code_challenge, and assert it equals the base64url-encoded SHA-256 digest of the persisted pending.codeVerifier. Keep the existing format checks and redirect side-effect assertions.app/web/src/components/AuthPage.test.tsx (1)
91-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
vi.stubGlobal('fetch', ...)is installed without a matching restore in both new test files.vi.clearAllMocks()clears call history only; it does not remove a stubbed global or an implementation set bymockResolvedValue. Each stub therefore stays active for the tests that follow, which makes both suites depend on declaration order. Add an explicit restore step in each file.
app/web/src/components/AuthPage.test.tsx#L91-L95: move thefetchstub intobeforeEachand add anafterEachthat callsvi.unstubAllGlobals(), so the rejecting stub from the disconnected-Hub test does not reach the close-button test.app/web/src/api/hubAuth.test.ts#L380-L409: callvi.unstubAllGlobals()in the existingfinallyblock, next to thewindow.locationrestore, and remove theagenthub_oidc_pkce_pendingentry written during the redirect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/web/src/components/AuthPage.test.tsx` around lines 91 - 95, The fetch stub installed in test setup is not being cleaned up between tests, causing state to leak across test cases. In app/web/src/components/AuthPage.test.tsx at lines 91-95, move any fetch stub call from test bodies into the beforeEach block, and add a corresponding afterEach block that calls vi.unstubAllGlobals() to restore the fetch global and prevent stubs from interfering with subsequent tests. In app/web/src/api/hubAuth.test.ts at lines 380-409, add vi.unstubAllGlobals() to the existing finally block alongside the window.location restore, and remove the agenthub_oidc_pkce_pending entry that is written during the redirect test to prevent it from persisting to other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/desktop/src/api/auth/desktopPorts.ts`:
- Around line 114-149: Handle rejected promises from both oidc-callback and
oidc-callback-error listen subscriptions by settling the result with a
listenFailed OidcError, clearing the timeout, and cleaning up any established
listener. Add rejection handlers alongside the existing fulfillment handlers,
ensuring late subscription resolution is still unsubscribed when settled and
avoiding unhandled rejections.
- Around line 100-150: Update the Promise setup around the OIDC callback
listeners to define a shared cleanup() function that invokes both unlisten and
unlistenError, while preserving timeout cleanup. Call cleanup() from the success
handler, error handler, and timeout path so both subscriptions are removed on
every settle path; retain the existing settled guard and result/error behavior.
In `@app/shared/src/api/auth/authStateMachine.ts`:
- Around line 86-91: In-memory state is mutated before the awaited storage
writes complete, so if a write rejects, state holds tokens that storage does not
have and subscribers are not notified of the failure. At
app/shared/src/api/auth/authStateMachine.ts lines 86-91 in completeLogin,
reorder the operations to await saveRefreshToken and saveAccessToken before
assigning state.token, state.refreshToken, and state.tokenSource; wrap the await
calls in a try-catch that calls clearSession() and rethrows on rejection. At
app/shared/src/api/auth/authStateMachine.ts lines 343-346 in the tryAutoLogin
refresh fallback, reorder to await saveAccessToken(res.access_token) and
saveRefreshToken(res.refresh_token) before assigning state.token and
state.refreshToken, so a failed write does not leave an unpersisted refreshed
session in memory.
- Around line 316-358: The tryAutoLogin method can be called concurrently,
causing multiple refresh attempts that may revoke tokens and destroy the
session. Add an instance variable to track an in-flight tryAutoLogin promise,
and return that same promise from later callers until the operation completes.
At the start of tryAutoLogin, check if a promise is already in progress and
return it if so, otherwise proceed with the full flow and store the returned
promise. Clear the stored promise when tryAutoLogin completes, whether it
succeeds or fails, so subsequent calls can start fresh.
In `@app/shared/src/api/auth/pkce.ts`:
- Around line 21-26: Update computeCodeChallenge to explicitly validate that
crypto.subtle is available before calling digest, and throw the established
OidcError with an appropriate i18n code and usable message when it is missing,
preserving the existing hashing path when available.
In `@app/shared/src/api/auth/types.ts`:
- Around line 65-76: Update the OidcError constructor to accept an optional
cause argument, pass it through the Error super call using the native cause
option, and remove the class-level cause declaration so it is not shadowed.
Update OIDC start and token exchange callers to provide cause during
construction instead of assigning it afterward.
In `@app/web/src/api/auth/webPorts.ts`:
- Around line 97-100: Update pendingStorage.save to wrap sessionStorage.setItem
and JSON serialization in a try/catch, matching the safeguards used by
readTokenSource and saveTokenSource. Preserve the undefined-storage guard and
ensure storage failures are swallowed so loginWithTokenDance can continue to the
redirect.
- Around line 119-130: The callback promise returned by the browser start()
method never settles, so when window.location.assign fails or is blocked during
the redirect, the login flow hangs without rejecting. Modify the start() method
to detect and handle redirect failures: either wrap the redirect logic inside
the callback promise so it can reject when window.location.assign throws or is
blocked, or wrap the redirect call in a try-catch and have the callback promise
reject with the caught error if navigation cannot be initiated.
---
Nitpick comments:
In `@app/desktop/src/api/auth/desktopPorts.test.ts`:
- Around line 92-124: Update the two callback-channel tests around
createDesktopHubAuthPorts to make listenMock return distinct unlisten spies for
the oidc-callback and oidc-callback-error subscriptions instead of relying on
captureListeners’ no-op cleanup. After awaiting each callback’s settlement,
assert that both unlisten spies have been called, covering cleanup in both
success and error paths.
- Around line 68-80: Update the beforeEach setup to reset windowOpenMock
alongside invokeMock, listenMock, and shellOpenMock, while preserving its
existing implementation and window.open spy configuration.
In `@app/shared/src/api/auth/authStateMachine.test.ts`:
- Around line 420-470: Add an ordering assertion to the full OIDC flow test
around loginWithTokenDance, verifying pendingStorage.save completes before
ports.redirectOpener.open is invoked. Use call-order tracking or equivalent
assertions after login resolves, while preserving the existing authorization,
callback, and authenticated-state checks.
- Around line 310-349: Add a test near the existing successful callback test
that invokes tryAutoLogin twice with the same pending state and browser callback
URL, asserting the first attempt succeeds, the second returns false, and
client.oidcCallback is called only once. Reuse the existing fake ports,
pending-state setup, and valid callback response patterns.
In `@app/shared/src/api/auth/authStateMachine.ts`:
- Around line 64-67: Update notify() so each listeners callback invocation is
isolated with its own error handling, allowing all subscribers to receive the
snapshot even if one throws. Prevent listener exceptions from propagating into
completeLogin, markAuthenticated, clearSession, tryAutoLogin, or logout while
preserving the existing notification order and snapshot behavior.
- Around line 258-273: Validate authorizeResp.state and
authorizeResp.authorization_url before constructing the BrowserOIDCPending
object or calling redirectOpener.open in the authorization flow. If either field
is missing or invalid, throw an OidcError using the existing startFailed code so
the login UI can map the failure, and preserve the existing redirect and
pending-storage behavior for valid responses.
In `@app/web/README.md`:
- Line 3: Update the README header’s 最后更新 date to reflect the current
documentation change, leaving the newly added architecture documentation and its
valid ../shared/src/api/auth/ path unchanged.
In `@app/web/src/api/auth/webPorts.ts`:
- Around line 35-46: Update readTokenSource to validate the value returned by
sessionStorage.getItem against the allowed HubTokenSource values before
returning it, and return null for missing or unexpected strings. Remove the
direct cast so arbitrary stored data cannot enter the shared auth state as a
valid token source.
- Around line 31-33: Normalize the configured OIDC callback path when
initializing OIDC_CALLBACK_PATH so it always begins with a single leading slash
and has no trailing slash before readBrowserCallback performs its strict
pathname comparison. Preserve the existing derived default path behavior while
applying the same normalization to VITE_OIDC_CALLBACK_PATH.
In `@app/web/src/api/hubAuth.test.ts`:
- Around line 366-399: Update the browser-redirect test around
createHubAuth/loginWithTokenDance to capture the authorization request body,
retain the sent code_challenge, and assert it equals the base64url-encoded
SHA-256 digest of the persisted pending.codeVerifier. Keep the existing format
checks and redirect side-effect assertions.
In `@app/web/src/components/AuthPage.test.tsx`:
- Around line 91-95: The fetch stub installed in test setup is not being cleaned
up between tests, causing state to leak across test cases. In
app/web/src/components/AuthPage.test.tsx at lines 91-95, move any fetch stub
call from test bodies into the beforeEach block, and add a corresponding
afterEach block that calls vi.unstubAllGlobals() to restore the fetch global and
prevent stubs from interfering with subsequent tests. In
app/web/src/api/hubAuth.test.ts at lines 380-409, add vi.unstubAllGlobals() to
the existing finally block alongside the window.location restore, and remove the
agenthub_oidc_pkce_pending entry that is written during the redirect test to
prevent it from persisting to other tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ddeb784-1c1b-4261-9c37-508794ae70bc
📒 Files selected for processing (18)
app/desktop/README.mdapp/desktop/src/api/auth/desktopPorts.test.tsapp/desktop/src/api/auth/desktopPorts.tsapp/desktop/src/api/hubAuth.test.tsapp/desktop/src/api/hubAuth.tsapp/shared/README.mdapp/shared/package.jsonapp/shared/src/api/auth/authStateMachine.test.tsapp/shared/src/api/auth/authStateMachine.tsapp/shared/src/api/auth/index.tsapp/shared/src/api/auth/pkce.tsapp/shared/src/api/auth/ports.tsapp/shared/src/api/auth/types.tsapp/web/README.mdapp/web/src/api/auth/webPorts.tsapp/web/src/api/hubAuth.test.tsapp/web/src/api/hubAuth.tsapp/web/src/components/AuthPage.test.tsx
| const result = new Promise<OidcCallbackResult>((resolve, reject) => { | ||
| let settled = false; | ||
|
|
||
| const timeout = setTimeout(() => { | ||
| if (settled) return; | ||
| settled = true; | ||
| unlisten(); | ||
| unlistenError(); | ||
| reject(new OidcError('timeout', 'Login timed out — no callback received within 5 minutes.')); | ||
| }, 5 * 60_000); | ||
|
|
||
| let unlisten: () => void = () => {}; | ||
| let unlistenError: () => void = () => {}; | ||
|
|
||
| listen<{ code: string; state: string }>('oidc-callback', (event) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timeout); | ||
| unlistenError(); | ||
| resolve({ code: event.payload.code, state: event.payload.state }); | ||
| }).then((u) => { | ||
| if (settled) { | ||
| u(); | ||
| return; | ||
| } | ||
| unlisten = u; | ||
| }); | ||
|
|
||
| listen<{ error: string; description?: string }>( | ||
| 'oidc-callback-error', | ||
| (event) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timeout); | ||
| unlisten(); | ||
| reject( | ||
| new OidcError( | ||
| 'callbackError', | ||
| `OIDC error: ${event.payload.error}${event.payload.description ? ` — ${event.payload.description}` : ''}`, | ||
| `${event.payload.error}${event.payload.description ? ` — ${event.payload.description}` : ''}`, | ||
| ), | ||
| ); | ||
| }, | ||
| ).then((u) => { | ||
| if (settled) { | ||
| u(); | ||
| return; | ||
| } | ||
| unlistenError = u; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unregister both event listeners on every settle path.
The success handler calls unlistenError() but not unlisten(). The error handler calls unlisten() but not unlistenError(). Each settle path leaves its own listener registered in the webview. Every login attempt then adds a permanent oidc-callback or oidc-callback-error subscription, and a stale handler can fire during a later flow.
Use one cleanup() that removes both listeners.
🔒️ Proposed fix: single cleanup for both listeners
const result = new Promise<OidcCallbackResult>((resolve, reject) => {
let settled = false;
+ let unlisten: () => void = () => {};
+ let unlistenError: () => void = () => {};
+
+ const cleanup = () => {
+ unlisten();
+ unlistenError();
+ };
const timeout = setTimeout(() => {
if (settled) return;
settled = true;
- unlisten();
- unlistenError();
+ cleanup();
reject(new OidcError('timeout', 'Login timed out — no callback received within 5 minutes.'));
}, 5 * 60_000);
- let unlisten: () => void = () => {};
- let unlistenError: () => void = () => {};
-
listen<{ code: string; state: string }>('oidc-callback', (event) => {
if (settled) return;
settled = true;
clearTimeout(timeout);
- unlistenError();
+ cleanup();
resolve({ code: event.payload.code, state: event.payload.state });
}).then((u) => {
if (settled) {
u();
return;
}
unlisten = u;
});
listen<{ error: string; description?: string }>(
'oidc-callback-error',
(event) => {
if (settled) return;
settled = true;
clearTimeout(timeout);
- unlisten();
+ cleanup();
reject(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = new Promise<OidcCallbackResult>((resolve, reject) => { | |
| let settled = false; | |
| const timeout = setTimeout(() => { | |
| if (settled) return; | |
| settled = true; | |
| unlisten(); | |
| unlistenError(); | |
| reject(new OidcError('timeout', 'Login timed out — no callback received within 5 minutes.')); | |
| }, 5 * 60_000); | |
| let unlisten: () => void = () => {}; | |
| let unlistenError: () => void = () => {}; | |
| listen<{ code: string; state: string }>('oidc-callback', (event) => { | |
| if (settled) return; | |
| settled = true; | |
| clearTimeout(timeout); | |
| unlistenError(); | |
| resolve({ code: event.payload.code, state: event.payload.state }); | |
| }).then((u) => { | |
| if (settled) { | |
| u(); | |
| return; | |
| } | |
| unlisten = u; | |
| }); | |
| listen<{ error: string; description?: string }>( | |
| 'oidc-callback-error', | |
| (event) => { | |
| if (settled) return; | |
| settled = true; | |
| clearTimeout(timeout); | |
| unlisten(); | |
| reject( | |
| new OidcError( | |
| 'callbackError', | |
| `OIDC error: ${event.payload.error}${event.payload.description ? ` — ${event.payload.description}` : ''}`, | |
| `${event.payload.error}${event.payload.description ? ` — ${event.payload.description}` : ''}`, | |
| ), | |
| ); | |
| }, | |
| ).then((u) => { | |
| if (settled) { | |
| u(); | |
| return; | |
| } | |
| unlistenError = u; | |
| }); | |
| }); | |
| const result = new Promise<OidcCallbackResult>((resolve, reject) => { | |
| let settled = false; | |
| let unlisten: () => void = () => {}; | |
| let unlistenError: () => void = () => {}; | |
| const cleanup = () => { | |
| unlisten(); | |
| unlistenError(); | |
| }; | |
| const timeout = setTimeout(() => { | |
| if (settled) return; | |
| settled = true; | |
| cleanup(); | |
| reject(new OidcError('timeout', 'Login timed out — no callback received within 5 minutes.')); | |
| }, 5 * 60_000); | |
| listen<{ code: string; state: string }>('oidc-callback', (event) => { | |
| if (settled) return; | |
| settled = true; | |
| clearTimeout(timeout); | |
| cleanup(); | |
| resolve({ code: event.payload.code, state: event.payload.state }); | |
| }).then((u) => { | |
| if (settled) { | |
| u(); | |
| return; | |
| } | |
| unlisten = u; | |
| }); | |
| listen<{ error: string; description?: string }>( | |
| 'oidc-callback-error', | |
| (event) => { | |
| if (settled) return; | |
| settled = true; | |
| clearTimeout(timeout); | |
| cleanup(); | |
| reject( | |
| new OidcError( | |
| 'callbackError', | |
| `OIDC error: ${event.payload.error}${event.payload.description ? ` — ${event.payload.description}` : ''}`, | |
| `${event.payload.error}${event.payload.description ? ` — ${event.payload.description}` : ''}`, | |
| ), | |
| ); | |
| }, | |
| ).then((u) => { | |
| if (settled) { | |
| u(); | |
| return; | |
| } | |
| unlistenError = u; | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/desktop/src/api/auth/desktopPorts.ts` around lines 100 - 150, Update the
Promise setup around the OIDC callback listeners to define a shared cleanup()
function that invokes both unlisten and unlistenError, while preserving timeout
cleanup. Call cleanup() from the success handler, error handler, and timeout
path so both subscriptions are removed on every settle path; retain the existing
settled guard and result/error behavior.
| listen<{ code: string; state: string }>('oidc-callback', (event) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timeout); | ||
| unlistenError(); | ||
| resolve({ code: event.payload.code, state: event.payload.state }); | ||
| }).then((u) => { | ||
| if (settled) { | ||
| u(); | ||
| return; | ||
| } | ||
| unlisten = u; | ||
| }); | ||
|
|
||
| listen<{ error: string; description?: string }>( | ||
| 'oidc-callback-error', | ||
| (event) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timeout); | ||
| unlisten(); | ||
| reject( | ||
| new OidcError( | ||
| 'callbackError', | ||
| `OIDC error: ${event.payload.error}${event.payload.description ? ` — ${event.payload.description}` : ''}`, | ||
| `${event.payload.error}${event.payload.description ? ` — ${event.payload.description}` : ''}`, | ||
| ), | ||
| ); | ||
| }, | ||
| ).then((u) => { | ||
| if (settled) { | ||
| u(); | ||
| return; | ||
| } | ||
| unlistenError = u; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle rejection of the listen() promises.
Both listen(...) calls only attach a fulfillment handler. If listen rejects, for example on a missing event permission, the returned result promise stays pending for the full 5 minutes and the runtime reports an unhandled rejection. The user sees a hang instead of a listenFailed error.
Reject result when either subscription fails.
♻️ Proposed fix: reject on subscription failure
}).then((u) => {
if (settled) {
u();
return;
}
unlisten = u;
- });
+ }, (err: unknown) => {
+ if (settled) return;
+ settled = true;
+ clearTimeout(timeout);
+ const detail = err instanceof Error ? err.message : String(err);
+ cleanup();
+ reject(new OidcError('listenFailed', `Failed to listen for OIDC callback: ${detail}`, detail));
+ });Apply the same rejection handler to the oidc-callback-error subscription.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/desktop/src/api/auth/desktopPorts.ts` around lines 114 - 149, Handle
rejected promises from both oidc-callback and oidc-callback-error listen
subscriptions by settling the result with a listenFailed OidcError, clearing the
timeout, and cleaning up any established listener. Add rejection handlers
alongside the existing fulfillment handlers, ensuring late subscription
resolution is still unsubscribed when settled and avoiding unhandled rejections.
| await tokenStorage.saveRefreshToken(refreshToken); | ||
| state.token = token; | ||
| state.refreshToken = refreshToken; | ||
| state.tokenSource = source; | ||
| await tokenStorage.saveAccessToken(token); | ||
| tokenStorage.saveTokenSource(source); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
In-memory state is mutated before the awaited storage write in both token-write paths. Both paths assign the new tokens to state and then await the persistence calls. If a storage write rejects, the rejection propagates while state already holds tokens that storage does not have, and no notify() runs, so subscribers never learn the write failed. The session then works in the current tab but is gone after a reload. The Desktop tokenStorage writes to a Tauri credential store, so a rejection is plausible.
app/shared/src/api/auth/authStateMachine.ts#L86-L91: incompleteLogin, awaitsaveRefreshTokenandsaveAccessTokenfirst, then assignstate.token,state.refreshToken, andstate.tokenSource. On a rejection, callclearSession()and rethrow.app/shared/src/api/auth/authStateMachine.ts#L343-L346: in thetryAutoLoginrefresh fallback, awaitsaveAccessToken(res.access_token)andsaveRefreshToken(res.refresh_token)before assigningstate.tokenandstate.refreshToken, so a failed write does not leave an unpersisted refreshed session.
📍 Affects 1 file
app/shared/src/api/auth/authStateMachine.ts#L86-L91(this comment)app/shared/src/api/auth/authStateMachine.ts#L343-L346
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/shared/src/api/auth/authStateMachine.ts` around lines 86 - 91, In-memory
state is mutated before the awaited storage writes complete, so if a write
rejects, state holds tokens that storage does not have and subscribers are not
notified of the failure. At app/shared/src/api/auth/authStateMachine.ts lines
86-91 in completeLogin, reorder the operations to await saveRefreshToken and
saveAccessToken before assigning state.token, state.refreshToken, and
state.tokenSource; wrap the await calls in a try-catch that calls clearSession()
and rethrows on rejection. At app/shared/src/api/auth/authStateMachine.ts lines
343-346 in the tryAutoLogin refresh fallback, reorder to await
saveAccessToken(res.access_token) and saveRefreshToken(res.refresh_token) before
assigning state.token and state.refreshToken, so a failed write does not leave
an unpersisted refreshed session in memory.
| async tryAutoLogin() { | ||
| // Browser-redirect mode: process the OIDC callback landing on this page. | ||
| const browserCallback = callbackChannel.readBrowserCallback(); | ||
| if (browserCallback) { | ||
| return handleBrowserCallback(browserCallback); | ||
| } | ||
|
|
||
| // Restore the access token from platform storage. | ||
| if (!state.token) { | ||
| const stored = await tokenStorage.loadAccessToken(); | ||
| if (stored) { | ||
| state.token = stored; | ||
| state.tokenSource = tokenStorage.loadTokenSource(); | ||
| } | ||
| } | ||
| if (!state.token) return false; | ||
| authClient = authedClient(); | ||
| try { | ||
| state.user = await authClient.me(); | ||
| markAuthenticated(); | ||
| return true; | ||
| } catch { | ||
| const refreshToken = state.refreshToken ?? (await tokenStorage.loadRefreshToken()); | ||
| if (refreshToken) { | ||
| try { | ||
| const refreshClient = publicClient(); | ||
| const res = await refreshClient.refresh(refreshToken); | ||
| state.token = res.access_token; | ||
| state.refreshToken = res.refresh_token; | ||
| await tokenStorage.saveAccessToken(res.access_token); | ||
| await tokenStorage.saveRefreshToken(res.refresh_token); | ||
| authClient = authedClient(); | ||
| state.user = await authClient.me(); | ||
| markAuthenticated(); | ||
| return true; | ||
| } catch { | ||
| // Refresh failed — fall through to full cleanup. | ||
| } | ||
| } | ||
| await clearSession(); | ||
| return false; | ||
| } | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate tryAutoLogin callers and Hub refresh-token rotation behavior.
set -euo pipefail
rg -nP -C 6 '\btryAutoLogin\s*\(' app --type=ts --type=tsx
echo "=== refresh token rotation ==="
rg -nP -C 6 'refresh_token' --glob '!**/node_modules/**' -g '*.go' -g '*.ts' .Repository: TokenDanceLab/AgentHub
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tryAutoLogin call sites ==="
rg -n -C 8 '\btryAutoLogin\b' app --glob '*.ts' --glob '*.tsx' || true
echo "=== authStateMachine relevant section ==="
sed -n '280,365p' app/shared/src/api/auth/authStateMachine.ts
echo "=== refresh token rotation references ==="
rg -n -C 6 'refresh_token|refreshToken|RefreshToken|RefreshToken' --glob '!**/node_modules/**' --glob '*.go' --glob '*.ts' . || true
echo "=== token storage implementation references ==="
rg -n -C 8 'loadRefreshToken|requestRefresh|refresh\(|refresh\(' --glob '!**/node_modules/**' app/shared/src/api app/shared/src hfs --glob '*.ts' --glob '*.tsx' || trueRepository: TokenDanceLab/AgentHub
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== authStateMachine top-level declarations and return object ==="
sed -n '1,120p' app/shared/src/api/auth/authStateMachine.ts
sed -n '300,365p' app/shared/src/api/auth/authStateMachine.ts
echo "=== tryAutoLogin call sites ==="
rg -n -C 8 'tryAutoLogin\b' app --glob '*.ts' --glob '*.tsx'
echo "=== refresh token model/entity ==="
fd -a -t f 'refresh' . | sed 's#^\./##' | head -100
rg -n -C 8 'type RefreshToken|type Refresh;|type Refresh |struct Refresh|refreshToken|RefreshToken|refresh_token' hub-server --glob '*.go' | head -300
echo "=== Hub refresh endpoint / refresh client in shared code ==="
rg -n -C 8 'function .*refresh|refresh\(|buildRefresh|requestRefresh|client\.refresh|\.refresh\b|refresh_token' app/shared/src --glob '*.ts' --glob '*.tsx'Repository: TokenDanceLab/AgentHub
Length of output: 50379
🌐 Web query:
hub-server refresh_tokens token_hash expires_at revoke refresh endpoint revokes old refresh token
💡 Result:
In a secure refresh token rotation implementation, the refresh endpoint effectively revokes the old refresh token by invalidating its usage, typically through a rotation and reuse detection mechanism [1][2][3]. When a client uses a refresh token to request a new access token, the server executes the following process: 1. Verification and Rotation: The server verifies the provided refresh token's hash against the stored token_hash and checks if it is active and not expired (expires_at) [4][5][6]. Upon successful validation, the server issues a new pair of tokens (access and refresh) and marks the old refresh token as consumed or revoked [1][5][2]. 2. Reuse Detection: To prevent replay attacks, the server tracks token "families" (using a family_id) [4][7][5]. If a token that has already been marked as consumed or revoked is presented to the refresh endpoint, the system detects a reuse attempt [5][6][3]. 3. Family Revocation: In the event of detected reuse, secure systems do not merely reject the specific request [8]. Instead, they treat the event as an indicator of a potential credential compromise and revoke the entire "family" of tokens, forcing the user to re-authenticate [2][9][3]. This mechanism ensures that if an attacker intercepts a refresh token and uses it, the legitimate client's subsequent use of the same (or any associated) token will trigger the reuse detection, leading to the immediate invalidation of the entire session [2][3]. Some implementations include a short grace period (e.g., 5-10 seconds) to allow for legitimate network retries without triggering full revocation [9][3]. Data models for this process typically include: - token_hash: The SHA-256 hash of the token (for secure lookup) [4][7][5]. - expires_at: The timestamp indicating when the token is no longer valid [4][10][5]. - family_id: A UUID grouping all tokens in a rotation chain [4][7][6]. - revoked_at: A timestamp marking the specific revocation time [10][1][7]. - replaced_by: A reference to the ID of the new token issued during rotation [4][1][7].
Citations:
- 1: https://dev.to/moose978/refresh-tokens-done-right-323
- 2: https://auth0.com/docs/secure/tokens/refresh-tokens/refresh-token-rotation
- 3: https://devopsbeast.com/blog/refresh-token-rotation-reuse-detection
- 4: https://github.com/heyitskuril/auth-implementation-guide/blob/main/docs/05-refresh-token-rotation.md
- 5: https://nileshblog.tech/refresh-token-rotation-reuse-detection/
- 6: https://amanksingh.com/blog/refresh-tokens-done-right
- 7: https://www.techinterview.org/post/3233468216/lld-token-refresh/
- 8: https://www.session-management.com/oidc-oauth-20-implementation/secure-token-refresh-and-rotation-patterns/
- 9: https://developer.okta.com/docs/guides/refresh-tokens/main/
- 10: https://github.com/rvnhq/raven/blob/master/crates/raven-server/src/db/sessions.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== focus RefreshToken model fields ==="
rg -n -C 10 'type RefreshToken|RefreshToken.*struct|type.*Refresh.*struct|RefreshToken.*refresh_token|replaced_by|revoked|family|token_hash' hub-server --glob '*.go' | head -400
echo "=== focused refresh endpoint files ==="
rg -n -l '/client/auth/refresh|auth/refresh|refreshToken|refreshToken|RefreshToken|jwtutil.GenerateRefreshToken|token_hash.*Where|revoked' hub-server --glob '*.go'
echo "=== focused auto-login hook file ==="
sed -n '1,70p' app/web/src/hooks/useWebAuth.tsRepository: TokenDanceLab/AgentHub
Length of output: 37612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== refresh token handler implementation ==="
sed -n '80,240p' hub-server/internal/service/auth.go
sed -n '1,180p' hub-server/internal/repository/refresh_token.go
echo "=== rotation behavior in tests ==="
sed -n '220,285p' hub-server/tests/integration/auth_edge_cases_test.go
echo "=== clearSession reference in core ==="
sed -n '338,360p' app/shared/src/api/auth/authStateMachine.tsRepository: TokenDanceLab/AgentHub
Length of output: 9601
Serialize concurrent tryAutoLogin refresh attempts.
tryAutoLogin can enter the refresh path while another call is already calling refresh(). On the same device, Hub refresh rotation revokes the original refresh token (first call succeeds, second call receives auth_refresh_invalid), so the second call falls through to clearSession() and destroys the restored session.
Return the same in-flight tryAutoLogin() promise for later callers until the operation completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/shared/src/api/auth/authStateMachine.ts` around lines 316 - 358, The
tryAutoLogin method can be called concurrently, causing multiple refresh
attempts that may revoke tokens and destroy the session. Add an instance
variable to track an in-flight tryAutoLogin promise, and return that same
promise from later callers until the operation completes. At the start of
tryAutoLogin, check if a promise is already in progress and return it if so,
otherwise proceed with the full flow and store the returned promise. Clear the
stored promise when tryAutoLogin completes, whether it succeeds or fails, so
subsequent calls can start fresh.
| /** S256 code challenge derived from the verifier. */ | ||
| export async function computeCodeChallenge(verifier: string): Promise<string> { | ||
| const encoder = new TextEncoder(); | ||
| const digest = await crypto.subtle.digest('SHA-256', encoder.encode(verifier)); | ||
| return base64UrlEncode(new Uint8Array(digest)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against a missing crypto.subtle.
crypto.subtle is only defined in a secure context. On a plain-HTTP origin that is not localhost, crypto.subtle is undefined and this line throws a TypeError. authStateMachine.ts calls computeCodeChallenge at line 225 outside any try block, so the raw TypeError reaches the caller instead of an OidcError with an i18n code. The login UI then shows an unmapped error.
Add an explicit check so the failure carries a usable message.
🛡️ Proposed guard
/** S256 code challenge derived from the verifier. */
export async function computeCodeChallenge(verifier: string): Promise<string> {
+ if (typeof crypto === 'undefined' || !crypto.subtle) {
+ throw new Error('Web Crypto subtle API unavailable — PKCE requires a secure context (HTTPS or localhost).');
+ }
const encoder = new TextEncoder();
const digest = await crypto.subtle.digest('SHA-256', encoder.encode(verifier));
return base64UrlEncode(new Uint8Array(digest));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** S256 code challenge derived from the verifier. */ | |
| export async function computeCodeChallenge(verifier: string): Promise<string> { | |
| const encoder = new TextEncoder(); | |
| const digest = await crypto.subtle.digest('SHA-256', encoder.encode(verifier)); | |
| return base64UrlEncode(new Uint8Array(digest)); | |
| } | |
| /** S256 code challenge derived from the verifier. */ | |
| export async function computeCodeChallenge(verifier: string): Promise<string> { | |
| if (typeof crypto === 'undefined' || !crypto.subtle) { | |
| throw new Error('Web Crypto subtle API unavailable — PKCE requires a secure context (HTTPS or localhost).'); | |
| } | |
| const encoder = new TextEncoder(); | |
| const digest = await crypto.subtle.digest('SHA-256', encoder.encode(verifier)); | |
| return base64UrlEncode(new Uint8Array(digest)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/shared/src/api/auth/pkce.ts` around lines 21 - 26, Update
computeCodeChallenge to explicitly validate that crypto.subtle is available
before calling digest, and throw the established OidcError with an appropriate
i18n code and usable message when it is missing, preserving the existing hashing
path when available.
| export class OidcError extends Error { | ||
| code: string; | ||
| detail?: string; | ||
| cause?: unknown; | ||
|
|
||
| constructor(code: string, fallbackMessage: string, detail?: string) { | ||
| super(fallbackMessage); | ||
| this.name = 'OidcError'; | ||
| this.code = code; | ||
| if (detail) this.detail = detail; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the shared package TS target and class-field semantics.
set -euo pipefail
fd -t f 'tsconfig*.json' app/shared app | while IFS= read -r f; do
echo "=== $f ==="
cat "$f"
doneRepository: TokenDanceLab/AgentHub
Length of output: 3425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== candidate files ==="
fd -t f 'auth.*' app/shared/src | sed -n '1,120p'
fd -t f 'authStateMachine.*' app/shared/src | sed -n '1,120p'
echo
echo "=== OidcError definition ==="
cat -n app/shared/src/api/auth/types.ts | sed -n '55,85p'
echo
echo "=== OidcError construction usages ==="
rg -n "new OidcError|oidcErr\.cause|instanceof OidcError|throw OidcError" app/shared/src -S
echo
echo "=== surrounding throw call sites ==="
python3 - <<'PY'
from pathlib import Path
text = Path('app/shared/src/api/auth/authStateMachine.ts').read_text()
for needle in ["new OidcError", "oidcErr.cause"]:
print(f"## {needle}")
for i,line in enumerate(text.splitlines(),1):
if needle in line:
start=max(1,i-8); end=min(len(text.splitlines()),i+8)
print(f"-- lines {start}-{end}:")
for n,l in enumerate(text.splitlines()[start-1:end], start):
print(f"{n}: {l}")
PY
echo
echo
echo "=== compiler semantic probe: class field redeclaration vs Error#cause ==="
tmp="$(mktemp -d)"
cat > "$tmp/probe.ts" <<'TS'
class OidcError extends Error {
code: string;
detail?: string;
cause?: unknown;
constructor(code: string, fallbackMessage: string, detail?: string) {
super(fallbackMessage);
this.name = 'OidcError';
this.code = code;
if (detail) this.detail = detail;
}
}
const err = new OidcError('code', 'fallback', 'detail');
console.log(JSON.stringify({ hasOwnCause: Object.prototype.hasOwnProperty.call(err, 'cause'), ownCauseValue: (err as any).cause }, null, 2));
TS
if command -v npx >/dev/null 2>&1; then
cd "$tmp"
npm init -y >/dev/null 2>&1
npm install --no-save typescript 2>&1 | tail -3
npx tsc probe.ts --strict --target ES2021 --module ESNext --lib ES2021 --useDefineForClassFields true 2>&1
echo "--- emitted js ---"
cat probe.js | sed -n '1,160p'
else
echo "npx not available; source probe.ts only"
cat "$tmp/probe.ts"
fi
rm -rf "$tmp"Repository: TokenDanceLab/AgentHub
Length of output: 8929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
cat > "$tmp/probe.ts" <<'TS'
class OidcError extends Error {
code: string;
detail?: string;
cause?: unknown;
constructor(code: string, fallbackMessage: string, detail?: string) {
super(fallbackMessage);
this.name = 'OidcError';
this.code = code;
if (detail) this.detail = detail;
}
}
const err = new OidcError('code', 'fallback', 'detail');
console.log(JSON.stringify({
hasOwnPropertyCause: Object.prototype.hasOwnProperty.call(err, 'cause'),
ownCauseValue: (err as any).cause
}, null, 2));
TS
cd "$tmp"
npm init -y >/dev/null 2>&1
npm install --no-save typescript 2>&1 | tail -3
npx tsc probe.ts --strict --target ES2021 --module ESNext --lib ES2021,DOM --useDefineForClassFields true 2>&1
echo "--- emitted js ---"
cat probe.js
cat probe.js | sed -n '1,220p'
echo
echo "--- class field redeclaration without lib cause ---"
cat > "$tmp/probe2.ts" <<'TS'
class A { cause?: unknown; }
class B extends A {
cause?: unknown;
constructor() {
super();
(this as any).cause = Object.create(null);
}
}
function show() {
const b = new B();
console.log(JSON.stringify({
hasOwnPropertyCause: Object.prototype.hasOwnProperty.call(b, 'cause'),
ownCauseValue: b.cause === Object.create(null),
ownCauseType: typeof b.cause
}));
}
show();
TS
npx tsc probe2.ts --strict --target ES2021 --module ESNext --lib ES2021,DOM --useDefineForClassFields true 2>&1
echo "--- emitted js ---"
cat probe2.js
cat probe2.js | sed -n '1,220p'
rm -rf "$tmp"Repository: TokenDanceLab/AgentHub
Length of output: 3091
Accept cause in OidcError and remove the redundant field.
app/shared/tsconfig.json uses ES2021, so extending Error preserves instanceof, and super() has no native cause here. Still, constructor callers currently assign cause as a second step for OIDC start and token exchange failures. Add cause?: unknown to the constructor, pass { cause } through super, and drop the class field so the native field is not shadowed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/shared/src/api/auth/types.ts` around lines 65 - 76, Update the OidcError
constructor to accept an optional cause argument, pass it through the Error
super call using the native cause option, and remove the class-level cause
declaration so it is not shadowed. Update OIDC start and token exchange callers
to provide cause during construction instead of assigning it afterward.
| save(pending: BrowserOIDCPending) { | ||
| if (typeof sessionStorage === 'undefined') return; | ||
| sessionStorage.setItem(OIDC_PENDING_KEY, JSON.stringify(pending)); | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard pendingStorage.save against storage exceptions.
readTokenSource (Line 36) and saveTokenSource (Line 49) both wrap storage access in try/catch. save does not. The typeof sessionStorage === 'undefined' check does not cover a throwing setItem. Safari private mode and quota exhaustion both throw here, which aborts loginWithTokenDance before the redirect.
🛡️ Suggested guard
save(pending: BrowserOIDCPending) {
if (typeof sessionStorage === 'undefined') return;
- sessionStorage.setItem(OIDC_PENDING_KEY, JSON.stringify(pending));
+ try {
+ sessionStorage.setItem(OIDC_PENDING_KEY, JSON.stringify(pending));
+ } catch {
+ /* storage disabled */
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| save(pending: BrowserOIDCPending) { | |
| if (typeof sessionStorage === 'undefined') return; | |
| sessionStorage.setItem(OIDC_PENDING_KEY, JSON.stringify(pending)); | |
| }, | |
| save(pending: BrowserOIDCPending) { | |
| if (typeof sessionStorage === 'undefined') return; | |
| try { | |
| sessionStorage.setItem(OIDC_PENDING_KEY, JSON.stringify(pending)); | |
| } catch { | |
| /* storage disabled */ | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/web/src/api/auth/webPorts.ts` around lines 97 - 100, Update
pendingStorage.save to wrap sessionStorage.setItem and JSON serialization in a
try/catch, matching the safeguards used by readTokenSource and saveTokenSource.
Preserve the undefined-storage guard and ensure storage failures are swallowed
so loginWithTokenDance can continue to the redirect.
| async start() { | ||
| if (typeof window === 'undefined') { | ||
| throw new Error('TokenDance ID login requires a browser window.'); | ||
| } | ||
| const redirectUri = `${window.location.origin}${OIDC_CALLBACK_PATH}`; | ||
| return { | ||
| redirectUri, | ||
| // The redirect unloads this document; the callback promise never | ||
| // settles — tryAutoLogin() processes the callback after the return. | ||
| callback: new Promise<OidcCallbackResult>(() => {}), | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the shared login path awaits the callback promise after opening the authorization URL.
fd -t f 'authStateMachine.ts' app/shared/src/api/auth --exec rg -n -C 6 'callbackChannel|redirectOpener|loginWithTokenDance' {}Repository: TokenDanceLab/AgentHub
Length of output: 4049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant auth files =="
fd -t f 'authStateMachine.ts|webPorts.ts|LoginForm.tsx|auth.*Port|auth.*Ports' app | sort
echo
echo "== webPorts relevant section =="
cat -n app/web/src/api/auth/webPorts.ts | sed -n '1,170p'
echo
echo "== authStateMachine relevant section =="
cat -n app/shared/src/api/auth/authStateMachine.ts | sed -n '218,314p'
echo
echo "== LoginForm handleTokenDanceLogin references =="
fd -t f 'LoginForm.tsx' app/web/src/components -x sh -c 'echo "--- $1"; ast-grep outline "$1" --view expanded 2>/dev/null | rg -n "handleTokenDanceLogin|tokenDance|identityLoading|loginWithTokenDance" -C 3 || cat -n "$1" | sed -n "1,140p"' sh {}
echo
echo "== Shared auth core tests around token dance =="
fd -t f '*.test.*' app | rg 'auth|TokenDance|tokenDance|authStateMachine|webPorts|LoginForm' | sort | head -80
for f in $(fd -t f '*.test.*' app | rg 'auth|TokenDance|tokenDance|authStateMachine|webPorts|LoginForm' | sort); do
echo "--- $f"
rg -n -C 5 'callbackChannel|redirectOpener|loginWithTokenDance|handleTokenDanceLogin|identityLoading|tokenDance' "$f" || true
doneRepository: TokenDanceLab/AgentHub
Length of output: 13947
Handle failed browser redirects before awaiting the never-resolving callback promise.
callbackChannel.start() returns a callback promise that only settles for local-callback-server mode. loginWithTokenDance() then awaits redirectOpener.open(authUrl) and immediately awaits callback. If window.location.assign throws or the browser blocks the navigation, the login promise never rejects, so LoginForm.handleTokenDanceLogin keeps identityLoading true and the login button disabled with no error shown. Add an error path for failures before the redirect occurs, or make the browser start() reject instead of returning a never-settling promise when navigation cannot be started.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/web/src/api/auth/webPorts.ts` around lines 119 - 130, The callback
promise returned by the browser start() method never settles, so when
window.location.assign fails or is blocked during the redirect, the login flow
hangs without rejecting. Modify the start() method to detect and handle redirect
failures: either wrap the redirect logic inside the callback promise so it can
reject when window.location.assign throws or is blocked, or wrap the redirect
call in a try-catch and have the callback promise reject with the caught error
if navigation cannot be initiated.
- 新增 @shared/api/auth：Hub 认证状态机 SSOT（OIDC PKCE、token 生命周期、refresh fallback、logout 清理），web/desktop 只注入平台 Port - web/desktop hubAuth.ts 变薄壳，保留 createHubAuth/HubAuth/HubAuthState/OidcError 表面 - 新增 webPorts/desktopPorts 平台实现；行为与旧实现兼容 - 补充 shared 状态机测试、desktop Tauri 模式 Port 测试、web 登录流与登录 UI 测试 Co-authored-by: Codex <codex@vectorcontrol.tech>
Summary
将 Web / Desktop 的 Hub 认证状态机抽取到
app/shared（issue #1537），两个平台只保留薄壳与平台 Port 注入。@shared/api/auth：Hub 认证状态机 SSOT（OIDC PKCE、token 生命周期、refresh fallback、logout 清理、snapshot/listener），纯 TypeScript，无浏览器/Tauri 依赖app/web/app/desktop的hubAuth.ts变薄壳：注入平台 Port（token 存储位置、OIDC callback 捕获、authorization URL 打开、device identity、hubStore 同步），保留历史createHubAuth()/HubAuthState/HubAuth/OidcError导出面，调用方零改动webPorts.ts（sessionStorage token、Vite 回调路由、当前窗口跳转）、desktopPorts.ts（Tauri 凭证库 + 本地 callback server + 系统浏览器跳转，Vite dev 走浏览器回调）测试
app/shared：197 文件 / 1758 测试通过，0 skippedapp/web：28 文件 / 230 测试通过（新增登录流 + LoginForm/AuthPage UI 测试）app/desktop：63 文件 / 596 测试通过（新增 Tauri 模式 Port 测试），edge-integration 24 测试通过verify-coverage-baseline.ps1：通过（shared/web 全部高于基线，desktop 在容差内）pnpm lint（0/0）、pnpm typecheck、webpnpm build通过；git diff --check干净app/shared的pnpm lint（tsc --noEmit）在 master 上即有大量既有失败（stories/test 文件），本次变更未引入新错误验证命令
pnpm test（shared / web / desktop）pnpm lint、pnpm typecheck（web / desktop）scripts/verify/verify-coverage-baseline.ps1Summary by CodeRabbit
New Features
Bug Fixes
Documentation