feat(admin): add route-based OAuth operations - #400
Open
xizhibei wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe change adds backend OAuth administration, wires a shared authorization flow through server and admin routes, introduces OAuth API/session state, and restructures the admin console into route-specific workspaces with OAuth actions and feedback. ChangesBackend OAuth operations
HTTP integration
API and session model
Admin console workspaces
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+318
to
+331
| router.post('/api/oauth/:serviceId/authorize', async (req, res) => { | ||
| if (!options.oauthService) { | ||
| res.status(503).json({ error: 'backend_oauth_runtime_unavailable' }); | ||
| return; | ||
| } | ||
|
|
||
| const serviceId = parseAdminOAuthServiceId(req, res); | ||
| if (!serviceId) return; | ||
| const result = await options.oauthService.authorizeService({ | ||
| context: buildAdminOperationContext(req, options, { type: 'backend_oauth_service', id: serviceId }), | ||
| serviceId, | ||
| }); | ||
| sendAdminOAuthOperationResult(res, result); | ||
| }); |
Comment on lines
+333
to
+346
| router.post('/api/oauth/:serviceId/restart', async (req, res) => { | ||
| if (!options.oauthService) { | ||
| res.status(503).json({ error: 'backend_oauth_runtime_unavailable' }); | ||
| return; | ||
| } | ||
|
|
||
| const serviceId = parseAdminOAuthServiceId(req, res); | ||
| if (!serviceId) return; | ||
| const result = await options.oauthService.restartService({ | ||
| context: buildAdminOperationContext(req, options, { type: 'backend_oauth_service', id: serviceId }), | ||
| serviceId, | ||
| }); | ||
| sendAdminOAuthOperationResult(res, result); | ||
| }); |
![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
🧹 Nitpick comments (11)
web/admin/src/controller.test.tsx (1)
405-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the stale authorization does not redirect.
The test proves the button re-enables after the session changes, but the real hazard is the in-flight promise resolving against the new session and calling
location.assign. Resolving at Line 439 without a follow-up assertion leaves that unverified (and unawaited).💚 Suggested addition
authorization.resolve({ serviceId: 'github', redirectUrl: 'https://provider.example/authorize' }); + await waitFor(() => expect(routeWindow.location.assign).not.toHaveBeenCalled());🤖 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 `@web/admin/src/controller.test.tsx` around lines 405 - 440, Extend the test around the deferred authorization in “clears an in-flight OAuth busy state when the Admin Session changes” by resolving authorization after the session change, awaiting pending UI updates, and asserting routeWindow.location.assign was not called. Ensure the assertion specifically verifies the stale OAuth result cannot redirect under the new session.src/domains/admin/adminDomain.test.ts (1)
182-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
createOAuthFlowinto the shared mock factories.This exact helper is duplicated verbatim in
src/domains/admin/adminOAuthService.test.ts(lines 249-258) andsrc/transport/http/routes/adminRoutes.test.ts(lines 2344-2353). Extracting it keeps theOAuthAuthorizationFlowmock in one place as the interface evolves.As per coding guidelines, "Use
test/unit-utils/MockFactories.tsfor consistent mock data and mock factories."🤖 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 `@src/domains/admin/adminDomain.test.ts` around lines 182 - 191, Move the duplicated createOAuthFlow helper into test/unit-utils/MockFactories.ts, export it as the shared OAuthAuthorizationFlow mock factory, and update adminDomain.test.ts, adminOAuthService.test.ts, and adminRoutes.test.ts to import and use it instead of local definitions.Source: Coding guidelines
web/admin/src/api/adminApi.ts (1)
429-463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two OAuth calls into one helper.
authorizeOAuthServiceandrestartOAuthServicediffer only in the path suffix and the idempotency action.♻️ Sketch
+ async function oauthOperation(action: 'authorize' | 'restart', input: { serviceId: string; csrfToken: string }) { + const response = await request<{ result: OAuthAuthorizationRedirectResult }>( + `/admin/api/oauth/${encodeURIComponent(input.serviceId)}/${action}`, + { + method: 'POST', + headers: { + 'X-CSRF-Token': input.csrfToken, + 'Idempotency-Key': idempotencyKey({ action: `oauth-${action}`, targetName: input.serviceId }), + }, + body: '{}', + }, + ); + return response.result; + }🤖 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 `@web/admin/src/api/adminApi.ts` around lines 429 - 463, Consolidate the duplicated request logic in authorizeOAuthService and restartOAuthService into a shared private/helper method parameterized by the OAuth path suffix and idempotency action. Keep both public methods’ existing signatures and return behavior, passing their respective values to the helper while preserving the CSRF header, POST body, and response handling.web/admin/src/components/AdminConsoleApp.tsx (2)
168-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an exhaustiveness guard on the route switch.
Adding a member to
AdminConsoleRoutewithout a case here makesConsoleWorkspacereturnundefined, which React 18 types accept as a valid node — so the gap surfaces as a blank workspace at runtime rather than a type error. Adefault: { const _exhaustive: never = navigation.route; return null; }makes it a compile-time failure.🤖 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 `@web/admin/src/components/AdminConsoleApp.tsx` around lines 168 - 196, Update the route switch in ConsoleWorkspace to add a default exhaustiveness guard that assigns navigation.route to never and returns null. This must make additions to AdminConsoleRoute fail compilation until a corresponding workspace case is implemented.
231-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isSamePageNavigationis duplicated verbatim across two modules. Both the sidebarNavItemand the dashboardSummaryLinkimplement the same modifier-key rules independently, so the two link behaviours can silently drift.
web/admin/src/components/AdminConsoleApp.tsx#L231-L233: remove the local definition and import the helper from a shared module (e.g.AdminConsoleShared.tsx).web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx#L243-L245: remove this copy and import the same shared helper.🤖 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 `@web/admin/src/components/AdminConsoleApp.tsx` around lines 231 - 233, The isSamePageNavigation helper is duplicated across both link modules; create or reuse one shared export, remove the local definitions, and import that shared helper in web/admin/src/components/AdminConsoleApp.tsx (lines 231-233) and web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx (lines 243-245), preserving the existing modifier-key behavior at both call sites.web/admin/src/components/AdminConsoleApp.test.tsx (1)
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSummary-card counts are asserted only in aggregate.
getAllByText('1').length >= 4passes as long as four nodes anywhere contain "1"; it never ties a count to its card. Scoping each value to its own card (e.g.within(screen.getByText('Enabled servers').closest('a')!)) would actually catch a mis-wired counter.🤖 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 `@web/admin/src/components/AdminConsoleApp.test.tsx` around lines 59 - 63, Update the summary-card assertions in the AdminConsoleApp test so each count is queried within its corresponding card or link, using the card label such as “Enabled servers,” “Disabled servers,” “OAuth attention,” and “Failed audits” to locate its container. Replace the aggregate getAllByText('1') check with individual scoped assertions that verify each card displays the expected count.web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx (2)
156-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
WorkspaceHeadingis shared but lives in a route-specific module.
OAuthServicesWorkspaceimports it fromRuntimeOperationsWorkspace, which couples unrelated workspaces.AdminConsoleShared.tsx(already home toPanel/DetailRow/EmptyState) is the natural location.🤖 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 `@web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx` around lines 156 - 166, Move the shared WorkspaceHeading component and its associated props typing from RuntimeOperationsWorkspace.tsx into AdminConsoleShared.tsx, then update OAuthServicesWorkspace and any other consumers to import it from AdminConsoleShared. Remove the route-specific export/import dependency while preserving the component’s existing API and behavior.
133-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the copy-feedback state + handler into a hook.
copyText/copyFeedbackhere is byte-identical toDashboardWorkspace(Lines 23-33), andOAuthServicesWorkspace.copyServiceIdis a third near-copy. A smalluseCopyFeedback(configuredServers.copy)hook returning{ message, copy }would collapse all three.🤖 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 `@web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx` around lines 133 - 140, Extract the copy feedback state and copyText handler into a reusable useCopyFeedback hook accepting configuredServers.copy and returning { message, copy }. Replace the local copyFeedback state and copyText implementation in RuntimeOperationsWorkspace with the hook, preserving the existing success and failure messages, then reuse the hook in DashboardWorkspace and OAuthServicesWorkspace.copyServiceId to remove the duplicate logic.src/domains/admin/adminOAuthService.ts (1)
28-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared status→code mapping and preserving the original error.
authorizeServiceandrestartServiceare structurally identical except for the flow call and the success status. Also, thecatch {}discards the underlying error entirely, so nothing (not even a log line) retains why the flow threw — that makes production triage ofbackend_oauth_authorization_start_failedhard.♻️ Sketch: shared helper with cause preservation
+ private async runOAuthOperation( + input: AdminOAuthOperationInput, + operationName: string, + start: () => Promise<{ status: string; redirectUrl?: string }>, + successStatus: string, + ): Promise<AdminOperationResult<AdminOAuthRedirectResult>> { + return this.options.operationService.executeMutation({ + context: { ...input.context, target: { type: 'backend_oauth_service', id: input.serviceId } }, + operationName, + run: async () => { + let result; + try { + result = await start(); + } catch (error) { + throw new Error('backend_oauth_authorization_start_failed', { cause: error }); + } + if (result.status === 'service_not_found') throw new Error('backend_oauth_service_not_found'); + if (result.status === 'runtime_unavailable') throw new Error('backend_oauth_runtime_unavailable'); + if (result.status !== successStatus || !result.redirectUrl) { + throw new Error('backend_oauth_authorization_start_failed'); + } + return { serviceId: input.serviceId, redirectUrl: result.redirectUrl }; + }, + }); + }🤖 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 `@src/domains/admin/adminOAuthService.ts` around lines 28 - 94, Extract the duplicated OAuth result-status validation from authorizeService and restartService into a shared helper, parameterized for the expected success status and redirect requirement, while preserving the existing error-code mapping. Update both methods to use the helper and change their catch blocks to retain the original thrown error as the cause when raising backend_oauth_authorization_start_failed.test/e2e/admin-spa-browser-smoke.e2e.test.ts (2)
196-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
elementHandle()is nullable and the handle is never disposed.If it resolves to
nullthe comparison can never be true and the test fails as an opaquewaitForFunctiontimeout.applyButton.evaluate((el) => el === globalThis.document.activeElement)(retried) avoids both the nullable handle and the leak.🤖 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 `@test/e2e/admin-spa-browser-smoke.e2e.test.ts` around lines 196 - 197, Update the apply-button focus assertion around applyButton to use applyButton.evaluate with a retried wait instead of elementHandle and waitForFunction. Remove the nullable handle path and ensure no element handle is created or leaked while preserving the active-element verification.
306-319: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueListeners are added per call and never removed.
If
expectCenteredLoginGateruns more than once against the samepage, theconsole/pageerrorhandlers accumulate and duplicate captures. Also,page.locator('body').innerText()inside thecatchcan itself throw and mask the original failure — wrap it or fall back to'unavailable'.🤖 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 `@test/e2e/admin-spa-browser-smoke.e2e.test.ts` around lines 306 - 319, The expectCenteredLoginGate flow must avoid accumulating console and pageerror listeners across repeated calls on the same page; register handlers for the current invocation and remove them before returning or rethrowing. In its error path, safely obtain the body text with a fallback of “unavailable” so failures from innerText do not replace the original assertion error.
🤖 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 `@src/transport/http/routes/adminRoutes.ts`:
- Around line 1240-1243: Update operationNameForRequest to explicitly recognize
/api/oauth/:serviceId/authorize and /restart, returning the corresponding
operation names used by AdminOperationService instead of falling through to
listConfiguredServers. Ensure both OAuth fingerprint call sites, including the
backendOAuthRequestFingerprint construction, receive the correct distinct
operation name while preserving existing behavior for other routes.
In `@web/admin/src/components/workspaces/OAuthServicesWorkspace.tsx`:
- Around line 130-134: Keep the aria-live Text in OAuthServicesWorkspace.tsx
always mounted and conditionally provide only its authorization status message.
In RuntimeOperationsWorkspace.tsx, update CopyFeedback so its Alert or wrapping
live region is always rendered and swaps between feedback messages instead of
returning null; apply these changes at both listed sites.
In `@web/admin/src/session/AdminConsoleSession.tsx`:
- Around line 85-87: Update the oauthCallbackFeedback initialization around
oauthCallbackOutcome to immediately scrub the OAuth callback query parameters
with history.replaceState, navigating to /admin/oauth while preserving the
current feedback value. Ensure the URL is cleaned after the outcome is read so
feedback does not reappear on reload or shared links.
- Around line 522-531: Update the error-message lookup in AdminConsoleSession so
query-supplied values such as toString, constructor, and valueOf cannot resolve
through Object.prototype. Use a null-prototype map or an own-property check,
while preserving the existing fallback message for unknown OAuth error keys and
returning the same error object shape.
---
Nitpick comments:
In `@src/domains/admin/adminDomain.test.ts`:
- Around line 182-191: Move the duplicated createOAuthFlow helper into
test/unit-utils/MockFactories.ts, export it as the shared OAuthAuthorizationFlow
mock factory, and update adminDomain.test.ts, adminOAuthService.test.ts, and
adminRoutes.test.ts to import and use it instead of local definitions.
In `@src/domains/admin/adminOAuthService.ts`:
- Around line 28-94: Extract the duplicated OAuth result-status validation from
authorizeService and restartService into a shared helper, parameterized for the
expected success status and redirect requirement, while preserving the existing
error-code mapping. Update both methods to use the helper and change their catch
blocks to retain the original thrown error as the cause when raising
backend_oauth_authorization_start_failed.
In `@test/e2e/admin-spa-browser-smoke.e2e.test.ts`:
- Around line 196-197: Update the apply-button focus assertion around
applyButton to use applyButton.evaluate with a retried wait instead of
elementHandle and waitForFunction. Remove the nullable handle path and ensure no
element handle is created or leaked while preserving the active-element
verification.
- Around line 306-319: The expectCenteredLoginGate flow must avoid accumulating
console and pageerror listeners across repeated calls on the same page; register
handlers for the current invocation and remove them before returning or
rethrowing. In its error path, safely obtain the body text with a fallback of
“unavailable” so failures from innerText do not replace the original assertion
error.
In `@web/admin/src/api/adminApi.ts`:
- Around line 429-463: Consolidate the duplicated request logic in
authorizeOAuthService and restartOAuthService into a shared private/helper
method parameterized by the OAuth path suffix and idempotency action. Keep both
public methods’ existing signatures and return behavior, passing their
respective values to the helper while preserving the CSRF header, POST body, and
response handling.
In `@web/admin/src/components/AdminConsoleApp.test.tsx`:
- Around line 59-63: Update the summary-card assertions in the AdminConsoleApp
test so each count is queried within its corresponding card or link, using the
card label such as “Enabled servers,” “Disabled servers,” “OAuth attention,” and
“Failed audits” to locate its container. Replace the aggregate getAllByText('1')
check with individual scoped assertions that verify each card displays the
expected count.
In `@web/admin/src/components/AdminConsoleApp.tsx`:
- Around line 168-196: Update the route switch in ConsoleWorkspace to add a
default exhaustiveness guard that assigns navigation.route to never and returns
null. This must make additions to AdminConsoleRoute fail compilation until a
corresponding workspace case is implemented.
- Around line 231-233: The isSamePageNavigation helper is duplicated across both
link modules; create or reuse one shared export, remove the local definitions,
and import that shared helper in web/admin/src/components/AdminConsoleApp.tsx
(lines 231-233) and
web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx (lines
243-245), preserving the existing modifier-key behavior at both call sites.
In `@web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx`:
- Around line 156-166: Move the shared WorkspaceHeading component and its
associated props typing from RuntimeOperationsWorkspace.tsx into
AdminConsoleShared.tsx, then update OAuthServicesWorkspace and any other
consumers to import it from AdminConsoleShared. Remove the route-specific
export/import dependency while preserving the component’s existing API and
behavior.
- Around line 133-140: Extract the copy feedback state and copyText handler into
a reusable useCopyFeedback hook accepting configuredServers.copy and returning {
message, copy }. Replace the local copyFeedback state and copyText
implementation in RuntimeOperationsWorkspace with the hook, preserving the
existing success and failure messages, then reuse the hook in DashboardWorkspace
and OAuthServicesWorkspace.copyServiceId to remove the duplicate logic.
In `@web/admin/src/controller.test.tsx`:
- Around line 405-440: Extend the test around the deferred authorization in
“clears an in-flight OAuth busy state when the Admin Session changes” by
resolving authorization after the session change, awaiting pending UI updates,
and asserting routeWindow.location.assign was not called. Ensure the assertion
specifically verifies the stale OAuth result cannot redirect under the new
session.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64a06a1e-1653-402c-bfc5-70a24e6a30fc
📒 Files selected for processing (24)
src/domains/admin/adminDomain.test.tssrc/domains/admin/adminDomain.tssrc/domains/admin/adminOAuthService.test.tssrc/domains/admin/adminOAuthService.tssrc/transport/http/routes/adminRoutes.test.tssrc/transport/http/routes/adminRoutes.tssrc/transport/http/routes/oauthRoutes.test.tssrc/transport/http/routes/oauthRoutes.tssrc/transport/http/server.test.tssrc/transport/http/server.tstest/e2e/admin-spa-browser-smoke.e2e.test.tsweb/admin/src/api/adminApi.test.tsweb/admin/src/api/adminApi.tsweb/admin/src/components/AdminConsoleApp.fixtures.tsxweb/admin/src/components/AdminConsoleApp.test.tsxweb/admin/src/components/AdminConsoleApp.tsxweb/admin/src/components/OperationsStatusPanels.tsxweb/admin/src/components/adminConsoleUtils.test.tsweb/admin/src/components/workspaces/OAuthServicesWorkspace.tsxweb/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsxweb/admin/src/controller.test.tsxweb/admin/src/session/AdminConsoleSession.tsxweb/admin/src/session/AdminConsoleSessionModel.tsweb/admin/src/styles.css
Comment on lines
+1240
to
+1243
| requestFingerprint: | ||
| target.type === 'backend_oauth_service' | ||
| ? backendOAuthRequestFingerprint(operationName, target.id) | ||
| : configuredServerRequestFingerprint(operationName, target.id, req.body), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
OAuth fingerprints carry the wrong operationName.
operationNameForRequest (Lines 1436-1453) has no branch for /api/oauth/:serviceId/authorize|restart, so both fall through to listConfiguredServers. The resulting fingerprint is identical for authorize and restart on the same service and misnames the operation. It isn't currently exploitable because AdminOperationService scopes the idempotency key by the real operation name, but the value is misleading and fragile if scoping ever changes.
🔧 Proposed fix
function operationNameForRequest(req: Request): string {
+ if (req.path.startsWith('/api/oauth/')) {
+ return req.path.endsWith('/restart') ? 'restartBackendOAuth' : 'authorizeBackendOAuth';
+ }
if (req.path.endsWith('/enable') || req.path.endsWith('/enable-server')) {Also applies to: 1351-1360
🤖 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 `@src/transport/http/routes/adminRoutes.ts` around lines 1240 - 1243, Update
operationNameForRequest to explicitly recognize /api/oauth/:serviceId/authorize
and /restart, returning the corresponding operation names used by
AdminOperationService instead of falling through to listConfiguredServers.
Ensure both OAuth fingerprint call sites, including the
backendOAuthRequestFingerprint construction, receive the correct distinct
operation name while preserving existing behavior for other routes.
Comment on lines
+130
to
+134
| {busy ? ( | ||
| <Text size="xs" c="dimmed" aria-live="polite"> | ||
| {busy === 'authorize' ? 'Starting authorization...' : 'Restarting authorization...'} | ||
| </Text> | ||
| ) : null} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
aria-live regions are mounted together with their content, so nothing is announced. Assistive tech only announces mutations to a live region that was already in the DOM; conditionally rendering the element that carries aria-live defeats it. Both new feedback surfaces share this pattern.
web/admin/src/components/workspaces/OAuthServicesWorkspace.tsx#L130-L134: keep theTextwitharia-live="polite"always mounted and toggle only its message.web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx#L235-L241: render theAlert(or a wrapping live region) unconditionally inCopyFeedbackand swap the text instead of returningnull.
📍 Affects 2 files
web/admin/src/components/workspaces/OAuthServicesWorkspace.tsx#L130-L134(this comment)web/admin/src/components/workspaces/RuntimeOperationsWorkspace.tsx#L235-L241
🤖 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 `@web/admin/src/components/workspaces/OAuthServicesWorkspace.tsx` around lines
130 - 134, Keep the aria-live Text in OAuthServicesWorkspace.tsx always mounted
and conditionally provide only its authorization status message. In
RuntimeOperationsWorkspace.tsx, update CopyFeedback so its Alert or wrapping
live region is always rendered and swaps between feedback messages instead of
returning null; apply these changes at both listed sites.
Comment on lines
+85
to
+87
| const [oauthCallbackFeedback] = useState<OAuthFeedback | null>(() => | ||
| oauthCallbackOutcome(windowRef.location?.search ?? ''), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Callback feedback sticks to the URL across reloads.
oauthCallbackFeedback is read once from location.search but the query string is never scrubbed, so ?success=1 / ?error=... stays in the address bar and the banner re-appears on every reload or shared link. Consider a history.replaceState to /admin/oauth right after reading the outcome.
🤖 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 `@web/admin/src/session/AdminConsoleSession.tsx` around lines 85 - 87, Update
the oauthCallbackFeedback initialization around oauthCallbackOutcome to
immediately scrub the OAuth callback query parameters with history.replaceState,
navigating to /admin/oauth while preserving the current feedback value. Ensure
the URL is cleaned after the outcome is read so feedback does not reappear on
reload or shared links.
Comment on lines
+522
to
+531
| const error = query.get('error'); | ||
| if (!error) return null; | ||
| const message = | ||
| { | ||
| access_denied: 'OAuth authorization was denied.', | ||
| missing_code: 'The OAuth provider callback did not include an authorization code.', | ||
| callback_failed: 'The runtime could not complete the OAuth provider callback.', | ||
| runtime_unavailable: 'The OAuth runtime became unavailable during the callback.', | ||
| }[error] ?? 'OAuth authorization did not complete.'; | ||
| return { kind: 'error', message }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prototype keys leak through the message lookup.
error comes straight from the URL query, and indexing an object literal reaches the prototype chain. /admin/oauth?error=toString (or constructor, valueOf) resolves to a function, ?? does not fall back, and message is then rendered by FeedbackAlert as a React child. Use a null-prototype map or an own-property check.
🐛 Proposed fix
+const OAUTH_CALLBACK_MESSAGES: Record<string, string> = Object.assign(Object.create(null), {
+ access_denied: 'OAuth authorization was denied.',
+ missing_code: 'The OAuth provider callback did not include an authorization code.',
+ callback_failed: 'The runtime could not complete the OAuth provider callback.',
+ runtime_unavailable: 'The OAuth runtime became unavailable during the callback.',
+});
+
function oauthCallbackOutcome(search: string): OAuthFeedback | null {
const query = new URLSearchParams(search);
if (query.get('success') === '1') {
return { kind: 'success', message: 'OAuth authorization completed.' };
}
const error = query.get('error');
if (!error) return null;
- const message =
- {
- access_denied: 'OAuth authorization was denied.',
- missing_code: 'The OAuth provider callback did not include an authorization code.',
- callback_failed: 'The runtime could not complete the OAuth provider callback.',
- runtime_unavailable: 'The OAuth runtime became unavailable during the callback.',
- }[error] ?? 'OAuth authorization did not complete.';
+ const message = OAUTH_CALLBACK_MESSAGES[error] ?? 'OAuth authorization did not complete.';
return { kind: 'error', message };
}📝 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.
Suggested change
| const error = query.get('error'); | |
| if (!error) return null; | |
| const message = | |
| { | |
| access_denied: 'OAuth authorization was denied.', | |
| missing_code: 'The OAuth provider callback did not include an authorization code.', | |
| callback_failed: 'The runtime could not complete the OAuth provider callback.', | |
| runtime_unavailable: 'The OAuth runtime became unavailable during the callback.', | |
| }[error] ?? 'OAuth authorization did not complete.'; | |
| return { kind: 'error', message }; | |
| const OAUTH_CALLBACK_MESSAGES: Record<string, string> = Object.assign(Object.create(null), { | |
| access_denied: 'OAuth authorization was denied.', | |
| missing_code: 'The OAuth provider callback did not include an authorization code.', | |
| callback_failed: 'The runtime could not complete the OAuth provider callback.', | |
| runtime_unavailable: 'The OAuth runtime became unavailable during the callback.', | |
| }); | |
| function oauthCallbackOutcome(search: string): OAuthFeedback | null { | |
| const query = new URLSearchParams(search); | |
| if (query.get('success') === '1') { | |
| return { kind: 'success', message: 'OAuth authorization completed.' }; | |
| } | |
| const error = query.get('error'); | |
| if (!error) return null; | |
| const message = OAUTH_CALLBACK_MESSAGES[error] ?? 'OAuth authorization did not complete.'; | |
| return { kind: 'error', message }; | |
| } |
🤖 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 `@web/admin/src/session/AdminConsoleSession.tsx` around lines 522 - 531, Update
the error-message lookup in AdminConsoleSession so query-supplied values such as
toString, constructor, and valueOf cannot resolve through Object.prototype. Use
a null-prototype map or an own-property check, while preserving the existing
fallback message for unknown OAuth error keys and returning the same error
object shape.
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.
Description
Add first-class, route-backed Admin Console workspaces and OAuth operations. The Admin SPA now uses stable /admin routes for dashboard, server inventory, OAuth services, audit trail, presets, and about views. Backend OAuth authorize and restart actions flow through authenticated, CSRF-protected, idempotent Admin Operations endpoints, with sanitized service identities and callback redirects returning operators to /admin/oauth.
The change also splits the runtime workspace into focused views and extends domain, route, UI, session, and browser smoke coverage.
Type of Change
Testing
Checklist
Verification
Summary by CodeRabbit