From d2ba4cb83373eadfa5903b700802bc72ae9d2dbb Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 10:27:58 +0000 Subject: [PATCH] =?UTF-8?q?fix(app-registry):=20apps.organization=5Fid=20i?= =?UTF-8?q?s=20NOT=20NULL=20=E2=80=94=20no=20more=20org-less=20rows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Owner ruling 2026-08-25 (portal-registration-gap investigation): "I think there should be no app without orgid. at the minimum fuzefront itself the root org is the orgid for our own apps. like google owns docs, sheets, etc." Root cause of the visibility-query disagreement this investigation set out to reconcile: every first-party FuzeFront product ended up with `organization_id IS NULL` (registerBuiltin's hardcoded `null`, and the app-registry's POST /apps route defaulting an omitted organizationId to `null` even for platform-admin callers), and the production visibility query (service.ts list()) carried an `organization_id IS NULL` branch to keep those rows visible at all — a branch that, as a side effect, made ANY org-less row visible to EVERY caller regardless of its declared visibility. The legacy query (routes/apps.ts scopeAppsQuery) lacked that branch AND didn't check visibility on its org-membership branch, so the two disagreed in both directions. Closing the null-organization_id hole makes both questions moot together, per the owner's framing: it isn't a branch to reconcile, it's a state that should not exist. Sequenced so nothing currently visible silently disappears: 1. Migrations backfill every existing `organization_id IS NULL` row to the platform root org (ROOT_ORG_ID, seeded by backend/src's 015_seed_root_platform_organization.ts), then set the column NOT NULL with that root org as its DEFAULT. Added to BOTH migration trees against the shared `apps` table (backend/applications/src/migrations/011 and backend/src/migrations/026 — this table's schema has historically been duplicated across both trees, e.g. each has its own *_update_apps_for_organizations.ts), each idempotent so whichever runs first does the real work. 2. Registration paths not touching a client-chosen id (identifier-standard §1 — organizationId here is a reference, not identity) now default to ROOT_ORG_ID instead of null: - app-registry/service.ts upsertBuiltin() (fuzeagent, fuzesocial, clock, fuzequality, and any future BUILTIN_MANIFESTS entry) - routes/app-registry.ts's POST /apps: a platform-admin caller (every register.sh service-account token) omitting organizationId now gets ROOT_ORG_ID, not null. Non-admins omitting it are still rejected (403, unchanged). - routes/apps.ts's org-scoped POST /:organizationId/apps: a caller whose verified context carries no org now falls back to ROOT_ORG_ID instead of writing an explicit NULL. 3. Only then do the queries drop the org-less branch: - app-registry/service.ts list(): removed `.orWhereNull('organization_id')` — that branch is now simply "visibility IN (public, marketplace)". - routes/apps.ts scopeAppsQuery(): removed the null-org branch it never should have needed, and added the visibility check its org-membership branch was missing, so it now agrees with production instead of disagreeing in the opposite direction. - canRead() in service.ts flips its (now unreachable, constraint-guarded) `!app.organizationId` branch from fail-open (`return true`, "platform-global -> readable") to fail-closed (`return false`) — a defensive backstop in case the invariant is ever violated, not a behavior change for any row that can actually exist post-migration. Tests: backend/tests/apps.test.ts adds coverage that (a) an 'organization'-visibility app is visible to a member of its org and excluded for a non-member (BOLA), (b) inserting `organization_id: null` directly against the DB is REJECTED by the new constraint (asserts the constraint itself, not just app-level behavior — "passes on current data" is not evidence a migration did anything), and (c) omitting organization_id on insert falls back to the column DEFAULT (the root org), not NULL. backend/applications/tests/app-registry.unit.test.ts's "platform-global (org-less) apps are readable" case is inverted to match the new fail-closed canRead(). backend/applications/tests/ portal-catalog.integration.test.ts's three org-less fixture inserts are repointed to ROOT_ORG_ID so they do not violate the new constraint (they were already visibility:'public', so this is a data-shape fix, not a behavior change for what they test). Still open / explicitly out of scope for this change: `canRead`/`canMutate` elsewhere in service.ts and the legacy `backend/applications/src/routes/ apps.ts` GET / handler (a THIRD, unrelated implementation found during this investigation, which lists ALL active apps with NO org/visibility scoping at all — a real BOLA-shaped bug, but for a different, older "arbitrary MFE app installation" feature, not the app-registry catalog the portal menu reads from; flagging for a separate fix rather than widening this change's blast radius). Which of the two originally-named implementations production actually serves: confirmed by tracing the frontend's actual call path (frontend/src/platform/appRegistry.tsx -> APP_REGISTRY_BASE_URL -> /api/v1/app-registry/apps) and backend/src/index.ts's own comment that the ingress routes `/api/apps` to fuzefront-applications, never to this backend's own `routes/apps.ts` mount. service.ts IS what backs the portal menu in prod; routes/apps.ts is bypassed in a real K8s deployment but still serves `/api/apps` in local dev (docker-compose has no such ingress split) and is covered by its own integration tests, so it is not dead code — fixed rather than deleted, per instruction to verify before deleting anything. Co-Authored-By: Claude --- .../applications/src/app-registry/service.ts | 61 +++++-- .../011_apps_organization_id_not_null.ts | 88 ++++++++++ .../applications/src/routes/app-registry.ts | 15 +- .../tests/app-registry.unit.test.ts | 11 +- .../tests/portal-catalog.integration.test.ts | 6 +- .../026_apps_organization_id_not_null.ts | 64 +++++++ backend/src/routes/apps.ts | 52 +++++- backend/tests/apps.test.ts | 160 +++++++++++++++++- 8 files changed, 421 insertions(+), 36 deletions(-) create mode 100644 backend/applications/src/migrations/011_apps_organization_id_not_null.ts create mode 100644 backend/src/migrations/026_apps_organization_id_not_null.ts diff --git a/backend/applications/src/app-registry/service.ts b/backend/applications/src/app-registry/service.ts index c00965783..335b51757 100644 --- a/backend/applications/src/app-registry/service.ts +++ b/backend/applications/src/app-registry/service.ts @@ -13,6 +13,24 @@ import { navSectionRank, } from './manifest.schema' +// Owner ruling 2026-08-19/2026-08-25: "there should be no app without orgid. +// at the minimum fuzefront itself the root org is the orgid for our own +// apps. like google owns docs, sheets, etc." `organization_id` on `apps` is +// NEVER null post-migration (see backend/applications/src/migrations — +// backfill + NOT NULL) — a first-party FuzeFront product (registered by a +// platform-admin service account, or a BUILTIN_MANIFESTS entry) is owned by +// the platform root org, not left org-less. +// +// This is the applications-service's own copy of the SAME fixed id +// backend/src/migrations/015_seed_root_platform_organization.ts seeds and +// exports as ROOT_ORG_ID — cross-service TS imports don't resolve (separate +// deployables), so every service that needs it re-declares the literal +// (same pattern as backend/applications/tests/portal-catalog.integration.test.ts +// and backend/security/src/migrations/014_seed_root_platform_organization.ts). +// Changing this value without a coordinated migration across every service +// orphans every row already keyed to it. +export const ROOT_ORG_ID = '00000000-0000-0000-0000-000000000010' + /** * Derived side-menu ordering columns for a manifest. `manifest.nav` is the source * of truth; these columns exist only so the list query can ORDER BY / keyset-paginate @@ -76,13 +94,17 @@ function rowToApp(row: any): AppRecord { * - visibility is public|marketplace (everyone), OR * - visibility is organization AND the app's org is one the caller belongs to, OR * - visibility is private AND the app's org is one the caller belongs to (owner org). - * An org-less (platform-global) app is treated as public for read. + * `organization_id` is NOT NULL on `apps` (see ROOT_ORG_ID's doc comment + * above) — the `!app.organizationId` branch below is unreachable defensive + * code, kept rather than deleted so a future regression on the DB + * constraint fails safe (deny) here instead of this predicate silently + * assuming a shape the schema no longer allows. */ export function canRead(app: AppRecord, caller: AppCaller): boolean { if (caller.isPlatformAdmin) return true const visibility = app.manifest.visibility ?? 'private' if (visibility === 'public' || visibility === 'marketplace') return true - if (!app.organizationId) return true // platform-global → readable + if (!app.organizationId) return false // should be unreachable; fail closed, not open const inOrg = caller.organizationIds.includes(app.organizationId) if (visibility === 'organization' || visibility === 'private') return inOrg return false @@ -221,22 +243,24 @@ export class AppRegistryService { const applyPortalGate = portalCtx?.mode === 'scoped' query = query.where(builder => { - builder.where(orgLessOrPublic => { - // IMPORTANT: the OR-pair below MUST be its own explicit group - // (nested `.where(w => ...)`), not top-level calls on - // `orgLessOrPublic` directly — SQL's `AND` binds tighter than - // `OR`, so `visibility IN (...) OR organization_id IS NULL AND - // EXISTS(...)` parses as `visibility IN (...) OR (organization_id - // IS NULL AND EXISTS(...))`, which lets ANY `public`/`marketplace` - // app bypass the portal gate entirely regardless of EXISTS — the - // exact leak this filter exists to close. Caught by the S2 - // no-leak integration test. - orgLessOrPublic.where(w => { - w.whereIn('visibility', ['public', 'marketplace']).orWhereNull('organization_id') - }) + // `organization_id IS NULL` was a THIRD branch here (public/marketplace + // visibility OR org-less), removed by owner ruling 2026-08-25: "there + // should be no app without orgid ... fuzefront itself [is] the orgid + // for our own apps" (see ROOT_ORG_ID's doc comment above). It was a + // latent hole, not a feature — it made ANY row with no owning org + // visible to every caller regardless of `visibility`, and every + // first-party FuzeFront product ended up in exactly that state + // (registered with no organizationId). `organization_id` is NOT NULL + // on this table as of the migration backfilling those rows to + // ROOT_ORG_ID (backend/applications/src/migrations), so this branch + // is now simply "public/marketplace visibility" — nothing was ever + // org-less on purpose, so nothing is lost by requiring an org match + // for everything else. + builder.where(publicOrMarketplace => { + publicOrMarketplace.whereIn('visibility', ['public', 'marketplace']) if (applyPortalGate) { const portalId = portalCtx!.portalId as string - orgLessOrPublic.whereExists(function (this: any) { + publicOrMarketplace.whereExists(function (this: any) { this.select(1) .from('portal_apps') .whereRaw('portal_apps.app_id = apps.id') @@ -498,7 +522,10 @@ export class AppRegistryService { status, mode: manifest.mode, builtin: true, - organization_id: null, + // ROOT_ORG_ID, not null (owner ruling — see ROOT_ORG_ID's own doc + // comment above): a builtin is a first-party FuzeFront product like + // any other, owned by the platform root org rather than org-less. + organization_id: ROOT_ORG_ID, visibility: (manifest.visibility ?? 'public') as Visibility, is_active: status === 'activated', heartbeat_token: heartbeatToken, diff --git a/backend/applications/src/migrations/011_apps_organization_id_not_null.ts b/backend/applications/src/migrations/011_apps_organization_id_not_null.ts new file mode 100644 index 000000000..31800f419 --- /dev/null +++ b/backend/applications/src/migrations/011_apps_organization_id_not_null.ts @@ -0,0 +1,88 @@ +import { Knex } from 'knex' + +// applications-service migration 011. +// +// Owner ruling 2026-08-25 (portal-registration-gap investigation): "I think +// there should be no app without orgid. at the minimum fuzefront itself the +// root org is the orgid for our own apps. like google owns docs, sheets, +// etc." An org-less `apps` row was never a deliberate feature — every +// first-party FuzeFront product ended up in that state simply because +// nothing ever set `organization_id` for it (registerBuiltin() hardcoded +// `null`; the app-registry's POST /apps route defaulted an omitted +// organizationId straight to `null` even for platform-admin callers). The +// visibility query then had to carry an `organization_id IS NULL` branch to +// keep those apps visible at all — a branch that, as a side effect, made +// ANY org-less row visible to EVERY caller regardless of its declared +// `visibility`. See backend/applications/src/app-registry/service.ts's +// `list()` for the query-side half of this fix, which REMOVES that branch; +// it must not run until this migration has guaranteed no row can be null, +// or apps that currently appear would silently disappear. +// +// This migration: backfill every existing org-less row to the platform root +// org, then make `organization_id` NOT NULL (with that root org as the +// column DEFAULT, so an insert path that forgets to set it explicitly lands +// on the root org instead of reintroducing the hole). The NOT NULL +// constraint is the part that makes this durable — a backfill alone is a +// point-in-time fix the next registration can silently undo. +// +// ROOT_ORG_ID is this service's own copy of the fixed id +// backend/src/migrations/015_seed_root_platform_organization.ts seeds and +// exports. Cross-service TS imports don't resolve (separate deployables), +// so every service that needs it re-declares the literal — same pattern as +// backend/applications/src/app-registry/service.ts's own copy (kept +// independent of that one deliberately: a migration must stay replayable on +// its own even if application code changes around it later) and +// backend/applications/tests/portal-catalog.integration.test.ts. +const ROOT_ORG_ID = '00000000-0000-0000-0000-000000000010' + +export async function up(knex: Knex): Promise { + const hasColumn = await knex.schema.hasColumn('apps', 'organization_id') + if (!hasColumn) { + // Never observed in practice — migration 002_update_apps_for_organizations + // always adds this column first — but fail loudly rather than silently + // no-op if a partially-migrated database somehow lacks it. + throw new Error( + 'apps.organization_id does not exist — 002_update_apps_for_organizations must run first' + ) + } + + // The root org itself must exist before anything can reference it or be + // backfilled to it. It is seeded by a DIFFERENT service's migration tree + // (backend/src's 015_seed_root_platform_organization) against the SAME + // shared `organizations`/`apps` tables. If this service's migrations ever + // run in an environment where that one has not, backfilling to a + // still-absent id would violate apps_organization_id_foreign — fail loudly + // with a clear cause instead of a mysterious later FK violation. + const root = await knex('organizations').where({ id: ROOT_ORG_ID }).first() + if (!root) { + throw new Error( + `organizations.${ROOT_ORG_ID} (the platform root org) does not exist yet — ` + + 'backend/src migration 015_seed_root_platform_organization must run before this one.' + ) + } + + const backfilled = await knex('apps') + .whereNull('organization_id') + .update({ organization_id: ROOT_ORG_ID }) + if (backfilled > 0) { + // eslint-disable-next-line no-console + console.log( + `[011] backfilled ${backfilled} org-less app(s) to the platform root org ${ROOT_ORG_ID}` + ) + } + + // SET DEFAULT / SET NOT NULL on an already-conforming column is a no-op in + // Postgres, not an error — safe to run every boot, exactly like every + // other migration in this tree. + await knex.raw(`ALTER TABLE apps ALTER COLUMN organization_id SET DEFAULT '${ROOT_ORG_ID}'::uuid`) + await knex.raw('ALTER TABLE apps ALTER COLUMN organization_id SET NOT NULL') +} + +export async function down(knex: Knex): Promise { + await knex.raw('ALTER TABLE apps ALTER COLUMN organization_id DROP NOT NULL') + await knex.raw('ALTER TABLE apps ALTER COLUMN organization_id DROP DEFAULT') + // Deliberately does NOT un-backfill rows back to NULL — that would + // resurrect the exact hole this migration exists to close, and there is no + // way to tell which rows were genuinely org-less before vs. root-owned by + // this migration. +} diff --git a/backend/applications/src/routes/app-registry.ts b/backend/applications/src/routes/app-registry.ts index 2972c1e55..c10c64442 100644 --- a/backend/applications/src/routes/app-registry.ts +++ b/backend/applications/src/routes/app-registry.ts @@ -20,7 +20,7 @@ import { billingProfileSchema, toValidationErrorBody, } from '../app-registry/manifest.schema' -import { appRegistryService, canRead, canMutate } from '../app-registry/service' +import { appRegistryService, canRead, canMutate, ROOT_ORG_ID } from '../app-registry/service' import { resolveCaller } from '../app-registry/caller' import { checkAppRegistryPermission } from '../app-registry/permit' import { getAppRegistryEmitter } from '../app-registry/events' @@ -124,9 +124,18 @@ router.post('/apps', authenticateConsumerOrSession, async (req: any, res) => { return res.status(400).json(toValidationErrorBody((parsed as any).error)) } const { manifest, organizationId } = parsed.data - const orgId = organizationId ?? null - const caller = await resolveCaller(req.user) + // organization_id is NEVER null on `apps` (owner ruling 2026-08-25 — see + // ROOT_ORG_ID's doc comment in ../app-registry/service.ts). A platform + // admin (register.sh's service-account token, i.e. every first-party + // FuzeFront product self-registering) that omits organizationId is + // registering a first-party app and is attributed to the platform root + // org, never left org-less — this was the actual mechanism that put + // every builtin/self-registered product into the `organization_id IS + // NULL` state the visibility query used to special-case. A non-admin + // caller omitting it is still rejected below; they must name a real org + // they belong to. + const orgId = organizationId ?? (caller.isPlatformAdmin ? ROOT_ORG_ID : null) // release flag (default OFF): the new write surface is dark until released. if (!(await v1WriteGate(caller, orgId, res))) return diff --git a/backend/applications/tests/app-registry.unit.test.ts b/backend/applications/tests/app-registry.unit.test.ts index fd456aaff..b99e2bf5e 100644 --- a/backend/applications/tests/app-registry.unit.test.ts +++ b/backend/applications/tests/app-registry.unit.test.ts @@ -74,8 +74,15 @@ describe('app-registry BOLA visibility (canRead)', () => { expect(canRead(appWith({ visibility: 'private', organizationId: 'org-z' }), platformAdmin)).toBe(true) }) - it('platform-global (org-less) apps are readable', () => { - expect(canRead(appWith({ visibility: 'private', organizationId: null }), memberOfOrgA)).toBe(true) + // Owner ruling 2026-08-25: "there should be no app without orgid ... + // fuzefront itself [is] the orgid for our own apps" — `organization_id` + // is NOT NULL on `apps` (migration 011_apps_organization_id_not_null.ts), + // so this state should be unreachable. It used to be treated as "readable + // by anyone" (a first-party app landed here because nothing ever set its + // org, not by design); canRead now fails CLOSED for it instead, so a + // future regression on the DB constraint denies rather than leaks. + it('an org-less app (should be unreachable — see the NOT NULL constraint) is denied, not universally readable', () => { + expect(canRead(appWith({ visibility: 'private', organizationId: null }), memberOfOrgA)).toBe(false) }) }) diff --git a/backend/applications/tests/portal-catalog.integration.test.ts b/backend/applications/tests/portal-catalog.integration.test.ts index a8df6b4bb..987127ffe 100644 --- a/backend/applications/tests/portal-catalog.integration.test.ts +++ b/backend/applications/tests/portal-catalog.integration.test.ts @@ -115,7 +115,7 @@ async function seedApps(): Promise { status: 'activated', mode: 'portal', builtin: false, - organization_id: null, + organization_id: ROOT_ORG_ID, // NOT NULL (011_apps_organization_id_not_null.ts) — a public, org-less-in-spirit catalog fixture is now owned by the platform root org like any other first-party app. visibility: 'public', manifest: JSON.stringify({ manifestVersion: '1', @@ -283,7 +283,7 @@ describe('FF-EPIC-12-S1 — PortalAppCatalogService (real Postgres)', () => { status: 'activated', mode: 'portal', builtin: false, - organization_id: null, + organization_id: ROOT_ORG_ID, // NOT NULL (011_apps_organization_id_not_null.ts) — a public, org-less-in-spirit catalog fixture is now owned by the platform root org like any other first-party app. visibility: 'public', manifest: JSON.stringify({ manifestVersion: '1', @@ -670,7 +670,7 @@ describe('FF-EPIC-12-S3 — portal catalog admin routes', () => { status: 'activated', mode: 'portal', builtin: false, - organization_id: null, + organization_id: ROOT_ORG_ID, // NOT NULL (011_apps_organization_id_not_null.ts) — a public, org-less-in-spirit catalog fixture is now owned by the platform root org like any other first-party app. visibility: 'public', manifest: JSON.stringify({ manifestVersion: '1', diff --git a/backend/src/migrations/026_apps_organization_id_not_null.ts b/backend/src/migrations/026_apps_organization_id_not_null.ts new file mode 100644 index 000000000..857e76fc6 --- /dev/null +++ b/backend/src/migrations/026_apps_organization_id_not_null.ts @@ -0,0 +1,64 @@ +import { Knex } from 'knex' +import { ROOT_ORG_ID } from './015_seed_root_platform_organization' + +// Owner ruling 2026-08-25 (portal-registration-gap investigation): "I think +// there should be no app without orgid. at the minimum fuzefront itself the +// root org is the orgid for our own apps. like google owns docs, sheets, +// etc." An org-less `apps` row was never a deliberate feature — see +// backend/applications/src/migrations/011_apps_organization_id_not_null.ts, +// this migration's sibling in the applications-service's OWN migration tree +// against the SAME shared `apps` table (this repo's schema for that table +// has historically been duplicated across both trees — see e.g. this +// service's 006_update_apps_for_organizations.ts and the applications +// service's 002_update_apps_for_organizations.ts, which apply the identical +// DDL). Kept in lock-step here for the same reason: whichever service's +// migrations happen to run first against a given database does the real +// work, and the other is a no-op — SET DEFAULT / SET NOT NULL on an +// already-conforming column raises nothing. +// +// `backend/src/routes/apps.ts`'s `scopeAppsQuery` is reconciled to match the +// production app-registry rule in the SAME change that adds this migration +// — see that file's own comment for the query-side half. + +export async function up(knex: Knex): Promise { + const hasColumn = await knex.schema.hasColumn('apps', 'organization_id') + if (!hasColumn) { + throw new Error( + 'apps.organization_id does not exist — 006_update_apps_for_organizations must run first' + ) + } + + // This service is the one that SEEDS the root org (migration 015, in this + // same tree) — unlike the applications-service's copy of this migration, + // there is no cross-service ordering hazard to guard against here, but the + // assertion stays for the same reason: fail loudly with a clear cause + // instead of a bare FK-violation stack trace if that invariant is ever + // broken by a future change. + const root = await knex('organizations').where({ id: ROOT_ORG_ID }).first() + if (!root) { + throw new Error( + `organizations.${ROOT_ORG_ID} (the platform root org) does not exist yet — ` + + '015_seed_root_platform_organization must run before this one.' + ) + } + + const backfilled = await knex('apps') + .whereNull('organization_id') + .update({ organization_id: ROOT_ORG_ID }) + if (backfilled > 0) { + // eslint-disable-next-line no-console + console.log( + `[026] backfilled ${backfilled} org-less app(s) to the platform root org ${ROOT_ORG_ID}` + ) + } + + await knex.raw(`ALTER TABLE apps ALTER COLUMN organization_id SET DEFAULT '${ROOT_ORG_ID}'::uuid`) + await knex.raw('ALTER TABLE apps ALTER COLUMN organization_id SET NOT NULL') +} + +export async function down(knex: Knex): Promise { + await knex.raw('ALTER TABLE apps ALTER COLUMN organization_id DROP NOT NULL') + await knex.raw('ALTER TABLE apps ALTER COLUMN organization_id DROP DEFAULT') + // Deliberately does NOT un-backfill rows back to NULL — see the sibling + // applications-service migration's down() for why. +} diff --git a/backend/src/routes/apps.ts b/backend/src/routes/apps.ts index fbd220d58..778e4276d 100644 --- a/backend/src/routes/apps.ts +++ b/backend/src/routes/apps.ts @@ -6,6 +6,7 @@ import { requireAppPermission } from '../middleware/permissions' import { App } from '../types/shared' import { isPrefixedIdsEnabled } from '../identity/flags' import { prefixDtoIds } from '../identity/serializer' +import { ROOT_ORG_ID } from '../migrations/015_seed_root_platform_organization' const router = express.Router() @@ -172,16 +173,50 @@ async function getMemberOrgIds(userId: string): Promise { /** * Apply org/visibility scoping to an apps query for the given user. + * + * MUST agree with the production rule in + * backend/applications/src/app-registry/service.ts `list()` (~line 215-270) + * — this route is bypassed by the ingress in a real K8s deployment (see the + * NOTE above the `/api/apps` mount in ../index.ts: the ingress routes + * `/api/apps` to fuzefront-applications, not this service), but it IS what + * actually serves `/api/apps` locally (docker-compose has no such ingress + * split) and is covered by this file's own integration tests, so it is not + * dead code — just not what a real deploy's traffic reaches. Before this fix + * the two disagreed, in both directions: + * - production had an `organization_id IS NULL` branch this function + * lacked (org-less rows were visible in production, hidden here) — REMOVED + * from both as of the migration below, per owner ruling 2026-08-25 + * ("there should be no app without orgid ... fuzefront itself [is] the + * orgid for our own apps"): `organization_id` is NOT NULL on `apps` now + * (see 026_apps_organization_id_not_null.ts, and its applications-service + * sibling 011_apps_organization_id_not_null.ts, which backfill every + * existing org-less row to the platform root org before either + * constrains the column), so there is no such row left to special-case. + * - this function's org-membership branch didn't check `visibility` at + * all, so any row sharing an organization_id with the caller was shown + * regardless of its declared visibility, where production requires + * visibility IN ('organization','private') on that branch. Fixed here + * to match. + * * The caller may see an app when ANY of: - * - it belongs to an org they're an active member of, OR - * - its visibility is 'public' or 'marketplace'. - * Private/organization apps of orgs they don't belong to are excluded (BOLA). + * - its visibility is 'public'/'marketplace', OR + * - its visibility is 'organization'/'private' AND its organization_id is + * one the caller is an active member of. + * An organization/private app of an org the caller does NOT belong to is + * excluded either way (BOLA). Does not implement the app-registry's + * platform-admin bypass or scoped-portal gate (portal_apps) — this table + * predates that multi-tenant portal concept; only the core visibility/org + * rule is kept in parity here. */ function scopeAppsQuery(query: any, memberOrgIds: string[]) { return query.where(function (this: any) { this.whereIn('apps.visibility', ['public', 'marketplace']) if (memberOrgIds.length > 0) { - this.orWhereIn('apps.organization_id', memberOrgIds) + this.orWhere((ownOrg: any) => { + ownOrg + .whereIn('apps.visibility', ['organization', 'private']) + .whereIn('apps.organization_id', memberOrgIds) + }) } }) } @@ -788,7 +823,14 @@ router.post( scope, module, description, - organization_id: organizationId || null, + // organizationId || null WAS the bug this comment used to only + // half-describe: a caller whose verified context carries no org + // (organizationId undefined) got an explicit NULL row — the exact + // org-less state the NOT NULL constraint on this column (see + // scopeAppsQuery's doc comment above and + // 026_apps_organization_id_not_null.ts) now forbids. Fall back to + // the platform root org instead, same as the column's own DEFAULT. + organization_id: organizationId || ROOT_ORG_ID, visibility: 'private', }) diff --git a/backend/tests/apps.test.ts b/backend/tests/apps.test.ts index 98c0dfcd9..5cf4927cd 100644 --- a/backend/tests/apps.test.ts +++ b/backend/tests/apps.test.ts @@ -567,12 +567,11 @@ describe('Apps Registration Routes', () => { for (const appData of retrievalApps) { const res = await postApp(appData) expect(res.status).toBe(201) - // GET /api/apps is now object-level scoped (org membership + visibility, - // appsec HIGH-4). Apps created via POST /api/apps have no organization_id - // and default to visibility 'private', so they are correctly excluded - // from a non-member's listing. To assert the listing/shape behaviour we - // mark these fixtures 'public' so the authenticated caller is entitled - // to see them. + // GET /api/apps is object-level scoped (org membership + visibility, + // appsec HIGH-4; see scopeAppsQuery). Mark these fixtures 'public' + // regardless of what the org/visibility scoping would otherwise do, + // so this suite asserts the listing/shape behaviour independent of + // that scoping (which has its own dedicated coverage below). await db('apps') .where('id', res.body.id) .update({ visibility: 'public' }) @@ -620,6 +619,155 @@ describe('Apps Registration Routes', () => { }) }) + // --------------------------------------------------------------------------- + // portal-registration-gap: scopeAppsQuery must agree with the production + // app-registry visibility rule (backend/applications/src/app-registry/ + // service.ts `list()`), not the pre-fix version of itself, AND + // `apps.organization_id` must actually be impossible to leave null. + // + // Owner ruling 2026-08-25 superseded the original "reconcile the two + // org-less branches" framing: an org-less app is not a state to be + // handled by either query, it is a state the schema must no longer allow + // ("there should be no app without orgid ... fuzefront itself [is] the + // orgid for our own apps"). So this suite tests the CONSTRAINT, not a + // null-organization_id code path — "it passes on the current data" is not + // evidence a NOT NULL migration actually did anything. + // --------------------------------------------------------------------------- + describe('GET /api/apps - visibility/org scoping parity, and apps.organization_id NOT NULL', () => { + const ADMIN_USER_ID = '8dbf6a1b-c0a1-462a-9bf5-934c8c7339c3' + let scopedOrgId: string + let otherOrgId: string + let demoToken: string + let ownOrgAppId: string + let otherOrgAppId: string + + beforeAll(async () => { + // A non-owner, non-member caller for the "does NOT belong to this org" + // assertions: the seeded demo user (roles: ['user']). + const demoLogin = await request(app).post('/api/auth/login').send({ + email: 'demo@fuzefront.dev', + password: 'demo123', + }) + expect(demoLogin.status).toBe(200) + demoToken = demoLogin.body.token + + // An org the admin (authToken) IS an active member of. + scopedOrgId = uuidv4() + await db('organizations').insert({ + id: scopedOrgId, + name: 'Visibility Parity Org', + slug: `visibility-parity-org-${scopedOrgId.slice(0, 8)}`, + owner_id: ADMIN_USER_ID, + type: 'organization', + settings: JSON.stringify({}), + metadata: JSON.stringify({}), + is_active: true, + }) + await db('organization_memberships').insert({ + id: uuidv4(), + user_id: ADMIN_USER_ID, + organization_id: scopedOrgId, + role: 'owner', + status: 'active', + joined_at: new Date(), + permissions: JSON.stringify({}), + metadata: JSON.stringify({}), + }) + + // An org NEITHER caller belongs to, for the BOLA-exclusion assertion. + otherOrgId = uuidv4() + + // Case 1: 'organization' visibility, owned by an org the admin belongs to. + ownOrgAppId = uuidv4() + createdAppNames.add('Parity Own-Org App') + await db('apps').insert({ + id: ownOrgAppId, + name: 'Parity Own-Org App', + url: 'http://localhost:9301', + integration_type: 'iframe', + organization_id: scopedOrgId, + visibility: 'organization', + is_active: true, + }) + + // Case 2: 'organization' visibility, owned by a DIFFERENT org neither + // caller belongs to -- must stay excluded for both (BOLA). + otherOrgAppId = uuidv4() + createdAppNames.add('Parity Other-Org App') + await db('apps').insert({ + id: otherOrgAppId, + name: 'Parity Other-Org App', + url: 'http://localhost:9302', + integration_type: 'iframe', + organization_id: otherOrgId, + visibility: 'organization', + is_active: true, + }) + }) + + afterAll(async () => { + await db('organization_memberships') + .where('organization_id', scopedOrgId) + .del() + await db('apps').whereIn('id', [ownOrgAppId, otherOrgAppId]).del() + await db('organizations').where('id', scopedOrgId).del() + }) + + it("shows an 'organization'-visibility app to a member of that org", async () => { + const response = await request(app) + .get('/api/apps') + .set('Authorization', `Bearer ${authToken}`) + .expect(200) + const names = response.body.map((a: any) => a.name) + expect(names).toContain('Parity Own-Org App') + }) + + it("excludes an 'organization'-visibility app of an org the caller does not belong to (BOLA)", async () => { + const response = await request(app) + .get('/api/apps') + .set('Authorization', `Bearer ${demoToken}`) + .expect(200) + const names = response.body.map((a: any) => a.name) + expect(names).not.toContain('Parity Own-Org App') + expect(names).not.toContain('Parity Other-Org App') + }) + + it('rejects an INSERT with organization_id explicitly NULL (026_apps_organization_id_not_null)', async () => { + // Not "does the app show up right" — a direct assertion that the + // constraint itself is live, so a future change cannot silently drop + // it and only be caught by a data audit months later. + await expect( + db('apps').insert({ + id: uuidv4(), + name: 'Constraint Probe — should never be visible or exist', + url: 'http://localhost:9399', + integration_type: 'iframe', + organization_id: null, + visibility: 'private', + is_active: false, + }) + ).rejects.toThrow(/organization_id|not-null|null value/i) + }) + + it('omitting organization_id on INSERT falls back to the column DEFAULT (the platform root org), not NULL', async () => { + const ROOT_ORG_ID = '00000000-0000-0000-0000-000000000010' + const id = uuidv4() + createdAppNames.add('Parity Default-Org App') + await db('apps').insert({ + id, + name: 'Parity Default-Org App', + url: 'http://localhost:9303', + integration_type: 'iframe', + // organization_id deliberately omitted. + visibility: 'private', + is_active: true, + }) + const row = await db('apps').where({ id }).first() + expect(row.organization_id).toBe(ROOT_ORG_ID) + await db('apps').where({ id }).del() + }) + }) + // --------------------------------------------------------------------------- // appsec #100: authentication on previously-OPEN routes (CRITICAL-1/2) // ---------------------------------------------------------------------------