feat: 006 per-scan artifact ingress 및 workload identity 검증 - #255
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a per-scan, write-only SAST artifact ingress endpoint with mTLS workload identity checks, canonical envelope validation, idempotent streaming storage, durable ingestion lifecycle state, scanner-run lifecycle persistence, schema constraints, tests, and deployment contract updates. ChangesSAST artifact ingress
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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: 1
🧹 Nitpick comments (4)
packages/shared/test/sast-runtime-behavior.test.mjs (1)
239-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the canonicalization test order-independent.
envelopeis constructed in canonical field order, so this does not prove that canonicalization normalizes arbitrary property order. Re-run the assertion with the same fields inserted in a different order.🤖 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/shared/test/sast-runtime-behavior.test.mjs` around lines 239 - 242, Update the test around canonicalizeScannerArtifactEnvelope to construct or pass an equivalent envelope with its fields inserted in a different order before asserting the canonical JSON output. Preserve the expected result as JSON.stringify(envelope) or the canonical field-order representation, ensuring the assertion verifies order-independent canonicalization.apps/api/src/scan-plane/sast-artifact-ingress.store.ts (1)
101-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an input object for
abortto match sibling methods.
reserve,complete,reject, andrecordRejectedRequestall take a single typed input object, butaborttakes three positional strings, two of which (reasonCode,occurredAt) are same-typed and easy to transpose at call sites without a compiler error.♻️ Proposed signature change
- abstract abort( - ingestionId: string, - reasonCode: string, - occurredAt: string - ): Promise<void>; + abstract abort( + input: Readonly<{ ingestionId: string; reasonCode: string; occurredAt: string }> + ): Promise<void>;🤖 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/scan-plane/sast-artifact-ingress.store.ts` around lines 101 - 105, Update the abstract abort method to accept one typed input object containing ingestionId, reasonCode, and occurredAt, matching the signatures of reserve, complete, reject, and recordRejectedRequest; update its implementations and all call sites to pass the object while preserving existing behavior.apps/api/src/scan-plane/sast-workload-identity.authenticator.ts (1)
64-88: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSPIFFE URI regex is more permissive than the spec.
Per the SPIFFE-ID standard, the trust domain (host) must be lowercase-only, and path segments must contain only
[a-zA-Z0-9._-]with no percent-encoding. This regex allows uppercase trust domains and permits%!$&'()*+,;=:@in path segments, letting non-canonical identity strings pass validation that a compliant SPIFFE issuer would never emit.Since downstream matching likely does exact-string comparison against a durably stored
workloadIdentityRef, this is unlikely to be an auth bypass, but it weakens the identity-format guarantee and could mask malformed/spoofed SANs.♻️ Tighten regex to match spec
- !/^spiffe:\/\/[A-Za-z0-9](?:[A-Za-z0-9._-]{0,252})(?:\/[A-Za-z0-9._~!$&'()*+,;=:@%-]+)+$/u.test( + !/^spiffe:\/\/[a-z0-9](?:[a-z0-9._-]{0,252})(?:\/[A-Za-z0-9._-]+)+$/u.test( identityRef )Please confirm against the current SPIFFE-ID specification whether uppercase trust domains and the extended path character set should actually be rejected here.
🤖 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/scan-plane/sast-workload-identity.authenticator.ts` around lines 64 - 88, Update extractSingleSpiffeUri to enforce the SPIFFE-ID format: require the trust domain to use lowercase characters and restrict every path segment to [a-zA-Z0-9._-], rejecting percent-encoding and the currently permitted extended characters. Preserve the existing single-URI, length, trimming, normalization, and null-return behavior.apps/api/prisma/migrations/20260724180000_sast_artifact_ingress/migration.sql (1)
9-13: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftScannerRun runtime-metadata check and the SastArtifactIngestion↔ScannerRun scope FK are removed/declared transactionally but only re-installed by a separate online script — confirm atomic deploy sequencing.
migration.sqldrops the oldScannerRun_runtime_metadata_checkconstraint and never creates the newScannerRun_ingress_scope_keyindex orSastArtifactIngestion_scanner_run_scope_fkeyforeign key in the same transaction;apply-online-sast-runtime-schema.mjsis the only place that installs the v2 check, the index, and the FK, andschema.prismamodels the end state as if it's always present. Application-level transactional checks (reserve/complete flows) reduce the practical risk, but the DB-level backstop is absent for any window between the migration and the script.
apps/api/prisma/migrations/20260724180000_sast_artifact_ingress/migration.sql#L9-L13: document/enforce that this migration must never be deployed withoutapply-online-sast-runtime-schema.mjsrunning immediately afterward, before any new-version traffic reachesScannerRun/SastArtifactIngestion.apps/api/scripts/apply-online-sast-runtime-schema.mjs#L62-L82: confirm the CI/CD pipeline runs this script as a blocking step directly afterprisma migrate deployfor this migration, not as an optional/async follow-up.apps/api/scripts/apply-online-sast-runtime-schema.mjs#L160-L166: same sequencing requirement applies to theSastArtifactIngestion_scanner_run_scope_fkeyaddition.apps/api/prisma/schema.prisma#L607-L657: add a comment near theScannerRun_ingress_scope_key/artifactIngestionrelation noting that the underlying index/FK is installed out-of-band by the online script, so readers don't assumeprisma migrate deployalone is sufficient.🤖 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/prisma/migrations/20260724180000_sast_artifact_ingress/migration.sql` around lines 9 - 13, Document and enforce atomic deployment sequencing: in apps/api/prisma/migrations/20260724180000_sast_artifact_ingress/migration.sql, state that apply-online-sast-runtime-schema.mjs must run immediately after this migration and before new-version traffic. Ensure the CI/CD pipeline invokes that script as a blocking step directly after prisma migrate deploy, covering both the v2 check and SastArtifactIngestion_scanner_run_scope_fkey installation at apps/api/scripts/apply-online-sast-runtime-schema.mjs:62-82 and :160-166. Add a schema.prisma comment near ScannerRun_ingress_scope_key and artifactIngestion clarifying that the index and FK are installed out-of-band by the online script.
🤖 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/scan-plane/sast-artifact-ingress.service.ts`:
- Around line 207-251: Ensure ingestion state advances even when object cleanup
fails in the catch paths of the SAST artifact ingress handler. Perform
store.reject() or store.abort() before deleteStoredObject(), or make deletion
best-effort after the durable transition, while preserving the existing error
responses and cleanup attempt.
---
Nitpick comments:
In
`@apps/api/prisma/migrations/20260724180000_sast_artifact_ingress/migration.sql`:
- Around line 9-13: Document and enforce atomic deployment sequencing: in
apps/api/prisma/migrations/20260724180000_sast_artifact_ingress/migration.sql,
state that apply-online-sast-runtime-schema.mjs must run immediately after this
migration and before new-version traffic. Ensure the CI/CD pipeline invokes that
script as a blocking step directly after prisma migrate deploy, covering both
the v2 check and SastArtifactIngestion_scanner_run_scope_fkey installation at
apps/api/scripts/apply-online-sast-runtime-schema.mjs:62-82 and :160-166. Add a
schema.prisma comment near ScannerRun_ingress_scope_key and artifactIngestion
clarifying that the index and FK are installed out-of-band by the online script.
In `@apps/api/src/scan-plane/sast-artifact-ingress.store.ts`:
- Around line 101-105: Update the abstract abort method to accept one typed
input object containing ingestionId, reasonCode, and occurredAt, matching the
signatures of reserve, complete, reject, and recordRejectedRequest; update its
implementations and all call sites to pass the object while preserving existing
behavior.
In `@apps/api/src/scan-plane/sast-workload-identity.authenticator.ts`:
- Around line 64-88: Update extractSingleSpiffeUri to enforce the SPIFFE-ID
format: require the trust domain to use lowercase characters and restrict every
path segment to [a-zA-Z0-9._-], rejecting percent-encoding and the currently
permitted extended characters. Preserve the existing single-URI, length,
trimming, normalization, and null-return behavior.
In `@packages/shared/test/sast-runtime-behavior.test.mjs`:
- Around line 239-242: Update the test around
canonicalizeScannerArtifactEnvelope to construct or pass an equivalent envelope
with its fields inserted in a different order before asserting the canonical
JSON output. Preserve the expected result as JSON.stringify(envelope) or the
canonical field-order representation, ensuring the assertion verifies
order-independent canonicalization.
🪄 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: 63768336-7fc4-48dd-991c-12acab89f302
📒 Files selected for processing (30)
apps/api/prisma/migrations/20260724180000_sast_artifact_ingress/migration.sqlapps/api/prisma/schema.prismaapps/api/scripts/apply-online-sast-runtime-schema.mjsapps/api/src/scan-plane/current-sast-workload-identity.decorator.tsapps/api/src/scan-plane/prisma-sast-artifact-ingress.store.tsapps/api/src/scan-plane/prisma-sast-scanner-runtime.store.tsapps/api/src/scan-plane/sast-artifact-ingress.controller.tsapps/api/src/scan-plane/sast-artifact-ingress.service.tsapps/api/src/scan-plane/sast-artifact-ingress.store.tsapps/api/src/scan-plane/sast-artifact-object-store.tsapps/api/src/scan-plane/sast-scanner-runtime.service.tsapps/api/src/scan-plane/sast-scanner-runtime.store.tsapps/api/src/scan-plane/sast-workload-identity.authenticator.tsapps/api/src/scan-plane/sast-workload-identity.guard.tsapps/api/src/scan-plane/scan-plane.dto.tsapps/api/src/scan-plane/scan-plane.module.tsapps/api/src/scan-plane/scanner-sandbox-runtime.provider.tsapps/api/test/scan-plane/prisma-sast-artifact-ingress.store.e2e-spec.tsapps/api/test/scan-plane/prisma-sast-scanner-runtime.store.e2e-spec.tsapps/api/test/scan-plane/sast-artifact-ingress.e2e-spec.tsapps/api/test/scan-plane/sast-scanner-runtime.e2e-spec.tsapps/api/test/scan-plane/scanner-runtime-persistence.e2e-spec.tsdeploy/scanner-sandbox/provisioning-contract.jsonpackages/shared/src/types/sast-runtime.tspackages/shared/test/sast-runtime-behavior.test.mjspackages/shared/test/sast-runtime.test.mjsspecs/006-production-sast-runtime-design/contracts/sast-runtime.mdspecs/006-production-sast-runtime-design/data-model.mdspecs/006-production-sast-runtime-design/quickstart.mdspecs/006-production-sast-runtime-design/tasks.md
1 participant
🎋 작업 중인 브랜치 및 이슈 - 브랜치:
feat/254-006-artifact-ingress- 이슈: #254 - 선행 PR: #253 ## 🔎 주요 변경 사항 -PUT /api/scan-plane/result-ingress/{scanRequestId}/scanner-runs/{scannerRunId}write-only artifact ingress를 추가했습니다. -application/octet-stream, 8 KiB 이하 canonical JSON base64url envelope, canonicalContent-Length, plan-bound idempotency key로 수신 계약을 고정했습니다. - 직접 승인된 mTLS peer certificate의 단일 SPIFFE URI SAN만 workload identity로 인정하고, lowercase trust domain·안전한 path segment를 강제하며 forwarded identity header는 신뢰하지 않습니다. - durable tenant/repository/scan/attempt/scanner-run/workload identity와 envelope를 결합하고SCANNING상태·signed deadline·RUNNINGscanner run을 재검증합니다. - scanner run을 실제 실행 전에RUNNING으로 영속화해 sandbox가 실행 중에만 해당 run으로 업로드할 수 있도록 lifecycle을 연결했습니다. - scanner-run별 immutable reservation과 exact replay idempotency를 구현하고, 변경된 envelope replay·cross-scan scope·수신 종료 후 제출을 차단했습니다. - Scan Plane object-store port는 immutableput과 cleanupdelete만 노출하고 read API를 제공하지 않으며, production 기본 provider는 Data/Security Plane adapter가 없으면 fail closed 합니다. -SastArtifactIngestion수신 상태와 복합 scope FK, lifecycle/digest/size/identity DB 제약을 추가하고 기존ScannerRun의 online schema를 ingress-aware v2로 확장했습니다. - ingress 예약·완료·cleanup stage 경합을 serializable transaction으로 차단하고 object key/raw bytes를 응답·사용자 조회·감사 metadata에서 제외했습니다.type/issue-number-short-feature형식을 따르나요? - [x] 이슈 제목과 PR 제목을 동일하게 작성했나요? - [x] 커밋 메시지가<type>: <description>형식을 따르나요? ## Check List - [x] Assignees 등록을 하였나요? - [x] 라벨(Label) 등록을 하였나요? - [x] PR 머지 전 반드시 CI가 정상적으로 작동하는지 확인했나요? ## 검증 -corepack pnpm lint-corepack pnpm test(shared 38, AI 17, web 54, API 76 suites / 322 tests, GitHub·CI·deployment contract 19) -corepack pnpm typecheck-corepack pnpm build-node --test test/runtime/*.test.mjs-prisma validate- online schema script syntax 및 provisioning JSON parse -git diff --check## 006 진행 상태 - T029 완료 - 다음 작업: T030 plan binding, schema, digest, byte/count, encoding, path, coordinate limit 검증 Closes feat: 006 per-scan artifact ingress 및 workload identity 검증 #254 ## Summary by CodeRabbit * New Features * Added write-only SAST artifact uploads with streaming, idempotent retries, and validation receipts. * Added direct mTLS workload identity verification for secure artifact submission. * Added scanner-run lifecycle tracking, including durable start, completion, failure, and timeout states. * Added strict envelope, size, digest, scan-scope, and repository-binding validation. * Documentation * Updated runtime, ingress, data model, quickstart, and implementation task documentation. * Tests * Added end-to-end coverage for uploads, replay handling, identity validation, lifecycle protection, and runtime persistence.