promote: IAM, audit attestation, and entitlement lease security slice - #46
Conversation
…etion Complete IAM, audit attestation, and entitlement lease security slice
📝 WalkthroughWalkthroughThis PR adds IAM invitation, registration, recovery, and service-account flows; AUD attestations; BUA entitlement leases; MFA reenrollment enforcement; persistence migrations; OpenAPI contracts; module wiring; tests; and operational evidence. ChangesIAM, AUD, and BUA security slices
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RegistrationController
participant RegistrationService
participant RegistrationRepository
Client->>RegistrationController: POST /v1/auth/register
RegistrationController->>RegistrationService: validate and register input
RegistrationService->>RegistrationRepository: persist hierarchy transactionally
RegistrationRepository-->>RegistrationService: registration result
RegistrationService-->>RegistrationController: identifiers and locale
RegistrationController-->>Client: registration response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
services/api/src/features/iam/adapter/iam-invitation-crypto.adapter.ts-7-11 (1)
7-11: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce a minimum HMAC key length.
validKeyaccepts any non-empty key, including a single-character string or byte. A short key materially weakensHmacSha256IamInvitationDigestAdapter, letting an attacker brute-force the key and forge or reverse invitation/email digests. Require a key at least as long as the SHA-256 output (32 bytes).🔒 Proposed fix to enforce a minimum key length
function validKey(key: IamInvitationDigestKeyV1): boolean { return ( - (typeof key === 'string' && key.length > 0) || (key instanceof Uint8Array && key.length > 0) + (typeof key === 'string' && key.length >= 32) || + (key instanceof Uint8Array && key.length >= 32) ); }🤖 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 `@services/api/src/features/iam/adapter/iam-invitation-crypto.adapter.ts` around lines 7 - 11, Update validKey to require both string and Uint8Array keys to have at least 32 bytes, matching the SHA-256 output length, instead of merely being non-empty; preserve rejection of invalid key types and ensure HmacSha256IamInvitationDigestAdapter uses this validation.services/api/src/features/iam/adapter/iam-recovery-crypto.adapter.ts-11-27 (1)
11-27: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNo minimum length is required for the recovery HMAC key.
validKeyaccepts any non-empty key, and the composition test fixtures encode that weak expectation with a 17-character key. The recovery token digest is the lookup key for password reset, so the key needs real entropy.
services/api/src/features/iam/adapter/iam-recovery-crypto.adapter.ts#L11-L27: require at least 32 bytes of key material invalidKey, measuring string keys withBuffer.byteLength(key, 'utf8').services/api/test/features/iam/recovery-composition.test.ts#L53-L120: replace therecoveryDigestKey: 'test-recovery-key'fixtures at lines 59, 79, and 103 with a key of at least 32 bytes, and add a test that assertsHmacSha256IamRecoveryDigestAdapterrejects a short key.🤖 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 `@services/api/src/features/iam/adapter/iam-recovery-crypto.adapter.ts` around lines 11 - 27, The recovery HMAC key validation must require at least 32 bytes of key material. In services/api/src/features/iam/adapter/iam-recovery-crypto.adapter.ts#L11-L27, update validKey to measure string keys with Buffer.byteLength(key, 'utf8') while retaining Uint8Array byte-length validation; in services/api/test/features/iam/recovery-composition.test.ts#L53-L120, replace the three short recoveryDigestKey fixtures with keys of at least 32 bytes and add coverage asserting HmacSha256IamRecoveryDigestAdapter rejects a short key.services/api/src/features/iam/adapter/prisma-recovery-repository.adapter.ts-238-247 (1)
238-247: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire
ACTIVEstatus incompleteRecovery.
findUserIdByEmailaccepts onlyACTIVE, but this check rejects onlyDEACTIVATED. A user whose status changes toSUSPENDEDcan still rotate credentials and incrementsecurityEpoch. Useuser.status !== 'ACTIVE'at line 241.🤖 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 `@services/api/src/features/iam/adapter/prisma-recovery-repository.adapter.ts` around lines 238 - 247, The completeRecovery user validation currently rejects only DEACTIVATED users; update the status check in the block around findUnique and before updateMany to require user.status === 'ACTIVE', while preserving the existing not-found and user-ID validation and subsequent revision-conflict handling.services/api/src/features/iam/adapter/in-memory-service-account-repository.adapter.ts-96-100 (1)
96-100: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCheck write scope against the stored account.
A workspace context can see an organization-scoped account through
visibleInScope. The replacement then validates only the incoming account and can move the stored account into the workspace scope. AddwritableInScope(context, current)before the revision checks.The Prisma adapter already blocks this transition because its
updateManypredicate matches the incomingworkspaceId; an organization-scoped row hasworkspaceId: null.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/api/src/features/iam/adapter/in-memory-service-account-repository.adapter.ts` around lines 96 - 100, In the replacement flow, update the scope validation to check the stored account as well: after retrieving and visibility-checking current in the repository method, call writableInScope(context, current) before the revision checks. Preserve the existing incoming-account validation while preventing organization-scoped records from being moved into a workspace scope.services/api/src/features/iam/application/invitation.service.ts-195-206 (1)
195-206: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not deliver the bearer before the invitation is persisted.
deliverruns inside the open database transaction and beforesaveInvitation. Two consequences follow. First, ifsaveInvitationor the commit fails, the recipient already holds a raw token that no persisted invitation backs, so redemption always fails. Second, mail latency holds the transaction and its row locks open, which reduces throughput under load.Persist the invitation first, then deliver after the transaction commits, or record a delivery intent inside the transaction and dispatch it from an outbox worker.
♻️ Sketch: commit first, then deliver
- try { - await this.delivery.deliver({ - invitationId: token.value.id, - membershipId: token.value.membershipId, - recipientEmail: normalizedEmail.value, - rawToken: raw, - expiresAt: token.value.expiresAt, - }); - } catch { - return rejected('DELIVERY_UNAVAILABLE'); - } await transaction.saveInvitation(context, token.value); return accepted({ invitationId: token.value.id, membershipId: token.value.membershipId, expiresAt: token.value.expiresAt, - deliveryStatus: 'DELIVERED' as const, + deliveryStatus: 'DELIVERED' as const, });Then call
this.delivery.deliverafterwithTransactionresolves, and map a delivery failure toDELIVERY_UNAVAILABLEwhile the invitation remains revocable.🤖 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 `@services/api/src/features/iam/application/invitation.service.ts` around lines 195 - 206, Update the invitation creation flow around transaction.saveInvitation and this.delivery.deliver so the invitation is persisted and the transaction commits before delivery begins. Move delivery outside the withTransaction callback, then map delivery failures to rejected('DELIVERY_UNAVAILABLE') while leaving the persisted invitation revocable.services/api/src/features/iam/application/invitation.service.ts-100-108 (1)
100-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMap the remaining repository error strings.
PrismaIamInvitationTransactionAdapterinservices/api/src/features/iam/adapter/prisma-iam-invitation-repository.adapter.tsthrowsIAM_INVITATION_REVISION_CONFLICT(lines 308, 310, 319),IAM_INVITATION_SCOPE_IMMUTABLE(line 306), andIAM_MEMBERSHIP_SCOPE_IMMUTABLE(line 342).applicationErrordoes not match those strings, so it returnsUNAVAILABLE. A concurrent redeem is a conflict, not an outage, so the caller receives the wrong problem type and clients retry incorrectly.🐛 Proposed fix for the error mapping
function applicationError(error: unknown): IamInvitationApplicationCodeV1 { const message = error instanceof Error ? error.message : ''; if (message === 'IAM_SCOPE_DENIED' || message === 'IAM_SCOPE_NARROWING_REQUIRED') return 'SCOPE_DENIED'; - if (message === 'IAM_INVITATION_CONFLICT' || message === 'IAM_REVISION_CONFLICT') + if ( + message === 'IAM_INVITATION_CONFLICT' || + message === 'IAM_REVISION_CONFLICT' || + message === 'IAM_INVITATION_REVISION_CONFLICT' || + message === 'IAM_INVITATION_SCOPE_IMMUTABLE' || + message === 'IAM_MEMBERSHIP_SCOPE_IMMUTABLE' + ) return 'CONFLICT'; if (message === 'IAM_INVITATION_INVALID') return 'INVALID_TOKEN'; return 'UNAVAILABLE'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/api/src/features/iam/application/invitation.service.ts` around lines 100 - 108, Update applicationError to map IAM_INVITATION_REVISION_CONFLICT to CONFLICT, alongside the existing revision-conflict handling. Map IAM_INVITATION_SCOPE_IMMUTABLE and IAM_MEMBERSHIP_SCOPE_IMMUTABLE to the appropriate non-UNAVAILABLE application result used for immutable scope violations, preserving UNAVAILABLE only for unrecognized errors.services/api/src/features/iam/adapter/prisma-iam-invitation-repository.adapter.ts-250-294 (1)
250-294: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a partial unique index for active invitations.
tokenDigestalready has a unique constraint. The migration only adds a non-unique index on(membership_id, status). Add a partial unique index onmembership_idwherestatus = 'ACTIVE'to prevent concurrent issuance from creating multiple active invitations.🤖 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 `@services/api/src/features/iam/adapter/prisma-iam-invitation-repository.adapter.ts` around lines 250 - 294, The invitation persistence schema/migration must enforce at most one ACTIVE invitation per membership under concurrency. Add a partial unique index on membership_id restricted to rows with status = 'ACTIVE', while retaining tokenDigest uniqueness and the existing lookup index; update the relevant migration/schema definition rather than relying only on the findFirst check in saveInvitation.services/api/src/features/iam/application/registration.service.ts-87-101 (1)
87-101: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftThe rejected-registration outcome discloses account existence.
findByEmailreturnsREGISTRATION_REJECTED.RegistrationControllermaps that code toREGISTRATION_REQUEST_REJECTED, andproblem-details.filter.tsreturns HTTP 400.RegistrationDtovalidation already rejects malformed payloads, so a well-formed request can receive 400 only when the email exists. An attacker can therefore enumerate registered accounts. This defeats the constant-work hashing defense documented on lines 57-58 and contradictsservices/api/src/features/iam/application/registration-repository.port.tsline 14, which states that callers must not use the lookup to disclose account existence.Return an indistinguishable outcome for both the created and the already-registered case. A common pattern is to accept the request, send a verification or "account already exists" email out of band, and return the same response body in both cases. That change affects the controller status code, the response DTO, and the API contract.
🤖 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 `@services/api/src/features/iam/application/registration.service.ts` around lines 87 - 101, The registration flow in the transaction callback must stop returning REGISTRATION_REJECTED when findByEmail detects an existing account. Make existing and newly created registrations produce the same accepted outcome, then update RegistrationController, RegistrationDto, and the API contract so both paths use the identical status and response body while preserving the existing out-of-band notification behavior.services/api/src/features/iam/api/registration.controller.ts-28-41 (1)
28-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd admission control for
POST /v1/auth/registerThe API application has no global throttler or route-level rate limit. Add per-IP and per-email admission control before
RegistrationService.registerperforms Argon2id hashing.🤖 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 `@services/api/src/features/iam/api/registration.controller.ts` around lines 28 - 41, Add admission control in the `register` method before calling `this.registration.register(input)`, enforcing limits independently per client IP and normalized email address. Reuse the application’s existing throttling or rate-limit mechanism if available, and reject over-limit requests without invoking `RegistrationService.register` or performing Argon2id hashing.services/api/src/features/iam/application/service-account.service.ts-168-178 (1)
168-178: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
serviceAccountPermissionsasserts a type it does not verify.The predicate declares
input is readonly PermissionV1[]. It only checksArray.isArray(input)and the absence of the three service-account permissions. It never checks that each element is a validPermissionV1, and it accepts an empty array.Line 205 relies on this predicate, so
input.permissionsreachescreateServiceAccountV1typed asreadonly PermissionV1[]while it can still hold arbitrary values.createServiceAccountV1currently rejects them throughINVALID_PERMISSION, so no escalation occurs today. The unsound assertion is the risk: any future caller that trusts the narrowed type gets no validation.Validate each element against
PERMISSIONS_V1, and rename the function to state what it checks.♻️ Proposed change
-function serviceAccountPermissions(input: unknown): input is readonly PermissionV1[] { - return ( - Array.isArray(input) && - !input.some( - (permission) => - permission === PERMISSIONS_V1.SERVICE_ACCOUNT_READ || - permission === PERMISSIONS_V1.SERVICE_ACCOUNT_MANAGE || - permission === PERMISSIONS_V1.SERVICE_ACCOUNT_REVOKE, - ) - ); -} +const SELF_MANAGEMENT_PERMISSIONS_V1: readonly PermissionV1[] = Object.freeze([ + PERMISSIONS_V1.SERVICE_ACCOUNT_READ, + PERMISSIONS_V1.SERVICE_ACCOUNT_MANAGE, + PERMISSIONS_V1.SERVICE_ACCOUNT_REVOKE, +]); + +const GRANTABLE_PERMISSIONS_V1: ReadonlySet<string> = new Set( + Object.values(PERMISSIONS_V1).filter( + (permission) => !SELF_MANAGEMENT_PERMISSIONS_V1.includes(permission), + ), +); + +/** Accepts a non-empty list of known permissions that excludes service-account self-management. */ +function grantableServiceAccountPermissions(input: unknown): input is readonly PermissionV1[] { + return ( + Array.isArray(input) && + input.length > 0 && + input.every((permission) => typeof permission === 'string') && + input.every((permission: string) => GRANTABLE_PERMISSIONS_V1.has(permission)) + ); +}Update the call site at line 205 to the new name.
🤖 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 `@services/api/src/features/iam/application/service-account.service.ts` around lines 168 - 178, Update serviceAccountPermissions to a name that reflects validation, and require every array element to be a valid PERMISSIONS_V1 value while continuing to reject the three service-account permissions and empty arrays. Preserve the readonly PermissionV1[] type guard, and update its call site near createServiceAccountV1 to use the new function name.services/api/src/features/iam/api/service-account.controller.ts-72-80 (1)
72-80: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftImplement idempotency for service-account creation
SessionRequestTenantContextAdapteralready storesidempotency-keyincontext.idempotencyKey; the controller variable is redundant.ServiceAccountService.createandServiceAccountRepositoryPortV1ignore this field, so retries create new accounts and credentials. Add a transactional, tenant-scoped idempotency lookup. Removing the controller parameter alone does not disable header processing. Store or otherwise preserve the original one-time secret if a retry must return the same response.🤖 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 `@services/api/src/features/iam/api/service-account.controller.ts` around lines 72 - 80, Implement tenant-scoped idempotency for service-account creation across the controller, ServiceAccountService.create, and ServiceAccountRepositoryPortV1: reuse context.idempotencyKey rather than the redundant controller header parameter, perform the lookup and create operation transactionally, and return the original result for retries instead of creating new accounts or credentials. Preserve the original one-time secret needed to reproduce the same response, and ensure header processing remains handled by SessionRequestTenantContextAdapter.services/api/openapi/v1.json-10164-10167 (1)
10164-10167: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
mfaReenrollmentRequiredis declared but never required in either session DTO. Both DTOs add the property and leave therequiredarray unchanged, so every client must treat the flag as absent-capable. A client that readsundefinedtreats it as false and skips the MFA re-enrollment gate, so a security control fails open.RecoveryCompleteResponseDtopins the same flag toenum: [true]and marks it required, andservices/api/prisma/schema/iam.prismagives the column a non-null default, so the server can always emit the field.
services/api/openapi/v1.json#L10164-L10167: add"mfaReenrollmentRequired"to therequiredarray ofCurrentSessionDto.services/api/openapi/v1.json#L10189-L10201: add"mfaReenrollmentRequired"to therequiredarray ofAuthSessionDto.🤖 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 `@services/api/openapi/v1.json` around lines 10164 - 10167, Make mfaReenrollmentRequired mandatory in both session DTO schemas by adding it to the required arrays for CurrentSessionDto at services/api/openapi/v1.json lines 10164-10167 and AuthSessionDto at services/api/openapi/v1.json lines 10189-10201. Preserve the existing property definitions.services/api/openapi/v1.json-10673-10680 (1)
10673-10680: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe millisecond-precision UTC pattern was removed from every request timestamp in this diff. One shared cause: the timestamp fields now declare
format: "date-time"alone. That accepts offset forms such as+07:00and any sub-second precision, where the previous contract accepted only UTC with millisecond precision. Retention, approval, and audit timestamps drive eligibility comparisons, so an offset-bearing value changes the result unless the server normalizes first. The components section still defines aUtcTimestampschema with aZ$pattern, which shows the repository convention these fields no longer follow. The PR summary does not state a reason for the relaxation, so confirm it is intentional; otherwise reference#/components/schemas/UtcTimestampat each site.
services/api/openapi/v1.json#L10673-L10680: restore the UTC constraint onevaluatedAt, the four retention timestamps, andapprovedAtinAuthorizeArtifactDeletionRequestDto.services/api/openapi/v1.json#L10642-L10646: restore the UTC constraint on the retention timestamps inCreateArtifactDeletionRequestDto.services/api/openapi/v1.json#L10656: restore the UTC constraint onrequestedAt.services/api/openapi/v1.json#L10607: restore the UTC constraint ondueAt, keeping the null union.services/api/openapi/v1.json#L11571: restore the UTC constraint onCreateSpreadsheetAuditResultDto.createdAt.🤖 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 `@services/api/openapi/v1.json` around lines 10673 - 10680, Restore the UTC millisecond-precision timestamp contract by referencing `#/components/schemas/UtcTimestamp` instead of using date-time alone. Apply this to services/api/openapi/v1.json lines 10673-10680 for AuthorizeArtifactDeletionRequestDto, 10642-10646 for CreateArtifactDeletionRequestDto, 10656 for requestedAt, 10607 for dueAt while preserving its null union, and 11571 for CreateSpreadsheetAuditResultDto.createdAt.services/api/openapi/v1.json-10782-10782 (1)
10782-10782: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDiscrete counts and size bounds changed from
integertonumber. One shared cause: these fields lost their integral constraint, so fractional values such as1.5now validate.AdmitArtifactDtois direct evidence that the widening is unintended, because the siblingactualByteSizeon line 10779 is stillintegerwhile the bound it is compared against is not. Apply the same guidance to the newly addedfirstSequenceandlastSequenceinCreateAuditAttestationDto, which are sequence numbers declared asnumber.
services/api/openapi/v1.json#L10782: restore"type": "integer"onmaxByteSizeso it matchesactualByteSize.services/api/openapi/v1.json#L11531-L11533: restore"type": "integer"onmaxRow,maxColumn, andformulaCount.🤖 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 `@services/api/openapi/v1.json` at line 10782, Restore integer types for discrete counts and size bounds in services/api/openapi/v1.json#L10782-L10782 and `#L11531-L11533`: update AdmitArtifactDto.maxByteSize, and maxRow, maxColumn, and formulaCount to use integer. Also update CreateAuditAttestationDto.firstSequence and lastSequence to integer, preserving integral validation for all sequence and count fields.services/api/openapi/v1.json-9810-9816 (1)
9810-9816: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the server clock for lease verification.
EntitlementLeaseService.verifyforwards the caller-suppliednowtoacceptEntitlementLeaseV1, which accepts any time beforeexpiresAt. A caller can submit a past timestamp and receivevalid: truefor an expired lease. Removenowfrom the production API and inject a clock only in tests.🤖 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 `@services/api/openapi/v1.json` around lines 9810 - 9816, Remove the now query parameter from the production API definition and update EntitlementLeaseService.verify and acceptEntitlementLeaseV1 to always use the server clock for lease verification. Preserve clock injection only through test-specific seams so callers cannot submit past timestamps to validate expired leases.services/api/src/features/aud/adapter/prisma-audit-attestation-repository.adapter.ts-126-146 (1)
126-146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd the missing project-scope clause to the workspace branch of
scopeWhere.The
projectbranch ofscopeWhereincludes OR clauses fororganization,workspace, andprojectscopeTypes. Theworkspacebranch omits theprojectclause. Since project rows persist their parentworkspaceId(seedatabaseScopeat Line 121 andpersistedScopeat Lines 73-82), a workspace-scoped context should see attestations scoped to projects nested in it, the same way a project-scoped context sees its ancestor workspace and organization.As written,
findAttestationcalled with a workspace-level context returnsundefinedfor a project-scoped attestation that belongs to that workspace, even thoughlistAttestations's localvisible()filter would accept the same record. Add the missing OR clause so both methods agree on visibility.🐛 Proposed fix for the workspace branch
if (context.tenantScope.scopeType === 'workspace') { return { organizationId: context.tenantScope.organizationId, OR: [ { scopeType: 'organization' }, { scopeType: 'workspace', workspaceId: context.tenantScope.workspaceId }, + { scopeType: 'project', workspaceId: context.tenantScope.workspaceId }, ], }; }🤖 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 `@services/api/src/features/aud/adapter/prisma-audit-attestation-repository.adapter.ts` around lines 126 - 146, Update the workspace branch of scopeWhere to include a project-scope OR clause matching project records by their projectId, alongside the existing organization and workspace clauses. Preserve the organization and project branches so workspace-scoped findAttestation visibility matches listAttestations.services/api/src/features/aud/application/audit-attestation.service.ts-93-100 (1)
93-100: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftThe attestation flow has no targeted seal lookup, so it scans every seal in the tenant scope. The port exposes only an unbounded
listSeals, and both attestation methods load the full result and filter in memory. The audit ledger is append-only, so the scanned set grows without bound on every attestation request.
services/api/src/features/aud/application/audit-attestation.service.ts#L93-L100: replace thelistSealscall and the in-memoryfindwith a single scoped lookup byfirstSequence,lastSequence, androotDigest, and apply the same change toverifyat Line 122.services/api/src/features/aud/application/audit-repository.port.ts#L42-L42: replacelistSealswith a selector-basedfindSeal(context, selector)method, and push the predicate into the Prisma adapter query.🤖 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 `@services/api/src/features/aud/application/audit-attestation.service.ts` around lines 93 - 100, The attestation flow must use a targeted seal lookup instead of loading and filtering all seals. In services/api/src/features/aud/application/audit-attestation.service.ts lines 93-100 and the verify path around line 122, replace listSeals and in-memory filtering with findSeal using firstSequence, lastSequence, and rootDigest; in services/api/src/features/aud/application/audit-repository.port.ts line 42, replace listSeals with the selector-based findSeal(context, selector) contract and update the Prisma adapter to apply the selector in its query.services/api/src/features/aud/application/audit-attestation.service.ts-85-85 (1)
85-85: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMap conflicting caller-supplied
attestationIdvalues to a request error.
attestationIdis HTTP-exposed. Conflicting existing IDs make both adapters throwAUD_IMMUTABLE_ATTESTATION, which the controller maps toAUDIT_ATTESTATION_UNAVAILABLEand HTTP 503. Preserve identical retries as successful idempotent writes. Use an atomic typed conflict result instead of a pre-read alone, because reads are scope-filtered and concurrent writes can still conflict. Add tests for identical retries and same- and cross-scope conflicts.🤖 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 `@services/api/src/features/aud/application/audit-attestation.service.ts` at line 85, Update the attestation creation flow around stableId and the persistence operation to atomically distinguish identical retries from conflicting attestationId reuse, including same- and cross-scope conflicts; map conflicts to the request-error behavior expected by the controller while preserving identical retries as successful idempotent writes. Use a typed atomic conflict result rather than relying on a scope-filtered pre-read, and add coverage for identical retries plus same- and cross-scope conflicting IDs.services/api/src/features/bua/adapter/prisma-entitlement-lease-repository.adapter.ts-144-148 (1)
144-148: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBoth lease adapters check immutability with order-sensitive serialization.
JSON.stringifyequality depends on property insertion order, so the immutability guard can report a false mismatch whenever a lease object is rebuilt instead of cloned. Replace serialization with field-wise comparison in one shared helper.
services/api/src/features/bua/adapter/prisma-entitlement-lease-repository.adapter.ts#L144-L148: comparepersistedLease(existing)withleasefield by field, including the tenant scope, becausepersistedLeaserebuildstenantScopethroughparseTenantScopeV1and can order its keys differently from the issued lease.services/api/src/features/bua/adapter/in-memory-entitlement-lease-repository.adapter.ts#L26-L28: use the same field-wise helper instead ofJSON.stringify, so the in-memory adapter stays equivalent to the Prisma adapter.🤖 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 `@services/api/src/features/bua/adapter/prisma-entitlement-lease-repository.adapter.ts` around lines 144 - 148, Replace the order-sensitive JSON.stringify immutability checks with one shared field-wise lease comparison helper, including tenantScope and all lease fields. Update the existing check in services/api/src/features/bua/adapter/prisma-entitlement-lease-repository.adapter.ts lines 144-148 and the equivalent check in services/api/src/features/bua/adapter/in-memory-entitlement-lease-repository.adapter.ts lines 26-28 to use that helper, preserving the BUA_IMMUTABLE_LEASE error behavior.services/api/src/features/bua/api/entitlement.controller.ts-141-156 (1)
141-156: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
@Type(() => Number)tosnapshotRevisionandsecurityEpoch.The global pipe disables implicit conversion. Without explicit conversion, query values remain strings and valid lease verification returns
ENTITLEMENT_REQUEST_INVALID(400).🤖 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 `@services/api/src/features/bua/api/entitlement.controller.ts` around lines 141 - 156, Add explicit numeric transformation with `@Type`(() => Number) to the snapshotRevision and securityEpoch fields in VerifyEntitlementLeaseDto so query-string values are converted before verifyLease passes them to leases.verify. Preserve the existing verifyLease flow and validation behavior.
🟡 Minor comments (11)
packages/domain/src/invitation/v1.ts-139-139 (1)
139-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport revoked tokens with
INVALID_STATE.Line 139 maps every non-ACTIVE status to
ALREADY_CONSUMED. A REVOKED token was never consumed. Callers that translate this code into a user-facing reason will state the wrong cause. Distinguish the two states.🐛 Proposed fix
- if (token.status !== 'ACTIVE') return rejected('ALREADY_CONSUMED'); + if (token.status === 'REDEEMED') return rejected('ALREADY_CONSUMED'); + if (token.status !== 'ACTIVE') return rejected('INVALID_STATE');🤖 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 `@packages/domain/src/invitation/v1.ts` at line 139, Update the token-status handling in the invitation validation flow so REVOKED tokens return rejected('INVALID_STATE'), while consumed or otherwise non-ACTIVE tokens retain the existing 'ALREADY_CONSUMED' result. Preserve the current behavior for ACTIVE tokens.packages/domain/src/entitlements/v1.ts-269-279 (1)
269-279: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winContain signer exceptions. Wrap
signer.sign(payload)intry/catchand returnrejected('LEASE_INVALID')when it throws. Add a test for a throwing signer.🤖 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 `@packages/domain/src/entitlements/v1.ts` around lines 269 - 279, Update the entitlement lease signing flow around canonicalLease to catch exceptions from signer.sign(payload) and return rejected('LEASE_INVALID') instead of propagating the error; add coverage using a signer that throws.services/api/src/features/iam/application/mfa-repository.port.ts-6-11 (1)
6-11: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake
clearRecoveryReenrollmentrequired onMfaTransactionPortV1.mfa.service.tscalls it after factor verification, andrecovery.service.tssetsmfaReenrollmentRequiredtotrue. An adapter can still omit the optional method and leave the gate active without a type error.🤖 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 `@services/api/src/features/iam/application/mfa-repository.port.ts` around lines 6 - 11, Update the MfaTransactionPortV1 interface to make clearRecoveryReenrollment a required method by removing its optional marker, while preserving its existing parameters, return type, and documentation.docs/operations/iam-010-invitation-token-2026-08-03.md-26-31 (1)
26-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe "Explicitly not complete" list contradicts the code in this PR.
Lines 28-29 state that the invitation HTTP/controller and production composition wiring remain future work. This PR adds
services/api/src/features/iam/api/invitation.controller.tsand composesinvitationServiceinservices/api/src/features/iam/iam.module.ts. Update this record so the remaining-work list matches the promoted state. Keep the items that are still open, for example the transactional AUD append and the email-provider adapter.🤖 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 `@docs/operations/iam-010-invitation-token-2026-08-03.md` around lines 26 - 31, The “Explicitly not complete” section in the IAM-010 record incorrectly lists the invitation HTTP/controller and production composition wiring as future work. Remove those completed items from the list while retaining genuinely open work such as transactional AUD append, unknown-recipient registration, resend/revocation administration, email-provider integration, and production evidence.services/api/test/features/iam/registration.service.test.ts-124-152 (1)
124-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe rollback test asserts against an unused repository.
Line 125 creates
repository, but line 127 passes an inline stub repository toRegistrationService. The service never touchesrepository. Therefore the assertion at line 151 is true before the test runs and proves nothing about rollback. The test name claims that persistence rolls back after staging.Drive the failure through the real adapter so the rollback path executes.
💚 Proposed fix
void test('[IAM-001] registration rolls back when persistence fails after staging', async () => { const repository = new InMemoryRegistrationRepositoryAdapter(); const service = new RegistrationService({ repository: { - withTransaction: async (work) => - work({ - findByEmail: async () => { - await Promise.resolve(); - return false; - }, - save: async () => { - await Promise.resolve(); - throw new Error('database unavailable'); - }, - }), + withTransaction: async (work) => + repository.withTransaction(async (transaction) => { + await work({ + findByEmail: transaction.findByEmail, + save: async (input) => { + await transaction.save(input); + throw new Error('database unavailable'); + }, + }); + throw new Error('database unavailable'); + }), }, passwordCredentials: passwordCredentials(), ids: ids(), });🤖 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 `@services/api/test/features/iam/registration.service.test.ts` around lines 124 - 152, Update the rollback test setup around RegistrationService so its withTransaction implementation delegates to the real InMemoryRegistrationRepositoryAdapter rather than an inline persistence stub, while still forcing the post-staging save to fail. Ensure repository.has('new@example.com') verifies the same adapter used by service.register and confirms the staged record was rolled back.services/api/src/features/iam/api/registration.dto.ts-5-8 (1)
5-8: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a valid email in the documented example.
@IsEmail()rejectsngu***@example.com``. Swagger prefills this example, so users who run the request receive a 400 response. Use a valid placeholder address.🐛 Proposed fix
- `@ApiProperty`({ example: 'ngu***`@example.com`', maxLength: 254 }) + `@ApiProperty`({ example: 'nguyen.an@example.com', maxLength: 254 })🤖 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 `@services/api/src/features/iam/api/registration.dto.ts` around lines 5 - 8, Update the email example in the registration DTO’s email property to use a syntactically valid placeholder address that passes `@IsEmail`(), while preserving the existing maxLength metadata and validation decorators.services/api/openapi/v1.json-11531-11533 (1)
11531-11533: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the sheet dimensions and formula count integers.
maxRow,maxColumn, andformulaCountchanged fromintegertonumber. All three are discrete counts, so fractional values must not validate. See the consolidated comment.🤖 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 `@services/api/openapi/v1.json` around lines 11531 - 11533, Update the maxRow, maxColumn, and formulaCount properties in the relevant OpenAPI schema to use type integer instead of number, while preserving their existing minimum and maximum constraints.services/api/openapi/v1.json-11571-11571 (1)
11571-11571: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore a UTC constraint on
createdAt.
createdAtlost the UTC pattern. See the consolidated comment.🤖 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 `@services/api/openapi/v1.json` at line 11571, Update the createdAt schema property to restore its UTC-specific pattern while retaining the existing string date-time format. Apply the same constraint expected by the consolidated comment so createdAt values explicitly require UTC.services/api/openapi/v1.json-10411-10420 (1)
10411-10420: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
format: "email"toRegistrationDto.emailand fix the example.
RecoveryRequestDto.emailandIssueInvitationDto.recipientEmailboth declareformat: "email".RegistrationDto.emaildeclares onlymaxLength, so the contract accepts any string on a public endpoint. The examplengu***@example.com`` is also a masked value that is not a valid address, so it fails the format it documents. Declare the format and use a valid example address.🔧 Proposed fix
"RegistrationDto": { "type": "object", "properties": { - "email": { "type": "string", "example": "ngu***`@example.com`", "maxLength": 254 }, + "email": { + "type": "string", + "format": "email", + "example": "nguyen@example.com", + "maxLength": 254 + }, "displayName": { "type": "string", "minLength": 1, "maxLength": 200 },🤖 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 `@services/api/openapi/v1.json` around lines 10411 - 10420, Update RegistrationDto.email to declare format "email" and replace the masked example with a valid email address, while preserving its existing maxLength constraint.services/api/openapi/v1.json-10607-10607 (1)
10607-10607: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore a UTC constraint on
dueAt.
dueAtlost the UTC pattern and now accepts any RFC 3339 date-time. See the consolidated comment.🤖 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 `@services/api/openapi/v1.json` at line 10607, Update the dueAt schema in the OpenAPI definition to restore validation that accepts only UTC date-time values, while retaining support for null. Preserve the existing oneOf structure and date-time typing, adding the UTC constraint to the string variant.services/api/test/prisma-foundation.test.mjs-553-561 (1)
553-561: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the regex escape character class.
Use
/[.*+?^${}()|[\]\\]/gso SQL statements are escaped beforenew RegExp()is called.🤖 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 `@services/api/test/prisma-foundation.test.mjs` around lines 553 - 561, Update the regex in the migration statement loop for prisma-foundation tests to use the correct escape character class `/[.*+?^${}()|[\]\\]/g` before constructing each `RegExp`, while preserving the existing statement matching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 623cb2bd-fe7f-4764-864b-478fa22337e7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (155)
docs/operations/iam-010-invitation-token-2026-08-03.mddocs/operations/iam-bua-security-slice-2026-08-03.mddocs/operations/iam-recovery-2026-08-03.mddocs/operations/iam-registration-2026-08-03.mddocs/plans/requirement-traceability.jsonpackages/domain/package.jsonpackages/domain/src/approval/v1.tspackages/domain/src/audit/v1.tspackages/domain/src/authorization/v1.tspackages/domain/src/entitlements/v1.tspackages/domain/src/invitation/v1.tspackages/domain/src/mfa/v1.tspackages/domain/src/permissions/v1.tspackages/domain/src/recovery/v1.tspackages/domain/src/service-account/v1.tspackages/domain/src/v1.tspackages/domain/test/audit-seal-attestation-v1.test.mjspackages/domain/test/audit-service-account-actions-v1.test.mjspackages/domain/test/built-public-api-smoke.mjspackages/domain/test/entitlement-lease-issuance-v1.test.mjspackages/domain/test/entitlements-v1.test.mjspackages/domain/test/invitation-v1.test.mjspackages/domain/test/mfa-v1.test.mjspackages/domain/test/permission-applicability-v1.test.mjspackages/domain/test/permissions-v1.test.mjspackages/domain/test/public-api-v1.test.mjspackages/domain/test/recovery-v1.test.mjspackages/domain/test/service-account-v1.test.mjspackages/i18n/src/catalogs-v1.tspackages/i18n/test/catalogs-v1.test.mjspnpm-workspace.yamlservices/api/openapi/v1.jsonservices/api/prisma/migrations/20260803040000_iam_invitation_tokens/migration.sqlservices/api/prisma/migrations/20260803050000_iam_recovery_challenges/migration.sqlservices/api/prisma/migrations/20260803060000_iam_service_accounts/migration.sqlservices/api/prisma/migrations/20260803070000_bua_entitlement_leases/migration.sqlservices/api/prisma/migrations/20260803080000_aud_seal_attestations/migration.sqlservices/api/prisma/schema/aud.prismaservices/api/prisma/schema/bua.prismaservices/api/prisma/schema/iam.prismaservices/api/src/features/aud/adapter/in-memory-audit-attestation-repository.adapter.tsservices/api/src/features/aud/adapter/prisma-audit-attestation-repository.adapter.tsservices/api/src/features/aud/api/audit-attestation.controller.tsservices/api/src/features/aud/api/audit-attestation.dto.tsservices/api/src/features/aud/application/audit-attestation-repository.port.tsservices/api/src/features/aud/application/audit-attestation.service.tsservices/api/src/features/aud/application/audit-equality.tsservices/api/src/features/aud/application/audit-problem.error.tsservices/api/src/features/aud/application/audit-repository.port.tsservices/api/src/features/aud/aud.module.tsservices/api/src/features/bua/adapter/hmac-entitlement-lease-signer.adapter.tsservices/api/src/features/bua/adapter/in-memory-entitlement-lease-repository.adapter.tsservices/api/src/features/bua/adapter/prisma-entitlement-lease-repository.adapter.tsservices/api/src/features/bua/api/entitlement-lease.dto.tsservices/api/src/features/bua/api/entitlement.controller.tsservices/api/src/features/bua/application/entitlement-lease-repository.port.tsservices/api/src/features/bua/application/entitlement-lease.service.tsservices/api/src/features/bua/application/entitlement-problem.error.tsservices/api/src/features/bua/bua.module.tsservices/api/src/features/iam/adapter/iam-invitation-crypto.adapter.tsservices/api/src/features/iam/adapter/iam-recovery-crypto.adapter.tsservices/api/src/features/iam/adapter/in-memory-iam-invitation-repository.adapter.tsservices/api/src/features/iam/adapter/in-memory-mfa-repository.adapter.tsservices/api/src/features/iam/adapter/in-memory-recovery-admission.adapter.tsservices/api/src/features/iam/adapter/in-memory-recovery-repository.adapter.tsservices/api/src/features/iam/adapter/in-memory-registration-repository.adapter.tsservices/api/src/features/iam/adapter/in-memory-service-account-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-credential-lookup.adapter.tsservices/api/src/features/iam/adapter/prisma-iam-invitation-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-identity-bootstrap-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-mfa-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-principal-email-lookup.adapter.tsservices/api/src/features/iam/adapter/prisma-recovery-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-registration-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-service-account-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-session-lifecycle.adapter.tsservices/api/src/features/iam/adapter/random-service-account-secret.adapter.tsservices/api/src/features/iam/adapter/redis-recovery-admission.adapter.tsservices/api/src/features/iam/api/auth-session.dto.tsservices/api/src/features/iam/api/authentication.controller.tsservices/api/src/features/iam/api/current-session.dto.tsservices/api/src/features/iam/api/invitation.controller.tsservices/api/src/features/iam/api/invitation.dto.tsservices/api/src/features/iam/api/recovery.controller.tsservices/api/src/features/iam/api/recovery.dto.tsservices/api/src/features/iam/api/registration.controller.tsservices/api/src/features/iam/api/registration.dto.tsservices/api/src/features/iam/api/service-account.controller.tsservices/api/src/features/iam/api/service-account.dto.tsservices/api/src/features/iam/application/authentication.port.tsservices/api/src/features/iam/application/invitation-problem.error.tsservices/api/src/features/iam/application/invitation-repository.port.tsservices/api/src/features/iam/application/invitation.service.tsservices/api/src/features/iam/application/mfa-repository.port.tsservices/api/src/features/iam/application/mfa.service.tsservices/api/src/features/iam/application/recovery-problem.error.tsservices/api/src/features/iam/application/recovery-repository.port.tsservices/api/src/features/iam/application/recovery.service.tsservices/api/src/features/iam/application/registration-problem.error.tsservices/api/src/features/iam/application/registration-repository.port.tsservices/api/src/features/iam/application/registration.service.tsservices/api/src/features/iam/application/service-account-problem.error.tsservices/api/src/features/iam/application/service-account-repository.port.tsservices/api/src/features/iam/application/service-account.service.tsservices/api/src/features/iam/application/tenant-context.tsservices/api/src/features/iam/iam.module.tsservices/api/src/features/jra/application/approval.service.tsservices/api/src/platform/http/problem-details.filter.tsservices/api/src/platform/http/session-tenant-context.adapter.tsservices/api/test/features/aud/aud.module.test.tsservices/api/test/features/aud/audit-attestation-contract.test.tsservices/api/test/features/aud/audit-attestation-repository.test.tsservices/api/test/features/aud/audit-attestation.controller.test.tsservices/api/test/features/aud/audit-attestation.service.test.tsservices/api/test/features/aud/prisma-audit-attestation-repository.test.tsservices/api/test/features/bua/bua.module.test.tsservices/api/test/features/bua/entitlement-lease-repository.test.tsservices/api/test/features/bua/entitlement-lease.service.test.tsservices/api/test/features/bua/entitlement.controller.test.tsservices/api/test/features/bua/hmac-entitlement-lease-signer.test.tsservices/api/test/features/bua/prisma-entitlement-lease-repository.test.tsservices/api/test/features/iam/iam-invitation-crypto.adapter.test.tsservices/api/test/features/iam/in-memory-invitation-repository.test.tsservices/api/test/features/iam/invitation-composition.test.tsservices/api/test/features/iam/invitation-controller.test.tsservices/api/test/features/iam/invitation-service.test.tsservices/api/test/features/iam/mfa.service.test.tsservices/api/test/features/iam/prisma-credential-lookup.test.tsservices/api/test/features/iam/prisma-iam-invitation-repository.test.tsservices/api/test/features/iam/prisma-mfa-repository.test.tsservices/api/test/features/iam/prisma-principal-email-lookup.adapter.test.tsservices/api/test/features/iam/prisma-recovery-repository.test.tsservices/api/test/features/iam/prisma-registration-repository.test.tsservices/api/test/features/iam/prisma-service-account-repository.test.tsservices/api/test/features/iam/prisma-session-lifecycle.test.tsservices/api/test/features/iam/recovery-admission.test.tsservices/api/test/features/iam/recovery-composition.test.tsservices/api/test/features/iam/recovery-controller.test.tsservices/api/test/features/iam/recovery-crypto.test.tsservices/api/test/features/iam/recovery-http.test.tsservices/api/test/features/iam/recovery.service.test.tsservices/api/test/features/iam/redis-recovery-admission.adapter.test.tsservices/api/test/features/iam/registration-composition.test.tsservices/api/test/features/iam/registration-controller.test.tsservices/api/test/features/iam/registration-http.test.tsservices/api/test/features/iam/registration.service.test.tsservices/api/test/features/iam/service-account-composition.test.tsservices/api/test/features/iam/service-account-repository.test.tsservices/api/test/features/iam/service-account-secret.adapter.test.tsservices/api/test/features/iam/service-account.controller.test.tsservices/api/test/features/iam/service-account.service.test.tsservices/api/test/features/jra/approval.service.test.tsservices/api/test/openapi.test.tsservices/api/test/platform/http/session-tenant-context.test.tsservices/api/test/prisma-foundation.test.mjs
| try { | ||
| return await this.ports.repository.withTransaction(async (transaction) => { | ||
| const userId = await transaction.findUserIdByEmail(normalized.value); | ||
| if (!userId) | ||
| return Object.freeze({ accepted: true as const, value: { requested: true as const } }); | ||
| const active = await transaction.findActiveChallengeForUser(userId); | ||
| const challenge = createRecoveryChallengeV1({ | ||
| id: challengeId, | ||
| userId, | ||
| tokenDigest, | ||
| emailDigest, | ||
| issuedAt, | ||
| expiresAt, | ||
| }); | ||
| if (!challenge.accepted) return unavailable(); | ||
| try { | ||
| await this.ports.delivery.deliver({ | ||
| challengeId: challenge.value.id, | ||
| recipientEmail: normalized.value, | ||
| rawToken: raw, | ||
| expiresAt: challenge.value.expiresAt, | ||
| }); | ||
| } catch { | ||
| return unavailable(); | ||
| } | ||
| if (active) { | ||
| const revoked = revokeRecoveryChallengeV1(active, issuedAt); | ||
| if (!revoked.accepted) return unavailable(); | ||
| await transaction.saveChallenge(revoked.value); | ||
| } | ||
| await transaction.saveChallenge(challenge.value); | ||
| return Object.freeze({ accepted: true as const, value: { requested: true as const } }); | ||
| }); | ||
| } catch { | ||
| return unavailable(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Move the email delivery out of the database transaction and after the challenge is persisted.
this.ports.delivery.deliver runs at line 136, inside withTransaction and before transaction.saveChallenge(challenge.value) at line 150. This creates two problems.
- The service sends the recovery link before it persists the challenge row. If
saveChallengethrows or the transaction rolls back, the user receives a link whose challenge does not exist. The token digest lookup incompletethen fails and the reset is impossible. deliveris an external network call that holds the open transaction and the row locks for its whole duration. No timeout is applied. Under load this exhausts the database connection pool.
Also note that line 134, line 143, and line 147 return unavailable() from inside the transaction callback instead of throwing, so the transaction commits. After the reorder, any partial write on those paths needs a throw to force a rollback.
Persist and commit the challenge first, then deliver. If delivery fails after the commit, revoke the challenge or return unavailable() and let the user retry.
🐛 Proposed reordering sketch
try {
- return await this.ports.repository.withTransaction(async (transaction) => {
+ const issued = await this.ports.repository.withTransaction(async (transaction) => {
const userId = await transaction.findUserIdByEmail(normalized.value);
- if (!userId)
- return Object.freeze({ accepted: true as const, value: { requested: true as const } });
+ if (!userId) return undefined;
const active = await transaction.findActiveChallengeForUser(userId);
const challenge = createRecoveryChallengeV1({
id: challengeId,
userId,
tokenDigest,
emailDigest,
issuedAt,
expiresAt,
});
- if (!challenge.accepted) return unavailable();
- try {
- await this.ports.delivery.deliver({
- challengeId: challenge.value.id,
- recipientEmail: normalized.value,
- rawToken: raw,
- expiresAt: challenge.value.expiresAt,
- });
- } catch {
- return unavailable();
- }
+ if (!challenge.accepted) throw new Error('IAM_RECOVERY_CHALLENGE_INVALID');
if (active) {
const revoked = revokeRecoveryChallengeV1(active, issuedAt);
- if (!revoked.accepted) return unavailable();
+ if (!revoked.accepted) throw new Error('IAM_RECOVERY_REVOKE_INVALID');
await transaction.saveChallenge(revoked.value);
}
await transaction.saveChallenge(challenge.value);
- return Object.freeze({ accepted: true as const, value: { requested: true as const } });
+ return challenge.value;
});
+ if (issued) {
+ await this.ports.delivery.deliver({
+ challengeId: issued.id,
+ recipientEmail: normalized.value,
+ rawToken: raw,
+ expiresAt: issued.expiresAt,
+ });
+ }
+ return Object.freeze({ accepted: true as const, value: { requested: true as const } });
} catch {
return unavailable();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| return await this.ports.repository.withTransaction(async (transaction) => { | |
| const userId = await transaction.findUserIdByEmail(normalized.value); | |
| if (!userId) | |
| return Object.freeze({ accepted: true as const, value: { requested: true as const } }); | |
| const active = await transaction.findActiveChallengeForUser(userId); | |
| const challenge = createRecoveryChallengeV1({ | |
| id: challengeId, | |
| userId, | |
| tokenDigest, | |
| emailDigest, | |
| issuedAt, | |
| expiresAt, | |
| }); | |
| if (!challenge.accepted) return unavailable(); | |
| try { | |
| await this.ports.delivery.deliver({ | |
| challengeId: challenge.value.id, | |
| recipientEmail: normalized.value, | |
| rawToken: raw, | |
| expiresAt: challenge.value.expiresAt, | |
| }); | |
| } catch { | |
| return unavailable(); | |
| } | |
| if (active) { | |
| const revoked = revokeRecoveryChallengeV1(active, issuedAt); | |
| if (!revoked.accepted) return unavailable(); | |
| await transaction.saveChallenge(revoked.value); | |
| } | |
| await transaction.saveChallenge(challenge.value); | |
| return Object.freeze({ accepted: true as const, value: { requested: true as const } }); | |
| }); | |
| } catch { | |
| return unavailable(); | |
| } | |
| try { | |
| const issued = await this.ports.repository.withTransaction(async (transaction) => { | |
| const userId = await transaction.findUserIdByEmail(normalized.value); | |
| if (!userId) return undefined; | |
| const active = await transaction.findActiveChallengeForUser(userId); | |
| const challenge = createRecoveryChallengeV1({ | |
| id: challengeId, | |
| userId, | |
| tokenDigest, | |
| emailDigest, | |
| issuedAt, | |
| expiresAt, | |
| }); | |
| if (!challenge.accepted) throw new Error('IAM_RECOVERY_CHALLENGE_INVALID'); | |
| if (active) { | |
| const revoked = revokeRecoveryChallengeV1(active, issuedAt); | |
| if (!revoked.accepted) throw new Error('IAM_RECOVERY_REVOKE_INVALID'); | |
| await transaction.saveChallenge(revoked.value); | |
| } | |
| await transaction.saveChallenge(challenge.value); | |
| return challenge.value; | |
| }); | |
| if (issued) { | |
| await this.ports.delivery.deliver({ | |
| challengeId: issued.id, | |
| recipientEmail: normalized.value, | |
| rawToken: raw, | |
| expiresAt: issued.expiresAt, | |
| }); | |
| } | |
| return Object.freeze({ accepted: true as const, value: { requested: true as const } }); | |
| } catch { | |
| return unavailable(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/api/src/features/iam/application/recovery.service.ts` around lines
120 - 155, Rework the recovery flow around withTransaction so it only creates,
revokes, and persists challenges, then completes before invoking
this.ports.delivery.deliver. Replace unavailable() returns for
persistence-related failures inside the transaction with thrown errors so
partial writes roll back; perform delivery afterward, and if it fails revoke the
committed challenge or return unavailable() while preserving retry behavior.
| const serviceAccountRepository = | ||
| options.serviceAccountRepository ?? | ||
| (options.serviceAccountDatabase === undefined | ||
| ? new InMemoryServiceAccountRepositoryAdapter() | ||
| : new PrismaServiceAccountRepositoryAdapter(options.serviceAccountDatabase)); | ||
| const serviceAccountService = | ||
| options.serviceAccountService ?? | ||
| (options.iamRepository === undefined | ||
| ? new UnavailableServiceAccountService() | ||
| : new ServiceAccountService( | ||
| serviceAccountRepository, | ||
| options.iamRepository, | ||
| options.serviceAccountSecretIssuer ?? new RandomServiceAccountSecretIssuer(), | ||
| options.serviceAccountClock, | ||
| options.serviceAccountIdGenerator, | ||
| )); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
serviceAccountService checks the wrong variable and silently disables itself when wired via iamDatabase.
The condition uses options.iamRepository === undefined, and the constructor call passes options.iamRepository. Both should use the derived local iamRepository variable (defined at lines 325-329), the same way membershipService does right above this block. As written, if the module is configured with iamDatabase only (not iamRepository directly), iamRepository is correctly built from PrismaIamRepositoryAdapter, but serviceAccountService still falls back to UnavailableServiceAccountService() because options.iamRepository stays undefined. This silently disables service-account lifecycle management in a common production wiring configuration.
🐛 Proposed fix to use the derived `iamRepository` variable
const serviceAccountService =
options.serviceAccountService ??
- (options.iamRepository === undefined
+ (iamRepository === undefined
? new UnavailableServiceAccountService()
: new ServiceAccountService(
serviceAccountRepository,
- options.iamRepository,
+ iamRepository,
options.serviceAccountSecretIssuer ?? new RandomServiceAccountSecretIssuer(),
options.serviceAccountClock,
options.serviceAccountIdGenerator,
));Run the following script to check whether existing tests wire the module via iamDatabase only and would have caught this:
#!/bin/bash
# Description: Check whether service-account composition tests exercise the iamDatabase-only wiring path.
set -euo pipefail
rg -n -B2 -A10 'IamModule.register' services/api/test/features/iam/service-account-composition.test.ts || echo "file or pattern not found"🤖 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 `@services/api/src/features/iam/iam.module.ts` around lines 447 - 462, Update
the serviceAccountService setup to use the derived iamRepository variable for
both the availability check and the ServiceAccountService constructor argument,
matching the membershipService composition pattern. Preserve the existing
fallback when iamRepository is unavailable while enabling service-account
lifecycle management when iamRepository is created from iamDatabase.
1 participant
Summary
devfast-uridependency resolution required by the hosted high-severity audit gatedevintegration history as one 69-commit release-candidate diffReview policy
This is the fresh promotion PR for CodeRabbit. The older PR #44 is intentionally left untouched because it already consumed its single CodeRabbit run. Please perform one full review on this PR only.
Validation
The source PR #45 passed repository, Android, Python, dependency/security, and CodeRabbit-skipped-on-dev checks before merge. Local
repo:check,repo:build, and API tests (446 passing) are green.Summary by CodeRabbit