promote: reviewed IAM recovery slice - #44
Merged
Merged
Conversation
…ical-20260804 fix: harden IAM registration and service-account replay
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
services/api/src/features/bua/adapter/in-memory-entitlement-lease-repository.adapter.ts (1)
50-66: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject nested transactions before queueing.
Line 55 waits for the outer transaction tail. If
workcallswithTransactionon this adapter, the inner call waits forrelease(). The outer call waits forwork, so both calls remain pending.Add an explicit non-reentrancy guard and a regression test. Preserve normal serialization for independent calls.
🤖 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/in-memory-entitlement-lease-repository.adapter.ts` around lines 50 - 66, Update the transaction method containing transactionTail to detect and reject reentrant withTransaction calls before awaiting or queueing on transactionTail; ensure the guard is scoped to the active transaction and always cleared in finally, while preserving rollback behavior and serialization for independent calls. Add a regression test that invokes withTransaction from within work and asserts prompt rejection without deadlock.services/api/openapi/v1.json (1)
3495-3507: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeclare success response schemas for service-account creation and rotation. Both endpoints return
{ account, secret }, but their OpenAPI success responses define headers only. Generated clients will omit the response body.🤖 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 3495 - 3507, Add the `{ account, secret }` success response schema to the 201 responses for both service-account creation and rotation in services/api/openapi/v1.json:3495-3507 and services/api/openapi/v1.json:3577-3589. Preserve the existing headers and reference the appropriate account and secret schema definitions so generated clients include the response body.
🧹 Nitpick comments (4)
services/api/test/prisma-foundation.test.mjs (1)
573-593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert index predicates and indexed fields.
The assertions accept an index with the correct name but an incorrect
WHEREpredicate or key columns. Match the complete index definitions. This protects active-only invitation uniqueness and scope-bound service-account idempotency.🤖 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 573 - 593, Update the migration assertions in the prisma-foundation test to match complete CREATE UNIQUE INDEX definitions, including indexed columns and WHERE predicates. Cover the active-only predicate for invitation_tokens_active_membership_key and the organization/workspace key columns plus their scope predicates for the service-account idempotency indexes, rather than asserting names alone.services/api/prisma/migrations/20260804000000_iam_invitation_active_membership_unique/migration.sql (1)
2-4: 🩺 Stability & Availability | 🔵 TrivialPlan online creation for production unique indexes. Normal PostgreSQL index builds block writes on their target tables. Confirm that the migration runner can execute non-transactional concurrent index creation, or schedule a maintenance window.
services/api/prisma/migrations/20260804000000_iam_invitation_active_membership_unique/migration.sql#L2-L4: deploy the active-invitation index without blocking invitation writes.services/api/prisma/migrations/20260804010000_iam_service_account_create_idempotency/migration.sql#L9-L34: deploy the idempotency indexes without blocking service-account writes.🤖 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/prisma/migrations/20260804000000_iam_invitation_active_membership_unique/migration.sql` around lines 2 - 4, Update both migration files to create their unique indexes concurrently so invitation and service-account writes remain available: use non-transactional concurrent index creation for services/api/prisma/migrations/20260804000000_iam_invitation_active_membership_unique/migration.sql lines 2-4 and services/api/prisma/migrations/20260804010000_iam_service_account_create_idempotency/migration.sql lines 9-34, and configure the migration runner to execute these statements outside a transaction if required.Source: Linters/SAST tools
services/api/src/features/iam/adapter/service-account-secret-envelope.adapter.ts (2)
40-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReject envelopes that carry extra framing parts.
envelope.split('.')returns every part, and the destructuring at Line 42 discards anything after the fourth. An envelope with a trailing part such asv1.<iv>.<tag>.<ciphertext>.junkstill decrypts, because GCM authenticates only the ciphertext.The existing test at
services/api/test/features/iam/service-account-secret-envelope.adapter.test.tsLine 37 passes only because that specific input also fails the IV length check. Validate the part count so the framing check is exact.♻️ Proposed strict framing check
public open(envelope: string): string | undefined { if (typeof envelope !== 'string') return undefined; - const [version, ivEncoded, tagEncoded, ciphertextEncoded] = envelope.split('.'); - if (version !== 'v1' || !ivEncoded || !tagEncoded || !ciphertextEncoded) return undefined; + const parts = envelope.split('.'); + if (parts.length !== 4) return undefined; + const [version, ivEncoded, tagEncoded, ciphertextEncoded] = parts; + if (version !== 'v1' || !ivEncoded || !tagEncoded || !ciphertextEncoded) return undefined;🤖 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/service-account-secret-envelope.adapter.ts` around lines 40 - 43, Update the open method’s envelope parsing to require exactly four dot-separated parts before decryption, rejecting any envelope with trailing or missing framing segments. Preserve the existing v1, IV, tag, and ciphertext validation for otherwise valid envelopes.
64-67: 🔒 Security & Privacy | 🔵 TrivialStability And Availability
Reachability: Internal · Exploitability: Theoretical
Reachability path
● Entry services/api/src/features/aud/adapter/prisma-audit-repository.adapter.ts:397 findSeal │ ▼ ● Sink services/api/src/features/iam/adapter/service-account-secret-envelope.adapter.tsConfigure a durable envelope key before multi-instance deployment.
When
serviceAccountSecretEnvelopeKeyis unset, each process uses a different random AES-GCM key. Retries handled by another instance or after a restart cannot open the persistedsecretEnvelope, soreplayCreatereturnsUNAVAILABLE. Set the same managed 32-byte base64url key for every instance. During rotation, retain the previous key until existing replays are no longer needed.🤖 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/service-account-secret-envelope.adapter.ts` around lines 64 - 67, Update randomServiceAccountSecretEnvelopeAdapter and its configuration path to require a shared managed 32-byte base64url serviceAccountSecretEnvelopeKey for multi-instance or durable deployments instead of silently relying on a process-local random key. Ensure every instance uses the same configured key, and preserve the previous key during rotation until persisted secretEnvelope replays are no longer required.
🤖 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 `@services/api/src/features/iam/application/service-account.service.ts`:
- Around line 208-231: Update createRequestHash to canonicalize
input.permissions before JSON.stringify by sorting a copied permissions array,
while preserving the original input and validation behavior. Hash the sorted
permission set so equivalent requests with different permission ordering produce
the same request hash used by replayCreate.
---
Outside diff comments:
In `@services/api/openapi/v1.json`:
- Around line 3495-3507: Add the `{ account, secret }` success response schema
to the 201 responses for both service-account creation and rotation in
services/api/openapi/v1.json:3495-3507 and
services/api/openapi/v1.json:3577-3589. Preserve the existing headers and
reference the appropriate account and secret schema definitions so generated
clients include the response body.
In
`@services/api/src/features/bua/adapter/in-memory-entitlement-lease-repository.adapter.ts`:
- Around line 50-66: Update the transaction method containing transactionTail to
detect and reject reentrant withTransaction calls before awaiting or queueing on
transactionTail; ensure the guard is scoped to the active transaction and always
cleared in finally, while preserving rollback behavior and serialization for
independent calls. Add a regression test that invokes withTransaction from
within work and asserts prompt rejection without deadlock.
---
Nitpick comments:
In
`@services/api/prisma/migrations/20260804000000_iam_invitation_active_membership_unique/migration.sql`:
- Around line 2-4: Update both migration files to create their unique indexes
concurrently so invitation and service-account writes remain available: use
non-transactional concurrent index creation for
services/api/prisma/migrations/20260804000000_iam_invitation_active_membership_unique/migration.sql
lines 2-4 and
services/api/prisma/migrations/20260804010000_iam_service_account_create_idempotency/migration.sql
lines 9-34, and configure the migration runner to execute these statements
outside a transaction if required.
In
`@services/api/src/features/iam/adapter/service-account-secret-envelope.adapter.ts`:
- Around line 40-43: Update the open method’s envelope parsing to require
exactly four dot-separated parts before decryption, rejecting any envelope with
trailing or missing framing segments. Preserve the existing v1, IV, tag, and
ciphertext validation for otherwise valid envelopes.
- Around line 64-67: Update randomServiceAccountSecretEnvelopeAdapter and its
configuration path to require a shared managed 32-byte base64url
serviceAccountSecretEnvelopeKey for multi-instance or durable deployments
instead of silently relying on a process-local random key. Ensure every instance
uses the same configured key, and preserve the previous key during rotation
until persisted secretEnvelope replays are no longer required.
In `@services/api/test/prisma-foundation.test.mjs`:
- Around line 573-593: Update the migration assertions in the prisma-foundation
test to match complete CREATE UNIQUE INDEX definitions, including indexed
columns and WHERE predicates. Cover the active-only predicate for
invitation_tokens_active_membership_key and the organization/workspace key
columns plus their scope predicates for the service-account idempotency indexes,
rather than asserting names alone.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e2447d4-2e7c-427f-ac79-6e84ac301aa8
📒 Files selected for processing (75)
docs/operations/iam-010-invitation-token-2026-08-03.mddocs/operations/iam-registration-2026-08-03.mdpackages/domain/src/entitlements/v1.tspackages/domain/src/invitation/v1.tspackages/domain/test/entitlement-lease-issuance-v1.test.mjspackages/domain/test/invitation-v1.test.mjsservices/api/openapi/v1.jsonservices/api/prisma/migrations/20260804000000_iam_invitation_active_membership_unique/migration.sqlservices/api/prisma/migrations/20260804010000_iam_service_account_create_idempotency/migration.sqlservices/api/prisma/schema/iam.prismaservices/api/src/features/aud/adapter/in-memory-audit-attestation-repository.adapter.tsservices/api/src/features/aud/adapter/in-memory-audit-repository.adapter.tsservices/api/src/features/aud/adapter/prisma-audit-attestation-repository.adapter.tsservices/api/src/features/aud/adapter/prisma-audit-repository.adapter.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-repository.port.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-equality.tsservices/api/src/features/bua/application/entitlement-lease.service.tsservices/api/src/features/iae/api/artifact-admission.dto.tsservices/api/src/features/iae/api/artifact-retention.dto.tsservices/api/src/features/iae/api/inbox-item.dto.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-service-account-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-mfa-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-recovery-repository.adapter.tsservices/api/src/features/iam/adapter/prisma-service-account-repository.adapter.tsservices/api/src/features/iam/adapter/service-account-secret-envelope.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/registration.controller.tsservices/api/src/features/iam/api/registration.dto.tsservices/api/src/features/iam/api/service-account.controller.tsservices/api/src/features/iam/application/invitation.service.tsservices/api/src/features/iam/application/mfa-repository.port.tsservices/api/src/features/iam/application/recovery.service.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-repository.port.tsservices/api/src/features/iam/application/service-account.service.tsservices/api/src/features/iam/iam.module.tsservices/api/src/features/sa/api/spreadsheet-audit.dto.tsservices/api/test/features/aud/audit-attestation-repository.test.tsservices/api/test/features/aud/audit-attestation.service.test.tsservices/api/test/features/aud/prisma-audit-attestation-repository.test.tsservices/api/test/features/aud/prisma-audit-repository.test.tsservices/api/test/features/bua/entitlement-equality.test.tsservices/api/test/features/bua/entitlement-lease.service.test.tsservices/api/test/features/bua/entitlement.controller.test.tsservices/api/test/features/iam/iam-invitation-crypto.adapter.test.tsservices/api/test/features/iam/invitation-service.test.tsservices/api/test/features/iam/prisma-recovery-repository.test.tsservices/api/test/features/iam/prisma-service-account-repository.test.tsservices/api/test/features/iam/recovery-composition.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/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-envelope.adapter.test.tsservices/api/test/features/iam/service-account.controller.test.tsservices/api/test/features/iam/service-account.service.test.tsservices/api/test/http-contract.test.tsservices/api/test/prisma-foundation.test.mjs
💤 Files with no reviewable changes (1)
- services/api/src/features/bua/api/entitlement.controller.ts
🚧 Files skipped from review as they are similar to previous changes (29)
- services/api/test/features/iam/service-account-composition.test.ts
- packages/domain/test/invitation-v1.test.mjs
- services/api/test/features/iam/recovery-http.test.ts
- services/api/test/features/iam/recovery-composition.test.ts
- services/api/test/features/iam/registration-http.test.ts
- services/api/test/features/iam/recovery-crypto.test.ts
- services/api/test/features/aud/audit-attestation-repository.test.ts
- services/api/src/features/aud/api/audit-attestation.dto.ts
- services/api/test/features/bua/entitlement.controller.test.ts
- services/api/src/features/bua/api/entitlement-lease.dto.ts
- services/api/test/features/iam/iam-invitation-crypto.adapter.test.ts
- services/api/test/features/bua/entitlement-lease.service.test.ts
- services/api/src/features/bua/application/entitlement-lease.service.ts
- services/api/src/features/iam/application/invitation.service.ts
- docs/operations/iam-registration-2026-08-03.md
- services/api/test/features/iam/service-account.controller.test.ts
- services/api/src/features/iam/application/registration.service.ts
- services/api/src/features/iam/api/service-account.controller.ts
- services/api/src/features/aud/application/audit-attestation.service.ts
- packages/domain/src/invitation/v1.ts
- services/api/src/features/bua/adapter/prisma-entitlement-lease-repository.adapter.ts
- services/api/src/features/iam/adapter/prisma-recovery-repository.adapter.ts
- services/api/src/features/iam/adapter/iam-invitation-crypto.adapter.ts
- services/api/src/features/iam/adapter/prisma-mfa-repository.adapter.ts
- services/api/prisma/schema/iam.prisma
- services/api/src/features/iam/adapter/iam-recovery-crypto.adapter.ts
- services/api/src/features/aud/adapter/prisma-audit-attestation-repository.adapter.ts
- packages/domain/src/entitlements/v1.ts
- services/api/src/features/iam/iam.module.ts
Comment on lines
+208
to
+231
| function createRequestHash( | ||
| input: CreateServiceAccountInputV1, | ||
| workspaceId: StableIdentifierV1 | undefined, | ||
| ): string | undefined { | ||
| const name = normalizedName(input.name); | ||
| const expiry = normalizedExpiry(input.secretExpiresAt); | ||
| if (!name || !serviceAccountPermissions(input.permissions)) return undefined; | ||
| if (input.secretExpiresAt !== undefined && expiry === undefined) return undefined; | ||
| try { | ||
| return createHash('sha256') | ||
| .update( | ||
| JSON.stringify({ | ||
| name, | ||
| workspaceId: workspaceId ?? null, | ||
| permissions: input.permissions, | ||
| secretExpiresAt: expiry ?? null, | ||
| }), | ||
| 'utf8', | ||
| ) | ||
| .digest('hex'); | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Canonicalize permissions before hashing the create request.
createRequestHash serializes input.permissions in the order the client sent them. serviceAccountPermissions accepts any order, so ['artifact.record.read', 'job.execution.create'] and the reversed array are both valid but produce different hashes.
A client that retries the same logical create with a reordered array then hits replayCreate at Line 238, which returns CONFLICT. The caller receives 409 and cannot recover the original one-time secret.
Sort the permissions before hashing so the hash depends on the permission set, not the array order.
🐛 Proposed fix to make the request hash order-independent
function createRequestHash(
input: CreateServiceAccountInputV1,
workspaceId: StableIdentifierV1 | undefined,
): string | undefined {
const name = normalizedName(input.name);
const expiry = normalizedExpiry(input.secretExpiresAt);
if (!name || !serviceAccountPermissions(input.permissions)) return undefined;
if (input.secretExpiresAt !== undefined && expiry === undefined) return undefined;
try {
return createHash('sha256')
.update(
JSON.stringify({
name,
workspaceId: workspaceId ?? null,
- permissions: input.permissions,
+ permissions: [...input.permissions].sort(),
secretExpiresAt: expiry ?? null,
}),
'utf8',
)
.digest('hex');
} catch {
return undefined;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function createRequestHash( | |
| input: CreateServiceAccountInputV1, | |
| workspaceId: StableIdentifierV1 | undefined, | |
| ): string | undefined { | |
| const name = normalizedName(input.name); | |
| const expiry = normalizedExpiry(input.secretExpiresAt); | |
| if (!name || !serviceAccountPermissions(input.permissions)) return undefined; | |
| if (input.secretExpiresAt !== undefined && expiry === undefined) return undefined; | |
| try { | |
| return createHash('sha256') | |
| .update( | |
| JSON.stringify({ | |
| name, | |
| workspaceId: workspaceId ?? null, | |
| permissions: input.permissions, | |
| secretExpiresAt: expiry ?? null, | |
| }), | |
| 'utf8', | |
| ) | |
| .digest('hex'); | |
| } catch { | |
| return undefined; | |
| } | |
| } | |
| function createRequestHash( | |
| input: CreateServiceAccountInputV1, | |
| workspaceId: StableIdentifierV1 | undefined, | |
| ): string | undefined { | |
| const name = normalizedName(input.name); | |
| const expiry = normalizedExpiry(input.secretExpiresAt); | |
| if (!name || !serviceAccountPermissions(input.permissions)) return undefined; | |
| if (input.secretExpiresAt !== undefined && expiry === undefined) return undefined; | |
| try { | |
| return createHash('sha256') | |
| .update( | |
| JSON.stringify({ | |
| name, | |
| workspaceId: workspaceId ?? null, | |
| permissions: [...input.permissions].sort(), | |
| secretExpiresAt: expiry ?? null, | |
| }), | |
| 'utf8', | |
| ) | |
| .digest('hex'); | |
| } catch { | |
| return undefined; | |
| } | |
| } |
🤖 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 208 - 231, Update createRequestHash to canonicalize input.permissions
before JSON.stringify by sorting a copied permissions array, while preserving
the original input and validation behavior. Hash the sorted permission set so
equivalent requests with different permission ordering produce the same request
hash used by replayCreate.
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.
Promotion
Promote the reviewed
devintegration branch tomainafter PR #43.Included slice
Verification
The source PR passed repository, Android, Python, security, and affected-runtime checks. The full repository gate passed locally with
corepack pnpm repo:check.Required review
Please perform one full CodeRabbit review on this promotion PR. Do not rerun CodeRabbit after the initial review; valid findings will be fixed in focused commits and rejected findings documented.
Summary by CodeRabbit
New Features
Documentation