Security Review: PR #64

Commit: 727f247878801614907a738921115720e9a19427

Summary

Reviewed the GitHub App Manifest provisioning slice with a focus on the four credential-handling primitives: the AES-256-GCM SecretCipherService, the HMAC-signed CSRF state, the convert-manifest conversion endpoint, and secret persistence. The cryptographic core is well-built — correct AEAD construction, fresh random IV per encryption, constant-time signature comparison, fail-fast key validation, and ciphertext-only persistence. No CRITICAL or HIGH issues. The findings below are MEDIUM-and-below, and the documented v0.1 tradeoffs in AgDR-0006 are acceptable for a one-time, pre-auth, first-run provisioning flow.

Checklist Results

• Secrets & Credentials: Pass — only ciphertext persisted; .env.test key is a verified dummy (see below)
• Injection Prevention: Pass — MikroORM parameterised writes; code URL-encoded into the GitHub path
• XSS Prevention: Pass — no v-html, no secret reflection; Vue escapes interpolation
• Auth & Authorisation: Pass (with documented v0.1 caveat) — endpoint is intentionally unauthenticated pre-#22; CSRF state is the only guard
• Data Protection: Pass
• API Security: Partial — no input-validation pipe, no rate limiting (see MEDIUM-1, MEDIUM-2)

Crypto core — verified correct

SecretCipherService (src/modules/crypto/secret-cipher.service.ts)

• AES-256-GCM via Node built-in; 32-byte key enforced at construction (fail-fast on missing/wrong length) — good.
• Fresh 12-byte random IV per encrypt() call (randomBytes(IV_LENGTH)) — IV uniqueness is correct; no nonce reuse risk.
• Auth tag captured via getAuthTag() and verified via setAuthTag() on decrypt; decipher.final() throws on tamper/wrong-key — correct AEAD failure mode.
• Length guard (< IV + TAG) before slicing prevents a malformed-token panic — good.

StateSigner (src/app/github-app/state-signer.ts)

• HMAC-SHA256 with a domain-separated subkey derived from the root key (HMAC(root, "github-app-state-v1")) — correct separation from the encryption key.
• Signature compared with timingSafeEqual behind a length pre-check — constant-time, correct.
• Expiry enforced (payload.exp > now); malformed/garbage payload caught and rejected. Solid.

Security Issues Found

MEDIUM-1 — No input-validation pipe on the conversion endpoint. There is no global ValidationPipe, no class-validator decorators on ConvertManifestRequest, and forbidNonWhitelisted is not set. code/state are typed but not validated at the framework boundary — the only guards are the manual if (!request.code) and stateSigner.verify(request.state) inside the service. verify() does call String.split / Buffer.from on state, so a non-string state (e.g. {state: {}} or an array) would reach those calls and throw an unhandled TypeError → 500 rather than a clean 400. Impact is low (DoS-resistant, no bypass), but the boundary should reject malformed input deterministically.
Remediation: register a global ValidationPipe({ whitelist: true, forbidNonWhitelisted: true, transform: true }) and add @IsString() @IsNotEmpty() to both DTO fields. This also hardens every future endpoint.

MEDIUM-2 — No rate limiting on GET /manifest or POST /conversions. Both endpoints are unauthenticated (by design, pre-#22). POST /conversions accepts a code + state; an attacker who obtains a valid unexpired state (it travels through the browser and GitHub) can replay it freely within the 10-min window, and GET /manifest can be hammered to mint unlimited state tokens. No DB-backed nonce store means there is no server-side single-use enforcement.
Remediation: add @nestjs/throttler on these routes. Acceptable to defer to the #22 follow-up if explicitly tracked, but it should be noted as a known gap, not silent.

LOW-1 — State is not single-use (replay within TTL). The nonce in the payload is generated but never persisted, so the state token is replayable until exp. AgDR-0006 documents the not-session-bound limitation and defers binding to #22, which covers the cross-browser CSRF angle. The single-use angle is a narrower instance of the same class. Acceptable for v0.1 one-time provisioning; flag it in the #22 scope so the session-bound rewrite also makes state single-use (e.g. consume-on-verify against a store or session).

LOW-2 — Upstream GitHub error body is propagated to the client. GitHubManifestClient.convertManifest builds the thrown message from response.status + body (raw GitHub response text), and ConvertManifestService rewraps it verbatim in BadGatewayException(error.message). GitHub's manifest-conversion error bodies don't contain Marsa secrets, but echoing an upstream provider's raw response to the caller is an information-disclosure smell and can leak internal detail if the upstream contract changes. The FE already shows a generic message, so the detailed body only helps an attacker probing the endpoint.
Remediation: log the upstream body server-side; return a generic 502 message to the client.

LOW-3 — Conversion code and decrypted secrets rely on absence-of-logging rather than explicit redaction. The service never logs creds and never returns secrets in the response (verified: response is appSlug/appName/htmlUrl/installUrl only) — good. But there's no structural guard (e.g. a toJSON/redaction wrapper) preventing a future logger.debug(creds) or an exception serializer from spilling clientSecret/webhookSecret/pem. Given the value of these secrets, consider wrapping them in a redacting type or at least an inline comment at the credential boundary. Informational.

Notes that are NOT issues (verified)

• .env.test key is safe. APP_SECRETS_ENCRYPTION_KEY=Kioq…Kio= decodes to exactly 32 bytes, all 0x2A (*) — a verified dummy fixture, not a real secret. No concern. Keep the inline (NOT real secrets) comment.
• Only ciphertext persisted. GitHubApp entity stores clientSecretEnc / webhookSecretEnc / privateKeyPemEnc as text; client_id / app id / slug / name / htmlUrl / owner are non-secret and stored plaintext, consistent with AgDR-0006. Confirmed no plaintext secret column.
• code handled safely. encodeURIComponent(code) into the GitHub path prevents path injection; the code is short-lived (~1h) and never persisted or logged.
• No injection / XSS surface. Writes go through MikroORM (parameterised); FE uses interpolation only, no v-html, no secret reflection.

Assessment of AgDR-0006 tradeoffs (v0.1)

Acceptable. The choices — AES-256-GCM with zero new deps, env-var key with operator-owned key management, stateless HMAC state instead of session-bound, built-in fetch over Octokit — are sound and explicitly scoped to a one-time first-run flow with the session-binding gap consciously deferred to #22. The AgDR correctly calls out that pre-auth signed-state is weaker than a session and that key rotation is not implemented. Recommend the #22 follow-up also (a) bind state to the admin session AND (b) make it single-use, and that MEDIUM-1/MEDIUM-2 are tracked rather than silently inherited.

Verdict

COMMENT — no blocking (CRITICAL/HIGH) findings. The cryptographic core is correct and secrets are handled properly. MEDIUM-1 (validation pipe) and MEDIUM-2 (rate limiting) should be addressed or explicitly tracked before this surface is reachable in production; LOW items are hardening. Safe to proceed for v0.1 with those follow-ups recorded.

🛡️ Reviewed by Hakim (Security Auditor)
📌 Reviewed commit: 727f247878801614907a738921115720e9a19427

Code Review: PR #64

Commit: 727f247878801614907a738921115720e9a19427

Posted as a comment because GitHub blocks request-changes on one's own PR. Verdict is CHANGES REQUESTED — treat this as blocking; the Rex approval marker has NOT been written.

Summary

Implements per-install GitHub App provisioning via the Manifest flow (#58): a github-app NestJS vertical slice (get-manifest + convert-manifest use-cases), a cross-cutting AES-256-GCM SecretCipher in src/modules/crypto, the project's first MikroORM entity + migration (github_app), and two Nuxt pages (/setup/github + callback). Architecture, AgDR linkage, glossary, and test design are all strong. The blocker is red CI, not code quality.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass (with one CI-surfaced item, below)
• Performance: Pass
• PR Description & Glossary: Pass
• Summary Bullet Narrative: Pass (narrative bullets with rationale)
• Technical Decisions (AgDR):Pass — AgDR-0006 (credential storage) + AgDR-0007 (migration) linked and cover the diff
• Adopter Handbooks: Pass (Migration Safety blocking handbook: N/A — purely additive table)

Issues Found

⛔ BLOCKER — CI is red. Gate 5 (workflow-gates.md) + "No Red CI Before Merge" (pr-quality.md) cannot pass. Two checks fail:

1. SAST (Semgrep) — blocking finding introduced by THIS PR.
   apps/api/src/modules/crypto/secret-cipher.service.ts:57 — rule javascript.node-crypto.security.gcm-no-tag-length. createDecipheriv('aes-256-gcm', key, iv) is called without an explicit { authTagLength: 16 }. Semgrep flags this as blocking because an unconstrained GCM tag length can, in general, let an attacker present a truncated tag.

   Your code already slices the tag to a fixed 16 bytes and setAuthTag rejects a wrong-length buffer, so real-world forgery risk is low — but it is a blocking SAST finding that fails CI and must be cleared. One-line fix:
   ```ts
   const decipher = createDecipheriv(ALGORITHM, this.key, iv, { authTagLength: AUTH_TAG_LENGTH })
   ```
   Pass { authTagLength: AUTH_TAG_LENGTH } to createCipheriv too for symmetry. Prefer the code fix over a // nosemgrep suppression.

2. Dependency audit (pnpm) — pre-existing critical advisory, unrelated to this PR.
   pnpm audit --audit-level=high fails on GHSA-5xrq-8626-4rwp (vitest <4.1.0, critical) via apps/web's vitest@3.2.4 (catalog ^3.0.0 on main). Not introduced here — it pre-dates the branch. But a pre-existing red check still blocks merge: bump vitest + @vitest/coverage-v8 catalog entries to >=4.1.0 in a separate commit/PR, re-verify the web suite, rebase this PR onto green, then merge.

Handbook Findings

Migration Safety (blocking) — handbooks/architecture/migration-safety.md — N/A. Migration20260607130731.ts only runs create table "github_app" (additive, first table) with a drop table if exists down(). No DROP/RENAME/narrowing on existing data. AgDR-0007 documents rollback, zero blast radius, sole-consumer analysis. No violation.

Clean Architecture Layers — handbooks/architecture/clean-architecture-layers.md — no violations. HTTP/DI/persistence sit in the right places; SecretCipher/StateSigner are cross-cutting infra; the entity is feature-owned per the api CLAUDE.md vertical-slice convention.

TypeScript Strict Mode — handbooks/language/typescript/strict-mode.md — no violations. No bare any in hand-written code; test doubles use as unknown as over runtime-shaped objects (accepted pattern). Generated app/api/*.gen.ts is out of scope.

Suggestions (non-blocking)

• convert-manifest.service.ts:28 — let creds is untyped; annotate let creds: GitHubAppCredentials for readability.
• convert-manifest.controller.ts — ConvertManifestRequest carries only @ApiProperty, no class-validator decorators, and no global ValidationPipe is wired. The service defensively handles empty code (400) and bad/missing state (400 via verify), so it's adequately guarded today. If a ValidationPipe lands later, add @IsString()/@IsNotEmpty() here.
• github-manifest.client.ts:51 — GitHub's conversion response is cast as ConversionResponse with no runtime shape check. Low risk (trusted endpoint), but a small zod/guard on data.id/data.pem would fail loudly if GitHub's shape drifts. Optional.

Verdict

CHANGES REQUESTED — code and design are approve-quality; the gate is the red CI. (1) Add { authTagLength: 16 } to the GCM cipher/decipher calls to clear the blocking Semgrep finding. (2) Get the pre-existing critical vitest advisory green (separate bump + rebase). Re-request review once CI is green on the same HEAD.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: `727f247878801614907a738921115720e9a19427`

Actionable comments posted: 2

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/app/github-app/use-cases/convert-manifest/convert-manifest.use-case.ts`:
- Around line 54-73: The current findOne → conditional persistAndFlush flow on
em.fork() for GitHubApp is vulnerable to a TOCTOU race causing unique constraint
errors; update convert-manifest.use-case.ts to wrap the insert path (the branch
that calls em.persistAndFlush(app)) in a try/catch that catches
UniqueConstraintViolationException (import from `@mikro-orm/core`), and on
catching it re-query the existing GitHubApp via em.findOne({ githubAppId }) and
then em.assign(existing, { slug, name, htmlUrl, ownerLogin, clientId,
clientSecretEnc, webhookSecretEnc, privateKeyPemEnc }) followed by em.flush()
(or alternatively retry the upsert once), so concurrent inserts are handled
defensively instead of erroring the request.

In `@docs/agdr/AgDR-0010-migration-manifest-state-db-backed.md`:
- Line 108: Update the "Commits / PRs" line to remove the placeholder
parenthetical; change the text "Commits / PRs: PR `#64` (filled in as the
migration ships)" to simply "Commits / PRs: PR `#64`" (or replace with the commit
SHA if your docs convention requires SHAs) in
AgDR-0010-migration-manifest-state-db-backed.md so the PR reference is no longer
a placeholder.

---

Nitpick comments:
In
`@apps/api/src/app/github-app/use-cases/convert-manifest/convert-manifest.use-case.ts`:
- Around line 25-27: Remove the manual runtime checks for command.code in the
ConvertManifestUseCase and rely on the existing class-validator decorators on
ConvertManifestCommand to validate inputs; delete the typeof/length guard and
let validation middleware raise BadRequestException. Also keep the
BadGatewayException thrown in the use-case (the error path in the
ConvertManifestUseCase/execute flow) but update the corresponding controller's
OpenAPI decorators to explicitly document that BadGatewayException as a possible
response so the API contract remains complete. Ensure references to
ConvertManifestCommand and the use-case method where the exception is thrown are
updated accordingly.

In `@apps/api/src/app/github-app/use-cases/get-manifest/get-manifest.use-case.ts`:
- Around line 36-43: appName currently transforms webUrl without guarding
against empty or malformed input (e.g., "" or "https://") which yields invalid
or trailing-only names; update the appName(webUrl: string) helper to defensively
validate webUrl after trimming protocol and stripping non-alphanumerics: ensure
the resulting host segment is non-empty, and if it is either throw a clear Error
or return a safe fallback (e.g., throw new Error("invalid webUrl for appName")
or return a deterministic default name), keeping the final truncation and
dash-trimming logic intact so callers like get-manifest.use-case get a
well-formed GitHub App name.