fix(auth): attribute API-key mutations to the key's creator, not the org owner - #3472
Merged
Merged
Conversation
…org owner API keys were org-scoped with no recorded creator, so ActingUserResolver attributed every API-key/MCP mutation to the org's oldest owner. In the audit trail this made all automation look like the owner performed it, masking the real actor — a problem for a compliance product. - Add ApiKey.createdByMemberId (nullable, FK to Member, onDelete: SetNull). Populated on creation with the acting member. Nullable so legacy keys and keys whose creator was removed fall back cleanly. - create-api-key now records the creating member; validateApiKey surfaces it. - HybridAuthGuard puts it on the request; ActingUserResolver attributes the mutation to the creator (when still an active member of the org), else falls back to the org owner as before. - Tests for both the creator-attribution path and the deactivated-creator fallback. No backfill: existing keys have no recorded creator and keep falling back to the org owner. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
The legacy-key branch's select was missing createdByMemberId (its deeper indentation meant the earlier bulk edit didn't cover it), so the return that references legacyMatch.createdByMemberId failed to typecheck. Add the field to the legacy select to match the primary path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed against the latest diff
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The global AuditLogInterceptor early-returned when request.userId was absent, so API-key (and MCP) mutations were never audit-logged at all — the createdByMemberId attribution added earlier had no effect on the trail. Inject ActingUserResolver and resolve the responsible user (key creator, else org owner) when there's no session userId; skip logging only when no user can be attributed (no null-FK rows). Also fixes a pre-existing bug surfaced once the spec could load: the control mapping/unmapping descriptions read "policie" because the resolver naive-stripped the trailing "s" of "policies". Use the known Prisma model name (policies→policy) with an "ies"→"y" fallback. The interceptor spec never ran before (it pulled better-auth's ESM subpaths via permission.guard); mock @trycompai/auth like the other specs so all 41 tests execute. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…nt sinks
The interceptor fix covered automatic @RequirePermission audit logging. This
covers the remaining sinks where an API-key mutation succeeded but attribution
was silently lost or credited to the org owner instead of the responsible user:
- vendors create + triggerAssessment: the controller now resolves the acting
user and threads it as createdByUserId, so the auto-generated risk-assessment
task ("created this task") credits the key creator, not the admin fallback.
- cloud-security scan: attribute scan_completed via ActingUserResolver instead
of raw req.userId (was skipped entirely for API keys).
- policies publish-all: per-policy audit rows were dropped for API-key auth
(authContext.userId undefined) — resolve the actor first.
- isms createRow: enteredById (a Member FK) was null for API keys — resolve the
acting member.
ActingUserResolver now populates memberId on every path (session member, key
creator, or fallback owner's member), so Member-FK sinks like isms enteredById
attribute correctly. Owner lookup selects the member id alongside the user id.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- vendors create/triggerAssessment: 400 when no actor resolves (org has no owner) instead of creating a vendor whose assessment task has no attributed user — matches ActingUserResolver's contract. - cloud-security scan: attribution is best-effort (try/catch) so a transient resolver/audit failure can't fail an already-completed scan and invite a re-run. - isms bulkCreateMeasurements: resolve the acting member (session-first, then api-key creator/owner) so bulk saves via API key don't persist null enteredById. - hybrid-auth guard: service-token x-user-id now sets request.memberId so Member-FK sinks can attribute service-token-acting mutations. - tests: createApiKey creator attribution (session forwards memberId, api-key forwards null), vendors 400-on-null, isms bulk api-key attribution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Cubic follow-up: the x-user-id member lookup didn't filter deactivated / inactive memberships, so an offboarded user supplied via x-user-id could receive new audit / enteredById attribution. Add deactivated:false + isActive:true to the lookup (matching ActingUserResolver's filters); an inactive member now resolves to no acting user and falls back to owner resolution downstream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Contributor
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".husky/pre-push">
<violation number="1">
P2: The new hook tells developers to bypass all client-side checks with `--no-verify`, contradicting repository policy and undermining the stated security gate. Remove the emergency-bypass guidance; retain the explicit review attestation path instead.</violation>
<violation number="2">
P1: Security-sensitive committed changes pass unchecked when `origin/main` is unavailable, because fallback `git diff HEAD` only inspects uncommitted worktree changes. Fail closed when no merge base can be resolved (or derive a real committed baseline) so this gate cannot silently become a no-op.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The agent hook fired on non-git-push commands and blocked them (its allow/block semantics were inverted and the `if` filter didn't scope). The PR GitHub Action is the reliable auto-run gate; drop the local hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Set run-every-commit so the security Action re-reviews the latest diff on each push to a PR. Without it the action runs once per PR and skips later commits, leaving code added after the first review unchecked while the required check stays green. Findings remain advisory PR comments (no merge block on noise). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…or-attribution # Conflicts: # packages/docs/openapi.json
Regenerated from the merged code so the spec carries #3469's CreatePolicyVersionDto rename plus the current ISMS-audit schema (main's committed spec had drifted). Written as-generated by the dev boot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
claudfuen
pushed a commit
that referenced
this pull request
Jul 22, 2026
# [3.106.0](v3.105.0...v3.106.0) (2026-07-22) ### Bug Fixes * **auth:** attribute API-key mutations to the key's creator, not the org owner ([#3472](#3472)) ([206ed96](206ed96)), closes [hi#risk](https://github.com/hi/issues/risk) * **deps:** bump adm-zip 0.5.18 -> 0.6.0 in apps/api (Dependabot [#88](https://github.com/trycompai/comp/issues/88)/[#89](https://github.com/trycompai/comp/issues/89)) ([#3462](#3462)) ([300f2a1](300f2a1)), closes [#3451](#3451) * **deps:** override tar to ^7.5.19 to clear node-tar Dependabot alerts ([#94](https://github.com/trycompai/comp/issues/94)-[#104](https://github.com/trycompai/comp/issues/104)) ([#3466](#3466)) ([8ab5709](8ab5709)) * **deps:** patch engine.io ([#93](#93)) and body-parser ([#92](#92)) Dependabot alerts ([#3464](#3464)) ([94c33b1](94c33b1)) * **isms:** harden internal-audit validation and edge cases from deploy review ([#3473](#3473)) ([c6c7379](c6c7379)) * **policies:** create draft version on policy regenerate instead of overwriting published ([#3471](#3471)) ([ff31dbd](ff31dbd)) * **policies:** delete detached PDF objects when regenerating a draft ([#3474](#3474)) ([ecd1bd0](ecd1bd0)) * **policies:** rename CreateVersionDto to avoid swagger collision with automations ([#3469](#3469)) ([2d5290a](2d5290a)) ### Features * **isms:** internal audit programme, plan and report — clause 9.2 (CS-724) ([#3468](#3468)) ([42e5ebd](42e5ebd)), closes [hi#impact](https://github.com/hi/issues/impact)
Contributor
|
🎉 This PR is included in version 3.106.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This branch was successfully deployed
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
2 participants
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.
Problem
API keys are org-scoped and record no creator, so
ActingUserResolverattributes every API-key / MCP / automation mutation to the org's oldest owner. In the audit trail this makes all automation look like the owner performed it, masking the real actor — a meaningful gap for a compliance product (an auditor sees "one person did everything").Fix
Record who created each key (as a Member, since keys are org-scoped) and attribute mutations to that member.
ApiKey.createdByMemberId— nullable, FK →Member,onDelete: SetNull(matches theFindingCreatedByconvention). Nullable so legacy keys, and keys whose creator was removed, fall back cleanly. Reverse relationMember.createdApiKeys. Migration:20260721193000_add_api_key_creator.api-key.service.ts:create()stores the creating member;validateApiKey()selects + returns it (both the primary and legacy-key paths);ApiKeyValidationResultgainscreatedByMemberId.organization.controller.ts: the create endpoint passes the session member's id (authContext.memberId); null when a key is created via API key/service token.hybrid-auth.guard.ts+types.ts: carryapiKeyCreatedByMemberIdon the request.acting-user.service.ts: new rule — attribute to the key's creator if they're still an active member of the org; otherwise fall back to the org owner exactly as before.Behavior
x-user-idNo backfill
Existing keys have no recorded creator, so they keep attributing to the org owner. Recreating a key after this ships records the new creator.
Tests
acting-user.service.spec.ts: +2 cases (creator attribution; deactivated-creator fallback). 13/13 pass..spec.tserrors turbo reports also fail onmain).Deploy note
Additive migration (nullable column + FK + index) — no data change, no downtime.
Summary by cubic
Attributes API‑key mutations to the key’s creator and ensures they’re fully audit‑logged with clear provenance. Adds a PR security‑review GitHub Action that re‑reviews every commit; removes the misfiring PreToolUse hook and the Husky pre‑push block.
Bug Fixes
ApiKey.createdByMemberId(nullable FK →Member);createApiKeystores the session member (else null) andvalidateApiKeyreturns it (primary + legacy).HybridAuthGuard: forwardsapiKeyCreatedByMemberId; restricts service‑tokenx-user-idto active members and setsrequest.memberIdfor acting; preserves API‑key org scoping.ActingUserResolver: attributes to the key’s creator when active, else falls back to the oldest active owner; returnsuserId,memberId, and acallerLabel.AuditLogInterceptor: resolves the actor for non‑session requests so API‑key/MCP mutations are logged; appends provenance in the description andviain audit JSON; skips only when no user can be resolved; fixes pluralization in control‑mapping messages.createRowandbulkCreateMeasurements(session‑first; else resolvedmemberIdforenteredById), and cloud‑security scan completion (best‑effort attribution with provenance label).Security ReviewGitHub Action with repo‑specific guidance that re‑reviews the diff on every commit; removed the misfiring.claudePreToolUse hook and the.husky/pre-pushblock.packages/docs/openapi.jsonto match current code (ISMS audit schema updates and recent DTO renames).Migration
20260721193000_add_api_key_creator(additive). Existing keys keep owner fallback until recreated.Written for commit 3096f90. Summary will update on new commits.