From 1f964ed3988f766d34fede3a335c2f2533357fc4 Mon Sep 17 00:00:00 2001 From: Billy Dunn Date: Fri, 7 Aug 2026 23:06:19 -0500 Subject: [PATCH 1/2] fix(api): gate partner-wide script writes on canManagePartnerWidePolicies (#3262) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Partner SCOPE was standing in for the partner-wide CAPABILITY. A partner user with `partner_users.org_access = 'selected'` — scoped to, say, three of eighty customers — could create a script that runs as SYSTEM across all eighty, including orgs they hold no grant for and orgs onboarded later. `canManagePartnerWidePolicies` exists precisely to prevent this substitution; its doc comment says so. It appeared nowhere in scripts.ts. Four sites, not one. The issue names the create path; the other three are the same privilege reached by a different verb: 1. CREATE (`availability: 'partner'`) — the reported vector. 2. WIDEN on edit — `resolveRescopeTarget`'s `availability === 'partner'` branch checked scope only, so an existing org script could be promoted partner-wide. This is a second creation vector for the same privilege. 3. EDIT an existing partner-wide script — the prior guard blocked only `auth.scope === 'organization'`, so a partner 'selected' user could rewrite the body of a script already running as SYSTEM everywhere. 4. DELETE an existing partner-wide script — same guard, same gap; deleting removes automation from every org under the partner. `RescopeAuth` gains `partnerOrgAccess` because the capability is deliberately not derivable from `accessibleOrgIds`, and its `scope` narrows from `string` to `AuthContext['scope']` so the capability check typechecks. Follows `peripheralControl.ts:508` — `PARTNER_WIDE_WRITE_DENIED_MESSAGE` with 403 — matching the nine other routes that already do this correctly. Tests: four denials (create / widen / edit / delete as a 'selected' partner user) plus a positive control proving a full-partner admin can still widen. The two existing partner-auth helpers now default to `partnerOrgAccess: 'all'`, which is the user those positive cases always meant to describe. All five verified against the un-fixed code: with the capability neutered to `return true`, exactly the four denials fail and the positive control still passes. Note the first control attempt only neutered two of the four guards — the edit/delete guards use a different expression shape — so it under-reported; the numbers above are from neutering the capability itself. Gate: tsc exit 0; vitest 1285 files / 20525 tests / 0 failures; eslint clean. --- apps/api/src/routes/scripts.test.ts | 118 +++++++++++++++++++++++++++- apps/api/src/routes/scripts.ts | 41 +++++++++- 2 files changed, 155 insertions(+), 4 deletions(-) diff --git a/apps/api/src/routes/scripts.test.ts b/apps/api/src/routes/scripts.test.ts index 4d4d6a69bf..88480619c0 100644 --- a/apps/api/src/routes/scripts.test.ts +++ b/apps/api/src/routes/scripts.test.ts @@ -904,11 +904,16 @@ describe('scripts routes', () => { // ── Task 8: Partner-wide create + edit/delete guard ─────────────────────── describe('Task 8: partner-wide create + org-user read-only guard', () => { - function makePartnerAuth() { + // #3262: partner-wide writes need the CAPABILITY (org_access = 'all'), not + // just partner scope. These helpers default to a full-partner admin so the + // existing positive cases keep describing the user they always meant; pass + // 'selected' to exercise the denial. + function makePartnerAuth(partnerOrgAccess: 'all' | 'selected' = 'all') { return { user: { id: 'user-123', email: 'test@example.com', name: 'Test User' }, scope: 'partner' as const, partnerId: PARTNER_ID, + partnerOrgAccess, orgId: null, token: { sub: 'user-123', email: 'test@example.com', roleId: 'role-123', @@ -974,6 +979,31 @@ describe('scripts routes', () => { expect(insertedValues?.partnerId).toBe(PARTNER_ID); }); + // #3262: the reported vector — partner scope alone was enough to create a + // script that runs as SYSTEM across every org under the partner. + it('#3262: a selected-access partner user cannot create a partner-wide script', async () => { + const { authMiddleware } = await import('../middleware/auth'); + vi.mocked(authMiddleware).mockImplementationOnce((c: any, next: any) => { + c.set('auth', makePartnerAuth('selected')); + return next(); + }); + + const res = await app.request('/scripts', { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: 'Bearer valid-token' }, + body: JSON.stringify({ + name: 'Partner Script', + osTypes: ['windows'], + language: 'powershell', + content: 'echo hi', + availability: 'partner' + }) + }); + + expect(res.status).toBe(403); + expect(vi.mocked(db.insert)).not.toHaveBeenCalled(); + }); + it('partner user with availability=org + orgId creates an org-specific script', async () => { const { authMiddleware } = await import('../middleware/auth'); vi.mocked(authMiddleware).mockImplementationOnce((c: any, next: any) => { @@ -1117,11 +1147,16 @@ describe('scripts routes', () => { }); describe('Task 9: re-scope on edit (issue #1734)', () => { - function makePartnerAuth() { + // #3262: partner-wide writes need the CAPABILITY (org_access = 'all'), not + // just partner scope. These helpers default to a full-partner admin so the + // existing positive cases keep describing the user they always meant; pass + // 'selected' to exercise the denial. + function makePartnerAuth(partnerOrgAccess: 'all' | 'selected' = 'all') { return { user: { id: 'user-123', email: 'test@example.com', name: 'Test User' }, scope: 'partner' as const, partnerId: PARTNER_ID, + partnerOrgAccess, orgId: null, token: { sub: 'user-123', email: 'test@example.com', roleId: 'role-123', @@ -1214,6 +1249,85 @@ describe('scripts routes', () => { expect(getSet().partnerId).toBe(PARTNER_ID); }); + // ── #3262: partner-wide writes require org_access = 'all' ─────────────── + // Scripts run as SYSTEM on every endpoint, and a partner-wide script covers + // every org under the partner including ones onboarded later. Partner SCOPE + // is not the capability; `partnerOrgAccess: 'all'` is. + it('#3262: a selected-access partner user cannot widen a script to partner-wide', async () => { + await withAuth(makePartnerAuth('selected')); + mockScriptLookup({ + id: SCRIPT_ID_1, name: 'Org Script', orgId: ORG_ID, partnerId: PARTNER_ID, + isSystem: false, content: 'echo hi', version: 1, + }, 0); + + const res = await app.request(`/scripts/${SCRIPT_ID_1}`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json', Authorization: 'Bearer valid-token' }, + body: JSON.stringify({ availability: 'partner' }), + }); + + expect(res.status).toBe(403); + expect(vi.mocked(db.update)).not.toHaveBeenCalled(); + }); + + it('#3262: a selected-access partner user cannot edit an existing partner-wide script', async () => { + await withAuth(makePartnerAuth('selected')); + mockScriptLookup({ + id: SCRIPT_ID_1, name: 'Partner Script', orgId: null, partnerId: PARTNER_ID, + isSystem: false, content: 'echo hi', version: 1, + }, 0); + + const res = await app.request(`/scripts/${SCRIPT_ID_1}`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json', Authorization: 'Bearer valid-token' }, + // A plain content edit — no re-scope. The script is already partner-wide, + // so rewriting its body is rewriting code that runs everywhere. + body: JSON.stringify({ content: 'echo pwned' }), + }); + + expect(res.status).toBe(403); + expect(vi.mocked(db.update)).not.toHaveBeenCalled(); + }); + + it('#3262: a selected-access partner user cannot delete an existing partner-wide script', async () => { + await withAuth(makePartnerAuth('selected')); + mockScriptLookup({ + id: SCRIPT_ID_1, name: 'Partner Script', orgId: null, partnerId: PARTNER_ID, + isSystem: false, content: 'echo hi', version: 1, + }, 0); + + const res = await app.request(`/scripts/${SCRIPT_ID_1}`, { + method: 'DELETE', + headers: { Authorization: 'Bearer valid-token' }, + }); + + expect(res.status).toBe(403); + expect(vi.mocked(db.update)).not.toHaveBeenCalled(); + }); + + // Control: the gate denies on capability, not on partner scope generally — + // without this, all three assertions above would pass on a handler that + // simply rejected every partner write. + it('#3262: a full-partner admin still can widen a script to partner-wide', async () => { + await withAuth(makePartnerAuth('all')); + mockScriptLookup({ + id: SCRIPT_ID_1, name: 'Org Script', orgId: ORG_ID, partnerId: PARTNER_ID, + isSystem: false, content: 'echo hi', version: 1, + }, 0); + const getSet = captureUpdate({ + id: SCRIPT_ID_1, name: 'Org Script', orgId: null, partnerId: PARTNER_ID, version: 1, + }); + + const res = await app.request(`/scripts/${SCRIPT_ID_1}`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json', Authorization: 'Bearer valid-token' }, + body: JSON.stringify({ availability: 'partner' }), + }); + + expect(res.status).toBe(200); + expect(getSet().orgId).toBeNull(); + }); + it('partner user moves a script org→org (when no references exist)', async () => { await withAuth(makePartnerAuth()); mockScriptLookup({ diff --git a/apps/api/src/routes/scripts.ts b/apps/api/src/routes/scripts.ts index ab750e7f43..a563bf6abc 100644 --- a/apps/api/src/routes/scripts.ts +++ b/apps/api/src/routes/scripts.ts @@ -16,10 +16,14 @@ import { configPolicyFeatureLinks, configurationPolicies } from '../db/schema'; -import { authMiddleware, requireMfa, requirePermission, requireScope } from '../middleware/auth'; +import { authMiddleware, requireMfa, requirePermission, requireScope, type AuthContext } from '../middleware/auth'; import { canAccessSite, PERMISSIONS, type UserPermissions } from '../services/permissions'; import { writeRouteAudit } from '../services/auditEvents'; import { executeScriptOnDevices } from '../services/scriptExecution'; +import { + canManagePartnerWidePolicies, + PARTNER_WIDE_WRITE_DENIED_MESSAGE, +} from '../services/partnerWideAccess'; export const scriptRoutes = new Hono(); @@ -70,8 +74,13 @@ function resolveScriptAuditOrgId( } type RescopeAuth = { - scope: string; + scope: AuthContext['scope']; partnerId: string | null; + // #3262: the partner-wide capability is NOT derivable from the other fields — + // a 'selected' user whose selection happens to cover every current org still + // must not administer partner-wide state. Carried explicitly so the widening + // branch below can check it. + partnerOrgAccess?: AuthContext['partnerOrgAccess']; accessibleOrgIds: string[] | null; canAccessOrg: (orgId: string) => boolean; }; @@ -119,6 +128,13 @@ function resolveRescopeTarget( } if (availability === 'partner') { + // #3262: widening an existing script to partner-wide is a second creation + // vector for the same privilege — it ends with a script running as SYSTEM + // on every org under the partner, including orgs onboarded later. Gate it + // exactly like the create path. + if (!canManagePartnerWidePolicies(auth)) { + return { error: PARTNER_WIDE_WRITE_DENIED_MESSAGE, status: 403 }; + } return { orgId: null, partnerId }; } @@ -505,6 +521,14 @@ scriptRoutes.post( partnerId = auth.partnerId ?? null; // denormalized for RLS } else if (auth.scope === 'partner') { if (data.availability === 'partner') { + // #3262: partner SCOPE is not the same as partner-wide CAPABILITY. A + // partner user with org_access = 'selected' may be scoped to three of + // eighty customers; without this gate they could create a script that + // runs as SYSTEM across all eighty, including orgs they hold no grant + // for and orgs created later. + if (!canManagePartnerWidePolicies(auth)) { + return c.json({ error: PARTNER_WIDE_WRITE_DENIED_MESSAGE }, 403); + } orgId = null; partnerId = auth.partnerId ?? null; if (!partnerId) return c.json({ error: 'Partner context required' }, 403); @@ -592,6 +616,13 @@ scriptRoutes.put( if (script.orgId === null && script.partnerId !== null && auth.scope === 'organization') { return c.json({ error: 'This script is shared across your organization and is read-only here' }, 403); } + // #3262: and within the partner, only a full-partner admin. Someone who + // cannot create a partner-wide script must not be able to edit one either — + // otherwise the body of a script already running as SYSTEM everywhere is + // rewritable by a 'selected'-access user. + if (script.orgId === null && script.partnerId !== null && !canManagePartnerWidePolicies(auth)) { + return c.json({ error: PARTNER_WIDE_WRITE_DENIED_MESSAGE }, 403); + } // Cannot edit system scripts unless system scope if (script.isSystem && auth.scope !== 'system') { return c.json({ error: 'System scripts are read-only' }, 403); @@ -760,6 +791,12 @@ scriptRoutes.delete( if (script.orgId === null && script.partnerId !== null && auth.scope === 'organization') { return c.json({ error: 'This script is shared across your organization and is read-only here' }, 403); } + // #3262: and within the partner, only a full-partner admin — same reasoning + // as the edit path. Deleting a partner-wide script removes automation from + // every org under the partner. + if (script.orgId === null && script.partnerId !== null && !canManagePartnerWidePolicies(auth)) { + return c.json({ error: PARTNER_WIDE_WRITE_DENIED_MESSAGE }, 403); + } // Cannot delete system scripts unless system scope if (script.isSystem && auth.scope !== 'system') { return c.json({ error: 'System scripts are read-only' }, 403); From 7a5ded7b0c3d7fb2b2999e3273ca0d8575243861 Mon Sep 17 00:00:00 2001 From: Todd Hebebrand Date: Fri, 7 Aug 2026 23:22:08 -0600 Subject: [PATCH 2/2] fix(web): gate the partner-wide script option on the #3262 capability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the server-side gate: - /users/me now surfaces canManagePartnerWide (UX only; the server still gates every partner-wide write), and the auth store merges it on the same ride-along path as isPlatformAdmin/permissions. - ScriptForm no longer defaults new scripts to an availability the save would 403: selected-access partner users get the org option by default, a disabled "All my organizations" radio with a hint, and a read-only notice when editing an existing partner-wide script. - Positive coverage for the gate's allow branches: a selected-access user still creates org-scoped scripts, and a full-partner admin still deletes a partner-wide script — so an over-broad gate can't ship green. Co-Authored-By: Claude Fable 5 --- apps/api/src/routes/scripts.test.ts | 67 +++++++++++++++++++ apps/api/src/routes/users.ts | 6 ++ .../components/scripts/ScriptForm.test.tsx | 64 ++++++++++++++++++ .../web/src/components/scripts/ScriptForm.tsx | 28 +++++++- apps/web/src/locales/de-DE/scripts.json | 4 +- apps/web/src/locales/en/scripts.json | 4 +- apps/web/src/locales/es-419/scripts.json | 4 +- apps/web/src/locales/fr-CA/scripts.json | 4 +- apps/web/src/locales/fr-FR/scripts.json | 4 +- apps/web/src/locales/it-IT/scripts.json | 4 +- apps/web/src/locales/pt-BR/scripts.json | 4 +- apps/web/src/stores/auth.ts | 10 +++ 12 files changed, 194 insertions(+), 9 deletions(-) diff --git a/apps/api/src/routes/scripts.test.ts b/apps/api/src/routes/scripts.test.ts index 88480619c0..c7cdd34a73 100644 --- a/apps/api/src/routes/scripts.test.ts +++ b/apps/api/src/routes/scripts.test.ts @@ -1045,6 +1045,51 @@ describe('scripts routes', () => { expect(insertedValues?.partnerId).toBe(PARTNER_ID); }); + // #3262 control: the capability gate covers ONLY partner-wide writes. A + // selected-access user must keep every org-scoped ability — without this, + // a refactor that hoists the gate above the availability branch (denying + // ALL partner-scope writes) would pass the suite green. + it('#3262: a selected-access partner user still creates an org-scoped script', async () => { + const { authMiddleware } = await import('../middleware/auth'); + vi.mocked(authMiddleware).mockImplementationOnce((c: any, next: any) => { + c.set('auth', makePartnerAuth('selected')); + return next(); + }); + + let insertedValues: any; + vi.mocked(db.insert).mockReturnValue({ + values: vi.fn().mockImplementation((vals: any) => { + insertedValues = vals; + return { + returning: vi.fn().mockResolvedValue([{ + id: SCRIPT_ID_1, + name: 'Org Script', + orgId: ORG_ID, + partnerId: PARTNER_ID, + isSystem: false + }]) + }; + }) + } as any); + + const res = await app.request('/scripts', { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: 'Bearer valid-token' }, + body: JSON.stringify({ + name: 'Org Script', + osTypes: ['windows'], + language: 'powershell', + content: 'echo hi', + orgId: ORG_ID, + availability: 'org' + }) + }); + + expect(res.status).toBe(201); + expect(insertedValues?.orgId).toBe(ORG_ID); + expect(insertedValues?.partnerId).toBe(PARTNER_ID); + }); + it('org user editing a partner-wide script (org_id=null, partner_id set) → 403', async () => { const { authMiddleware } = await import('../middleware/auth'); vi.mocked(authMiddleware).mockImplementationOnce((c: any, next: any) => { @@ -1328,6 +1373,28 @@ describe('scripts routes', () => { expect(getSet().orgId).toBeNull(); }); + // Control for the DELETE gate: it must deny on capability, not on partner + // scope generally — a gate that 403'd every partner delete of a + // partner-wide script would pass the denial tests above. + it('#3262: a full-partner admin still deletes a partner-wide script', async () => { + await withAuth(makePartnerAuth('all')); + // Serves the script lookup AND the active-executions count (0 = none). + mockScriptLookup({ + id: SCRIPT_ID_1, name: 'Partner Script', orgId: null, partnerId: PARTNER_ID, + isSystem: false, content: 'echo hi', version: 1, + }, 0); + const getSet = captureUpdate({ id: SCRIPT_ID_1 }); + + const res = await app.request(`/scripts/${SCRIPT_ID_1}`, { + method: 'DELETE', + headers: { Authorization: 'Bearer valid-token' }, + }); + + expect(res.status).toBe(200); + // Soft delete: the handler stamps deletedAt rather than issuing a DELETE. + expect(getSet().deletedAt).toBeInstanceOf(Date); + }); + it('partner user moves a script org→org (when no references exist)', async () => { await withAuth(makePartnerAuth()); mockScriptLookup({ diff --git a/apps/api/src/routes/users.ts b/apps/api/src/routes/users.ts index e35bd9caf4..9675224056 100644 --- a/apps/api/src/routes/users.ts +++ b/apps/api/src/routes/users.ts @@ -23,6 +23,7 @@ import { getUserPermissions, PERMISSIONS } from '../services/permissions'; +import { canManagePartnerWidePolicies } from '../services/partnerWideAccess'; import { getScopeContext, getScopedRole, @@ -393,6 +394,11 @@ userRoutes.get('/me', async (c) => { scope: auth.scope, partnerDefaultLocale, permissions: userPerms?.permissions ?? [], + // #3262: lets forms with an "All organizations" owner option (scripts, + // policies) hide/disable it for partner users without org_access = 'all'. + // UX only — every partner-wide write is still gated server-side via + // canManagePartnerWidePolicies. + canManagePartnerWide: canManagePartnerWidePolicies(auth), requiresSetup }); }); diff --git a/apps/web/src/components/scripts/ScriptForm.test.tsx b/apps/web/src/components/scripts/ScriptForm.test.tsx index 7cf2bc1c20..c93b37994a 100644 --- a/apps/web/src/components/scripts/ScriptForm.test.tsx +++ b/apps/web/src/components/scripts/ScriptForm.test.tsx @@ -53,6 +53,7 @@ vi.mock('@/stores/orgStore', async () => { }); import ScriptForm from './ScriptForm'; +import { useAuthStore } from '@/stores/auth'; describe('ScriptForm Monaco lifecycle (issue #1186)', () => { beforeEach(() => { @@ -219,3 +220,66 @@ describe('ScriptForm availability picker — partner-scope gate', () => { expect(queryByText('Available to')).toBeNull(); }); }); + +// #3262: the server 403s partner-wide writes for partner users without +// org_access = 'all', so the picker must not offer (or default to) an option +// the save would reject. Capability rides on /users/me → auth store. +describe('ScriptForm availability picker — partner-wide capability gate (#3262)', () => { + const baseUser = { + id: 'u-1', email: 'tech@example.com', name: 'Selected Tech', mfaEnabled: true + }; + + beforeEach(() => { + editorInstances.length = 0; + getJwtClaimsMock.mockReturnValue({ scope: 'partner', partnerId: 'p-1', orgId: null }); + orgStoreMock.mockReturnValue({ + organizations: [{ id: 'o-1', name: 'Org One' }, { id: 'o-2', name: 'Org Two' }], + partners: [], + sites: [] + }); + }); + afterEach(() => { + useAuthStore.setState({ user: null }); + vi.clearAllMocks(); + }); + + it('disables "All my organizations" and defaults to a specific org for a selected-access user', async () => { + useAuthStore.setState({ user: { ...baseUser, canManagePartnerWide: false } }); + const { findByText, getByLabelText } = render(); + await findByText('Available to'); + + const partnerRadio = getByLabelText('All my organizations') as HTMLInputElement; + const orgRadio = getByLabelText(/A specific organization/) as HTMLInputElement; + expect(partnerRadio.disabled).toBe(true); + expect(partnerRadio.checked).toBe(false); + expect(orgRadio.checked).toBe(true); + expect(await findByText(/Requires full partner org access/)).toBeTruthy(); + }); + + it('keeps the partner-wide default enabled for a full-access user', async () => { + useAuthStore.setState({ user: { ...baseUser, canManagePartnerWide: true } }); + const { findByText, getByLabelText, queryByText } = render(); + await findByText('Available to'); + + const partnerRadio = getByLabelText('All my organizations') as HTMLInputElement; + expect(partnerRadio.disabled).toBe(false); + expect(partnerRadio.checked).toBe(true); + expect(queryByText(/Requires full partner org access/)).toBeNull(); + }); + + it('treats an absent capability field (pre-field session) as capable — server still enforces', async () => { + useAuthStore.setState({ user: { ...baseUser } }); + const { findByText, getByLabelText } = render(); + await findByText('Available to'); + expect((getByLabelText('All my organizations') as HTMLInputElement).disabled).toBe(false); + }); + + it('warns that an existing partner-wide script is read-only for a selected-access user', async () => { + useAuthStore.setState({ user: { ...baseUser, canManagePartnerWide: false } }); + const { findByText } = render( + + ); + await findByText('Available to'); + expect(await findByText(/Editing it requires full partner org access/)).toBeTruthy(); + }); +}); diff --git a/apps/web/src/components/scripts/ScriptForm.tsx b/apps/web/src/components/scripts/ScriptForm.tsx index 8d316dd82e..5e1add15ca 100644 --- a/apps/web/src/components/scripts/ScriptForm.tsx +++ b/apps/web/src/components/scripts/ScriptForm.tsx @@ -26,6 +26,7 @@ import { useScriptAiStore } from '@/stores/scriptAiStore'; import type { ScriptFormBridge } from '@/stores/scriptAiStore'; import type { OSType } from './ScriptList'; import { useOrgStore } from '@/stores/orgStore'; +import { useAuthStore } from '@/stores/auth'; import { getJwtClaims } from '@/lib/authScope'; import { scriptSchema, languageOptions, categoryOptions, @@ -134,6 +135,12 @@ export default function ScriptForm({ // navigation — scripts-list -> editor — because this component is unmounted // on the list page. See that file for the full rationale. + // #3262: partner users with org_access = 'selected' cannot create or modify + // partner-wide scripts — the server 403s them — so don't default the form to + // an option they can't save. Absent (sessions persisted before /users/me + // carried the field) is treated as capable: UX only, the server enforces. + const canManagePartnerWide = useAuthStore(s => s.user?.canManagePartnerWide ?? true); + const { register, handleSubmit, @@ -156,7 +163,7 @@ export default function ScriptForm({ timeoutSeconds: 300, runAs: 'system', exitCodeSeverityMapping: [], - availability: 'partner', + availability: canManagePartnerWide ? 'partner' : 'org', ...defaultValues } }); @@ -335,14 +342,31 @@ export default function ScriptForm({ {t('scriptForm.availability.description')}

)} -