fix(security): P1 sweep - GH-273, GH-257, GH-103, GH-101, GH-100, GH-093, GH-079, GH-078, GH-053, GH-054 - #3576
Draft
dennisofficial wants to merge 9 commits into
Draft
dennisofficial wants to merge 9 commits into
dennisofficial wants to merge 9 commits into
Conversation
GH-273: the streamable HTTP server bound 0.0.0.0 with Access-Control-Allow-Origin: * and no request authentication, so any website the operator visited could drive the local MCP server with the CLI-configured API key, and the LAN could reach it too. - bind to 127.0.0.1 by default (new --host flag to opt out) - drop the wildcard CORS middleware (MCP HTTP clients are not browsers) - reject POST /mcp with a non-localhost Origin header (browsers always send one; header-less local MCP clients keep working with --apikey)
GH-93 / GH-79 / GH-245: portal policy endpoints looked policies and policy versions up by bare id with no organization scoping, letting any authenticated portal user sign policies in other tenants or fetch a presigned URL for another org's policy PDF. - mark-policy-completed: resolve the member within the policy's organization (also fixes the arbitrary-membership pick for multi-org users) - accept-policies: skip policies outside the member's organization - policy-pdf-url: scope the versionId lookup to the policy whose org membership was already validated
GH-101: session replay ran in production with maskAllText: false and blockAllMedia: false while the sentry-mask escape hatch was used in zero of ~1470 components, recording customer compliance data verbatim. Default to masking everything; data-sentry-unmask is now the opt-in for elements provably safe to record.
GH-100: the unsubscribe-token HMAC key silently fell back to the public literal 'fallback-secret' when neither UNSUBSCRIBE_SECRET nor AUTH_SECRET was set, and apps/api (which never sets either) verifies the unauthenticated POST /v1/email/unsubscribe endpoint with it, making every token forgeable from source. Resolve the secret lazily and fail closed: generating a token without a configured secret now throws. Documented UNSUBSCRIBE_SECRET in apps/api/.env.example.
GH-103: findSimilarContent and findSimilarContentBatch interpolated organizationId into the Upstash Vector metadata filter with no escaping, so a value containing a quote could append arbitrary filter clauses (e.g. OR organizationId GLOB "*") and dump every tenant's RAG chunks. Allowlist the prefixed-CUID shape and fail closed before the filter string is built, in both query paths and the sync readiness check. Adds regression tests for the literal exploit payload.
…pload GH-78: POST /v1/questionnaire/parse/upload/token validated the trust access token and ran RAG auto-answering without checking Trust.securityQuestionnaireEnabled, so an org that disabled the AI questionnaire was still fully served by any token holder. Check the flag after token validation and return 403 when disabled; defaults to enabled when no Trust row exists, matching the public overview helper's semantics.
GH-53: UpdateOrganizationDto.logo accepted any string, it was stored verbatim, and GET /v1/organization presigned it as a raw S3 key against the shared org-assets bucket - a cross-tenant read of any known key. Reject logo keys not prefixed with the organization's own id on update, refuse to presign out-of-org keys on read, and skip presigning out-of-org logos in the app layout.
…ions GH-54: evidence form file fields accepted any non-empty fileKey, stored it verbatim, and the submissions read path presigned it against the shared attachments bucket - a cross-tenant read given a leaked key. Validate every submitted fileKey against the caller's organization prefix on create, and skip presigning out-of-org keys when refreshing download URLs on read (defense in depth for legacy rows).
GH-257: GET /api/tasks/[taskId]/status returned any Trigger.dev run's output to any authenticated user with no ownership check, exposing other tenants' AI-generated policy, questionnaire, and vendor content to whoever learned a run id. Tag every user-triggerable tasks.trigger call with the owning organization id and require a matching tag before returning run data; mismatches get the same 404 as a missing run. research-vendor runs are tagged when an active org exists (its output is shared, not tenant data). Adds route tests for the cross-tenant and untagged cases.
|
|
Contributor
Author
|
@cubic-dev-ai review it |
Contributor
@dennisofficial I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 40 files
Confidence score: 2/5
- In
apps/api/src/evidence-forms/evidence-forms.service.ts,exportCsvcan presign a file key from another tenant for legacy or tampered submissions, exposing that tenant’s object; apply the organization-prefix check before signing. - In
apps/app/src/app/(app)/[orgId]/integrations/[slug]/actions/batch-fix.ts, the organization tag trusts caller-controlled input even though the batch uses the authenticated organization, risking incorrect cross-organization tagging; derive the tag from the created batch. - In
apps/app/src/app/api/tasks/[taskId]/status/route.ts, organization membership is enough to read run output, bypassing custom-role permissions; enforce the corresponding API RBAC check.
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="apps/api/src/evidence-forms/evidence-forms.service.ts">
<violation number="1" location="apps/api/src/evidence-forms/evidence-forms.service.ts:251">
P1: `exportCsv` still presigns `rawValue.fileKey` without checking the organization prefix, so legacy or tampered submissions can expose another tenant’s object through CSV. Apply the same prefix check before signing there.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/integrations/[slug]/actions/batch-fix.ts">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/integrations/[slug]/actions/batch-fix.ts:42">
P2: This tag uses caller-controlled `input.organizationId`, while `createBatch` scopes the batch to authenticated `@OrganizationId()`. Derive the tag from the created batch’s organization; otherwise a run can pass another tenant’s status-route ownership check when its ID is shared.</violation>
</file>
<file name="apps/app/src/app/api/tasks/[taskId]/status/route.ts">
<violation number="1" location="apps/app/src/app/api/tasks/[taskId]/status/route.ts:36">
P2: A matching organization tag is the only authorization here, so any organization member with a run ID can read its output even when their custom role lacks the corresponding permission. Add the appropriate API RBAC check before returning run data.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
|
||
| for (const [, fileObj] of this.findFileFieldEntries(data)) { | ||
| const fileKey = fileObj.fileKey as string; | ||
| if (!fileKey.startsWith(orgPrefix)) { |
Contributor
There was a problem hiding this comment.
P1: exportCsv still presigns rawValue.fileKey without checking the organization prefix, so legacy or tampered submissions can expose another tenant’s object through CSV. Apply the same prefix check before signing there.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/evidence-forms/evidence-forms.service.ts, line 251:
<comment>`exportCsv` still presigns `rawValue.fileKey` without checking the organization prefix, so legacy or tampered submissions can expose another tenant’s object through CSV. Apply the same prefix check before signing there.</comment>
<file context>
@@ -214,28 +214,74 @@ export class EvidenceFormsService {
+
+ for (const [, fileObj] of this.findFileFieldEntries(data)) {
+ const fileKey = fileObj.fileKey as string;
+ if (!fileKey.startsWith(orgPrefix)) {
+ throw new BadRequestException(
+ 'Submitted file does not belong to this organization',
</file context>
| organizationId: input.organizationId, | ||
| connectionId: input.connectionId, | ||
| }, | ||
| { tags: [input.organizationId] }, |
Contributor
There was a problem hiding this comment.
P2: This tag uses caller-controlled input.organizationId, while createBatch scopes the batch to authenticated @OrganizationId(). Derive the tag from the created batch’s organization; otherwise a run can pass another tenant’s status-route ownership check when its ID is shared.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/integrations/[slug]/actions/batch-fix.ts, line 42:
<comment>This tag uses caller-controlled `input.organizationId`, while `createBatch` scopes the batch to authenticated `@OrganizationId()`. Derive the tag from the created batch’s organization; otherwise a run can pass another tenant’s status-route ownership check when its ID is shared.</comment>
<file context>
@@ -32,11 +32,15 @@ export async function startBatchFix(
+ organizationId: input.organizationId,
+ connectionId: input.connectionId,
+ },
+ { tags: [input.organizationId] },
+ );
</file context>
| // owning organization's id. A run with no matching tag either belongs to | ||
| // another tenant or was never tagged, so it's not ours to read — return | ||
| // the same 404 as a genuinely missing run to avoid leaking existence. | ||
| if (!run.tags.includes(session.session.activeOrganizationId)) { |
Contributor
There was a problem hiding this comment.
P2: A matching organization tag is the only authorization here, so any organization member with a run ID can read its output even when their custom role lacks the corresponding permission. Add the appropriate API RBAC check before returning run data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/api/tasks/[taskId]/status/route.ts, line 36:
<comment>A matching organization tag is the only authorization here, so any organization member with a run ID can read its output even when their custom role lacks the corresponding permission. Add the appropriate API RBAC check before returning run data.</comment>
<file context>
@@ -29,6 +29,14 @@ export async function GET(
+ // owning organization's id. A run with no matching tag either belongs to
+ // another tenant or was never tagged, so it's not ours to read — return
+ // the same 404 as a genuinely missing run to avoid leaking existence.
+ if (!run.tags.includes(session.session.activeOrganizationId)) {
+ return NextResponse.json({ error: 'Run not found' }, { status: 404 });
+ }
</file context>
This branch has not been 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.
Draft PR fixing 10 confirmed P1 findings from the Glass House sweep (GH-273, GH-257, GH-103, GH-101, GH-100, GH-93, GH-79 + duplicate GH-245, GH-78, GH-53, GH-54). One commit per finding. All were re-validated against
origin/mainHEAD before fixing — none were stale.GH-273 — mcp-server
serve: wildcard CORS + 0.0.0.0 bind + static key fallbackWrong:
servebound 0.0.0.0:2718, setAccess-Control-Allow-Origin: *on every response, and POST /mcp had no auth —buildSDKsilently falls back to the CLI--apikey. Any website the operator visited could drive the local MCP server with their full-privilege key; the LAN could reach it too.Changed: bind 127.0.0.1 by default (new
--hostflag to opt out); wildcard CORS middleware removed (MCP HTTP clients are not browsers); POST /mcp rejects requests with a non-localhostOriginheader (browsers always send one; header-less local MCP clients keep working with--apikey).--disable-static-authsemantics unchanged.Verified:
tsc --noEmitclean in apps/mcp-server (own install — it's outside the bun workspaces).GH-257 — task status route returned any run's output to any authenticated user
Wrong:
GET /api/tasks/[taskId]/statuscalledruns.retrieve(taskId)and returned output/status/error with no ownership check — cross-tenant read of AI-generated policy/questionnaire/vendor content given a run id.Changed: every user-triggerable
tasks.triggercall site in apps/app now tags the run with the owning organizationId; the route requiresrun.tagsto include the caller's active org and returns the same 404 as a missing run otherwise.research-vendor(shared GlobalVendors data, reachable pre-onboarding) is tagged when an active org exists.Verified: new
route.test.ts(5 cases: unauthenticated, no org, cross-tenant 404, untagged 404, own-org 200) passes;tsc --noEmitclean on all 18 touched files.GH-103 — Upstash Vector filter injection via organizationId
Wrong:
findSimilarContent/findSimilarContentBatchbuilt the metadata filter by raw interpolation (organizationId = "${organizationId}"); a value containing"could append arbitrary clauses (OR organizationId GLOB "*") and dump every tenant's RAG chunks. The org id is reachable from theanswer-questiontask payload.Changed: strict allowlist (
/^[a-zA-Z0-9_-]+$/, the prefixed-CUID shape) with fail-closed guard before the filter string is built — in both query paths and the same-pattern sink insync-organization.ts(embedding readiness check). The apps/app copy does client-side post-filtering only and is not vulnerable.Verified: 14/14 jest tests pass in src/vector-store, incl. new tests replaying the literal exploit payload against both functions.
GH-101 — Sentry Session Replay unmasked (maskAllText: false)
Wrong: production replay ran with
maskAllText: false/blockAllMedia: false(10% of sessions, 100% of error sessions) and thesentry-maskescape hatch was used in zero of ~1470 components — verbatim capture of customer compliance data.Changed:
maskAllText: true,blockAllMedia: truein apps/app and apps/portal;data-sentry-unmaskis now the opt-in for provably safe elements.Verified: typecheck clean on both files.
GH-100 — hardcoded 'fallback-secret' HMAC key for unsubscribe tokens
Wrong:
@trycompai/emailderived the unsubscribe-token HMAC key fromUNSUBSCRIBE_SECRET || AUTH_SECRET || 'fallback-secret'; apps/api never sets either and verifies the unauthenticatedPOST /v1/email/unsubscribewith it — every token forgeable from source.Changed: fallback literal removed; the secret resolves lazily and token generation throws when unconfigured (fail closed; importing the package never throws).
UNSUBSCRIBE_SECRETdocumented in apps/api/.env.example. The separateapps/app/src/lib/unsubscribe.tsimplementation was checked — it has no fallback and needs no change.Verified: all call sites are request/task-time, so lazy resolution is safe; packages/email typecheck shows only pre-existing
render.test.tsxerrors.GH-93 — portal policy-signature endpoints write to any org's policies
Wrong:
mark-policy-completedlooked the member up with no org filter and the policy by bare id;accept-policiesverified the memberId but pushed it intosignedByof policies in any organization.Changed:
mark-policy-completedresolves the member within the policy's organization (403 otherwise — also fixes the arbitrary-membership pick for multi-org users);accept-policiesskips policies outside the member's organization.Verified:
tsc --noEmitclean on all touched portal files.GH-79 (+ duplicate GH-245) — portal policy-pdf-url trusts versionId
Wrong: the
versionIdbranch did a barefindUnique({ where: { id: versionId } })and presigned whateverpdfUrlcame back — a 15-minute presigned GET for another org's policy PDF. The main app's sibling implementation has exactly this check and comments why.Changed: version lookup scoped to the already-org-validated
policyId(where: { id: versionId, policyId }); mismatches fall through to the existing "No PDF found." response.Verified: typecheck clean.
GH-78 — trust questionnaire upload ignores securityQuestionnaireEnabled
Wrong:
POST /v1/questionnaire/parse/upload/tokenvalidated the trust access token and ran RAG auto-answering without checkingTrust.securityQuestionnaireEnabled— orgs that disabled the AI questionnaire were still fully served to any token holder.Changed: flag checked after token validation (new
isSecurityQuestionnaireEnabledForOrganizationon TrustAccessService, defaulting to enabled when no Trust row exists, matching the public-overview helper); 403 when disabled. This is the only@Public()token-authenticated endpoint in the controller.Verified: 92/92 jest tests pass in src/questionnaire, incl. 3 new cases (disabled → 403 and
autoAnswerAndExportnever called; enabled → proceeds; no Trust row → proceeds).GH-53 — organization logo is an unvalidated S3 key that gets presigned
Wrong:
UpdateOrganizationDto.logowas a bare string, stored verbatim, andGET /v1/organizationpresigned it as a raw S3 key against the shared org-assets bucket (which also holds trust documents) — cross-tenant read for any org owner who knows a key. The Next.js layout duplicated the unchecked presign.Changed: update rejects logo keys not prefixed with the org's own id (empty/null still clears);
getLogoSignedUrltakes the organizationId and returns null for out-of-org keys; the app layout skips presigning out-of-org logos. Legit keys are${organizationId}/logo/...(written byuploadLogo).Verified: 33/33 jest tests pass in src/organization incl. new cross-org rejection cases; app typecheck clean on the layout.
GH-54 — evidence-form fileKey accepted arbitrary keys, presigned on read
Wrong:
fileKey: z.string().min(1)was stored verbatim in submission data andrefreshFileUrlspresigned whatever was stored against the shared attachments bucket — cross-tenant read given a leaked full key.Changed: submission create rejects any file field whose fileKey doesn't start with
${organizationId}/(legit keys are${organizationId}/attachments/evidence-forms/...);refreshFileUrlstakes the organizationId and skips presigning (downloadUrl: null) for out-of-org keys — defense in depth for legacy rows.Verified: 32/32 jest tests pass in src/evidence-forms, incl. 4 new guard tests.
Needs human action
UNSUBSCRIBE_SECRETin the apps/api production environment (and anywhere@trycompai/emailsends mail). Without it, email sending now throws instead of silently using the public fallback. Treat any secret material previously derivable as compromised: tokens minted underfallback-secretwere forgeable by design, so invalidating them by setting the secret is the intended effect.logodoesn't start with its own id stops rendering a logo (logoUrl → null). Worth a one-off data check (SELECT id, logo FROM organization WHERE logo IS NOT NULL AND logo NOT LIKE id || '/%'); affected rows can re-upload.downloadUrl: nullinstead of a presigned URL.serve/impl.tsandserve/command.tscarry the Speakeasy "DO NOT EDIT" header but are hand-maintained in-repo (prior feature commits edit them too) — a future Speakeasy regeneration could drop this fix; consider upstreaming the hardening into the generation config..env.exampledocumentsUNSUBSCRIBE_SECRET=with no value — a real secret must be generated and added to the deployed env out of band.Verification summary
bunx jest— src/vector-store 14/14, src/organization 33/33, src/evidence-forms 32/32, src/questionnaire 92/92, all passing;tsc --noEmitreports zero errors in touched files (remaining failures are pre-existing errors in unrelated spec files, identical at HEAD).bunx vitest runnew status-route test 5/5;tsc --noEmitclean on all touched files.tsc --noEmitclean on touched files.tsc --noEmitclean (workspace-excluded package, ownbun install).render.test.tsxerrors (confirmed identical with the change stashed).Do not merge — draft for review per the sweep protocol.
Summary by cubic
Fixes 10 confirmed P1 security findings from the Glass House sweep across
apps/api,apps/app,apps/portal,apps/mcp-server, andpackages/email.Closes a cross-tenant read path (GH-257) by tagging every user-triggerable task run with its owning organization and requiring a matching tag on the status route. Closes S3 key validation gaps (GH-53, GH-54) so logo keys, evidence-form fileKeys, and presigned download URLs are scoped to the caller's organization. Closes Upstash Vector filter injection via
organizationId(GH-103) with a strict allowlist. Closes the portal policy endpoints (GH-93, GH-79) by scoping lookups to the member's organization, and honorssecurityQuestionnaireEnabledon token-authenticated uploads (GH-78). Closes local MCP server exposure (GH-273) by binding to127.0.0.1by default and rejecting non-localhostOriginheaders. Removes the hardcodedfallback-secretHMAC key for unsubscribe tokens (GH-100) and masks all text/media in Sentry Session Replay (GH-101).Migration
UNSUBSCRIBE_SECRETin theapps/apiproduction environment before deploy, otherwise email sending throws instead of silently using the public fallback. Tokens minted under the fallback secret were forgeable; resetting the secret invalidates them.logonot prefixed by their own id stop rendering a logo, and legacy evidence submissions holding out-of-org fileKeys renderdownloadUrl: null. Existing replays in Sentry contain unmasked customer data — decide whether to purge replay history.Written for commit e2f27d6. Summary will update on new commits.