refactor: implement local-session for internal client auth
#1606
Conversation
WalkthroughRemoved API-key-based CLI/admin key services and dev keys; introduced local-session authentication (service, strategy, lifecycle, guard and AuthService integration); added a canonical tokenized internal GraphQL client and generalized factory to accept API-key, cookie, or local-session auth; updated DI tokens, modules, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / internal caller
participant Canonical as CanonicalInternalClientService
participant Local as LocalSessionService
participant Factory as InternalGraphQLClientFactory
CLI->>Canonical: getClient({ enableSubscriptions?, origin? })
alt cached
Canonical-->>CLI: ApolloClient (cached)
else create
Canonical->>Local: getLocalSession()
Local-->>Canonical: token | null
Canonical->>Factory: createClient({ getLocalSession, getCookieAuth?, getApiKey?, enableSubscriptions, origin })
Factory-->>Canonical: ApolloClient
Canonical-->>CLI: ApolloClient
end
sequenceDiagram
autonumber
participant Request as Incoming HTTP request
participant Guard as AuthenticationGuard
participant LSS as LocalSessionStrategy
participant AS as AuthService
participant LSV as LocalSessionService
Request->>Guard: authenticate (ServerHeader, LocalSession, UserCookie)
Guard->>LSS: validate(request)
LSS->>AS: validateLocalSession(token)
AS->>LSV: validateLocalSession(token)
LSV-->>AS: boolean
AS->>AS: getLocalSessionUser(), sync roles
AS-->>LSS: UserAccount | null
LSS-->>Guard: UserAccount | null
Guard-->>Request: Continue or 401
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @elibosley's task —— View job Code Review in Progress
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
api/src/unraid-api/cli/internal-client.service.ts (2)
18-21: Cache the CLI key to avoid repeated disk I/O in getApiKey callbacksgetApiKey is invoked for each client request; repeatedly hitting the filesystem is unnecessary since the CLI key is stable for the process lifetime. Cache after first successful read (file or service), and reuse.
Apply this diff:
export class CliInternalClientService { private readonly logger = new Logger(CliInternalClientService.name); private client: ApolloClient<NormalizedCacheObject> | null = null; private creatingClient: Promise<ApolloClient<NormalizedCacheObject>> | null = null; + // Cache to avoid repeated FS reads per request + private cachedApiKey: string | null = null; /** * Get the admin API key for CLI operations. * First tries to read from /var/run/unraid-api/cli.key, then falls back to AdminKeyService. */ private async getLocalApiKey(): Promise<string> { + if (this.cachedApiKey) { + return this.cachedApiKey; + } // Try to read ephemeral key from /var/run first try { const ephemeralKey = await readFile('/var/run/unraid-api/cli.key', 'utf-8'); if (ephemeralKey) { this.logger.debug('Using ephemeral CLI key from runtime directory'); - return ephemeralKey.trim(); + this.cachedApiKey = ephemeralKey.trim(); + return this.cachedApiKey; } } catch (error) { // File doesn't exist or not readable, fall back to service this.logger.debug('Ephemeral key file not found, falling back to AdminKeyService'); } // Fall back to AdminKeyService try { - return await this.adminKeyService.getOrCreateLocalAdminKey(); + const key = await this.adminKeyService.getOrCreateLocalAdminKey(); + this.cachedApiKey = key; + return key; } catch (error) { this.logger.error('Failed to get admin API key:', error); throw new Error( 'Unable to get admin API key for internal client. Ensure the API server is running.' ); } }Also applies to: 33-53
41-43: Improve debug signal for read failuresThe log implies “file not found,” but permission or other IO errors will also land here. Include error.code to aid triage without increasing verbosity.
Apply this diff:
- } catch (error) { - // File doesn't exist or not readable, fall back to service - this.logger.debug('Ephemeral key file not found, falling back to AdminKeyService'); + } catch (error: any) { + // File missing or unreadable; fall back to service + this.logger.debug( + `Ephemeral key file unavailable (reason: ${error?.code ?? 'unknown'}), falling back to AdminKeyService` + ); }api/src/unraid-api/cli/admin-key.service.ts (2)
4-4: Remove unused importjoin is not used.
Apply this diff:
-import { join } from 'path';
53-58: Tighten runtime directory permissionsSince the key is sensitive, prefer a restrictive directory mode when creating /var/run/unraid-api. Existing dirs won’t be changed, but this helps on first creation.
Apply this diff:
- await mkdir(runDir, { recursive: true }); + await mkdir(runDir, { recursive: true, mode: 0o700 }); await writeFile( AdminKeyService.RUNTIME_KEY_PATH, this.ephemeralKey, { mode: 0o600 } // Root only );packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts (1)
96-106: On-demand token generation is fine; consider guarding for interface driftIf updating @unraid/shared is delayed, the extended type handles compile-time. Optionally, at runtime, guard for missing method and fall back to a persistent key to avoid startup failure.
Proposed pattern (no diff required):
- If 'registerModule' in this.apiKeyService is not a function, call this.apiKeyService.getOrCreateLocalKey('Module-Connect', 'Persistent key fallback', [Role.CONNECT]) and use that as the token.
api/src/unraid-api/auth/api-key.service.ts (3)
7-7: Remove unused imports (watch, environment)File watching was removed and environment isn’t referenced.
Apply this diff:
-import { watch } from 'chokidar'; @@ -import { environment } from '@app/environment.js';Also applies to: 14-14
101-109: Consider allowing hyphens/underscores in persistent key namesEphemeral names like “Module-connect” contain hyphens; create() disallows them via sanitizeName. For consistency and to avoid surprising UX, broaden allowed chars.
Apply this diff:
- if (/^[\p{L}\p{N} ]+$/u.test(name)) { + if (/^[\p{L}\p{N}_\- ]+$/u.test(name)) { return name; } else { throw new GraphQLError( 'API key name must contain only letters, numbers, and spaces (Unicode letters are supported)' ); }And update the error message accordingly if you adopt this change.
277-295: Minor DRY opportunity in lookupsfindByField and findByKey duplicate iteration logic. A small helper to query ephemeral first, then persistent, would reduce drift. Low priority.
Example (no diff required):
- Implement a private iterateKeys(): Iterable that yields ephemeral then persistent; reuse in both methods.
Also applies to: 297-315
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
api/src/unraid-api/auth/api-key.service.ts(7 hunks)api/src/unraid-api/cli/admin-key.service.ts(2 hunks)api/src/unraid-api/cli/internal-client.service.ts(2 hunks)packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/cli/internal-client.service.tspackages/unraid-api-plugin-connect/src/authn/connect-api-key.service.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/admin-key.service.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/cli/internal-client.service.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/admin-key.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/cli/internal-client.service.tspackages/unraid-api-plugin-connect/src/authn/connect-api-key.service.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/admin-key.service.ts
🧠 Learnings (4)
📚 Learning: 2024-11-04T20:44:46.432Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying `apiKey.roles` in `removeRoleFromApiKey` and `addRoleToApiKey` within `api/src/unraid-api/auth/auth.service.ts`, concurrency issues are not a concern because the keys are stored in the file system.
Applied to files:
api/src/unraid-api/cli/internal-client.service.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/admin-key.service.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/cli/internal-client.service.ts
📚 Learning: 2024-11-04T20:41:22.303Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:41:22.303Z
Learning: In `api/src/unraid-api/auth/auth.service.ts`, the `addRoleToApiKey` function operates on API keys stored as JSON files in a directory, not a database, so concurrency is not a concern for modifying `apiKey.roles`.
Applied to files:
api/src/unraid-api/cli/internal-client.service.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/admin-key.service.ts
📚 Learning: 2024-11-15T16:22:03.485Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:176-188
Timestamp: 2024-11-15T16:22:03.485Z
Learning: Atomic writes are not required for the `saveApiKey` method in `ApiKeyService` (`api/src/unraid-api/auth/api-key.service.ts`) unless specifically needed.
Applied to files:
api/src/unraid-api/cli/internal-client.service.tsapi/src/unraid-api/auth/api-key.service.tsapi/src/unraid-api/cli/admin-key.service.ts
🧬 Code Graph Analysis (3)
packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts (2)
api/src/unraid-api/auth/api-key.service.ts (1)
Injectable(25-555)packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.ts (1)
Injectable(13-74)
api/src/unraid-api/auth/api-key.service.ts (2)
web/composables/gql/graphql.ts (1)
ApiKeyWithSecret(202-211)api/src/utils.ts (1)
batchProcess(58-71)
api/src/unraid-api/cli/admin-key.service.ts (2)
api/src/unraid-api/cli/internal-client.service.ts (1)
Injectable(16-111)api/src/unraid-api/auth/api-key.service.ts (1)
Injectable(25-555)
🪛 GitHub Actions: CI - Main (API)
packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts
[error] 21-21: Step 'npm run build' failed in package 'packages/unraid-api-plugin-connect' with TS2339: Property 'registerModule' does not exist on type 'ApiKeyService' (packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts:21:57).
🪛 GitHub Check: CodeQL
api/src/unraid-api/cli/admin-key.service.ts
[notice] 4-4: Unused variable, import, function or class
Unused import join.
🔇 Additional comments (5)
api/src/unraid-api/cli/internal-client.service.ts (1)
66-99: Client creation concurrency guard looks solidThe promise-sharing pattern with the equality check prevents double-creation and misassignment. finally resets creatingClient only if current. Good.
api/src/unraid-api/cli/admin-key.service.ts (1)
38-64: Ephemeral CLI key generation and in-memory registration aligns with the new modelSecure random generation, in-memory registration via ApiKeyService, and best-effort write to /var/run provide a robust flow with graceful fallback.
Also applies to: 66-80
packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts (2)
1-16: Imports follow ESM .js specifiers; goodImports use .js where applicable for ESM compatibility with our TS settings.
18-26: Use pnpm to rebuild only the Connect plugin after the type fixThe monorepo uses pnpm workspaces rather than npm workspaces, so the original
npm run build -w …command will not work. To verify that the TS2339 error has been resolved, rebuild just the Connect package with:pnpm --filter unraid-api-plugin-connect run buildThis will compile
packages/unraid-api-plugin-connectin isolation and confirm that the type extension fix removes the TS2339 error.Likely an incorrect or invalid review comment.
api/src/unraid-api/auth/api-key.service.ts (1)
39-54: Separation of persistent vs. ephemeral keys is coherent and internally consistent
- Startup loads disk-backed keys, cleans legacy, and rebuilds the in-memory view.
- Lookups prioritize ephemeral keys; findAll returns only persistent.
- delete/update correctly distinguish persistence boundaries and refresh caches.
Also applies to: 68-75, 321-369, 431-455, 490-498
packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts
Outdated
Show resolved
Hide resolved
packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts
Outdated
Show resolved
Hide resolved
pujitm
force-pushed
the
feat/memory-keys
branch
3 times, most recently
from
b87d158 to
1c6fcfb
Compare
pujitm
changed the title
local-session for internal client auth
pujitm
changed the title
local-session for internal client authlocal-session for internal client auth
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts (1)
26-26: Controller is registered as a provider; routes won’t be mountedNestJS requires controllers to be listed under the
controllersarray, notproviders. With the current registration, HTTP routes fromMothershipControllerwill not be exposed.Apply this diff to move the controller to the correct slot:
@Module({ imports: [RemoteAccessModule], providers: [ ConnectStatusWriterService, MothershipConnectionService, MothershipGraphqlClientService, MothershipHandler, MothershipSubscriptionHandler, TimeoutCheckerJob, CloudService, CloudResolver, - MothershipController, ], + controllers: [MothershipController], exports: [], }) export class MothershipModule {}packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (1)
137-144: Validate port server-side for static forwarding.UI validation exists, but API should also enforce bounds when access is enabled with STATIC forwarding.
if (settings.accessType) { + if ( + settings.accessType === WAN_ACCESS_TYPE.ALWAYS && + settings.forwardType === WAN_FORWARD_TYPE.STATIC + ) { + const p = Number(settings.port); + if (!Number.isInteger(p) || p < 1 || p > 65535) { + throw new GraphQLError( + 'Port must be an integer between 1 and 65535 when using static port forwarding.' + ); + } + } await this.updateRemoteAccess({ accessType: settings.accessType, forwardType: settings.forwardType, port: settings.port, }); }api/src/unraid-api/auth/authentication.guard.ts (1)
98-105: Potential crash parsing cookies on GraphQL WS handshake when no Cookie headerIf the Cookie header is missing, headerIndex becomes -1 and rawHeaders[headerIndex + 1] indexes the first header value, and parseCookies(undefined) will throw. Guard against missing/invalid header values.
Apply this diff:
- if (fullContext.connectionParams) { - const rawHeaders = fullContext.req.extra.request.rawHeaders; - const headerIndex = rawHeaders.findIndex( - (headerOrValue) => headerOrValue.toLowerCase() === 'cookie' - ); - const cookieString = rawHeaders[headerIndex + 1]; - request.cookies = parseCookies(cookieString); - } + if (fullContext.connectionParams) { + const rawHeaders = fullContext.req?.extra?.request?.rawHeaders ?? []; + const headerIndex = rawHeaders.findIndex( + (h) => typeof h === 'string' && h.toLowerCase() === 'cookie' + ); + if (headerIndex !== -1 && typeof rawHeaders[headerIndex + 1] === 'string') { + const cookieString = rawHeaders[headerIndex + 1] as string; + request.cookies = parseCookies(cookieString); + } + }api/src/unraid-api/cli/api-report.service.ts (1)
146-154: Avoid unhandled rejections with Promise.race timeouts; factor awithTimeouthelperUsing
Promise.racewith a rejecting timeout Promise can produce unhandledRejection warnings when the query resolves first and the timer later rejects. Wrap the awaited promise so the timer is always cleared and only a single promise settles.Apply these diffs to replace each timeout block and add a helper:
- systemResult = await Promise.race([ - client.query({ - query: SYSTEM_REPORT_QUERY, - }), - new Promise<never>((_, reject) => - setTimeout(() => reject(new Error('Query timeout after 10 seconds')), 10000) - ), - ]); + systemResult = await this.withTimeout( + client.query({ query: SYSTEM_REPORT_QUERY }), + 'System query', + 10_000 + ); @@ - const connectResult = await Promise.race([ - client.query({ - query: CONNECT_STATUS_QUERY, - }), - new Promise<never>((_, reject) => - setTimeout(() => reject(new Error('Connect query timeout after 10 seconds')), 10000) - ), - ]); + const connectResult = await this.withTimeout( + client.query({ query: CONNECT_STATUS_QUERY }), + 'Connect query', + 10_000 + ); @@ - const servicesResult = await Promise.race([ - client.query({ - query: SERVICES_QUERY, - }), - new Promise<never>((_, reject) => - setTimeout(() => reject(new Error('Services query timeout after 10 seconds')), 10000) - ), - ]); + const servicesResult = await this.withTimeout( + client.query({ query: SERVICES_QUERY }), + 'Services query', + 10_000 + );Add this helper method (outside the selected ranges):
private withTimeout<T>(promise: Promise<T>, label: string, ms = 10_000): Promise<T> { return new Promise<T>((resolve, reject) => { const timer = setTimeout(() => reject(new Error(`${label} timeout after ${ms / 1000} seconds`)), ms); promise.then( (val) => { clearTimeout(timer); resolve(val); }, (err) => { clearTimeout(timer); reject(err); }, ); }); }Also applies to: 165-172, 181-187
🧹 Nitpick comments (29)
api/src/unraid-api/auth/cookie.service.ts (2)
70-75: Improve error logging to preserve stack and context
Logger.errorexpects a message and optional stack; passing the error object as the message drops stack/context. Log a clear message and the stack.- } catch (e) { - this.logger.error(e, 'Error reading session file'); - return false; - } + } catch (e) { + const err = e as Error; + this.logger.error('Error reading session file', err?.stack ?? String(e), CookieService.name); + return false; + }
70-71: Encoding: prefer utf8 over ascii for session filesIf session payloads include non-ASCII characters,
'ascii'may corrupt data.'utf8'is safer for PHP session contents.- const sessionData = await readFile(sessionFile, 'ascii'); + const sessionData = await readFile(sessionFile, 'utf8');- const sessionData = await readFile(join(this.opts.sessionDir, sessionFile), 'ascii'); + const sessionData = await readFile(join(this.opts.sessionDir, sessionFile), 'utf8');Also applies to: 106-106
packages/unraid-shared/src/types/internal-graphql-client.factory.ts (1)
9-14: Make “at least one auth source” a type-level guaranteeWith all auth getters optional, callers can compile with none provided and fail at runtime. Encode “at least one of getApiKey | getCookieAuth | getLocalSession” in the type to prevent misuse.
-export interface InternalGraphQLClientFactory { - createClient(options: { - getApiKey?: () => Promise<string>; - getCookieAuth?: () => Promise<{ sessionId: string; csrfToken: string } | null>; - getLocalSession?: () => Promise<string | null>; - enableSubscriptions?: boolean; - origin?: string; - }): Promise<ApolloClient<NormalizedCacheObject>>; -} +type AtLeastOne<T, Keys extends keyof T = keyof T> = + Partial<T> & { [K in Keys]-?: Required<Pick<T, K>> & Partial<Omit<T, K>> }[Keys]; + +export interface InternalGraphQLClientFactory { + createClient(options: AtLeastOne<{ + getApiKey: () => Promise<string>; + getCookieAuth: () => Promise<{ sessionId: string; csrfToken: string } | null>; + getLocalSession: () => Promise<string | null>; + }> & { + enableSubscriptions?: boolean; + origin?: string; + }): Promise<ApolloClient<NormalizedCacheObject>>; +}packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
8-13: Extract an options type and narrow the public surface.Defining a named options type improves reuse and avoids inline structural typing in the interface.
Apply this change to the method signature:
- getClient(options?: { enableSubscriptions?: boolean; origin?: string }): Promise<ApolloClient<NormalizedCacheObject>>; + getClient(options?: CanonicalInternalClientOptions): Promise<ApolloClient<NormalizedCacheObject>>;Add this type alongside the interface (above it):
export type CanonicalInternalClientOptions = Readonly<{ enableSubscriptions?: boolean; origin?: string; }>;packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (3)
184-189: Sanitize error surface on sign-in and log the original error.Avoid leaking raw error objects to clients. Log internally; return a generic message.
- } catch (error) { - throw new GraphQLError(`Failed to login user: ${error}`, { - extensions: { code: 'INTERNAL_SERVER_ERROR' }, - }); - } + } catch (error) { + this.logger.error('Failed to login user', error as Error); + throw new GraphQLError('Failed to login user.', { + extensions: { code: 'INTERNAL_SERVER_ERROR' }, + }); + }
14-14: Remove unused importdecodeJwt.Not referenced in this file.
-import { decodeJwt } from 'jose';
122-147: Return valuerestartRequiredis never toggled.The method advertises a “restart required” signal but always returns false. Either compute it (e.g., based on effective changes) or drop the return value.
Would you like me to propose a small change-tracking helper that compares prior vs. updated settings and sets this flag accordingly?
api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts (1)
89-89: Tests: avoid asserting exact error messages in rejects.toThrow.Per our testing guidelines, prefer
.rejects.toThrow()without arguments to reduce brittleness.- await expect(resolver.create(input)).rejects.toThrow('Create failed'); + await expect(resolver.create(input)).rejects.toThrow(); - await expect(resolver.create(input)).rejects.toThrow('Sync failed'); + await expect(resolver.create(input)).rejects.toThrow(); - await expect(resolver.addRole(input)).rejects.toThrow('API key not found'); + await expect(resolver.addRole(input)).rejects.toThrow();Also applies to: 101-102, 142-143
api/src/unraid-api/auth/authentication.guard.ts (1)
51-52: Preserve stack when logging errorsLogger.error(message, stack?) expects a string stack/context. Passing the Error object may stringify poorly. Prefer logging stack (or message) explicitly.
- this.logger.error('Error in handleRequest', err); + this.logger.error('Error in handleRequest', (err as Error)?.stack ?? String(err));api/src/unraid-api/rest/__test__/rest-module.integration.test.ts (1)
3-3: Type the override to the canonical internal client interfaceAdd the interface import and type the override to catch shape drift at compile time.
-import { CANONICAL_INTERNAL_CLIENT_TOKEN } from '@unraid/shared'; +import type { CanonicalInternalClientService } from '@unraid/shared'; +import { CANONICAL_INTERNAL_CLIENT_TOKEN } from '@unraid/shared'; @@ - .overrideProvider(CANONICAL_INTERNAL_CLIENT_TOKEN) - .useValue({ getClient: vi.fn() }) + .overrideProvider(CANONICAL_INTERNAL_CLIENT_TOKEN) + .useValue({ getClient: vi.fn() } as unknown as CanonicalInternalClientService)Also applies to: 66-67
api/src/unraid-api/cli/__test__/api-report.service.test.ts (1)
22-25: Optional: enforce mock shape withsatisfiesUse the
satisfiesoperator so the mock must implement the interface without needing type assertions.const mockInternalClientService = { getClient: vi.fn().mockResolvedValue(mockClient), clearClient: vi.fn(), } satisfies CanonicalInternalClientService;api/src/unraid-api/auth/auth.service.spec.ts (3)
101-104: Align error assertions with testing guidelinesPer our testing guidelines, prefer
.rejects.toThrow()without arguments to avoid coupling tests to error classes/messages unless those are the subject under test.Here’s an example; apply similarly to the other cases highlighted above:
- await expect(authService.validateCookiesWithCsrfToken(mockRequest)).rejects.toThrow( - UnauthorizedException - ); + await expect(authService.validateCookiesWithCsrfToken(mockRequest)).rejects.toThrow();Another example:
- await expect(authService.syncApiKeyRoles('test-id', ['new-role'])).rejects.toThrow( - 'Failed to delete role' - ); + await expect(authService.syncApiKeyRoles('test-id', ['new-role'])).rejects.toThrow();Also applies to: 112-115, 137-140, 176-178, 186-189, 223-228, 259-262
79-79: Minor naming nit: describe blockThe block name "validateCookiesCasbin" doesn’t match the method under test (
validateCookiesWithCsrfToken). Consider renaming for clarity in test output.
63-73: Add coverage for the local-session pathSince LocalSessionService is a new auth path, consider a focused test that exercises
validateLocalSessioninteractions (success and failure flows) in AuthService to lock in behavior.If you want, I can draft a minimal test suite that stubs
validateLocalSessionto return a user and verifies role syncing and CSRF behavior gates.api/src/unraid-api/auth/auth.module.ts (1)
21-25: Remove redundant module-leveldefaultStrategyconfigurationThe
AuthenticationGuardexplicitly extendsAuthGuardwith the exact same strategy list ([ServerHeaderStrategy.key, LocalSessionStrategy.key, UserCookieStrategy.key]), so thedefaultStrategydefined inauth.module.tsis redundant and may cause confusion about which list is actually in effect. Consider removing or simplifying it to a single fallback.
- In api/src/unraid-api/auth/auth.module.ts (lines 21–25), drop the multi-strategy array:
PassportModule.register({- defaultStrategy: [
ServerHeaderStrategy.key,LocalSessionStrategy.key,UserCookieStrategy.key,- ],
}),- Retain or define only a single `defaultStrategy` here if you need a module-wide fallback for any guards that don’t specify their own. - Since **api/src/unraid-api/auth/authentication.guard.ts** line 41 already specifies the full chain, all route handlers using this guard will continue to work as intended. </blockquote></details> <details> <summary>api/src/unraid-api/auth/local-session.strategy.ts (3)</summary><blockquote> `23-31`: **Optional: restrict acceptance to loopback to reduce exposure** Even with a strong token, accepting x-local-session from any source increases risk if a reverse proxy misroutes headers. Consider enforcing loopback-only requests for this strategy. Apply: ```diff async validate(request: FastifyRequest): Promise<UserAccount | null> { try { const localSessionToken = request.headers['x-local-session'] as string; if (!localSessionToken) { this.logger.verbose('No local session token found in request headers'); return null; } + // Reduce attack surface: only allow local loopback sources + const ip = (request as any).ip ?? (request as any).socket?.remoteAddress; + const isLoopback = ip === '127.0.0.1' || ip === '::1' || ip === '::ffff:127.0.0.1'; + if (!isLoopback) { + this.logger.warn(`Rejected local-session auth from non-loopback address: ${ip}`); + return null; + }
41-44: Fix logger usage in catch; current call swaps message/contextLogger.verbose(message, context?) doesn’t accept an error/trace parameter; your call logs the error object as the message and the string as context. Use error() to include a stack or format explicitly.
- this.logger.verbose(error, `Local session validation failed`); + this.logger.error( + 'Local session validation failed', + error instanceof Error ? error.stack : String(error) + );
23-38: Add tests for happy-path, missing header, and invalid tokenTo prevent regressions:
- No header → returns null
- Invalid token → returns null
- Valid token → returns UserAccount with ADMIN role (per AuthService.getLocalSessionUser)
I can generate Jest tests that mock AuthService.validateLocalSession to cover these cases. Want me to add them?
api/src/unraid-api/cli/sso/validate-token.command.ts (2)
52-52: Query error handling may miss GraphQL errors unless errorPolicy is 'all'Depending on Apollo Client configuration, GraphQL errors often throw instead of populating result.errors. If you intend to read result.errors, set errorPolicy: 'all'.
- const client = await this.internalClient.getClient({ enableSubscriptions: false }); + const client = await this.internalClient.getClient({ enableSubscriptions: false }); const { data, errors } = await client.query({ query: VALIDATE_OIDC_SESSION_QUERY, variables: { token }, + errorPolicy: 'all', });
52-52: Optionally tag origin for observabilityIf the internal client forwards an Origin header, consider setting a recognizable origin to aid server-side diagnostics.
- const client = await this.internalClient.getClient({ enableSubscriptions: false }); + const client = await this.internalClient.getClient({ + enableSubscriptions: false, + origin: 'cli://validate-token', + });packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts (1)
32-36: Canonical internal client injection via token is correct; be explicit about WS needsConstructor DI via CANONICAL_INTERNAL_CLIENT_TOKEN is aligned with the shared contract. Downstream, consider being explicit about (dis)enabling subscriptions to avoid unintended WS initialization.
- const client = await this.internalClientService.getClient(); + const client = await this.internalClientService.getClient({ enableSubscriptions: true });And for query-only paths:
- const internalClient = await this.internalClientService.getClient(); + const internalClient = await this.internalClientService.getClient({ enableSubscriptions: false });api/src/unraid-api/auth/auth.service.ts (2)
263-263: Consider usingdebuglog level for consistency.The log level for
getSessionUser()was changed fromdebugtoverbose(line 263), butgetLocalSessionUser()also usesverbose(line 280). For consistency with other authentication methods in this service, consider usingdebuglevel for both methods.- this.logger.verbose('getSessionUser called!'); + this.logger.debug('getSessionUser called!');- this.logger.verbose('getLocalSessionUser called!'); + this.logger.debug('getLocalSessionUser called!');Also applies to: 280-280
279-288: Document the user ID convention for system accounts.The local session user uses ID
-2to distinguish it from the regular session user (ID-1). This convention should be documented to prevent ID conflicts.Consider adding a comment or constants to clarify the ID scheme:
+ // System account IDs: + // -1: Web GUI session user (admin) + // -2: Local CLI/system session user (local-admin) async getLocalSessionUser(): Promise<UserAccount> {api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
114-152: Consider extracting authentication header construction to reduce duplication.The authentication logic is duplicated between HTTP headers (lines 114-152) and WebSocket connectionParams (lines 166-190). Consider extracting this to a shared helper method.
+ private async getAuthHeaders( + getApiKey?: () => Promise<string>, + getLocalSession?: () => Promise<string | null>, + getCookieAuth?: () => Promise<{ sessionId: string; csrfToken: string } | null> + ): Promise<Record<string, string>> { + if (getApiKey) { + const apiKey = await getApiKey(); + return { 'x-api-key': apiKey }; + } else if (getLocalSession) { + const localSession = await getLocalSession(); + if (!localSession) { + throw new Error('No valid local session found'); + } + return { 'x-local-session': localSession }; + } else if (getCookieAuth) { + const cookieAuth = await getCookieAuth(); + if (!cookieAuth) { + throw new Error('No valid session found for cookie authentication'); + } + return { + 'x-csrf-token': cookieAuth.csrfToken, + cookie: `unraid_${cookieAuth.sessionId}=${cookieAuth.sessionId}`, + }; + } + return {}; + } // Create auth link that dynamically fetches authentication info for each request const authLink = setContext(async (_, { headers }) => { - if (getApiKey) { - // Use API key authentication - ... - } else if (getLocalSession) { - // Use local session authentication - ... - } else if (getCookieAuth) { - // Use cookie-based authentication - ... - } - - return { headers }; + const authHeaders = await this.getAuthHeaders(getApiKey, getLocalSession, getCookieAuth); + return { + headers: { + ...headers, + ...authHeaders, + }, + }; });Then reuse in the WebSocket connectionParams:
connectionParams: async () => { - if (getApiKey) { - ... - } else if (getLocalSession) { - ... - } else if (getCookieAuth) { - ... - } - return {}; + return await this.getAuthHeaders(getApiKey, getLocalSession, getCookieAuth); },api/src/unraid-api/auth/local-session.service.ts (2)
14-14: Consider making the session file path configurable.The hardcoded path
/var/run/unraid-api/local-sessionmight not be suitable for all deployment environments (e.g., development, testing).- private static readonly SESSION_FILE_PATH = '/var/run/unraid-api/local-session'; + private static readonly SESSION_FILE_PATH = process.env.LOCAL_SESSION_PATH || '/var/run/unraid-api/local-session';
81-90: Consider using Node.js built-incrypto.timingSafeEqualfor constant-time comparison.While the current implementation is correct, Node.js provides a native constant-time comparison function that's more efficient.
+import { randomBytes, timingSafeEqual } from 'crypto'; /** * Constant-time string comparison to prevent timing attacks */ private constantTimeEqual(a: string, b: string): boolean { if (a.length !== b.length) return false; - - let result = 0; - for (let i = 0; i < a.length; i++) { - result |= a.charCodeAt(i) ^ b.charCodeAt(i); - } - - return result === 0; + + const bufferA = Buffer.from(a); + const bufferB = Buffer.from(b); + return timingSafeEqual(bufferA, bufferB); }api/src/unraid-api/cli/cli.module.spec.ts (2)
27-31: Type module.get() for safer assertions.Provide the token’s interface to
module.getso failures are caught at compile time.Apply this diff:
-it('should provide CanonicalInternalClient', () => { - const service = module.get(CANONICAL_INTERNAL_CLIENT_TOKEN); +it('should provide CanonicalInternalClient', () => { + const service = module.get<import('@unraid/shared').CanonicalInternalClientService>( + CANONICAL_INTERNAL_CLIENT_TOKEN + ); expect(service).toBeDefined(); expect(service.getClient).toBeInstanceOf(Function); });
39-58: Minor: also type the dependency resolution check.Keeps the test self-documenting and type-safe.
Apply this diff:
-const service = module.get(CANONICAL_INTERNAL_CLIENT_TOKEN); +const service = module.get<import('@unraid/shared').CanonicalInternalClientService>( + CANONICAL_INTERNAL_CLIENT_TOKEN +);api/src/unraid-api/cli/developer/developer-tools.service.ts (1)
63-64: After restarting the API, clear the cached internal client to refresh auth/links.When sandbox toggling triggers a restart, the server’s local session token may rotate. Clearing the cached client ensures the next call rebuilds transport and auth with fresh state (especially important if any consumer uses subscriptions).
Add after Line 80 (right after
await this.restartCommand.run();):// Recreate internal GraphQL client on next use after restart this.internalClient.clearClient();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (36)
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json(0 hunks)api/dev/keys/fc91da7b-0284-46f4-9018-55aa9759fba9.json(0 hunks)api/src/unraid-api/auth/auth.module.ts(3 hunks)api/src/unraid-api/auth/auth.service.spec.ts(3 hunks)api/src/unraid-api/auth/auth.service.ts(5 hunks)api/src/unraid-api/auth/authentication.guard.ts(2 hunks)api/src/unraid-api/auth/cookie.service.ts(3 hunks)api/src/unraid-api/auth/local-session.service.ts(1 hunks)api/src/unraid-api/auth/local-session.strategy.ts(1 hunks)api/src/unraid-api/cli/__test__/api-report.service.test.ts(2 hunks)api/src/unraid-api/cli/__test__/developer-tools.service.test.ts(4 hunks)api/src/unraid-api/cli/admin-key.service.ts(0 hunks)api/src/unraid-api/cli/api-report.service.ts(4 hunks)api/src/unraid-api/cli/cli-services.module.ts(1 hunks)api/src/unraid-api/cli/cli.module.spec.ts(3 hunks)api/src/unraid-api/cli/cli.module.ts(0 hunks)api/src/unraid-api/cli/developer/developer-tools.service.ts(2 hunks)api/src/unraid-api/cli/internal-client.service.spec.ts(0 hunks)api/src/unraid-api/cli/internal-client.service.ts(0 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(3 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts(1 hunks)api/src/unraid-api/plugin/global-deps.module.ts(4 hunks)api/src/unraid-api/rest/__test__/rest-module.integration.test.ts(2 hunks)api/src/unraid-api/shared/internal-client.service.ts(2 hunks)api/src/unraid-api/shared/internal-graphql-client.factory.ts(3 hunks)packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts(0 hunks)packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.spec.ts(0 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts(2 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts(1 hunks)packages/unraid-shared/src/index.ts(1 hunks)packages/unraid-shared/src/tokens.ts(1 hunks)packages/unraid-shared/src/types/canonical-internal-client.interface.ts(1 hunks)packages/unraid-shared/src/types/internal-graphql-client.factory.ts(1 hunks)
💤 Files with no reviewable changes (8)
- api/src/unraid-api/cli/cli.module.ts
- api/src/unraid-api/cli/internal-client.service.spec.ts
- api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
- packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts
- api/dev/keys/fc91da7b-0284-46f4-9018-55aa9759fba9.json
- packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.spec.ts
- api/src/unraid-api/cli/internal-client.service.ts
- api/src/unraid-api/cli/admin-key.service.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
packages/unraid-shared/src/types/internal-graphql-client.factory.tspackages/unraid-shared/src/index.tspackages/unraid-shared/src/tokens.tsapi/src/unraid-api/cli/cli-services.module.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/auth/auth.service.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tspackages/unraid-shared/src/types/canonical-internal-client.interface.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/auth/local-session.strategy.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/api-report.service.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.tsapi/src/unraid-api/auth/local-session.service.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect.module.tsapi/src/unraid-api/auth/authentication.guard.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/cli/cli.module.spec.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/auth/auth.module.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
packages/unraid-shared/src/types/internal-graphql-client.factory.tspackages/unraid-shared/src/index.tspackages/unraid-shared/src/tokens.tsapi/src/unraid-api/cli/cli-services.module.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/auth/auth.service.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tspackages/unraid-shared/src/types/canonical-internal-client.interface.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/auth/local-session.strategy.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/api-report.service.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.tsapi/src/unraid-api/auth/local-session.service.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect.module.tsapi/src/unraid-api/auth/authentication.guard.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/cli/cli.module.spec.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/auth/auth.module.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/cli/cli-services.module.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/auth/auth.service.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/auth/local-session.strategy.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/api-report.service.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/authentication.guard.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/cli/cli.module.spec.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/auth/auth.module.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/cli/cli.module.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/cli/cli.module.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/cli/cli.module.spec.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/cli/__test__/api-report.service.test.ts
🧠 Learnings (4)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/cli/cli.module.spec.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/auth/auth.module.ts
📚 Learning: 2024-11-15T16:22:03.485Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:176-188
Timestamp: 2024-11-15T16:22:03.485Z
Learning: Atomic writes are not required for the `saveApiKey` method in `ApiKeyService` (`api/src/unraid-api/auth/api-key.service.ts`) unless specifically needed.
Applied to files:
api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts
🧬 Code graph analysis (15)
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts (3)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/index.ts (1)
CanonicalInternalClientService(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/shared/internal-graphql-client.factory.ts (2)
packages/unraid-api-plugin-connect/src/mothership-proxy/connection.service.ts (1)
getApiKey(162-164)api/src/unraid-api/auth/local-session.service.ts (1)
getLocalSession(55-63)
api/src/unraid-api/auth/auth.service.ts (1)
api/src/utils.ts (1)
handleAuthError(213-245)
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts (1)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
packages/unraid-shared/src/index.ts (1)
CanonicalInternalClientService(7-7)
api/src/unraid-api/auth/local-session.strategy.ts (3)
api/src/unraid-api/auth/auth.service.ts (1)
Injectable(15-289)api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(10-98)api/src/unraid-api/types/fastify.ts (1)
FastifyRequest(25-30)
api/src/unraid-api/cli/sso/validate-token.command.ts (2)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/cli/__test__/developer-tools.service.test.ts (2)
packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
api/src/unraid-api/cli/api-report.service.ts (2)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/auth/local-session.service.ts (4)
api/src/unraid-api/auth/auth.service.ts (1)
Injectable(15-289)api/src/unraid-api/auth/local-session.strategy.ts (1)
Injectable(14-46)api/src/unraid-api/shared/internal-client.service.ts (1)
Injectable(15-87)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(26-232)
api/src/unraid-api/cli/__test__/api-report.service.test.ts (1)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
api/src/unraid-api/plugin/global-deps.module.ts (2)
api/src/unraid-api/auth/cookie.service.ts (1)
SESSION_COOKIE_CONFIG(10-10)packages/unraid-shared/src/tokens.ts (4)
COOKIE_SERVICE_TOKEN(6-6)NGINX_SERVICE_TOKEN(4-4)CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)INTERNAL_CLIENT_SERVICE_TOKEN(5-5)
api/src/unraid-api/cli/developer/developer-tools.service.ts (2)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/cli/cli.module.spec.ts (1)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
api/src/unraid-api/shared/internal-client.service.ts (7)
api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(10-98)api/src/unraid-api/cli/api-report.service.ts (1)
Injectable(62-201)api/src/unraid-api/cli/developer/developer-tools.service.ts (1)
Injectable(13-235)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(26-232)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)packages/unraid-shared/src/tokens.ts (1)
INTERNAL_CLIENT_SERVICE_TOKEN(5-5)packages/unraid-shared/src/types/internal-graphql-client.factory.ts (1)
InternalGraphQLClientFactory(7-15)
🔇 Additional comments (37)
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts (1)
15-27: ConnectApiKeyService Removed; Verify InternalClientService References
ConnectApiKeyService
- No occurrences found in the repo—removal confirmed.
InternalClientService
- Defined in
api/src/unraid-api/shared/internal-client.service.ts- Imported and provided in
api/src/unraid-api/plugin/global-deps.module.ts(lines 24, 64–68)- Token constant in
packages/unraid-shared/src/tokens.ts(line 5)
These hits correspond to the new canonical internal‐client implementation.Exports
- No modules are still exporting either service (verified).
Action: Please confirm that each
InternalClientServicereference is indeed the new canonical implementation (and not a leftover import of the old service). Remove any legacy imports or providers if they remain.packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts (2)
14-31: Removal of ConnectApiKeyService from module metadata looks correctThis aligns with the canonical internal client flow; providers/exports are consistent and imports already use .js specifiers.
14-31: Verified: No remaining dependencies on ConnectApiKeyServiceI ran a full-text search for
ConnectApiKeyServiceacross the repository and found zero occurrences outside ofnode_modules. This confirms that no providers or resolvers are still injecting it, so you shouldn’t encounter any DI runtime errors related to it.packages/unraid-shared/src/tokens.ts (1)
6-7: New DI tokens are reasonable and follow existing namingCOOKIE_SERVICE_TOKEN and CANONICAL_INTERNAL_CLIENT_TOKEN fit the existing token pattern. No conflicts spotted.
api/src/unraid-api/auth/cookie.service.ts (2)
78-80: Centralized validity check is good; keep the predicate tightReusing
isSessionValidin both paths reduces drift. The predicate is minimal; if abuse is a concern, consider verifying expected key/value patterns rather than just substrings. No action required now.
30-37: Good: sameSite is set to 'lax' per prior guidanceMatches the documented preference for session cookies in this codebase.
packages/unraid-shared/src/types/internal-graphql-client.factory.ts (1)
9-14: Auth Precedence Is Explicit—Please Document and Confirm Against PR GoalsI verified that the factory’s
authLink(and WebSocketconnectionParams) enforces a strict if/else order:
getApiKey→ API key headersgetLocalSession→x-local-sessionheadergetCookieAuth→Cookie: sessionId=…; csrfToken=…To align with the PR’s intent of “prioritize ephemeral keys, then others,” please:
- Update the JSDoc above
createClientininternal-graphql-client.factory.tsto explicitly list this precedence.- Confirm that the order of local-session vs. cookie-based auth matches the PR objectives (i.e. whether cookie auth should come before or after local session).
- (Optional) Add a unit test where both
getLocalSessionandgetCookieAuthare provided to assert the intended branch is taken.packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
1-1: LGTM on ESM import specifier.Type-only import uses .js extension and aligns with our ESM guideline for TS. No concerns.
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (1)
129-136: Review comment incorrect:WAN_ACCESS_TYPE.DYNAMICis defined and the SSL check is reachable.I’ve confirmed that in connect.model.ts, the
WAN_ACCESS_TYPEenum includesDYNAMIC(alongsideALWAYSandDISABLED), so there’s no compile-time error and the branch insyncSettingscan be hit by clients specifying dynamic access. The original concern about a missing enum member isn’t valid.Likely an incorrect or invalid review comment.
packages/unraid-shared/src/index.ts (1)
7-7: LGTM: type-only export with .js path keeps bundle clean.The new public type export follows our ESM path convention and won’t affect runtime.
api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts (1)
51-53: LGTM: AuthService now accepts LocalSessionService (constructor updated).Constructor wiring matches the new dependency order and keeps the test isolated by stubbing validateLocalSession.
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts (1)
46-48: LGTM: constructor update for LocalSessionService in tests.The added stub for LocalSessionService aligns with the new AuthService signature and keeps the resolver tests focused.
api/src/unraid-api/auth/authentication.guard.ts (2)
16-16: Importing LocalSessionStrategy looks correct and ESM-safeUsing the .js extension aligns with our ESM guideline. No issues here.
41-41: Confirm intended auth precedence with LocalSession before UserCookieStrategy order dictates precedence. Placing LocalSessionStrategy between ServerHeader and UserCookie changes behavior for requests that could satisfy both local-session and cookie auth. Confirm this is intentional across GraphQL, REST, and WS upgrades.
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts (1)
3-3: Token import LGTMUsing the shared token keeps the test decoupled from concrete client implementations.
api/src/unraid-api/cli/__test__/api-report.service.test.ts (1)
3-4: Canonical client token/type import LGTMThe test now depends on the stable shared token and interface, aligning with production DI.
api/src/unraid-api/auth/auth.service.spec.ts (1)
11-11: AuthService constructor update wired correctlyLocalSessionService is mocked and injected in the new constructor position. Looks good.
Also applies to: 21-21, 69-73
api/src/unraid-api/cli/api-report.service.ts (3)
1-4: DI migration to canonical client LGTMInjecting via the shared token/interface decouples the service from concrete client implementations and improves testability.
Also applies to: 65-67
141-141: Explicitly disabling subscriptions is appropriateThe CLI report workflow doesn’t need WS; passing
{ enableSubscriptions: false }is correct.
196-197: Safer access to system data LGTMSwitch to
systemResult?.dataavoids NPEs when the first query fails. Good defensive change.api/src/unraid-api/auth/auth.module.ts (3)
74-78: Exporting LocalSession providers enables reuse—LGTMExporting LocalSessionStrategy and LocalSessionService from AuthModule enables clean DI in downstream modules. Looks good.
65-67: No changes needed:defaultOpts()already enforces SameSite: 'lax'The
CookieService.defaultOpts()implementation inapi/src/unraid-api/auth/cookie.service.tsexplicitly setssameSite: 'lax',on line 36, ensuring that all session cookies use
SameSite: 'lax'as per our prior decision.
60-64: AuthModule imports and provider singleton verifiedAuthModule is only imported by:
- app/src/unraid-api/app/app.module.ts (line 13)
- api/src/unraid-api/graph/resolvers/resolvers.module.ts (line 3)
- api/src/unraid-api/graph/resolvers/api-key/api-key.module.ts (line 4)
AuthModule does not import any of these modules in turn, so there are no circular dependencies. Moreover, NestJS modules are singleton-scoped by design—importing the same module into multiple parent modules does not re-create its providers. Therefore, LocalSessionService will only be instantiated once and its
onModuleInithook will only run a single time for token generation.No further changes are needed here. If you’d like to simplify consumer imports even more, you can annotate AuthModule with
@Global()and addLocalSessionServiceto itsexportsarray—this will make the service available app-wide without explicit imports.api/src/unraid-api/cli/cli-services.module.ts (1)
24-25: No lingering AdminKeyService/CliInternalClientService references foundRan the provided ripgrep search for any imports or usages of
AdminKeyServiceorCliInternalClientService. The only match is a comment in the test filepackages/unraid-shared/src/services/internal-graphql-client-usage.spec.tsreference (“Service that uses the factory pattern (like CliInternalClientService)”), not an actual import or code usage. No further action is required to update consumers incli-services.module.ts.api/src/unraid-api/auth/local-session.strategy.ts (1)
15-17: Strategy key and DI are cleanPassport key 'local-session' with a static key property is consistent with usage elsewhere. Constructor DI into AuthService is straightforward.
api/src/unraid-api/cli/sso/validate-token.command.ts (1)
19-21: Token-based DI for the canonical internal client—good moveSwitching to @Inject(CANONICAL_INTERNAL_CLIENT_TOKEN) decouples the command from a concrete class and aligns with the shared interface.
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts (1)
1-6: ESM import hygiene—LGTMUsing .js extensions for local ESM imports and type-only import for CanonicalInternalClientService is consistent with repo guidelines.
api/src/unraid-api/auth/auth.service.ts (1)
88-110: LGTM! Well-structured local session validation method.The implementation follows the established pattern for authentication methods in this service, with proper error handling, role synchronization, and logging.
api/src/unraid-api/plugin/global-deps.module.ts (2)
51-53: Good implementation of session cookie configuration.Using
CookieService.defaultOpts()ensures consistent cookie configuration across the application, which aligns with the previously learned best practice of usingsameSite: 'lax'for session cookies.
63-68: LGTM! Clean canonical client service aliasing.The aliasing pattern using
useExistingproperly establishesInternalClientServiceas the canonical implementation without creating duplicate instances.api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
52-56: Good validation of authentication options.The validation ensures at least one authentication method is provided, preventing misconfiguration.
api/src/unraid-api/cli/__test__/developer-tools.service.test.ts (1)
4-6: LGTM! Clean migration to canonical internal client token.The test correctly migrates from the legacy
CliInternalClientServiceto the newCanonicalInternalClientServiceusing the proper token pattern. This aligns well with the broader refactoring to standardize internal client usage across the codebase.Also applies to: 19-19, 46-47, 57-57
api/src/unraid-api/shared/internal-client.service.ts (2)
21-25: Good DI: factory via token + LocalSessionService injection aligns with the canonical client design.This simplifies consumers and centralizes auth via local session. Nice.
81-86: No action needed: ApolloClient.stop() is supported in v3.13.9We’ve verified that all workspaces pin
@apollo/clientat version 3.13.9. According to the Apollo Client v3 API docs,ApolloClientexposes astop(): voidmethod to terminate any active client processes, making it safe to dispose of the instance. Therefore, the existingthis.client?.stop()call is valid and no further changes are required. (apollographql.com)api/src/unraid-api/cli/cli.module.spec.ts (1)
4-4: LGTM: tokens imported from shared; aligns with DI-by-token approach.The switch to CANONICAL_INTERNAL_CLIENT_TOKEN and INTERNAL_CLIENT_SERVICE_TOKEN looks correct.
api/src/unraid-api/cli/developer/developer-tools.service.ts (2)
57-59: LGTM: Use canonical client via token.Token-based DI decouples the implementation and matches the new architecture.
63-64: Note on getClient({ enableSubscriptions: false }): relies on options-aware caching.If InternalClientService caches a single instance unconditionally, the first caller’s options win. Ensure the options-aware caching refactor in InternalClientService is adopted so this call won’t prevent other consumers from using subscriptions later.
If not adopting the refactor, consider always using subscriptions in the canonical client and letting HTTP-only consumers ignore the WS capability.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/cli/api-report.service.ts (1)
146-153: Fix unhandled promise rejections caused by Promise.race timeoutsThe
Promise.race([client.query(...), timeoutReject])pattern schedules a rejecting promise that is never cleared if the query resolves first. When the timer fires later, it can trigger an unhandled rejection. Use a helper that clears the timeout in a finally block.Apply these diffs to replace the three timeout usages:
- let systemResult: { data: SystemReportQuery } | null = null; - try { - systemResult = await Promise.race([ - client.query({ - query: SYSTEM_REPORT_QUERY, - }), - new Promise<never>((_, reject) => - setTimeout(() => reject(new Error('Query timeout after 10 seconds')), 10000) - ), - ]); - } catch (error) { + let systemResult: { data: SystemReportQuery } | null = null; + try { + systemResult = await withTimeout( + client.query({ query: SYSTEM_REPORT_QUERY }), + 10_000, + 'Query timeout after 10 seconds' + ); + } catch (error) { this.logger.error('Error querying system data: ' + error); return this.createApiReportData({ apiRunning, errorReason: 'System query failed', }); }- try { - const connectResult = await Promise.race([ - client.query({ - query: CONNECT_STATUS_QUERY, - }), - new Promise<never>((_, reject) => - setTimeout(() => reject(new Error('Connect query timeout after 10 seconds')), 10000) - ), - ]); - connectData = connectResult.data.connect; - } catch (error) { + try { + const connectResult = await withTimeout( + client.query({ query: CONNECT_STATUS_QUERY }), + 10_000, + 'Connect query timeout after 10 seconds' + ); + connectData = connectResult.data.connect; + } catch (error) { this.logger.debug('Connect plugin not available: ' + error); }- try { - const servicesResult = await Promise.race([ - client.query({ - query: SERVICES_QUERY, - }), - new Promise<never>((_, reject) => - setTimeout(() => reject(new Error('Services query timeout after 10 seconds')), 10000) - ), - ]); - servicesData = servicesResult.data.services || []; - } catch (error) { + try { + const servicesResult = await withTimeout( + client.query({ query: SERVICES_QUERY }), + 10_000, + 'Services query timeout after 10 seconds' + ); + servicesData = servicesResult.data.services || []; + } catch (error) { this.logger.debug('Error querying services: ' + error); }Add this helper inside the class (e.g., above generateReport):
private async withTimeout<T>(p: Promise<T>, ms: number, message: string): Promise<T> { let id: ReturnType<typeof setTimeout> | undefined; const timeout = new Promise<never>((_, reject) => { id = setTimeout(() => reject(new Error(message)), ms); }); try { return await Promise.race([p, timeout]); } finally { if (id) clearTimeout(id); } }Or, if you prefer a static helper:
const withTimeout = async <T>(p: Promise<T>, ms: number, message: string) => { /* same body */ };Then call
await this.withTimeout(...)in the diffs above.Also applies to: 165-172, 181-188
🧹 Nitpick comments (44)
packages/unraid-shared/src/types/internal-graphql-client.factory.ts (2)
9-11: Type-level guarantee: require at least one auth provider in createClient options.Runtime guards exist, but the type allows calling without any of getApiKey/getCookieAuth/getLocalSession. Tighten the type to prevent accidental misuse.
Apply:
export interface InternalGraphQLClientFactory { - createClient(options: { - getApiKey?: () => Promise<string>; - getCookieAuth?: () => Promise<{ sessionId: string; csrfToken: string } | null>; - getLocalSession?: () => Promise<string | null>; - enableSubscriptions?: boolean; - origin?: string; - }): Promise<ApolloClient<NormalizedCacheObject>>; + /** + * At least one of getApiKey, getCookieAuth, or getLocalSession must be provided. + */ + createClient(options: RequireAtLeastOne<{ + getApiKey?: () => Promise<string>; + getCookieAuth?: () => Promise<{ sessionId: string; csrfToken: string } | null>; + getLocalSession?: () => Promise<string | null>; + enableSubscriptions?: boolean; + origin?: string; + }, 'getApiKey' | 'getCookieAuth' | 'getLocalSession'>): Promise<ApolloClient<NormalizedCacheObject>>; } + +// Utility: require at least one of the specified keys +type RequireAtLeastOne<T, K extends keyof T = keyof T> = + Pick<T, Exclude<keyof T, K>> & + { [P in K]-?: Required<Pick<T, P>> & Partial<Pick<T, Exclude<K, P>>> }[K];
7-15: Clarify precedence semantics in docs.If multiple providers are supplied, document which one wins (e.g., API key > local session > cookie), matching the implementation, to prevent ambiguity for integrators.
api/src/unraid-api/auth/cookie.service.ts (2)
78-80: Session validity heuristic could allow expired sessions.The content check ensures required keys but does not account for expiration/TTL. Consider validating freshness via file mtime against configured max lifetime, if available.
Example (outline):
- Read file mtime with fs.stat and compare to a TTL (e.g., from config).
- Keep content checks as-is to avoid parsing cost.
46-51: Minor: hasValidAuthCookie can short-circuit.Optional micro-optimization: early return true on the first valid cookie instead of processing all cookies.
packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
5-7: Docstring: broaden beyond cookie auth.The canonical client can authenticate via local session (and potentially API key via factory). Update wording to avoid implying cookie-only.
Apply:
- * Get GraphQL client with cookie authentication. - * This is the canonical internal client for the application. + * Get GraphQL client for internal use. + * Authentication strategy (e.g., local session, cookie) is chosen by the underlying factory.packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (4)
171-176: Storing API key in config: re-evaluate persistence model.With the broader shift to ephemeral/persistent key separation and local-session auth, confirm that persisting connect.config.apikey is still required and acceptable from a security standpoint (plaintext at rest).
Options:
- Store an ephemeral token instead (rotated by AdminKeyService) and avoid persisting long-lived keys.
- If persistence is required, consider encrypting at rest or scoping to minimal privileges.
14-14: Unused import: decodeJwt.Appears unused; safe to remove.
-import { decodeJwt } from 'jose';
285-289: Port validation constraints are inconsistent across schemas.In the conditional 'then' schema, port minimum is 1..65535; in the base definition it's 0..65535. Recommend removing min/max from the base schema or aligning both to 1..65535 to avoid conflicting validation paths.
- port: { - // 'port' is null when remote access is disabled. - type: ['number', 'null'], - title: 'WAN Port', - minimum: 0, - maximum: 65535, - }, + port: { + // 'port' is null when remote access is disabled. + type: ['number', 'null'], + title: 'WAN Port' + },Also applies to: 426-428
96-99: isSignedIn relies solely on apikey presence.Given the move toward local-session auth, consider augmenting this check (e.g., via CookieService.getActiveSession or a LocalSessionService) so UI/UX gates reflect actual auth state.
Would you like me to propose a small integration using CANONICAL_INTERNAL_CLIENT_TOKEN to determine signed-in state?
Also applies to: 309-315
api/src/unraid-api/auth/local-session.service.ts (4)
81-90: Prefer crypto.timingSafeEqual(Buffer, Buffer) for robust constant-time comparisonYour XOR aggregation is good, but Node’s built-in
timingSafeEqualover Buffers is more robust and well-tested. Convert strings to Buffers and usetimingSafeEqual.Apply this diff:
@@ -import { randomBytes } from 'crypto'; +import { randomBytes, timingSafeEqual } from 'crypto'; @@ - private constantTimeEqual(a: string, b: string): boolean { - if (a.length !== b.length) return false; - - let result = 0; - for (let i = 0; i < a.length; i++) { - result |= a.charCodeAt(i) ^ b.charCodeAt(i); - } - - return result === 0; - } + private constantTimeEqual(a: string, b: string): boolean { + const aBuf = Buffer.from(a); + const bBuf = Buffer.from(b); + if (aBuf.length !== bBuf.length) return false; + try { + return timingSafeEqual(aBuf, bBuf); + } catch { + return false; + } + }Also applies to: 2-2
33-35: Harden directory permissions when creating the session directorySet the directory mode to 0700 to minimize exposure (owner-only). While umask may still affect final permissions, this sets a strong baseline.
- await mkdir(dirname(LocalSessionService.SESSION_FILE_PATH), { recursive: true }); + await mkdir(dirname(LocalSessionService.SESSION_FILE_PATH), { recursive: true, mode: 0o700 });
20-22: Improve error logging: pass stack traces to Logger.error and include messages in debug logsNest’s
Logger.erroraccepts a message and an optional stack trace. Passing the raw error as the second arg can miss stack info. Also include the error message when downgrading to debug.- this.logger.error('Failed to initialize local session:', error); + this.logger.error('Failed to initialize local session', (error as Error)?.stack); @@ - this.logger.error(`Failed to write local session: ${error}`); + this.logger.error('Failed to write local session', (error as Error)?.stack); @@ - this.logger.debug('Local session file not found or not readable'); + this.logger.debug(`Local session file not found or not readable: ${(error as Error)?.message}`);Also applies to: 46-49, 60-61
25-31: Confirm startup failure policy for token generationIf
generateLocalSession()throws during startup, the service logs the error but continues. Given other components depend on a valid local session (e.g., canonical internal client), do we want to fail fast instead and prevent the app from starting in a half-broken state?If you want to enforce fail-fast, change the
onModuleInitcatch to rethrow after logging. I can provide a small patch if that’s the desired behavior.api/src/unraid-api/rest/__test__/rest-module.integration.test.ts (2)
66-68: Stub the full CanonicalInternalClientService shape to prevent future breakageThe stub only provides
getClient. The interface also exposesclearClient(). Adding it avoids surprises if tests or modules start calling it.- .overrideProvider(CANONICAL_INTERNAL_CLIENT_TOKEN) - .useValue({ getClient: vi.fn() }) + .overrideProvider(CANONICAL_INTERNAL_CLIENT_TOKEN) + .useValue({ getClient: vi.fn(), clearClient: vi.fn() })
78-80: Avoid asserting on private fields; prefer behavior or container resolutionAccessing
restService['apiReportService']ties the test to implementation details. Since you already resolveApiReportServicefrom the container, you can assert module compilation and service resolution without peeking into private props.Possible alternative:
- Keep the existing
module.get<ApiReportService>(ApiReportService)as the assertion (it already ensures DI is wired).- Or assert a public method that uses
ApiReportServiceif appropriate.api/src/unraid-api/auth/authentication.guard.ts (2)
58-61: Sanitize connectionParams in logs to avoid leaking secretsLogging raw connectionParams may include tokens or API keys. Mask sensitive keys before logging.
- apiLogger.error( - 'No user found in request - connection params: %o', - fullContext.connectionParams ?? {} - ); + const cps = { ...(fullContext.connectionParams ?? {}) }; + for (const k of Object.keys(cps)) { + if (/(token|auth|key|secret)/i.test(k)) cps[k] = '[REDACTED]'; + } + apiLogger.error('No user found in request - connection params: %o', cps);
97-105: Guard cookie parsing on WS handshake to avoid mis-parsing/TypeErrorsIf the Cookie header is absent, cookie.parse may receive an unexpected string or undefined. Add a headerIndex check and default to an empty cookie map.
- if (fullContext.connectionParams) { - const rawHeaders = fullContext.req.extra.request.rawHeaders; - const headerIndex = rawHeaders.findIndex( - (headerOrValue) => headerOrValue.toLowerCase() === 'cookie' - ); - const cookieString = rawHeaders[headerIndex + 1]; - request.cookies = parseCookies(cookieString); - } + if (fullContext.connectionParams) { + const rawHeaders = fullContext.req.extra.request.rawHeaders ?? []; + const headerIndex = rawHeaders.findIndex( + (h) => typeof h === 'string' && h.toLowerCase() === 'cookie' + ); + if (headerIndex >= 0 && typeof rawHeaders[headerIndex + 1] === 'string') { + request.cookies = parseCookies(rawHeaders[headerIndex + 1] as string); + } else { + request.cookies = {}; + } + }api/src/unraid-api/auth/auth.module.ts (1)
21-25: Validate PassportModule.register defaultStrategy usage with string[]NestJS Passport commonly treats defaultStrategy as a string. Using a string[] may compile depending on local typings, but it’s non-standard. Since AuthenticationGuard explicitly enumerates strategies, consider keeping defaultStrategy to a single value (or omit it) to avoid confusion.
Option A: keep a single default
- defaultStrategy: [ - ServerHeaderStrategy.key, - LocalSessionStrategy.key, - UserCookieStrategy.key, - ], + defaultStrategy: ServerHeaderStrategy.key,Option B: remove and rely on AuthenticationGuard’s multi-strategy
- PassportModule.register({ - defaultStrategy: [ - ServerHeaderStrategy.key, - LocalSessionStrategy.key, - UserCookieStrategy.key, - ], - }), + PassportModule.register({}),Would you like me to open a follow-up to align this across modules?
api/src/unraid-api/auth/auth.service.spec.ts (2)
69-73: Type the localSessionService mock precisely (avoid any)Narrowing the mock type improves safety and editor help.
- localSessionService = { - validateLocalSession: vi.fn(), - } as any; - authService = new AuthService(cookieService, apiKeyService, localSessionService, authzService); + const localSessionMock = { + validateLocalSession: vi.fn(), + } as unknown as LocalSessionService; + localSessionService = localSessionMock; + authService = new AuthService(cookieService, apiKeyService, localSessionMock, authzService);
101-104: Align error assertions with testing guidelines (.rejects.toThrow() without args)Our testing guideline prefers avoiding exact messages and types for thrown errors unless they are the subject under test. Replace specific message/type assertions with a plain
.rejects.toThrow().- await expect(authService.validateCookiesWithCsrfToken(mockRequest)).rejects.toThrow( - UnauthorizedException - ); + await expect(authService.validateCookiesWithCsrfToken(mockRequest)).rejects.toThrow();- await expect(authService.validateCookiesWithCsrfToken(mockRequest)).rejects.toThrow( - UnauthorizedException - ); + await expect(authService.validateCookiesWithCsrfToken(mockRequest)).rejects.toThrow();- await expect(authService.validateCookiesWithCsrfToken(mockRequest)).rejects.toThrow( - new UnauthorizedException('Invalid CSRF token') - ); + await expect(authService.validateCookiesWithCsrfToken(mockRequest)).rejects.toThrow();- await expect(authService.syncApiKeyRoles('test-id', ['new-role'])).rejects.toThrow( - 'Failed to delete role' - ); + await expect(authService.syncApiKeyRoles('test-id', ['new-role'])).rejects.toThrow();- await expect(authService.syncApiKeyRoles('test-id', ['new-role'])).rejects.toThrow( - 'Failed to add role' - ); + await expect(authService.syncApiKeyRoles('test-id', ['new-role'])).rejects.toThrow();- await expect(authService.addRoleToApiKey('invalid-id', Role.GUEST)).rejects.toThrow( - UnauthorizedException - ); + await expect(authService.addRoleToApiKey('invalid-id', Role.GUEST)).rejects.toThrow();- await expect(authService.removeRoleFromApiKey('invalid-id', Role.GUEST)).rejects.toThrow( - UnauthorizedException - ); + await expect(authService.removeRoleFromApiKey('invalid-id', Role.GUEST)).rejects.toThrow();Also applies to: 112-115, 137-140, 176-179, 186-189, 225-228, 259-262
api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts (2)
88-90: Relax brittle error message assertionsPer guidelines, assert that an error was thrown without relying on exact messages.
- await expect(resolver.create(input)).rejects.toThrow('Create failed'); + await expect(resolver.create(input)).rejects.toThrow();- await expect(resolver.create(input)).rejects.toThrow('Sync failed'); + await expect(resolver.create(input)).rejects.toThrow();- await expect(resolver.addRole(input)).rejects.toThrow('API key not found'); + await expect(resolver.addRole(input)).rejects.toThrow();- await expect(resolver.removeRole(input)).rejects.toThrow('API key not found'); + await expect(resolver.removeRole(input)).rejects.toThrow();Also applies to: 101-102, 142-143, 163-164
78-79: Nit: assert with values from creation result to reduce couplingYou’re asserting sync with mockApiKey values, but the source of truth is the created key. Using the returned id/roles from create reduces fixture coupling.
- expect(authService.syncApiKeyRoles).toHaveBeenCalledWith(mockApiKey.id, mockApiKey.roles); + expect(authService.syncApiKeyRoles).toHaveBeenCalledWith( + mockApiKeyWithSecret.id, + input.roles + );api/src/unraid-api/cli/__test__/api-report.service.test.ts (1)
44-45: Pin the client-creation contract in the testApiReportService now calls getClient({ enableSubscriptions: false }). As a small safeguard, assert the call arguments so future regressions don’t silently re-enable subscriptions in this path.
Apply this addition near the other expectations:
const result = await apiReportService.generateReport(true); // Verify GraphQL client was called for all queries - expect(mockInternalClientService.getClient).toHaveBeenCalled(); + expect(mockInternalClientService.getClient).toHaveBeenCalledWith({ enableSubscriptions: false }); expect(mockClient.query).toHaveBeenCalledWith({ query: SYSTEM_REPORT_QUERY, });Also applies to: 131-138
api/src/unraid-api/cli/sso/validate-token.command.ts (1)
25-33: Mark createErrorAndExit as non-returning for stronger type safetyThis function always exits the process; give it a
neverreturn type to help TS flow analysis in callers.- private createErrorAndExit = (errorMessage: string) => { + private createErrorAndExit = (errorMessage: string): never => { this.logger.always( JSON.stringify({ error: errorMessage, valid: false, }) ); process.exit(1); };api/src/unraid-api/cli/api-report.service.ts (1)
98-111: Defensive null-check for connect.dynamicRemoteAccessIf
connectis present butdynamicRemoteAccessis null/undefined, this block will throw. Guard the nested fields.Example refactor:
connect: connectData ? { installed: true, dynamicRemoteAccess: connectData.dynamicRemoteAccess ? { enabledType: connectData.dynamicRemoteAccess.enabledType, runningType: connectData.dynamicRemoteAccess.runningType, error: connectData.dynamicRemoteAccess.error ?? null, } : undefined, } : { installed: false, reason: errorReason || 'Connect plugin not installed or not available', },api/src/unraid-api/cli/cli.module.spec.ts (1)
27-31: Type the token-based retrieval for stronger safety (and assert full surface).Use generics with the DI token so TS can catch mismatches at compile time, and assert clearClient is present just like in the dependency block.
@@ -import { CANONICAL_INTERNAL_CLIENT_TOKEN, INTERNAL_CLIENT_SERVICE_TOKEN } from '@unraid/shared'; +import { CANONICAL_INTERNAL_CLIENT_TOKEN, INTERNAL_CLIENT_SERVICE_TOKEN } from '@unraid/shared'; +import type { CanonicalInternalClientService } from '@unraid/shared'; @@ - it('should provide CanonicalInternalClient', () => { - const service = module.get(CANONICAL_INTERNAL_CLIENT_TOKEN); + it('should provide CanonicalInternalClient', () => { + const service = module.get<CanonicalInternalClientService>(CANONICAL_INTERNAL_CLIENT_TOKEN); expect(service).toBeDefined(); expect(service.getClient).toBeInstanceOf(Function); + expect(service.clearClient).toBeInstanceOf(Function); }); @@ - const service = module.get(CANONICAL_INTERNAL_CLIENT_TOKEN); + const service = module.get<CanonicalInternalClientService>(CANONICAL_INTERNAL_CLIENT_TOKEN); expect(service).toBeDefined();Also applies to: 39-45, 4-4
api/src/unraid-api/cli/__test__/developer-tools.service.test.ts (4)
82-84: Avoid brittle log-message assertions; match intent, not exact text.Use stringContaining so minor copy changes won’t break the test.
- expect(logService.info).toHaveBeenCalledWith('Enabling sandbox mode - restarting API...'); + expect(logService.info).toHaveBeenCalledWith(expect.stringContaining('Enabling sandbox mode'));
97-99: Same here: make the "disable" assertion resilient.- expect(logService.info).toHaveBeenCalledWith('Sandbox mode disabled successfully.'); + expect(logService.info).toHaveBeenCalledWith(expect.stringContaining('Sandbox mode disabled'));
70-71: Verify the internal client is requested with the expected options.This ensures the service honors the new getClient({ enableSubscriptions: false }) contract.
await service.setSandboxMode(true); + expect(internalClient.getClient).toHaveBeenCalledWith({ enableSubscriptions: false }); expect(mockClient.mutate).toHaveBeenCalledWith({ @@ await service.setSandboxMode(false); + expect(internalClient.getClient).toHaveBeenCalledWith({ enableSubscriptions: false }); expect(logService.info).toHaveBeenCalledWith('Sandbox mode disabled successfully.');Also applies to: 95-96
121-127: Align error assertions with testing guidelines.Per repo guidelines, avoid checking exact error messages; use .rejects.toThrow() without arguments.
- await expect(service.enableModalTest()).rejects.toThrow( - 'Directory does not exist: /usr/local/emhttp/plugins/dynamix.my.servers' - ); + await expect(service.enableModalTest()).rejects.toThrow();api/src/unraid-api/auth/local-session.strategy.ts (2)
23-31: Handle possible string[] header shape for robustness.Fastify/Node headers can be string|string[]; normalize before use.
- async validate(request: FastifyRequest): Promise<UserAccount | null> { + async validate(request: FastifyRequest): Promise<UserAccount | null> { try { - const localSessionToken = request.headers['x-local-session'] as string; + const raw = request.headers?.['x-local-session'] as unknown; + const localSessionToken = Array.isArray(raw) ? raw[0] : (raw as string | undefined); if (!localSessionToken) { this.logger.verbose('No local session token found in request headers'); return null; }
41-44: Log errors at error level and include stack, not as verbose.Keeps severity accurate and improves debuggability.
- } catch (error) { - this.logger.verbose(error, `Local session validation failed`); - return null; - } + } catch (error) { + this.logger.error('Local session validation failed', (error as Error)?.stack); + return null; + }api/src/unraid-api/auth/auth.service.ts (2)
88-110: Local-session validation path looks correct; consider minor hardening.Logic mirrors cookie flow and correctly syncs roles. Consider extracting the shared “sync roles for user and log” into a small helper to DRY with validateCookiesWithCsrfToken.
Example helper (for reference only):
private async finalizeUser(user: UserAccount, contextLabel: string): Promise<UserAccount> { await this.syncUserRoles(user.id, user.roles); const roles = await this.authzService.getRolesForUser(user.id); this.logger.debug(`${contextLabel} user ${user.id} has roles: ${roles}`); return user; }And then:
const user = await this.getLocalSessionUser(); return this.finalizeUser(user, 'Local session');
262-288: Two factory methods share nearly identical structure.getSessionUser and getLocalSessionUser differ only in IDs and labels. Consider a small factory function to reduce repetition.
api/src/unraid-api/cli/developer/developer-tools.service.ts (1)
17-18: Use consistent logging facilities for SSOFileModification.You’re constructing a new Nest Logger just to pass to SSOFileModification while the service otherwise uses LogService. If feasible, accept LogService in SSOFileModification or wrap it to keep formatting/transport consistent.
api/src/unraid-api/shared/internal-graphql-client.factory.ts (5)
70-71: Logger formatting uses printf-style placeholders; Nest Logger doesn’t format%sUse template literals (or pass a context string as the 2nd param) so the actual URI/path is logged.
- this.logger.debug('WebSocket subscriptions enabled: %s', wsUri); + this.logger.debug(`WebSocket subscriptions enabled: ${wsUri}`); ... - this.logger.debug('Creating GraphQL client using Unix socket: %s', socketPath); + this.logger.debug(`Creating GraphQL client using Unix socket: ${socketPath}`); ... - this.logger.debug('Creating GraphQL client using HTTP: %s', httpUri); + this.logger.debug(`Creating GraphQL client using HTTP: ${httpUri}`);Also applies to: 75-76, 101-102
52-56: Validation is good; minor copy edit and consider exposing which option was missing for quicker diagnosisError message is clear; optionally include which options were provided to aid debugging (kept as a nit).
113-152: Loosen hard-fail behavior and allow fallback across multiple provided auth providersRight now, if
getApiKeyexists but returns an empty string or throws, the code won’t trygetLocalSession/getCookieAuth. Same for local session before cookie. Suggest trying providers in priority order and only throwing if all fail. Also guard against empty strings.- const authLink = setContext(async (_, { headers }) => { - if (getApiKey) { - // Use API key authentication - const apiKey = await getApiKey(); - return { - headers: { - ...headers, - 'x-api-key': apiKey, - }, - }; - } else if (getLocalSession) { - // Use local session authentication - const localSession = await getLocalSession(); - if (!localSession) { - throw new Error('No valid local session found'); - } - return { - headers: { - ...headers, - 'x-local-session': localSession, - }, - }; - } else if (getCookieAuth) { - // Use cookie-based authentication - const cookieAuth = await getCookieAuth(); - if (!cookieAuth) { - throw new Error('No valid session found for cookie authentication'); - } - return { - headers: { - ...headers, - 'x-csrf-token': cookieAuth.csrfToken, - cookie: `unraid_${cookieAuth.sessionId}=${cookieAuth.sessionId}`, - }, - }; - } - - return { headers }; - }); + const authLink = setContext(async (_, { headers }) => { + let authHeaders: Record<string, string> | null = null; + + // 1) API key + if (getApiKey) { + try { + const apiKey = await getApiKey(); + if (apiKey) authHeaders = { 'x-api-key': apiKey }; + } catch (e) { + this.logger.warn('[GRAPHQL-CLIENT] getApiKey failed: %o', e); + } + } + + // 2) Local session + if (!authHeaders && getLocalSession) { + try { + const localSession = await getLocalSession(); + if (localSession) authHeaders = { 'x-local-session': localSession }; + } catch (e) { + this.logger.warn('[GRAPHQL-CLIENT] getLocalSession failed: %o', e); + } + } + + // 3) Cookie auth + if (!authHeaders && getCookieAuth) { + try { + const cookieAuth = await getCookieAuth(); + if (cookieAuth?.sessionId && cookieAuth?.csrfToken) { + authHeaders = { + 'x-csrf-token': cookieAuth.csrfToken, + // TODO: derive cookie name from config (see follow-up comment) + cookie: `unraid_${cookieAuth.sessionId}=${cookieAuth.sessionId}`, + }; + } + } catch (e) { + this.logger.warn('[GRAPHQL-CLIENT] getCookieAuth failed: %o', e); + } + } + + return { headers: { ...headers, ...(authHeaders ?? {}) } }; + });
154-158: Also log GraphQL errors, not just network errorsCapturing
graphQLErrorshelps troubleshoot server-side exceptions quickly.- const errorLink = onError(({ networkError }) => { - if (networkError) { - this.logger.warn('[GRAPHQL-CLIENT] NETWORK ERROR ENCOUNTERED %o', networkError); - } - }); + const errorLink = onError(({ graphQLErrors, networkError, operation }) => { + if (graphQLErrors?.length) { + this.logger.warn( + `[GRAPHQL-CLIENT] GraphQL errors in ${operation?.operationName ?? 'anonymous'}: ${graphQLErrors + .map((e) => e.message) + .join('; ')}` + ); + } + if (networkError) { + this.logger.warn('[GRAPHQL-CLIENT] NETWORK ERROR ENCOUNTERED %o', networkError); + } + });
166-190: Mirror the “try-next-provider” fallback for WebSocket auth connectionParamsSame issue as HTTP path: if one provider is present but unusable (empty/throws), we never attempt the others. Consider the same progressive fallback.
- if (getApiKey) { - const apiKey = await getApiKey(); - return { 'x-api-key': apiKey }; - } else if (getLocalSession) { - const localSession = await getLocalSession(); - if (!localSession) { - throw new Error( - 'No valid local session found for WebSocket authentication' - ); - } - return { 'x-local-session': localSession }; - } else if (getCookieAuth) { - const cookieAuth = await getCookieAuth(); - if (!cookieAuth) { - throw new Error( - 'No valid session found for WebSocket cookie authentication' - ); - } - return { - 'x-csrf-token': cookieAuth.csrfToken, - cookie: `unraid_${cookieAuth.sessionId}=${cookieAuth.sessionId}`, - }; - } - return {}; + // Try API key + if (getApiKey) { + try { + const apiKey = await getApiKey(); + if (apiKey) return { 'x-api-key': apiKey }; + } catch {} + } + // Try local session + if (getLocalSession) { + try { + const localSession = await getLocalSession(); + if (localSession) return { 'x-local-session': localSession }; + } catch {} + } + // Try cookie auth + if (getCookieAuth) { + try { + const cookieAuth = await getCookieAuth(); + if (cookieAuth?.sessionId && cookieAuth?.csrfToken) { + return { + 'x-csrf-token': cookieAuth.csrfToken, + cookie: `unraid_${cookieAuth.sessionId}=${cookieAuth.sessionId}`, + }; + } + } catch {} + } + return {};api/src/unraid-api/plugin/global-deps.module.ts (1)
35-37: Token naming vs. implementation type can confuse future readers
INTERNAL_CLIENT_SERVICE_TOKENpoints toInternalGraphQLClientFactory(a factory), while the name suggests a “service”. Consider renaming the token in a future PR to something likeINTERNAL_GRAPHQL_CLIENT_FACTORY_TOKENand updating consumers.Also applies to: 76-85
api/src/unraid-api/shared/internal-client.service.ts (3)
30-33: Defaulting enableSubscriptions to true changes prior behavior; verify consumers expect WS by defaultFactory’s default is
false, but this service flips it totrue. That can open WS connections even for clients that only need HTTP. Please confirm intended behavior or consider inheriting the factory’s default.If you decide to align with the factory’s default:
- const { enableSubscriptions = true, origin } = options || {}; + const { enableSubscriptions = false, origin } = options || {};Also applies to: 68-75
81-86: Potential WS resource leak when clearing the client
ApolloClient.stop()won’t necessarily dispose the underlyinggraphql-wsclient created inside the factory. Consider extending the factory to return a disposer you can call fromclearClient(), or store a handle to the WS client inside the Apollo link and expose adispose()hook.I can sketch a small abstraction on the factory to return
{ client, dispose }and thread it through here if you want.
30-33: Single cached client ignores differing option sets; document this or key the cache by optionsIf callers pass different
{ enableSubscriptions, origin }over time, the first one “wins” untilclearClient(). Either document this behavior, or (future enhancement) maintain a small keyed cache per option set.Also applies to: 64-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (36)
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json(0 hunks)api/dev/keys/fc91da7b-0284-46f4-9018-55aa9759fba9.json(0 hunks)api/src/unraid-api/auth/auth.module.ts(3 hunks)api/src/unraid-api/auth/auth.service.spec.ts(3 hunks)api/src/unraid-api/auth/auth.service.ts(5 hunks)api/src/unraid-api/auth/authentication.guard.ts(2 hunks)api/src/unraid-api/auth/cookie.service.ts(3 hunks)api/src/unraid-api/auth/local-session.service.ts(1 hunks)api/src/unraid-api/auth/local-session.strategy.ts(1 hunks)api/src/unraid-api/cli/__test__/api-report.service.test.ts(2 hunks)api/src/unraid-api/cli/__test__/developer-tools.service.test.ts(4 hunks)api/src/unraid-api/cli/admin-key.service.ts(0 hunks)api/src/unraid-api/cli/api-report.service.ts(4 hunks)api/src/unraid-api/cli/cli-services.module.ts(1 hunks)api/src/unraid-api/cli/cli.module.spec.ts(3 hunks)api/src/unraid-api/cli/cli.module.ts(0 hunks)api/src/unraid-api/cli/developer/developer-tools.service.ts(2 hunks)api/src/unraid-api/cli/internal-client.service.spec.ts(0 hunks)api/src/unraid-api/cli/internal-client.service.ts(0 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(3 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts(1 hunks)api/src/unraid-api/plugin/global-deps.module.ts(4 hunks)api/src/unraid-api/rest/__test__/rest-module.integration.test.ts(2 hunks)api/src/unraid-api/shared/internal-client.service.ts(2 hunks)api/src/unraid-api/shared/internal-graphql-client.factory.ts(3 hunks)packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts(0 hunks)packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.spec.ts(0 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts(2 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts(1 hunks)packages/unraid-shared/src/index.ts(1 hunks)packages/unraid-shared/src/tokens.ts(1 hunks)packages/unraid-shared/src/types/canonical-internal-client.interface.ts(1 hunks)packages/unraid-shared/src/types/internal-graphql-client.factory.ts(1 hunks)
💤 Files with no reviewable changes (8)
- api/src/unraid-api/cli/internal-client.service.spec.ts
- packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.spec.ts
- api/dev/keys/fc91da7b-0284-46f4-9018-55aa9759fba9.json
- api/src/unraid-api/cli/cli.module.ts
- api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
- packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts
- api/src/unraid-api/cli/internal-client.service.ts
- api/src/unraid-api/cli/admin-key.service.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tspackages/unraid-shared/src/tokens.tsapi/src/unraid-api/auth/local-session.strategy.tspackages/unraid-shared/src/index.tspackages/unraid-shared/src/types/canonical-internal-client.interface.tsapi/src/unraid-api/auth/authentication.guard.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.tsapi/src/unraid-api/auth/auth.module.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect.module.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/cli/api-report.service.tsapi/src/unraid-api/auth/auth.service.tspackages/unraid-shared/src/types/internal-graphql-client.factory.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/cli/cli-services.module.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/cli/cli.module.spec.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/auth/local-session.strategy.tsapi/src/unraid-api/auth/authentication.guard.tsapi/src/unraid-api/auth/auth.module.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/cli/api-report.service.tsapi/src/unraid-api/auth/auth.service.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/cli/cli-services.module.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/cli/cli.module.spec.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/cli.module.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/cli.module.spec.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tspackages/unraid-shared/src/tokens.tsapi/src/unraid-api/auth/local-session.strategy.tspackages/unraid-shared/src/index.tspackages/unraid-shared/src/types/canonical-internal-client.interface.tsapi/src/unraid-api/auth/authentication.guard.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.tsapi/src/unraid-api/auth/auth.module.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect.module.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/cli/api-report.service.tsapi/src/unraid-api/auth/auth.service.tspackages/unraid-shared/src/types/internal-graphql-client.factory.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/cli/cli-services.module.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/cli/cli.module.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.tsapi/src/unraid-api/cli/cli.module.spec.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/cli/__test__/api-report.service.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.ts
🧠 Learnings (6)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/cli/__test__/developer-tools.service.test.tsapi/src/unraid-api/cli/cli.module.spec.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts
📚 Learning: 2024-11-04T20:44:46.432Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying `apiKey.roles` in `removeRoleFromApiKey` and `addRoleToApiKey` within `api/src/unraid-api/auth/auth.service.ts`, concurrency issues are not a concern because the keys are stored in the file system.
Applied to files:
api/src/unraid-api/auth/auth.module.ts
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/auth/auth.module.tsapi/src/unraid-api/auth/cookie.service.ts
📚 Learning: 2025-03-10T18:43:13.016Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/types/fastify.ts:25-30
Timestamp: 2025-03-10T18:43:13.016Z
Learning: In the Unraid API, cookies should be marked as optional (with `?`) in the FastifyRequest type definition since the cookies may not always be present, which is consistent with the authentication flow throughout the codebase.
Applied to files:
packages/unraid-shared/src/types/internal-graphql-client.factory.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.ts
🧬 Code graph analysis (15)
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts (2)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts (1)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
api/src/unraid-api/auth/local-session.strategy.ts (4)
api/src/unraid-api/auth/auth.service.ts (1)
Injectable(15-289)api/src/unraid-api/auth/authentication.guard.ts (1)
Injectable(39-112)api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(10-98)api/src/unraid-api/types/fastify.ts (1)
FastifyRequest(25-30)
packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
packages/unraid-shared/src/index.ts (1)
CanonicalInternalClientService(7-7)
api/src/unraid-api/cli/developer/developer-tools.service.ts (3)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/index.ts (1)
CanonicalInternalClientService(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/cli/__test__/api-report.service.test.ts (1)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
api/src/unraid-api/auth/local-session.service.ts (4)
api/src/unraid-api/auth/auth.service.ts (1)
Injectable(15-289)api/src/unraid-api/auth/local-session.strategy.ts (1)
Injectable(14-46)api/src/unraid-api/shared/internal-client.service.ts (1)
Injectable(15-87)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(26-232)
api/src/unraid-api/cli/__test__/developer-tools.service.test.ts (2)
packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
api/src/unraid-api/plugin/global-deps.module.ts (2)
api/src/unraid-api/auth/cookie.service.ts (1)
SESSION_COOKIE_CONFIG(10-10)packages/unraid-shared/src/tokens.ts (4)
COOKIE_SERVICE_TOKEN(6-6)NGINX_SERVICE_TOKEN(4-4)CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)INTERNAL_CLIENT_SERVICE_TOKEN(5-5)
api/src/unraid-api/cli/sso/validate-token.command.ts (2)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/cli/api-report.service.ts (2)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/auth/auth.service.ts (1)
api/src/utils.ts (1)
handleAuthError(213-245)
api/src/unraid-api/shared/internal-client.service.ts (5)
api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(10-98)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(26-232)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)packages/unraid-shared/src/tokens.ts (1)
INTERNAL_CLIENT_SERVICE_TOKEN(5-5)packages/unraid-shared/src/types/internal-graphql-client.factory.ts (1)
InternalGraphQLClientFactory(7-15)
api/src/unraid-api/shared/internal-graphql-client.factory.ts (2)
packages/unraid-api-plugin-connect/src/mothership-proxy/connection.service.ts (1)
getApiKey(162-164)api/src/unraid-api/auth/local-session.service.ts (1)
getLocalSession(55-63)
api/src/unraid-api/cli/cli.module.spec.ts (1)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
Show resolved
Hide resolved
pujitm
force-pushed
the
feat/memory-keys
branch
from
0a8c36a to
f88464f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
api/src/unraid-api/auth/local-session.service.ts (3)
11-26: Add cleanup on service shutdown to prevent stale tokens.The service generates a new token on startup but doesn't clean up the session file on shutdown. This could leave stale tokens on the filesystem during normal restarts.
Implement
OnModuleDestroyto clean up:-import { Injectable, Logger, OnModuleInit } from '@nestjs/common'; +import { Injectable, Logger, OnModuleInit, OnModuleDestroy } from '@nestjs/common'; import { randomBytes, timingSafeEqual } from 'crypto'; -import { chmod, mkdir, readFile, writeFile } from 'fs/promises'; +import { chmod, mkdir, readFile, writeFile, unlink } from 'fs/promises'; import { dirname } from 'path'; -export class LocalSessionService implements OnModuleInit { +export class LocalSessionService implements OnModuleInit, OnModuleDestroy { private readonly logger = new Logger(LocalSessionService.name); private sessionToken: string | null = null; private static readonly SESSION_FILE_PATH = '/var/run/unraid-api/local-session'; // NOTE: do NOT cleanup the session file upon eg. module/application shutdown. // That would invalidate the session after each cli invocation, which is incorrect. // Instead, rely on the startup logic to invalidate/overwrite any obsolete session. async onModuleInit() { try { await this.generateLocalSession(); this.logger.verbose('Local session initialized'); } catch (error) { this.logger.error('Failed to initialize local session:', error); } } + async onModuleDestroy() { + try { + await unlink(LocalSessionService.SESSION_FILE_PATH); + this.logger.debug('Local session file cleaned up'); + } catch (error) { + this.logger.debug('Failed to clean up local session file:', error); + } + }
70-78: Optimize validateLocalSession to use cached token.Similarly, validation can use the cached token to avoid disk reads on every authentication request.
public async validateLocalSession(token: string): Promise<boolean> { if (!token) return false; + + // Compare against in-memory token first (fast path) + if (this.sessionToken) { + return timingSafeEqual(Buffer.from(token, 'utf-8'), Buffer.from(this.sessionToken, 'utf-8')); + } const currentToken = await this.getLocalSession(); if (!currentToken) return false; // Use constant-time comparison to prevent timing attacks return timingSafeEqual(Buffer.from(token, 'utf-8'), Buffer.from(currentToken, 'utf-8')); }
58-65: Use in-memory token to avoid per-request disk I/O.The current implementation reads from disk on every validation request, causing unnecessary I/O on hot paths. Since the token is already stored in memory, use it as the primary source.
public async getLocalSession(): Promise<string | null> { + // Prefer in-memory token to avoid disk I/O on hot paths + if (this.sessionToken) return this.sessionToken; try { - return await readFile(LocalSessionService.SESSION_FILE_PATH, 'utf-8'); + const token = await readFile(LocalSessionService.SESSION_FILE_PATH, 'utf-8'); + this.sessionToken = token.trim(); + return this.sessionToken; } catch (error) { this.logger.warn(error, 'Local session file not found or not readable'); return null; } }packages/unraid-shared/src/tokens.ts (1)
5-7: Export missing tokens in shared package indexThe
COOKIE_SERVICE_TOKENandCANONICAL_INTERNAL_CLIENT_TOKENconstants are not currently re-exported from the shared package entrypoint, which will prevent consumers from importing them. They are correctly wired in the API’sglobal-deps.module.ts, so we just need to expose them inpackages/unraid-shared/src/index.ts.• File needing update:
packages/unraid-shared/src/index.ts• Add the following exports at the bottom of that file:
export * from './other-module'; export * from './another-module'; +export { COOKIE_SERVICE_TOKEN } from './tokens'; +export { CANONICAL_INTERNAL_CLIENT_TOKEN } from './tokens';Once added, downstream packages can import these tokens directly via the shared package index.
api/src/unraid-api/shared/internal-client.service.ts (1)
41-44: Fix “first call wins” bug: make cache options-aware and track config.Current cache ignores per-call options; later callers can’t enable subscriptions or change origin.
Apply:
@@ - // If client already exists, return it - if (this.client) { - return this.client; - } + // Normalize desired configuration + const desiredEnableSubscriptions = options?.enableSubscriptions ?? true; + const desiredOrigin = options?.origin; + + // If we already have a client, only return it if it satisfies desired capabilities. + if (this.client && this.clientConfig) { + const { enableSubscriptions: hasSubs, origin: currentOrigin } = this.clientConfig; + const needsUpgrade = + (desiredEnableSubscriptions && !hasSubs) || + (desiredOrigin && desiredOrigin !== currentOrigin); + if (!needsUpgrade) { + return this.client; + } + // Upgrade path: drop existing instance so we can recreate with richer capabilities. + this.clearClient(); + } @@ - // Start client creation and store the promise - const creationPromise = this.createClient(options); + // Start client creation (normalized) and store the promise + const creationPromise = this.createClient({ + enableSubscriptions: desiredEnableSubscriptions, + origin: desiredOrigin, + }); @@ - this.logger.debug('Created canonical internal GraphQL client with local session authentication'); - return client; + // Track active client configuration for future reuse/upgrade decisions + this.clientConfig = { enableSubscriptions, origin }; + this.logger.debug( + `Created canonical internal GraphQL client with local session auth (subscriptions=${enableSubscriptions}, origin=${origin ?? 'http://localhost'})` + ); + return client; @@ this.client?.stop(); this.client = null; this.clientCreationPromise = null; + this.clientConfig = null;Add this field near other members:
// Track the config used to create the cached client private clientConfig: { enableSubscriptions: boolean; origin?: string } | null = null;Also applies to: 51-54, 84-86, 91-93
🧹 Nitpick comments (4)
api/src/unraid-api/auth/cookie.service.ts (1)
103-126: Consider selecting the most recent valid session by mtime.The current implementation returns the first valid session found, which may not be deterministic. Consider selecting the most recent session based on file modification time.
async getActiveSession(): Promise<string | null> { let sessionFiles: string[] = []; try { sessionFiles = await readdir(this.opts.sessionDir); } catch (e) { this.logger.warn(e, 'Error reading session directory'); return null; } + let mostRecentSession: { id: string; mtime: number } | null = null; for (const sessionFile of sessionFiles) { if (!sessionFile.startsWith('sess_')) { continue; } try { const sessionData = await readFile(join(this.opts.sessionDir, sessionFile), 'ascii'); if (this.isSessionValid(sessionData)) { - return sessionFile.replace('sess_', ''); + const stats = await stat(join(this.opts.sessionDir, sessionFile)); + const sessionId = sessionFile.replace('sess_', ''); + if (!mostRecentSession || stats.mtimeMs > mostRecentSession.mtime) { + mostRecentSession = { id: sessionId, mtime: stats.mtimeMs }; + } } } catch { // Ignore unreadable files and continue scanning continue; } } - return null; + return mostRecentSession?.id ?? null; }Don't forget to import
stat:-import { readdir, readFile } from 'fs/promises'; +import { readdir, readFile, stat } from 'fs/promises';api/src/unraid-api/cli/cli.module.spec.ts (2)
33-37: Avoid instance checks against concrete class; assert contract instead.Binding may switch implementations without changing the token. Prefer capability assertion.
Apply:
- expect(factory).toBeInstanceOf(InternalGraphQLClientFactory); + expect(typeof (factory as any).createClient).toBe('function');
39-50: Add a no-op idempotency check for clearClient.Low-cost sanity guard that doesn’t hit the network.
Apply:
expect(service.getClient).toBeDefined(); expect(service.clearClient).toBeDefined(); + // Idempotent dispose + expect(() => service.clearClient()).not.toThrow(); + expect(() => service.clearClient()).not.toThrow();api/src/unraid-api/plugin/global-deps.module.ts (1)
3-14: Minor: avoid token name ambiguity with class name.INTERNAL_CLIENT_FACTORY_TOKEN resolves to 'InternalClientService' which is also a class in this module; different token identities but confusing.
Consider renaming the token string in packages/unraid-shared to 'InternalGraphQLClientFactory' to reduce ambiguity. If you want, I can draft that follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
api/src/unraid-api/auth/auth.service.ts(6 hunks)api/src/unraid-api/auth/cookie.service.ts(4 hunks)api/src/unraid-api/auth/local-session.service.ts(1 hunks)api/src/unraid-api/cli/cli.module.spec.ts(3 hunks)api/src/unraid-api/plugin/global-deps.module.ts(5 hunks)api/src/unraid-api/shared/internal-client.service.ts(1 hunks)api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts(2 hunks)api/src/unraid-api/shared/internal-graphql-client.factory.ts(5 hunks)packages/unraid-shared/src/tokens.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/shared/internal-graphql-client.factory.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/shared/internal-graphql-client.factory.spec.tspackages/unraid-shared/src/tokens.tsapi/src/unraid-api/cli/cli.module.spec.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.service.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/shared/internal-graphql-client.factory.spec.tsapi/src/unraid-api/cli/cli.module.spec.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/shared/internal-graphql-client.factory.spec.tspackages/unraid-shared/src/tokens.tsapi/src/unraid-api/cli/cli.module.spec.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.service.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/shared/internal-graphql-client.factory.spec.tsapi/src/unraid-api/cli/cli.module.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/unraid-api/shared/internal-graphql-client.factory.spec.tsapi/src/unraid-api/cli/cli.module.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/shared/internal-graphql-client.factory.spec.tsapi/src/unraid-api/cli/cli.module.spec.ts
🧠 Learnings (7)
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.spec.tsapi/src/unraid-api/auth/cookie.service.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.spec.tsapi/src/unraid-api/cli/cli.module.spec.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should mock external dependencies and services
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/auth/auth.service.ts
📚 Learning: 2025-01-15T21:34:00.006Z
Learnt from: pujitm
PR: unraid/api#1047
File: api/src/unraid-api/graph/sandbox-plugin.ts:57-57
Timestamp: 2025-01-15T21:34:00.006Z
Learning: In the GraphQL sandbox (api/src/unraid-api/graph/sandbox-plugin.ts), CSRF token validation should fail silently with a fallback value to maintain sandbox accessibility, as it's a development tool where strict security measures aren't required.
Applied to files:
api/src/unraid-api/auth/auth.service.ts
📚 Learning: 2025-03-10T17:24:06.914Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/auth/cookie.strategy.ts:19-20
Timestamp: 2025-03-10T17:24:06.914Z
Learning: In the auth system, CSRF token validation and cookie validation have been unified in the `validateCookiesCasbin()` method in the AuthService class, which takes the entire FastifyRequest object and performs both validations sequentially.
Applied to files:
api/src/unraid-api/auth/auth.service.ts
🧬 Code graph analysis (6)
api/src/unraid-api/shared/internal-client.service.ts (5)
api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(10-86)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(28-235)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)packages/unraid-shared/src/tokens.ts (1)
INTERNAL_CLIENT_FACTORY_TOKEN(5-5)packages/unraid-shared/src/types/internal-graphql-client.factory.ts (1)
InternalGraphQLClientFactory(7-15)
api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts (1)
api/src/unraid-api/auth/cookie.service.ts (1)
SESSION_COOKIE_CONFIG(10-10)
api/src/unraid-api/cli/cli.module.spec.ts (2)
packages/unraid-shared/src/tokens.ts (2)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)INTERNAL_CLIENT_FACTORY_TOKEN(5-5)packages/unraid-shared/src/types/internal-graphql-client.factory.ts (1)
InternalGraphQLClientFactory(7-15)
api/src/unraid-api/plugin/global-deps.module.ts (2)
packages/unraid-shared/src/tokens.ts (4)
INTERNAL_CLIENT_FACTORY_TOKEN(5-5)COOKIE_SERVICE_TOKEN(6-6)NGINX_SERVICE_TOKEN(4-4)CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)api/src/unraid-api/auth/cookie.service.ts (1)
SESSION_COOKIE_CONFIG(10-10)
api/src/unraid-api/auth/local-session.service.ts (4)
api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(28-235)api/src/unraid-api/shared/internal-client.service.ts (1)
Injectable(15-94)api/src/unraid-api/auth/auth.service.ts (1)
Injectable(16-293)api/src/unraid-api/auth/local-session.strategy.ts (1)
Injectable(14-46)
api/src/unraid-api/auth/auth.service.ts (2)
api/src/utils.ts (1)
handleAuthError(213-245)api/src/store/index.ts (1)
getters(18-23)
🔇 Additional comments (11)
api/src/unraid-api/auth/auth.service.ts (3)
253-258: LGTM! Proper timing-safe comparison implemented.Good security improvement using
timingSafeEqualfor CSRF token comparison, preventing timing attacks.
89-111: LGTM! Local session validation properly implemented.The implementation correctly validates the local session token, syncs roles, and uses proper error handling.
283-292: LGTM! Local session user properly configured.The local session user is correctly configured with admin privileges and a descriptive identifier for CLI/system operations.
api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts (1)
53-61: LGTM! Test properly configured with SESSION_COOKIE_CONFIG.The test correctly provides the
SESSION_COOKIE_CONFIGtoken with appropriate test values.api/src/unraid-api/auth/cookie.service.ts (1)
78-80: LGTM! Good refactoring to centralize session validation.The extraction of session validation logic into a private helper method improves maintainability and reduces code duplication.
api/src/unraid-api/cli/cli.module.spec.ts (1)
27-31: Good smoke test for CanonicalInternalClient.Asserting presence of getClient is sufficient here.
api/src/unraid-api/plugin/global-deps.module.ts (4)
35-37: Factory wiring via token is correct.Token-based binding to InternalGraphQLClientFactory looks good.
50-57: CookieService DI and config export look correct.SESSION_COOKIE_CONFIG and COOKIE_SERVICE_TOKEN are properly provided.
62-68: Canonical client mapped to existing service.useExisting keeps the token stable while allowing internal refactors. Good.
81-85: Export set aligns with new auth path.COOKIE_SERVICE_TOKEN, INTERNAL_CLIENT_FACTORY_TOKEN, and CANONICAL_INTERNAL_CLIENT_TOKEN exports are appropriate.
api/src/unraid-api/shared/internal-client.service.ts (1)
75-82: Confirm default subscription behavior is intentional.You default enableSubscriptions to true (factory defaults to false). Ensure this is desired to avoid unnecessary WS links in CLI contexts.
If this should follow caller preference, keep the normalization above but don’t force true when options are undefined.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/auth/auth.module.ts (1)
57-69: Lifecycle service not provided → local session never generated/cleanedLocalSessionLifecycleService isn’t in providers, so its OnModuleInit/Destroy won’t run. Internal clients depending on x-local-session will fail.
Apply:
providers: [ AuthService, ApiKeyService, ServerHeaderStrategy, LocalSessionStrategy, UserCookieStrategy, CookieService, - LocalSessionService, + LocalSessionService, + LocalSessionLifecycleService, { provide: SESSION_COOKIE_CONFIG, useValue: CookieService.defaultOpts(), }, ],
♻️ Duplicate comments (1)
api/src/unraid-api/auth/local-session.service.ts (1)
46-53: Use in-memory cache, trim on read, and fix Logger arg orderRemoves hot-path disk I/O, stabilizes behavior across transient FS issues, and fixes warn signature.
Apply:
- public async getLocalSession(): Promise<string | null> { - try { - return await readFile(LocalSessionService.getSessionFilePath(), 'utf-8'); - } catch (error) { - this.logger.warn(error, 'Local session file not found or not readable'); - return null; - } - } + public async getLocalSession(): Promise<string | null> { + if (this.sessionToken) return this.sessionToken; + try { + const token = await readFile(LocalSessionService.getSessionFilePath(), 'utf-8'); + this.sessionToken = token.trim(); + return this.sessionToken; + } catch (error) { + this.logger.warn( + `Local session file not found or not readable: ${(error as Error)?.message ?? String(error)}` + ); + return null; + } + } @@ - public async validateLocalSession(token: string): Promise<boolean> { - if (!token) return false; - - const currentToken = await this.getLocalSession(); - if (!currentToken) return false; - - // Use constant-time comparison to prevent timing attacks - return timingSafeEqual(Buffer.from(token, 'utf-8'), Buffer.from(currentToken, 'utf-8')); - } + public async validateLocalSession(token: string): Promise<boolean> { + if (!token) return false; + const currentToken = this.sessionToken ?? (await this.getLocalSession()); + if (!currentToken) return false; + const a = Buffer.from(token, 'utf-8'); + const b = Buffer.from(currentToken, 'utf-8'); + if (a.length !== b.length) return false; + return timingSafeEqual(a, b); + }Also applies to: 58-66
🧹 Nitpick comments (3)
api/src/unraid-api/auth/local-session.service.ts (2)
68-74: Clear in-memory token on delete; downgrade ENOENT to debug/warn; fix Logger argsPrevents stale in-memory token use; avoids noisy errors when file is already gone.
Apply:
- public async deleteLocalSession(): Promise<void> { - try { - await unlink(LocalSessionService.getSessionFilePath()); - } catch (error) { - this.logger.error(error, 'Error deleting local session file'); - } - } + public async deleteLocalSession(): Promise<void> { + try { + await unlink(LocalSessionService.getSessionFilePath()); + } catch (error: any) { + if (error?.code === 'ENOENT') { + this.logger.debug('Local session file already removed'); + } else { + this.logger.warn(`Error deleting local session file: ${error?.message ?? String(error)}`); + } + } finally { + this.sessionToken = null; + } + }
33-35: Redundant chmod after writeFile with modewriteFile already sets 0600; keeping chmod is harmless but unnecessary.
Optionally remove the extra chmod to reduce syscalls.
api/src/unraid-api/auth/auth.module.ts (1)
70-81: Exporting lifecycle service likely unnecessaryIt’s API-internal; exporting increases surface without need.
Apply:
exports: [ AuthService, ApiKeyService, PassportModule, ServerHeaderStrategy, LocalSessionStrategy, UserCookieStrategy, CookieService, LocalSessionService, - LocalSessionLifecycleService, AuthZModule, ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
api/src/unraid-api/auth/auth.module.ts(3 hunks)api/src/unraid-api/auth/local-session-lifecycle.service.ts(1 hunks)api/src/unraid-api/auth/local-session.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.module.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.module.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.module.ts
🧠 Learnings (2)
📚 Learning: 2024-11-04T20:44:46.432Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying `apiKey.roles` in `removeRoleFromApiKey` and `addRoleToApiKey` within `api/src/unraid-api/auth/auth.service.ts`, concurrency issues are not a concern because the keys are stored in the file system.
Applied to files:
api/src/unraid-api/auth/auth.module.ts
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/auth/auth.module.ts
🧬 Code graph analysis (3)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(10-82)
api/src/unraid-api/auth/local-session.service.ts (5)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
Injectable(14-25)api/src/unraid-api/shared/internal-client.service.ts (1)
Injectable(15-94)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(28-235)api/src/unraid-api/auth/auth.service.ts (1)
Injectable(16-293)api/src/unraid-api/auth/local-session.strategy.ts (1)
Injectable(14-46)
api/src/unraid-api/auth/auth.module.ts (2)
api/src/unraid-api/app/app.module.ts (1)
Module(22-64)api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
Module(37-76)
🔇 Additional comments (3)
api/src/unraid-api/auth/auth.module.ts (1)
22-26: Strategy order looks goodDefault strategies include local-session between server-header and user-cookie; aligns with the new flow.
api/src/unraid-api/auth/local-session-lifecycle.service.ts (2)
18-24: LGTM: Ties session lifecycle to API lifecycleInit/Destroy delegation to LocalSessionService is clear and minimal.
15-16: Confirm AuthModule provider registrationThe snippet from
auth.module.tsshows:55- }), 56- ], 57: providers: [ 58- AuthService, 59- ApiKeyService, …It isn’t clear whether
LocalSessionLifecycleServiceappears later in this list. Please double-check thatAuthModuleincludesLocalSessionLifecycleServicein itsprovidersarray so that it will be instantiated during startup.
- If it’s missing, add it alongside the other services.
- If it’s already present further down, you can disregard this comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/src/unraid-api/auth/local-session.service.ts (2)
51-58: Use in-memory cache and trim; reduce hot-path I/O and normalize valueAlso improve log message ordering.
- public async getLocalSession(): Promise<string | null> { - try { - return await readFile(LocalSessionService.getSessionFilePath(), 'utf-8'); - } catch (error) { - this.logger.warn(error, 'Local session file not found or not readable'); - return null; - } - } + public async getLocalSession(): Promise<string | null> { + if (this.sessionToken) return this.sessionToken; + try { + const raw = await readFile(LocalSessionService.getSessionFilePath(), 'utf-8'); + this.sessionToken = raw.trim(); + return this.sessionToken; + } catch (error) { + this.logger.warn('Local session file not found or not readable: ' + String(error)); + return null; + } + }
63-71: Guard timingSafeEqual against length mismatch; trim inputs and prefer cached tokenCurrent code can throw and crash on malformed header.
- public async validateLocalSession(token: string): Promise<boolean> { - if (!token) return false; - - const currentToken = await this.getLocalSession(); - if (!currentToken) return false; - - // Use constant-time comparison to prevent timing attacks - return timingSafeEqual(Buffer.from(token, 'utf-8'), Buffer.from(currentToken, 'utf-8')); - } + public async validateLocalSession(token: string): Promise<boolean> { + if (!token) return false; + const expected = this.sessionToken ?? (await this.getLocalSession()); + if (!expected) return false; + const a = Buffer.from(token.trim(), 'utf-8'); + const b = Buffer.from(expected.trim(), 'utf-8'); + if (a.length !== b.length) return false; + return timingSafeEqual(a, b); + }
🧹 Nitpick comments (5)
api/.env.test (1)
16-18: Reorder key to satisfy dotenv-linter and keep PATHS_ grouping consistent*Place PATHS_LOCAL_SESSION_FILE before PATHS_LOGS_FILE.
-PATHS_LOGS_FILE=./dev/log/graphql-api.log -PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOGS_FILE=./dev/log/graphql-api.logapi/.env.development (1)
18-22: Reorder key to satisfy dotenv-linterMove PATHS_LOCAL_SESSION_FILE before PATHS_LOGS_FILE.
-PATHS_LOGS_FILE=./dev/log/graphql-api.log -PATHS_CONNECT_STATUS_FILE_PATH=./dev/connectStatus.json # Connect plugin status file -PATHS_OIDC_JSON=./dev/configs/oidc.local.json -PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOGS_FILE=./dev/log/graphql-api.log +PATHS_CONNECT_STATUS_FILE_PATH=./dev/connectStatus.json # Connect plugin status file +PATHS_OIDC_JSON=./dev/configs/oidc.local.jsonapi/src/unraid-api/auth/local-session.service.ts (3)
1-7: Prefer node: scheme for built-ins for consistency with rest of repoMinor consistency improvement.
-import { randomBytes, timingSafeEqual } from 'crypto'; -import { chmod, mkdir, readFile, unlink, writeFile } from 'fs/promises'; -import { dirname } from 'path'; +import { randomBytes, timingSafeEqual } from 'node:crypto'; +import { chmod, mkdir, readFile, unlink, writeFile } from 'node:fs/promises'; +import { dirname } from 'node:path';
37-39: Fix Logger argument orderPass message first; current usage logs the error as the message and sets context incorrectly.
- await chmod(LocalSessionService.getSessionFilePath(), 0o600).catch((error) => { - this.logger.warn(error, 'Failed to set permissions on local session file'); - }); + await chmod(LocalSessionService.getSessionFilePath(), 0o600).catch((error) => { + this.logger.warn('Failed to set permissions on local session file: ' + String(error)); + });
73-79: Don’t error on ENOENT during cleanup; downgrade severity and handle gracefullyAvoid noisy logs when file is already gone.
- } catch (error) { - this.logger.error(error, 'Error deleting local session file'); - } + } catch (error: any) { + if (error?.code === 'ENOENT') { + this.logger.debug('Local session file already removed'); + return; + } + this.logger.warn('Failed to delete local session file: ' + String(error)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
api/.env.development(1 hunks)api/.env.test(1 hunks)api/.gitignore(1 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/auth/auth.module.ts(3 hunks)api/src/unraid-api/auth/local-session.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/.gitignore
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/environment.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.module.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/environment.tsapi/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.module.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/auth/local-session.service.tsapi/src/unraid-api/auth/auth.module.ts
🧠 Learnings (2)
📚 Learning: 2024-11-04T20:44:46.432Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying `apiKey.roles` in `removeRoleFromApiKey` and `addRoleToApiKey` within `api/src/unraid-api/auth/auth.service.ts`, concurrency issues are not a concern because the keys are stored in the file system.
Applied to files:
api/src/unraid-api/auth/auth.module.ts
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/auth/auth.module.ts
🧬 Code graph analysis (1)
api/src/unraid-api/auth/local-session.service.ts (6)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
Injectable(14-25)api/src/unraid-api/shared/internal-client.service.ts (1)
Injectable(15-94)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(28-235)api/src/unraid-api/auth/auth.service.ts (1)
Injectable(16-293)api/src/unraid-api/auth/local-session.strategy.ts (1)
Injectable(14-46)api/src/environment.ts (1)
PATHS_LOCAL_SESSION_FILE(112-113)
🪛 dotenv-linter (3.3.0)
api/.env.test
[warning] 17-17: [UnorderedKey] The PATHS_LOCAL_SESSION_FILE key should go before the PATHS_LOGS_FILE key
(UnorderedKey)
api/.env.development
[warning] 21-21: [UnorderedKey] The PATHS_LOCAL_SESSION_FILE key should go before the PATHS_LOGS_FILE key
(UnorderedKey)
🔇 Additional comments (4)
api/src/environment.ts (1)
112-113: LGTM: adds PATHS_LOCAL_SESSION_FILE with a sensible defaultMatches new auth flow and mirrors other PATHS_* constants.
api/src/unraid-api/auth/auth.module.ts (3)
14-16: LGTM: import local-session services/strategyImports align with the new auth mechanism.
61-66: LGTM: providers include LocalSessionStrategy/Service/LifecycleCorrect DI wiring for the new flow.
76-81: LGTM: export local-session componentsMakes strategy/service available to consumers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (3)
18-20: Add explicit return type on lifecycle hookHelps readability and satisfies stricter TS/Nest linters.
- async onModuleInit() { + async onModuleInit(): Promise<void> { await this.localSessionService.generateLocalSession(); }
1-1: Consider cleaning up the session on shutdownIf the token should not persist after API stop, wire OnModuleDestroy to delete the file. If persistence is desired, ignore.
-import { Injectable, OnModuleInit } from '@nestjs/common'; +import { Injectable, OnModuleInit, OnModuleDestroy } from '@nestjs/common'; -@Injectable() -export class LocalSessionLifecycleService implements OnModuleInit { +@Injectable() +export class LocalSessionLifecycleService implements OnModuleInit, OnModuleDestroy { constructor(private readonly localSessionService: LocalSessionService) {} - async onModuleInit() { + async onModuleInit(): Promise<void> { await this.localSessionService.generateLocalSession(); } + + async onModuleDestroy(): Promise<void> { + await this.localSessionService.deleteLocalSession(); + } }Also applies to: 15-21
18-20: Optionally avoid regenerating when a valid token already existsPrevents unnecessary churn across restarts if stability is preferred.
- async onModuleInit(): Promise<void> { - await this.localSessionService.generateLocalSession(); - } + async onModuleInit(): Promise<void> { + const existing = await this.localSessionService.getLocalSession(); + if (!existing) { + await this.localSessionService.generateLocalSession(); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
api/src/unraid-api/auth/local-session-lifecycle.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/auth/local-session-lifecycle.service.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/auth/local-session-lifecycle.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/auth/local-session-lifecycle.service.ts
🧬 Code graph analysis (1)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(12-87)
🔇 Additional comments (2)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (2)
3-3: ESM import extension is correct per guidelinesImport uses .js specifier and the file lives under api/src/unraid-api/, matching the repo rules. No change needed.
18-20: Confirm desired failure semantics and multi-instance behaviorDo we want API startup to fail if generation throws (current behavior), or degrade gracefully? Also, in multi-instance runs, concurrent inits can race/overwrite the token—confirm single-instance assumption or add coordination.
- Introduced persistent and ephemeral key storage in ApiKeyService for better key management. - Updated onModuleInit to load persistent keys and clean up legacy internal keys. - Enhanced findAll and findByField methods to prioritize ephemeral keys. - Added methods for registering ephemeral keys and modules, improving CLI integration. - Updated AdminKeyService to generate and manage ephemeral keys for CLI operations. - Adjusted ConnectApiKeyService to utilize module tokens instead of static keys for better security.
elibosley
force-pushed
the
feat/memory-keys
branch
from
8a49af1 to
c6a2873
Compare
…puts and using constant-time comparison
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
api/src/unraid-api/auth/cookie.service.ts (1)
99-126: getActiveSession: pick the newest valid sess_ and harden FS ops.*Current scan returns the first valid file (non-deterministic across filesystems). Prefer newest by mtime and use withFileTypes+stat; keep per-file try/catch.
- async getActiveSession(): Promise<string | null> { - let sessionFiles: string[] = []; - try { - sessionFiles = await readdir(this.opts.sessionDir); - } catch (e) { - this.logger.warn(e, 'Error reading session directory'); - return null; - } - for (const sessionFile of sessionFiles) { - if (!sessionFile.startsWith('sess_')) { - continue; - } - try { - const sessionData = await readFile(join(this.opts.sessionDir, sessionFile), 'ascii'); - if (this.isSessionValid(sessionData)) { - return sessionFile.replace('sess_', ''); - } - } catch { - // Ignore unreadable files and continue scanning - continue; - } - } - return null; - } + async getActiveSession(): Promise<string | null> { + try { + const entries = await readdir(this.opts.sessionDir, { withFileTypes: true }); + let latest: { id: string; mtimeMs: number } | null = null; + for (const entry of entries) { + if (!entry.isFile() || !entry.name.startsWith('sess_')) continue; + const fullPath = join(this.opts.sessionDir, entry.name); + try { + const [data, stats] = await Promise.all([ + readFile(fullPath, 'utf8'), + (await import('fs/promises')).stat(fullPath), + ]); + if (this.isSessionValid(data)) { + const id = entry.name.slice('sess_'.length); + if (!latest || stats.mtimeMs > latest.mtimeMs) { + latest = { id, mtimeMs: stats.mtimeMs }; + } + } + } catch { + continue; // skip unreadable/corrupt files + } + } + return latest?.id ?? null; + } catch (e) { + this.logger.warn(e, 'Error reading session directory'); + return null; + } + }And update imports:
-import { readdir, readFile } from 'fs/promises'; +import { readdir, readFile, stat } from 'fs/promises';api/src/unraid-api/auth/local-session.service.ts (1)
83-89: Cleanup on shutdown to remove the session file.Implement OnModuleDestroy to unlink the file; avoids stale tokens during downtime/redeploy.
-import { Injectable, Logger } from '@nestjs/common'; +import { Injectable, Logger, OnModuleDestroy } from '@nestjs/common'; @@ -export class LocalSessionService { +export class LocalSessionService implements OnModuleDestroy { @@ public async deleteLocalSession(): Promise<void> { try { await unlink(LocalSessionService.getSessionFilePath()); } catch (error) { this.logger.error(error, 'Error deleting local session file'); } } + + async onModuleDestroy(): Promise<void> { + await this.deleteLocalSession(); + }api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
188-190: Fix: hardcoded cookie name in WS connectionParamsThe WebSocket branch still hardcodes unraid_ and will break if namePrefix is overridden. Mirror the HTTP logic.
return { 'x-csrf-token': cookieAuth.csrfToken, - cookie: `unraid_${cookieAuth.sessionId}=${cookieAuth.sessionId}`, + cookie: `${this.sessionCookieConfig.namePrefix}${cookieAuth.sessionId}=${cookieAuth.sessionId}`, };
🧹 Nitpick comments (9)
api/.env.test (1)
17-17: Reorder key to satisfy dotenv-linter (alphabetical).Place PATHS_LOCAL_SESSION_FILE before PATHS_LOGS_FILE.
- PATHS_LOGS_FILE=./dev/log/graphql-api.log - PATHS_LOCAL_SESSION_FILE=./dev/local-session + PATHS_LOCAL_SESSION_FILE=./dev/local-session + PATHS_LOGS_FILE=./dev/log/graphql-api.logapi/.env.development (1)
21-21: Reorder key to satisfy dotenv-linter (alphabetical).Put PATHS_LOCAL_SESSION_FILE before PATHS_LOGS_FILE for consistent ordering.
PATHS_LOG_BASE=./dev/log # Where we store logs -PATHS_LOGS_FILE=./dev/log/graphql-api.log +PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOGS_FILE=./dev/log/graphql-api.log -PATHS_LOCAL_SESSION_FILE=./dev/local-sessionapi/src/unraid-api/auth/cookie.service.ts (1)
63-76: Use utf8 when reading session files.Avoid 'ascii' to prevent accidental data loss; utf8 is standard.
- const sessionData = await readFile(sessionFile, 'ascii'); + const sessionData = await readFile(sessionFile, 'utf8');api/src/unraid-api/auth/local-session.service.ts (1)
51-58: Trim on read and prefer in-memory token to avoid per-request disk I/O.Avoid failures from trailing newlines and reduce hot-path I/O by caching.
- public async getLocalSession(): Promise<string | null> { - try { - return await readFile(LocalSessionService.getSessionFilePath(), 'utf-8'); - } catch (error) { - this.logger.warn(error, 'Local session file not found or not readable'); - return null; - } - } + public async getLocalSession(): Promise<string | null> { + if (this.sessionToken) return this.sessionToken; + try { + const token = await readFile(LocalSessionService.getSessionFilePath(), 'utf-8'); + this.sessionToken = token.trim(); + return this.sessionToken; + } catch (error) { + this.logger.warn(error, 'Local session file not found or not readable'); + return null; + } + } @@ - public async validateLocalSession(token: string): Promise<boolean> { + public async validateLocalSession(token: string): Promise<boolean> { // Coerce inputs to strings (or empty string if undefined) - const tokenStr = token || ''; - const currentToken = await this.getLocalSession(); - const currentTokenStr = currentToken || ''; + const tokenStr = (token || '').trim(); + const currentToken = await this.getLocalSession(); + const currentTokenStr = (currentToken || '').trim(); @@ - const tokenBuffer = Buffer.from(tokenStr, 'utf-8'); - const currentTokenBuffer = Buffer.from(currentTokenStr, 'utf-8'); + const tokenBuffer = Buffer.from(tokenStr, 'utf-8'); + const currentTokenBuffer = Buffer.from(currentTokenStr, 'utf-8');Also applies to: 63-81
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
18-21: Harden lifecycle: explicit return type + cleanup on shutdown (optional).Consider making the lifecycle explicit and cleaning up the session file during shutdown. This clarifies startup failure semantics and avoids leaving stale tokens.
- Is it intended for API startup to fail if session generation cannot write the file? If yes, keep rethrow; if no, add retry/backoff and degrade gracefully.
Apply:
-import { Injectable, OnModuleInit } from '@nestjs/common'; +import { Injectable, OnModuleInit, OnModuleDestroy } from '@nestjs/common'; -@Injectable() -export class LocalSessionLifecycleService implements OnModuleInit { +@Injectable() +export class LocalSessionLifecycleService implements OnModuleInit, OnModuleDestroy { constructor(private readonly localSessionService: LocalSessionService) {} - async onModuleInit() { + async onModuleInit(): Promise<void> { await this.localSessionService.generateLocalSession(); } + + async onModuleDestroy(): Promise<void> { + // Best-effort cleanup; ignore failures during shutdown + try { + await this.localSessionService.deleteLocalSession(); + } catch { + /* noop */ + } + } }Also applies to: 1-1, 15-16
api/src/unraid-api/shared/internal-graphql-client.factory.ts (3)
55-58: Guard against ambiguous auth configurationRight now, the factory only checks that at least one getter is provided. Consider warning or throwing when multiple getters are passed to avoid surprising precedence (API key > local-session > cookie). At minimum, document the precedence.
- if (!options.getApiKey && !options.getCookieAuth && !options.getLocalSession) { + const provided = [options.getApiKey, options.getCookieAuth, options.getLocalSession].filter(Boolean); + if (provided.length === 0) { throw new Error( 'One of getApiKey, getCookieAuth, or getLocalSession function is required for creating a GraphQL client' ); + } + if (provided.length > 1) { + // Consider: throw instead of warn if you want to enforce exclusivity + this.logger.warn('Multiple auth providers supplied; using precedence: apiKey > localSession > cookie'); + }
116-152: DRY the auth selection to prevent HTTP/WS driftThe selection and header building are duplicated for HTTP and WS, which caused the cookie prefix inconsistency. Centralize into a small helper to keep them in sync.
// outside createClient() private async buildAuthHeaders(getters: { getApiKey?: () => Promise<string>; getLocalSession?: () => Promise<string | null>; getCookieAuth?: () => Promise<{ sessionId: string; csrfToken: string } | null>; }): Promise<Record<string, string>> { const { getApiKey, getLocalSession, getCookieAuth } = getters; if (getApiKey) { const apiKey = await getApiKey(); if (!apiKey) throw new Error('No valid API key returned from getApiKey()'); return { 'x-api-key': apiKey }; } else if (getLocalSession) { const localSession = await getLocalSession(); if (!localSession) throw new Error('No valid local session found'); return { 'x-local-session': localSession }; } else if (getCookieAuth) { const cookieAuth = await getCookieAuth(); if (!cookieAuth) throw new Error('No valid session found for cookie authentication'); const cookie = `${this.sessionCookieConfig.namePrefix}${cookieAuth.sessionId}=${cookieAuth.sessionId}`; return { 'x-csrf-token': cookieAuth.csrfToken, cookie }; } return {}; }Then:
// HTTP - const authLink = setContext(async (_, { headers }) => { - if (getApiKey) { ... } else if (getLocalSession) { ... } else if (getCookieAuth) { ... } - return { headers }; - }); + const authLink = setContext(async (_, { headers }) => ({ + headers: { ...headers, ...(await this.buildAuthHeaders({ getApiKey, getLocalSession, getCookieAuth })) }, + })); // WS - connectionParams: async () => { - if (getApiKey) { ... } else if (getLocalSession) { ... } else if (getCookieAuth) { ... } - return {}; - }, + connectionParams: async () => await this.buildAuthHeaders({ getApiKey, getLocalSession, getCookieAuth }),Also applies to: 169-192
157-161: Avoid logging sensitive detailsonError logs the whole networkError object. Ensure it never includes headers/connection params (e.g., x-api-key, x-local-session, cookie).
- this.logger.warn('[GRAPHQL-CLIENT] NETWORK ERROR ENCOUNTERED %o', networkError); + const { name, message, statusCode } = (networkError as any) ?? {}; + this.logger.warn('[GRAPHQL-CLIENT] network error: %s (%s) %s', name ?? 'Error', String(statusCode ?? ''), message ?? '');api/src/unraid-api/plugin/global-deps.module.ts (1)
1-98: Optional: export SESSION_COOKIE_CONFIG if other modules must override/read itIf other modules need to override or read SESSION_COOKIE_CONFIG (e.g., environment-specific presets), consider exporting it. If not needed, ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (43)
api/.env.development(1 hunks)api/.env.test(1 hunks)api/.gitignore(1 hunks)api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json(0 hunks)api/dev/keys/fc91da7b-0284-46f4-9018-55aa9759fba9.json(0 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/auth/auth.module.ts(3 hunks)api/src/unraid-api/auth/auth.service.spec.ts(3 hunks)api/src/unraid-api/auth/auth.service.ts(7 hunks)api/src/unraid-api/auth/authentication.guard.ts(2 hunks)api/src/unraid-api/auth/cookie.service.ts(4 hunks)api/src/unraid-api/auth/local-session-lifecycle.service.ts(1 hunks)api/src/unraid-api/auth/local-session.service.ts(1 hunks)api/src/unraid-api/auth/local-session.strategy.ts(1 hunks)api/src/unraid-api/cli/__test__/api-report.service.test.ts(2 hunks)api/src/unraid-api/cli/__test__/developer-tools.service.test.ts(4 hunks)api/src/unraid-api/cli/admin-key.service.ts(0 hunks)api/src/unraid-api/cli/api-report.service.ts(4 hunks)api/src/unraid-api/cli/cli-services.module.ts(1 hunks)api/src/unraid-api/cli/cli.module.spec.ts(3 hunks)api/src/unraid-api/cli/cli.module.ts(0 hunks)api/src/unraid-api/cli/developer/developer-tools.service.ts(2 hunks)api/src/unraid-api/cli/internal-client.service.spec.ts(0 hunks)api/src/unraid-api/cli/internal-client.service.ts(0 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(3 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts(1 hunks)api/src/unraid-api/plugin/global-deps.module.ts(5 hunks)api/src/unraid-api/rest/__test__/rest-module.integration.test.ts(2 hunks)api/src/unraid-api/shared/internal-client.service.ts(1 hunks)api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts(2 hunks)api/src/unraid-api/shared/internal-graphql-client.factory.ts(5 hunks)packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts(0 hunks)packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.spec.ts(0 hunks)packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.ts(0 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts(2 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts(1 hunks)packages/unraid-shared/src/index.ts(1 hunks)packages/unraid-shared/src/tokens.ts(1 hunks)packages/unraid-shared/src/types/canonical-internal-client.interface.ts(1 hunks)packages/unraid-shared/src/types/internal-graphql-client.factory.ts(1 hunks)
💤 Files with no reviewable changes (9)
- api/src/unraid-api/cli/internal-client.service.ts
- api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
- api/dev/keys/fc91da7b-0284-46f4-9018-55aa9759fba9.json
- api/src/unraid-api/cli/internal-client.service.spec.ts
- api/src/unraid-api/cli/admin-key.service.ts
- packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.ts
- packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts
- api/src/unraid-api/cli/cli.module.ts
- packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.spec.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- api/.gitignore
- api/src/environment.ts
- packages/unraid-shared/src/types/canonical-internal-client.interface.ts
- api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts
- api/src/unraid-api/auth/local-session.strategy.ts
- api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts
- api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts
- api/src/unraid-api/auth/auth.service.ts
- packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts
- api/src/unraid-api/cli/test/developer-tools.service.test.ts
- packages/unraid-shared/src/types/internal-graphql-client.factory.ts
- packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
- api/src/unraid-api/cli/sso/validate-token.command.ts
- packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts
- api/src/unraid-api/cli/cli.module.spec.ts
- api/src/unraid-api/cli/cli-services.module.ts
- api/src/unraid-api/auth/auth.module.ts
- packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts
- api/src/unraid-api/cli/test/api-report.service.test.ts
- api/src/unraid-api/auth/auth.service.spec.ts
- api/src/unraid-api/cli/api-report.service.ts
- api/src/unraid-api/shared/internal-client.service.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
packages/unraid-shared/src/tokens.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/auth/authentication.guard.tspackages/unraid-shared/src/index.tsapi/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/auth/local-session.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
packages/unraid-shared/src/tokens.tsapi/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/auth/authentication.guard.tspackages/unraid-shared/src/index.tsapi/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/auth/local-session.service.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/auth/authentication.guard.tsapi/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/cookie.service.tsapi/src/unraid-api/plugin/global-deps.module.tsapi/src/unraid-api/auth/local-session.service.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts
🧠 Learnings (4)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.tsapi/src/unraid-api/auth/cookie.service.ts
📚 Learning: 2025-03-10T18:43:13.016Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/types/fastify.ts:25-30
Timestamp: 2025-03-10T18:43:13.016Z
Learning: In the Unraid API, cookies should be marked as optional (with `?`) in the FastifyRequest type definition since the cookies may not always be present, which is consistent with the authentication flow throughout the codebase.
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.ts
🧬 Code graph analysis (6)
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts (1)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)
api/src/unraid-api/shared/internal-graphql-client.factory.ts (3)
api/src/unraid-api/auth/cookie.service.ts (2)
SESSION_COOKIE_CONFIG(10-10)SessionCookieConfig(12-18)packages/unraid-api-plugin-connect/src/mothership-proxy/connection.service.ts (1)
getApiKey(162-164)api/src/unraid-api/auth/local-session.service.ts (1)
getLocalSession(51-58)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(12-97)
api/src/unraid-api/cli/developer/developer-tools.service.ts (2)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/plugin/global-deps.module.ts (2)
packages/unraid-shared/src/tokens.ts (4)
INTERNAL_CLIENT_FACTORY_TOKEN(5-5)COOKIE_SERVICE_TOKEN(6-6)NGINX_SERVICE_TOKEN(4-4)CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)api/src/unraid-api/auth/cookie.service.ts (1)
SESSION_COOKIE_CONFIG(10-10)
api/src/unraid-api/auth/local-session.service.ts (6)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
Injectable(14-21)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(28-235)api/src/unraid-api/shared/internal-client.service.ts (1)
Injectable(15-94)api/src/unraid-api/auth/auth.service.ts (1)
Injectable(22-380)api/src/unraid-api/auth/local-session.strategy.ts (1)
Injectable(14-46)api/src/environment.ts (1)
PATHS_LOCAL_SESSION_FILE(112-113)
🪛 dotenv-linter (3.3.0)
api/.env.test
[warning] 17-17: [UnorderedKey] The PATHS_LOCAL_SESSION_FILE key should go before the PATHS_LOGS_FILE key
(UnorderedKey)
api/.env.development
[warning] 21-21: [UnorderedKey] The PATHS_LOCAL_SESSION_FILE key should go before the PATHS_LOGS_FILE key
(UnorderedKey)
🔇 Additional comments (17)
api/src/unraid-api/auth/authentication.guard.ts (2)
16-16: Adding LocalSessionStrategy import is correct.Import path uses .js extension per ESM guideline. LGTM.
41-41: LocalSessionStrategy wiring and key confirmed
- Verified
LocalSessionStrategydefinesstatic readonly key = 'local-session'inapi/src/unraid-api/auth/local-session.strategy.ts:16.- Confirmed
AuthModuleregisters and exportsLocalSessionStrategy:
- providers list includes
LocalSessionStrategy(auth.module.ts:61)- exports list includes
LocalSessionStrategy(auth.module.ts:76)- Confirmed
PassportModule.registerdefault strategies includeLocalSessionStrategy.keyalongside the other strategies (auth.module.ts:23–26).No further action needed. Everything is correctly wired.
api/src/unraid-api/auth/cookie.service.ts (1)
12-18: Exporting SessionCookieConfig and sameSite: 'lax' aligns with prior guidance.Matches the recorded preference to use SameSite=Lax for session cookies.
api/src/unraid-api/auth/local-session.service.ts (1)
21-45: Secure token generation + 0600 permissions look good.mkdir with recursive and explicit chmod are appropriate.
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
3-3: ESM path correctness.Local import uses .js extension per repo ESM rule. LGTM.
api/src/unraid-api/rest/__test__/rest-module.integration.test.ts (2)
66-67: Good switch to DI token override.Overriding CANONICAL_INTERNAL_CLIENT_TOKEN keeps the test decoupled from concrete implementations.
3-3: Verify CANONICAL_INTERNAL_CLIENT_TOKEN is actually available to RestModuleIt looks like
CANONICAL_INTERNAL_CLIENT_TOKENis only provided inplugin/global-deps.module.ts(viaprovide: CANONICAL_INTERNAL_CLIENT_TOKEN, useExisting: InternalClientService) and your integration test forRestModuledoesn’t import that module. Without it (or a@Global()decorator onGlobalDepsModule), callingoverrideProvider(CANONICAL_INTERNAL_CLIENT_TOKEN)in your test won’t bind the token.Please check and adjust as needed:
api/src/unraid-api/plugin/global-deps.module.ts
• Confirm it’s decorated with@Global()so consumers don’t need to import it manually.
• If not global, export and import this module where needed.api/src/unraid-api/rest/rest.module.ts
• If you rely on a non-global module, addGlobalDepsModuleto theimportsarray.api/src/unraid-api/rest/test/rest-module.integration.test.ts
• InTest.createTestingModule({ imports: […] }), includeGlobalDepsModule(or the module that provides the token) alongsideRestModule.
• Example:const moduleRef = await Test.createTestingModule({ imports: [RestModule, GlobalDepsModule], // … }).compile();Ensuring the provider is actually loaded in RestModule’s injector graph will make your
overrideProvider(CANONICAL_INTERNAL_CLIENT_TOKEN)calls effective.packages/unraid-shared/src/index.ts (1)
8-8: Type re-export is ESM-compliant.Type-only export with .js specifier matches our ESM rule. LGTM.
packages/unraid-shared/src/tokens.ts (1)
5-7: Token rename verified and safe
- No occurrences of the old
INTERNAL_CLIENT_SERVICE_TOKENremain in the codebase.- All consumers now import and use
INTERNAL_CLIENT_FACTORY_TOKENconsistently (e.g., inglobal-deps.module.ts, service injections, and tests).- The
CANONICAL_INTERNAL_CLIENT_TOKENbinding and injections continue to function as intended.No further changes are necessary.
api/src/unraid-api/cli/developer/developer-tools.service.ts (2)
57-59: Nice DI switch to canonical client + explicit options.Injecting via CANONICAL_INTERNAL_CLIENT_TOKEN and disabling subscriptions for mutations is appropriate.
Also applies to: 63-63
1-1: AllCliInternalClientServicereferences have been removed from the CLI code
The only remaining occurrence is a commented note in
• packages/unraid-shared/src/services/internal-graphql-client-usage.spec.ts (line 58)
which does not import or execute the old service. All CLI classes now consistently inject the new client via
• @Inject(CANONICAL_INTERNAL_CLIENT_TOKEN) in
– api/src/unraid-api/cli/api-report.service.ts
– api/src/unraid-api/cli/sso/validate-token.command.ts
– api/src/unraid-api/cli/developer/developer-tools.service.tsNo further changes are required.
api/src/unraid-api/shared/internal-graphql-client.factory.ts (2)
33-36: Good: DI for session cookie config is in placeInjecting SESSION_COOKIE_CONFIG and using the typed SessionCookieConfig aligns with the new cookie-based auth flow.
146-151: Cookie header construction respects configured namePrefix — niceUsing this.sessionCookieConfig.namePrefix avoids hardcoding and matches the cookie service configuration.
api/src/unraid-api/plugin/global-deps.module.ts (4)
35-37: Provider wiring for INTERNAL_CLIENT_FACTORY_TOKEN looks correctBinding InternalGraphQLClientFactory to INTERNAL_CLIENT_FACTORY_TOKEN aligns with the refactor away from the old service token.
51-57: SESSION_COOKIE_CONFIG and CookieService registration are soundProviding CookieService.defaultOpts() and wiring COOKIE_SERVICE_TOKEN via useClass matches the new cookie-based auth flow. Per prior learnings, defaultOpts should set sameSite: 'lax'.
62-68: Canonical internal client mapping via useExisting is appropriateExporting CANONICAL_INTERNAL_CLIENT_TOKEN as useExisting: InternalClientService keeps a single implementation.
76-85: Token migration complete – no residual references detected
- No occurrences of INTERNAL_CLIENT_SERVICE_TOKEN remain in
api/orpackages/, confirming the old token has been fully removed.- INTERNAL_CLIENT_FACTORY_TOKEN is declared once in
packages/unraid-shared/src/tokens.tsand then injected or provided consistently across:
•api/src/unraid-api/plugin/global-deps.module.ts(providers & exports)
•api/src/unraid-api/shared/internal-client.service.ts(constructor injection)
•api/src/unraid-api/cli/cli.module.spec.ts(unit tests)- CANONICAL_INTERNAL_CLIENT_TOKEN is provided via
useExisting: InternalClientServicein
api/src/unraid-api/plugin/global-deps.module.tsand consumed by CLI services and plugin handlers without duplication.All token migrations have been verified; no further action required.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/cli/developer/developer-tools.service.ts (1)
65-76: Surface GraphQL errors from mutate() to avoid silent failures.Apollo doesn’t throw on GraphQL errors by default; check
result.errors.const result = await client.mutate({ mutation: UPDATE_SANDBOX_MUTATION, variables: { input: { api: { sandbox: enable, }, }, }, }); + if (result.errors?.length) { + throw new Error(result.errors.map(e => e.message).join('; ')); + } + if (result.data?.updateSettings.restartRequired) {
♻️ Duplicate comments (3)
packages/unraid-shared/src/tokens.ts (1)
6-7: Ensure tokens are re-exported and wired in DI (follow-up to earlier review)Re-export COOKIE_SERVICE_TOKEN and CANONICAL_INTERNAL_CLIENT_TOKEN from shared index and map them in the API DI module. This was flagged earlier and is still applicable.
Add to packages/unraid-shared/src/index.ts:
export { COOKIE_SERVICE_TOKEN, CANONICAL_INTERNAL_CLIENT_TOKEN, INTERNAL_CLIENT_FACTORY_TOKEN, INTERNAL_CLIENT_SERVICE_TOKEN } from './tokens.js';Confirm provider wiring in the API (example):
import { COOKIE_SERVICE_TOKEN, CANONICAL_INTERNAL_CLIENT_TOKEN } from '@unraid/unraid-shared'; import { CookieService } from '@app/unraid-api/auth/cookie.service.js'; import { CanonicalInternalClientService } from '@app/unraid-api/internal/canonical-internal-client.service.js'; export const GLOBAL_PROVIDERS = [ { provide: COOKIE_SERVICE_TOKEN, useClass: CookieService }, { provide: CANONICAL_INTERNAL_CLIENT_TOKEN, useClass: CanonicalInternalClientService }, ];api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
187-191: WebSocket cookie name hardcoded to 'unraid_' — use configured prefix to avoid auth breakage.HTTP path uses
sessionCookieConfig.namePrefix; WS path still hardcodesunraid_. Align both.- return { - 'x-csrf-token': cookieAuth.csrfToken, - cookie: `unraid_${cookieAuth.sessionId}=${cookieAuth.sessionId}`, - }; + const cookieName = + `${this.sessionCookieConfig.namePrefix}${cookieAuth.sessionId}`; + return { + 'x-csrf-token': cookieAuth.csrfToken, + cookie: `${cookieName}=${cookieAuth.sessionId}`, + };api/src/unraid-api/shared/internal-client.service.ts (1)
41-69: Fix “first call wins” caching: make the client options-aware and upgradeable.Current cache ignores
enableSubscriptionsandorigin. Track active config, reuse when it satisfies the request, or recreate when a later call needs subscriptions or a different origin.@@ export class InternalClientService implements CanonicalInternalClientService { private readonly logger = new Logger(InternalClientService.name); private client: ApolloClient<NormalizedCacheObject> | null = null; private clientCreationPromise: Promise<ApolloClient<NormalizedCacheObject>> | null = null; + // Track the config used to create the cached client + private clientConfig: { enableSubscriptions: boolean; origin?: string } | null = null; @@ - // If client already exists, return it - if (this.client) { - return this.client; - } + // Normalize desired configuration + const desiredEnableSubscriptions = options?.enableSubscriptions ?? true; + const desiredOrigin = options?.origin; + // If an existing client satisfies the request, reuse it; otherwise upgrade + if (this.client && this.clientConfig) { + const { enableSubscriptions: hasSubs, origin: currentOrigin } = this.clientConfig; + const needsUpgrade = + (desiredEnableSubscriptions && !hasSubs) || + (desiredOrigin && desiredOrigin !== currentOrigin); + if (!needsUpgrade) { + return this.client; + } + this.clearClient(); + } @@ - // Start client creation and store the promise - const creationPromise = this.createClient(options); + // Start client creation and store the promise + const creationPromise = this.createClient({ + enableSubscriptions: desiredEnableSubscriptions, + origin: desiredOrigin, + }); this.clientCreationPromise = creationPromise; @@ - this.logger.debug('Created canonical internal GraphQL client with local session authentication'); + // Track active client configuration for future reuse/upgrade decisions + this.clientConfig = { enableSubscriptions, origin }; + this.logger.debug( + `Created canonical internal GraphQL client with local session authentication (subscriptions=${enableSubscriptions}, origin=${origin ?? 'http://localhost'})` + ); return client; @@ public clearClient() { // Stop the Apollo client to terminate any active processes this.client?.stop(); this.client = null; this.clientCreationPromise = null; + this.clientConfig = null; }Also applies to: 71-86, 88-93, 17-20
🧹 Nitpick comments (7)
api/.env.test (1)
17-17: Reorder PATHS_LOCAL_SESSION_FILE to satisfy dotenv-linter and keep alphabetical groupingPlace PATHS_LOCAL_SESSION_FILE before PATHS_LOGS_FILE.
-PATHS_LOGS_FILE=./dev/log/graphql-api.log -PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOGS_FILE=./dev/log/graphql-api.logapi/.env.development (1)
21-21: Keep env keys ordered; move PATHS_LOCAL_SESSION_FILE above PATHS_LOGS_FILEAligns with dotenv-linter’s UnorderedKey rule.
-PATHS_LOGS_FILE=./dev/log/graphql-api.log -PATHS_CONNECT_STATUS_FILE_PATH=./dev/connectStatus.json # Connect plugin status file -PATHS_OIDC_JSON=./dev/configs/oidc.local.json -PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOCAL_SESSION_FILE=./dev/local-session +PATHS_LOGS_FILE=./dev/log/graphql-api.log +PATHS_CONNECT_STATUS_FILE_PATH=./dev/connectStatus.json # Connect plugin status file +PATHS_OIDC_JSON=./dev/configs/oidc.local.jsonapi/src/unraid-api/auth/auth.service.spec.ts (1)
60-64: Tighten LocalSessionService mock typing across all AuthService testsTo avoid using
as any, cast your façades toPartial<LocalSessionService>before asserting the full service type. Apply the same change in each test suite:•
api/src/unraid-api/auth/auth.service.spec.ts(around line 60)
•api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts(around line 37)
•api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts(around line 41)- const localSessionService = { - validateLocalSession: vi.fn(), - } as any; + const localSessionService = ({ + validateLocalSession: vi.fn(), + } as Partial<LocalSessionService>) as LocalSessionService;packages/unraid-shared/src/tokens.ts (1)
5-5: Token rename is breaking; provide a temporary alias to ease migrationConsider exporting a deprecation alias for one release cycle.
export const INTERNAL_CLIENT_FACTORY_TOKEN = 'InternalClientService'; +// DEPRECATED: remove after <next major>; maintained for backward compat +export const INTERNAL_CLIENT_SERVICE_TOKEN = INTERNAL_CLIENT_FACTORY_TOKEN;api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
18-20: Consider cleaning up the local session on shutdown (confirm lifecycle intent).If the local session should be scoped to the API’s lifetime, delete it on shutdown. If persistence across restarts is desired, ignore.
-import { Injectable, OnModuleInit } from '@nestjs/common'; +import { Injectable, OnModuleInit, OnApplicationShutdown } from '@nestjs/common'; @@ -@Injectable() -export class LocalSessionLifecycleService implements OnModuleInit { +@Injectable() +export class LocalSessionLifecycleService implements OnModuleInit, OnApplicationShutdown { @@ async onModuleInit() { await this.localSessionService.generateLocalSession(); } + + async onApplicationShutdown() { + await this.localSessionService.deleteLocalSession(); + } }api/src/unraid-api/shared/internal-graphql-client.factory.ts (2)
157-161: Also log GraphQL errors, not just network errors.Improves debuggability without changing behavior.
- const errorLink = onError(({ networkError }) => { - if (networkError) { - this.logger.warn('[GRAPHQL-CLIENT] NETWORK ERROR ENCOUNTERED %o', networkError); - } - }); + const errorLink = onError(({ graphQLErrors, networkError }) => { + if (graphQLErrors?.length) { + for (const err of graphQLErrors) { + this.logger.warn('[GRAPHQL-CLIENT] GRAPHQL ERROR %o', { + message: err.message, + path: err.path, + extensions: err.extensions, + }); + } + } + if (networkError) { + this.logger.warn('[GRAPHQL-CLIENT] NETWORK ERROR %o', networkError); + } + });
55-58: Tighten error message wording.Minor clarity/grammar tweak.
- throw new Error( - 'One of getApiKey, getCookieAuth, or getLocalSession function is required for creating a GraphQL client' - ); + throw new Error( + 'At least one of getApiKey, getCookieAuth, or getLocalSession is required to create a GraphQL client' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (43)
api/.env.development(1 hunks)api/.env.test(1 hunks)api/.gitignore(1 hunks)api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json(0 hunks)api/dev/keys/fc91da7b-0284-46f4-9018-55aa9759fba9.json(0 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/auth/auth.module.ts(3 hunks)api/src/unraid-api/auth/auth.service.spec.ts(3 hunks)api/src/unraid-api/auth/auth.service.ts(7 hunks)api/src/unraid-api/auth/authentication.guard.ts(2 hunks)api/src/unraid-api/auth/cookie.service.ts(4 hunks)api/src/unraid-api/auth/local-session-lifecycle.service.ts(1 hunks)api/src/unraid-api/auth/local-session.service.ts(1 hunks)api/src/unraid-api/auth/local-session.strategy.ts(1 hunks)api/src/unraid-api/cli/__test__/api-report.service.test.ts(2 hunks)api/src/unraid-api/cli/__test__/developer-tools.service.test.ts(4 hunks)api/src/unraid-api/cli/admin-key.service.ts(0 hunks)api/src/unraid-api/cli/api-report.service.ts(4 hunks)api/src/unraid-api/cli/cli-services.module.ts(1 hunks)api/src/unraid-api/cli/cli.module.spec.ts(3 hunks)api/src/unraid-api/cli/cli.module.ts(0 hunks)api/src/unraid-api/cli/developer/developer-tools.service.ts(2 hunks)api/src/unraid-api/cli/internal-client.service.spec.ts(0 hunks)api/src/unraid-api/cli/internal-client.service.ts(0 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(3 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts(1 hunks)api/src/unraid-api/plugin/global-deps.module.ts(5 hunks)api/src/unraid-api/rest/__test__/rest-module.integration.test.ts(2 hunks)api/src/unraid-api/shared/internal-client.service.ts(1 hunks)api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts(2 hunks)api/src/unraid-api/shared/internal-graphql-client.factory.ts(5 hunks)packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts(0 hunks)packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.spec.ts(0 hunks)packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.ts(0 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts(2 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts(1 hunks)packages/unraid-shared/src/index.ts(1 hunks)packages/unraid-shared/src/tokens.ts(1 hunks)packages/unraid-shared/src/types/canonical-internal-client.interface.ts(1 hunks)packages/unraid-shared/src/types/internal-graphql-client.factory.ts(1 hunks)
💤 Files with no reviewable changes (9)
- packages/unraid-api-plugin-connect/src/authn/connect-api-key.service.ts
- api/src/unraid-api/cli/internal-client.service.ts
- packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.ts
- api/src/unraid-api/cli/internal-client.service.spec.ts
- api/src/unraid-api/cli/cli.module.ts
- packages/unraid-api-plugin-connect/src/internal-rpc/internal.client.spec.ts
- api/src/unraid-api/cli/admin-key.service.ts
- api/dev/keys/fc91da7b-0284-46f4-9018-55aa9759fba9.json
- api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
🚧 Files skipped from review as they are similar to previous changes (25)
- api/src/unraid-api/cli/test/api-report.service.test.ts
- packages/unraid-shared/src/index.ts
- api/src/unraid-api/shared/internal-graphql-client.factory.spec.ts
- api/src/environment.ts
- packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
- api/src/unraid-api/cli/cli-services.module.ts
- api/src/unraid-api/auth/auth.service.ts
- packages/unraid-shared/src/types/internal-graphql-client.factory.ts
- api/src/unraid-api/rest/test/rest-module.integration.test.ts
- api/.gitignore
- api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts
- api/src/unraid-api/cli/test/developer-tools.service.test.ts
- api/src/unraid-api/auth/local-session.service.ts
- api/src/unraid-api/auth/local-session.strategy.ts
- packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts
- api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts
- api/src/unraid-api/auth/cookie.service.ts
- api/src/unraid-api/auth/auth.module.ts
- api/src/unraid-api/cli/cli.module.spec.ts
- api/src/unraid-api/cli/api-report.service.ts
- packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts
- api/src/unraid-api/cli/sso/validate-token.command.ts
- packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts
- packages/unraid-shared/src/types/canonical-internal-client.interface.ts
- api/src/unraid-api/plugin/global-deps.module.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/authentication.guard.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tspackages/unraid-shared/src/tokens.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/authentication.guard.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/shared/internal-graphql-client.factory.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/auth/auth.service.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/unraid-api/auth/auth.service.spec.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/auth/auth.service.spec.tsapi/src/unraid-api/cli/developer/developer-tools.service.tsapi/src/unraid-api/auth/authentication.guard.tsapi/src/unraid-api/shared/internal-client.service.tsapi/src/unraid-api/auth/local-session-lifecycle.service.tsapi/src/unraid-api/shared/internal-graphql-client.factory.tspackages/unraid-shared/src/tokens.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/auth/auth.service.spec.ts
🧠 Learnings (3)
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.ts
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.ts
📚 Learning: 2025-03-10T18:43:13.016Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/types/fastify.ts:25-30
Timestamp: 2025-03-10T18:43:13.016Z
Learning: In the Unraid API, cookies should be marked as optional (with `?`) in the FastifyRequest type definition since the cookies may not always be present, which is consistent with the authentication flow throughout the codebase.
Applied to files:
api/src/unraid-api/shared/internal-graphql-client.factory.ts
🧬 Code graph analysis (4)
api/src/unraid-api/cli/developer/developer-tools.service.ts (3)
packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)packages/unraid-shared/src/index.ts (1)
CanonicalInternalClientService(8-8)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)
api/src/unraid-api/shared/internal-client.service.ts (5)
api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(12-97)api/src/unraid-api/shared/internal-graphql-client.factory.ts (1)
Injectable(28-235)packages/unraid-shared/src/types/canonical-internal-client.interface.ts (1)
CanonicalInternalClientService(3-14)packages/unraid-shared/src/tokens.ts (1)
INTERNAL_CLIENT_FACTORY_TOKEN(5-5)packages/unraid-shared/src/types/internal-graphql-client.factory.ts (1)
InternalGraphQLClientFactory(7-15)
api/src/unraid-api/auth/local-session-lifecycle.service.ts (1)
api/src/unraid-api/auth/local-session.service.ts (1)
Injectable(12-97)
api/src/unraid-api/shared/internal-graphql-client.factory.ts (3)
api/src/unraid-api/auth/cookie.service.ts (2)
SESSION_COOKIE_CONFIG(10-10)SessionCookieConfig(12-18)packages/unraid-api-plugin-connect/src/mothership-proxy/connection.service.ts (1)
getApiKey(162-164)api/src/unraid-api/auth/local-session.service.ts (1)
getLocalSession(51-58)
🪛 dotenv-linter (3.3.0)
api/.env.development
[warning] 21-21: [UnorderedKey] The PATHS_LOCAL_SESSION_FILE key should go before the PATHS_LOGS_FILE key
(UnorderedKey)
api/.env.test
[warning] 17-17: [UnorderedKey] The PATHS_LOCAL_SESSION_FILE key should go before the PATHS_LOGS_FILE key
(UnorderedKey)
🔇 Additional comments (7)
api/.env.test (1)
17-17: Confirm dev token file is ignored from VCSEnsure dev/local-session is gitignored to avoid leaking local tokens (AI summary says it is, but please double-check).
api/src/unraid-api/auth/auth.service.spec.ts (2)
11-11: Import looks correct and ESM-friendlyUses .js extension per repo ESM guideline. No issues.
21-21: LocalSessionService test variable added—OKDeclaration aligns with the updated AuthService constructor.
api/src/unraid-api/auth/authentication.guard.ts (2)
16-16: Importing LocalSessionStrategy—goodImport path uses .js extension and matches the new strategy.
41-41: Strategy order: verify intended precedenceOrder is [ServerHeader, LocalSession, UserCookie]. Confirm this is the desired priority (internal header first, then local-session for CLI, finally user cookie). If local-session should supersede server header in some contexts, swap accordingly.
api/src/unraid-api/cli/developer/developer-tools.service.ts (1)
57-59: Switch to canonical internal client: LGTM.DI via
CANONICAL_INTERNAL_CLIENT_TOKENand type usage are correct.api/src/unraid-api/shared/internal-client.service.ts (1)
46-69: Concurrency guard and in-flight promise handling: solid.The promise pinning and equality checks avoid races and stale writes.
Remove the confusing API keys that were auto-generated for the CLI & Connect. Instead, support authentication via a custom
local-sessionheader.Summary by CodeRabbit
New Features
Security
Refactor
Tests / Chores