Repository navigation
fix(api): resolve Clean Architecture Dependency Rule violations - #81
Merged
Merged
Conversation
Audited apps/api against the Dependency Rule (inner layers must not depend on outer layers) and fixed the real violations found: - TOTP crypto bypassed DI entirely: three use cases (LoginWithTotpUseCase, GenerateTotpSecretUseCase, ConfirmTotpSetupUseCase) imported infrastructure/auth/totp.js and totpSecretCrypto.js directly, the one infra concern in the app with no port interface. Introduces ITotpProvider (generateSecret/getOtpauthUrl/verifyCode/encryptSecret/ decryptSecret) with a TotpProvider implementation, wired through DI like every other repository/provider in the app. Verified end-to-end against a live server: setup, confirm, login-with-TOTP, and disable all still work correctly. - apiToken, session, coverLetter, and jobDescription mutations/queries were the only ones in the schema calling use-cases and mappers directly instead of going through an interface-adapters resolver (every other domain — application, note, document, user, etc. — has one). Added ApiTokenResolver and SessionResolver, matching the existing pattern, and wired them into the DI container. Left coverLetter/jobDescription as direct schema-layer calls — each is a single trivial pass-through with no mapper and no business logic, so a resolver there would be pure ceremony. - apiTokenMutations.ts embedded a business rule in the GraphQL resolve() function (defaulting/coercing an invalid scope string to 'full' instead of validating it). Root-caused: the `scope` arg was a raw string instead of a GraphQL enum. Added ApiTokenScopeEnum so invalid values are now rejected by GraphQL argument validation instead of being silently coerced — a real (if narrow) correctness improvement, not just a refactor. - Fixed a backward import introduced in the pagination PR this session: interface-adapters/resolvers/ApplicationResolver.ts was importing a type from http/schema/. Moved ApplicationConnectionDTO to live next to ApplicationDTO in the mapper (interface-adapters), with http/ importing it from there instead. Confirmed clean and left alone: the domain layer (zero external imports anywhere), all port interfaces (no Prisma types leak through), mappers (pure field-by-field translation, no logic), McpController, and the DI container itself. bcrypt/crypto/qrcode/nanoid are used directly in several use cases without a port — a deliberate, consistent choice throughout the codebase for stateless utilities with no hidden config dependency, left as-is.
WalkthroughThe change centralises TOTP operations behind an injected provider, adds provider and resolver tests, registers new dependencies, and routes API token and session GraphQL operations through dedicated interface adapters. API token scope is now represented by a GraphQL enum. ChangesAuthentication and API integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL
participant Resolver
participant UseCase
participant Provider
Client->>GraphQL: Submit TOTP or API request
GraphQL->>Resolver: Delegate operation
Resolver->>UseCase: Execute application operation
UseCase->>Provider: Generate, verify, encrypt, or decrypt TOTP data
Provider-->>UseCase: Return TOTP result
UseCase-->>Resolver: Return application result
Resolver-->>GraphQL: Return mapped API response
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (2)
apps/api/src/use-cases/auth/LoginWithTotpUseCase.ts (1)
51-56: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid decrypting the TOTP secret for backup-code attempts.
decryptSecretruns unconditionally even whenisTotpFormatis false, so every backup-code login attempt pays for a secret decryption it never uses. Gate the decrypt behind the format check.⚡ Proposed fix
// TOTP codes are always 6 digits — anything else can only be a backup code. const isTotpFormat = /^\d{6}$/.test(input.code); - const secret = this.deps.totpProvider.decryptSecret(user.totpSecret); - const validTotp = isTotpFormat - ? await this.deps.totpProvider.verifyCode(secret, input.code) - : false; + const validTotp = isTotpFormat + ? await this.deps.totpProvider.verifyCode( + this.deps.totpProvider.decryptSecret(user.totpSecret), + input.code, + ) + : false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/use-cases/auth/LoginWithTotpUseCase.ts` around lines 51 - 56, Update the TOTP validation flow in LoginWithTotpUseCase so totpProvider.decryptSecret is only called when isTotpFormat is true. Preserve validTotp as false for backup-code attempts without decrypting the user's TOTP secret.apps/api/src/__tests__/application/auth/LoginWithTotpUseCase.test.ts (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
generateValidCodetest fixture across two files. Both test files define an identicalfixtureCrypto/fixtureBase32/generateValidCode(secret)helper (same code, same comment) purely to build valid TOTP codes for fixtures.
apps/api/src/__tests__/application/auth/LoginWithTotpUseCase.test.ts#L22-L29: remove the local fixture and import a sharedgenerateValidCodehelper fromapps/api/src/__tests__/helpers/mocks.tsinstead.apps/api/src/__tests__/application/user/ConfirmTotpSetupUseCase.test.ts#L11-L18: remove the local fixture and import the same shared helper.Move
fixtureCrypto,fixtureBase32, andgenerateValidCodeintomocks.ts(exported alongsidemakeTotpProvider) so both suites share one implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/__tests__/application/auth/LoginWithTotpUseCase.test.ts` around lines 22 - 29, Move fixtureCrypto, fixtureBase32, and the exported generateValidCode helper into apps/api/src/__tests__/helpers/mocks.ts alongside makeTotpProvider. Remove the duplicate local fixtures from apps/api/src/__tests__/application/auth/LoginWithTotpUseCase.test.ts (anchor, lines 22-29) and apps/api/src/__tests__/application/user/ConfirmTotpSetupUseCase.test.ts (sibling, lines 11-18), and import generateValidCode from mocks.ts in both suites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/http/schema/mutations/sessionMutations.ts`:
- Around line 13-14: Update the session revocation mutation around
sessionResolver.revokeSession to preserve the existing fromCodedError conversion
for use-case failures. Either retain the conversion in the mutation or implement
equivalent conversion within SessionResolver.revokeSession, ensuring failed
revocations continue exposing their established GraphQL error codes.
In `@apps/api/src/infrastructure/auth/TotpProvider.ts`:
- Line 13: Cache the scrypt-derived key in the singleton TotpProvider instance
instead of recomputing it in getKey() for every encryptSecret, decryptSecret, or
secret-generation call. Add instance-level lazy initialization so scryptSync
runs only once, then reuse the cached key for all subsequent operations while
preserving the existing key derivation inputs and behavior.
---
Nitpick comments:
In `@apps/api/src/__tests__/application/auth/LoginWithTotpUseCase.test.ts`:
- Around line 22-29: Move fixtureCrypto, fixtureBase32, and the exported
generateValidCode helper into apps/api/src/__tests__/helpers/mocks.ts alongside
makeTotpProvider. Remove the duplicate local fixtures from
apps/api/src/__tests__/application/auth/LoginWithTotpUseCase.test.ts (anchor,
lines 22-29) and
apps/api/src/__tests__/application/user/ConfirmTotpSetupUseCase.test.ts
(sibling, lines 11-18), and import generateValidCode from mocks.ts in both
suites.
In `@apps/api/src/use-cases/auth/LoginWithTotpUseCase.ts`:
- Around line 51-56: Update the TOTP validation flow in LoginWithTotpUseCase so
totpProvider.decryptSecret is only called when isTotpFormat is true. Preserve
validTotp as false for backup-code attempts without decrypting the user's TOTP
secret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 472c8fae-3ae6-4178-b511-ee58a2cbfe0a
📒 Files selected for processing (27)
apps/api/src/__tests__/application/auth/LoginWithTotpUseCase.test.tsapps/api/src/__tests__/application/user/ConfirmTotpSetupUseCase.test.tsapps/api/src/__tests__/application/user/GenerateTotpSecretUseCase.test.tsapps/api/src/__tests__/helpers/mocks.tsapps/api/src/__tests__/infrastructure/auth/TotpProvider.test.tsapps/api/src/__tests__/infrastructure/auth/totpSecretCrypto.test.tsapps/api/src/__tests__/interface-adapters/resolvers/ApiTokenResolver.test.tsapps/api/src/__tests__/interface-adapters/resolvers/SessionResolver.test.tsapps/api/src/http/container.tsapps/api/src/http/schema/index.tsapps/api/src/http/schema/mutations/apiTokenMutations.tsapps/api/src/http/schema/mutations/sessionMutations.tsapps/api/src/http/schema/queries/apiTokenQueries.tsapps/api/src/http/schema/queries/sessionQueries.tsapps/api/src/http/schema/types/ApplicationConnectionType.tsapps/api/src/http/schema/types/enums/ApiTokenScopeEnum.tsapps/api/src/infrastructure/auth/TotpProvider.tsapps/api/src/infrastructure/auth/totp.tsapps/api/src/infrastructure/auth/totpSecretCrypto.tsapps/api/src/interface-adapters/mappers/ApplicationMapper.tsapps/api/src/interface-adapters/resolvers/ApiTokenResolver.tsapps/api/src/interface-adapters/resolvers/ApplicationResolver.tsapps/api/src/interface-adapters/resolvers/SessionResolver.tsapps/api/src/use-cases/auth/LoginWithTotpUseCase.tsapps/api/src/use-cases/ports/ITotpProvider.tsapps/api/src/use-cases/user/ConfirmTotpSetupUseCase.tsapps/api/src/use-cases/user/GenerateTotpSecretUseCase.ts
💤 Files with no reviewable changes (3)
- apps/api/src/infrastructure/auth/totp.ts
- apps/api/src/tests/infrastructure/auth/totpSecretCrypto.test.ts
- apps/api/src/infrastructure/auth/totpSecretCrypto.ts
Comment on lines
+13
to
+14
| const { sessionResolver } = ctx.diScope.cradle; | ||
| return sessionResolver.revokeSession(ctx.user.sub, String(args.id)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve coded-error conversion for session revocation.
The previous mutation converted use-case failures with fromCodedError, while SessionResolver.revokeSession only propagates them. Failed revocations will therefore lose their established GraphQL error code unless conversion is retained here or moved into the resolver.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/http/schema/mutations/sessionMutations.ts` around lines 13 - 14,
Update the session revocation mutation around sessionResolver.revokeSession to
preserve the existing fromCodedError conversion for use-case failures. Either
retain the conversion in the mutation or implement equivalent conversion within
SessionResolver.revokeSession, ensuring failed revocations continue exposing
their established GraphQL error codes.
| const IV_BYTES = 12; | ||
| const AUTH_TAG_BYTES = 16; | ||
| const KEY_LENGTH = 32; | ||
| const SCRYPT_SALT = 'job-finder-totp-secret'; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Cache the derived key — scryptSync re-runs on every encrypt/decrypt call.
getKey() re-derives the key via scryptSync every time encryptSecret/decryptSecret is called, and TotpProvider is registered as a singleton. This means every TOTP login attempt (decryptSecret in LoginWithTotpUseCase), setup confirmation, and secret generation synchronously blocks the event loop for the full cost of a scrypt derivation, throttling all concurrent requests on the server.
⚡ Proposed fix: cache the derived key on the singleton instance
export class TotpProvider implements ITotpProvider {
+ private cachedKey?: Buffer;
+
private makeTotp(options: { secret?: string; label?: string } = {}): TOTP {
return new TOTP({ crypto, base32, issuer: TOTP_CONFIG.ISSUER, ...options });
}
private getKey(): Buffer {
+ if (this.cachedKey) return this.cachedKey;
const passphrase = process.env[ENV.TOTP_ENCRYPTION_KEY];
if (!passphrase) {
throw new Error(
`${ENV.TOTP_ENCRYPTION_KEY} is not configured — required to encrypt/decrypt TOTP secrets`,
);
}
- return scryptSync(passphrase, SCRYPT_SALT, KEY_LENGTH);
+ this.cachedKey = scryptSync(passphrase, SCRYPT_SALT, KEY_LENGTH);
+ return this.cachedKey;
}Also applies to: 20-28, 45-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/infrastructure/auth/TotpProvider.ts` at line 13, Cache the
scrypt-derived key in the singleton TotpProvider instance instead of recomputing
it in getKey() for every encryptSecret, decryptSecret, or secret-generation
call. Add instance-level lazy initialization so scryptSync runs only once, then
reuse the cached key for all subsequent operations while preserving the existing
key derivation inputs and behavior.
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audited
apps/apiagainst Clean Architecture's Dependency Rule (inner layers must not depend on outer layers). Found and fixed the real violations; confirmed the rest of the codebase clean.1. TOTP crypto bypassed DI entirely (the one exception in the whole app)
LoginWithTotpUseCase,GenerateTotpSecretUseCase, andConfirmTotpSetupUseCaseimportedinfrastructure/auth/totp.js/totpSecretCrypto.jsdirectly — the only infrastructure concern in this codebase with no port interface (every repository, storage provider, LLM provider, and rate limiter has one).ITotpProvider(generateSecret/getOtpauthUrl/verifyCode/encryptSecret/decryptSecret) and aTotpProviderimplementation, wired through the DI container like everything else. The three use cases now depend on the port via theirDepsinterface.2.
apiToken,session,coverLetter,jobDescriptionskipped the resolver layerinterface-adapters/resolvers/XResolver→ use case → mapper. These four called use-cases/mappers directly from the schema layer instead.ApiTokenResolverandSessionResolver, matching the existing pattern exactly, wired into the DI container.coverLetterandjobDescriptionas direct schema-layer calls — each is a single trivial pass-through with no mapper and no logic to protect, so a resolver there would be pure ceremony for no benefit.3. Business logic embedded in a GraphQL
resolve()functionapiTokenMutations.tshadargs.scope === 'read' ? 'read' : 'full'— silently coercing any invalid string (including typos) into full-scope access. Root cause:scopewas a raw string arg instead of a GraphQL enum.ApiTokenScopeEnum. Invalid values are now rejected by GraphQL's own argument validation instead of being silently coerced — a real correctness fix, not just a refactor (a typo like"raed"used to silently grant full scope).4. A backward import from earlier this session
interface-adapters/resolvers/ApplicationResolver.tsimporting a type fromhttp/schema/types/ApplicationConnectionType.ts— outer importing from... itself, backwards. MovedApplicationConnectionDTOto live next toApplicationDTOin the mapper;http/now imports frominterface-adapters/as it should.Confirmed clean, left alone
domain/anywhere.McpController: correctly goes through use cases only.DocumentResolvercallingstorageProvider.getSignedUrldirectly to enrich a DTO — goes through the injected port interface (not a concrete class), judged a reasonable, low-risk resolver responsibility rather than a violation worth the churn of extracting a use case for it.bcrypt/crypto/qrcode/nanoidused directly in ~15 use cases without a port — a deliberate, consistent convention for stateless utilities with no hidden config dependency, distinct from the TOTP case (which did read an env var internally). Left as-is.Test plan
pnpm typecheck,pnpm lint,pnpm buildall passTotpProvider,ApiTokenResolver, andSessionResolvertest files, minus the retiredtotpSecretCrypto.test.ts)createApiTokenwith the new enum-typed scope,listApiTokens,deleteApiToken,sessionsquery with correctcurrentflagging,revokeOtherSessions, and the full TOTP lifecycle (setup/confirm/login/disable) — all correctApplicationStatusenum), not something this PR introducedSummary by CodeRabbit
New Features
Bug Fixes