diff --git a/public/schemas/app-block/v1.json b/public/schemas/app-block/v1.json index 64e8f34d90..b64a1abf80 100644 --- a/public/schemas/app-block/v1.json +++ b/public/schemas/app-block/v1.json @@ -92,7 +92,7 @@ }, "scopeJustifications": { "type": "object", - "description": "OPTIONAL per-scope justification: a map of scope-id → free-text rationale explaining WHY the app needs that permission, shown to the moderator during review. Backward-compatible — omit it and the manifest stays valid; `scopes` is unchanged. Every key MUST be a scope also present in `scopes` (justifications for scopes you don't request are rejected). Each value is a non-empty string of at most 500 characters. NOTE: this captures the developer's STATED rationale only; the platform does not verify the claims.", + "description": "Per-scope justification: a map of scope-id → free-text rationale explaining WHY the app needs that permission, shown to the moderator during review. REQUIRED for SENSITIVE scopes — any declared scope that can spend or read the viewer's Buzz, read the viewer's private data, or write data other users see (e.g. `ai:write:budgeted`, `social:tip:self`, `buzz:read:self`, `collections:read:private`, `apps:storage:shared:write`) MUST carry a non-empty justification here, or the manifest is rejected at submit time. OPTIONAL for non-sensitive scopes — omit those and the manifest stays valid. Every key MUST be a scope also present in `scopes` (justifications for scopes you don't request are rejected). Each value is a non-empty string of at most 500 characters. The requirement is enforced imperatively by the manifest validator (not expressed as JSON-Schema conditionals here). NOTE: the justification captures the developer's STATED rationale only; the platform does not verify the truth of the claims.", "additionalProperties": { "type": "string", "minLength": 1, diff --git a/src/components/Apps/__tests__/offsiteReviewChecklist.test.ts b/src/components/Apps/__tests__/offsiteReviewChecklist.test.ts index a06db81bea..9ee1a861ae 100644 --- a/src/components/Apps/__tests__/offsiteReviewChecklist.test.ts +++ b/src/components/Apps/__tests__/offsiteReviewChecklist.test.ts @@ -3,6 +3,7 @@ import { getOffsiteReviewChecklist, getOnsiteReviewChecklist, getReviewChecklist, + unjustifiedSensitiveBlockScopes, unjustifiedSensitiveScopeKeys, type OffsiteChecklistData, } from '../offsiteReviewChecklist'; @@ -190,6 +191,71 @@ describe('getOffsiteReviewChecklist — connect sensitive-scope item (PR3)', () }); }); +describe('getOnsiteReviewChecklist — auto-derived scopes item (block sensitive scopes)', () => { + const scopesStatus = (data?: Parameters[0]) => + getOnsiteReviewChecklist(data).find((i) => i.id === 'scopes'); + + it('with NO data the scopes item stays a mod-judgment todo (backward compatible)', () => { + expect(scopesStatus()?.status).toBe('todo'); + }); + + it('a declared sensitive scope WITHOUT a justification → warn, hint names the scope', () => { + const item = scopesStatus({ scopes: ['apps:storage:shared:write'] }); + expect(item?.status).toBe('warn'); + expect(item?.hint).toContain('apps:storage:shared:write'); + }); + + it('every declared sensitive scope justified → ok, hint surfaces the justification', () => { + const item = scopesStatus({ + scopes: ['apps:storage:shared:write'], + scopeJustifications: { + 'apps:storage:shared:write': 'We persist shared gallery entries.', + }, + }); + expect(item?.status).toBe('ok'); + expect(item?.hint).toContain('apps:storage:shared:write'); + expect(item?.hint).toContain('We persist shared gallery entries.'); + }); + + it('data present but NO sensitive scopes declared → ok (nothing to justify)', () => { + const item = scopesStatus({ scopes: ['models:read:self'] }); + expect(item?.status).toBe('ok'); + }); + + it('multiple sensitive scopes, one justified one not → warn, hint names the unjustified one', () => { + const item = scopesStatus({ + scopes: ['collections:read:private', 'apps:storage:shared:write'], + scopeJustifications: { 'collections:read:private': 'We read private collections.' }, + }); + expect(item?.status).toBe('warn'); + expect(item?.hint).toContain('apps:storage:shared:write'); + }); +}); + +describe('unjustifiedSensitiveBlockScopes', () => { + it('returns declared sensitive block scopes lacking a non-empty justification', () => { + expect( + unjustifiedSensitiveBlockScopes({ + scopes: ['models:read:self', 'collections:read:private', 'apps:storage:shared:write'], + scopeJustifications: { 'collections:read:private': 'ok' }, // shared:write missing + }) + ).toEqual(['apps:storage:shared:write']); + }); + + it('empty when no sensitive scope is declared', () => { + expect(unjustifiedSensitiveBlockScopes({ scopes: ['models:read:self'] })).toEqual([]); + }); + + it('whitespace-only justification counts as missing', () => { + expect( + unjustifiedSensitiveBlockScopes({ + scopes: ['apps:storage:shared:write'], + scopeJustifications: { 'apps:storage:shared:write': ' ' }, + }) + ).toEqual(['apps:storage:shared:write']); + }); +}); + describe('unjustifiedSensitiveScopeKeys', () => { it('returns the enum-keys of sensitive requested scopes lacking a non-empty justification', () => { expect( diff --git a/src/components/Apps/offsiteReviewChecklist.ts b/src/components/Apps/offsiteReviewChecklist.ts index 400bacfc9f..ab86ff4737 100644 --- a/src/components/Apps/offsiteReviewChecklist.ts +++ b/src/components/Apps/offsiteReviewChecklist.ts @@ -1,4 +1,5 @@ import { validateExternalUrl } from '~/server/schema/blocks/external-app.schema'; +import { isSensitiveBlockScope } from '~/shared/constants/block-scope.constants'; import { SENSITIVE_TOKEN_SCOPES, tokenScopeMaskToList, @@ -77,13 +78,70 @@ export function unjustifiedSensitiveScopeKeys(data: { .map(({ key }) => key); } +/** The manifest facts the ON-SITE (App Block) scopes checklist item derives from. */ +export type OnsiteChecklistData = { + /** The manifest's declared block scopes (`manifest.scopes`). */ + scopes?: string[] | null; + /** The manifest's declared per-scope justifications (`manifest.scopeJustifications`). */ + scopeJustifications?: Record | null; +}; + +/** + * The declared BLOCK sensitive scopes (money / private data / cross-user writes) + * that have NO non-empty justification. Mirrors `unjustifiedSensitiveScopeKeys` + * but keyed on the App-Block sensitive set (`isSensitiveBlockScope`) + the + * manifest's `scopeJustifications` — NOT the connect token bitmask. Empty when + * no sensitive scope is declared or every one is justified. Since the manifest + * validator now ENFORCES a justification for these at submit time, an approved + * app returns `[]` here — a non-empty result flags a legacy/unenforced manifest. + */ +export function unjustifiedSensitiveBlockScopes(data: OnsiteChecklistData): string[] { + const scopes = data.scopes ?? []; + const justifications = data.scopeJustifications ?? {}; + return [...new Set(scopes)] + .filter((scope) => isSensitiveBlockScope(scope)) + .filter((scope) => { + const raw = justifications[scope]; + return !(typeof raw === 'string' && raw.trim().length > 0); + }); +} + /** * The deep ON-SITE (App Block) review checklist. These items are STATIC reminders * of the code/bundle review the mod performs in the existing modal panels — the * off-site content checklist deliberately OMITS every one of them (there is no * bundle / manifest / scopes / code to read for an external-link app). */ -export function getOnsiteReviewChecklist(): ReviewChecklistItem[] { +export function getOnsiteReviewChecklist(data?: OnsiteChecklistData): ReviewChecklistItem[] { + // The `scopes` item AUTO-DERIVES from the manifest when scope data is supplied + // (mirroring the off-site connect variant, but keyed on the BLOCK sensitive set + // + the manifest's `scopeJustifications`). Submit now ENFORCES a justification + // for every sensitive scope, so an approved app shows `ok`; a legacy/unenforced + // manifest missing one surfaces as `warn`. With no data the item stays a `todo` + // reminder (unchanged) — and the hint still surfaces the justifications to the + // mod so they can confirm each stated rationale is truthful. + const declaredSensitive = data ? (data.scopes ?? []).filter(isSensitiveBlockScope) : []; + const unjustifiedSensitive = data ? unjustifiedSensitiveBlockScopes(data) : []; + const justifications = data?.scopeJustifications ?? {}; + const scopesStatus: ReviewChecklistItemStatus = !data + ? 'todo' + : unjustifiedSensitive.length > 0 + ? 'warn' + : 'ok'; + const scopesHint = !data + ? 'Every requested scope is needed for the stated functionality.' + : unjustifiedSensitive.length > 0 + ? `Missing a justification for sensitive scope(s): ${unjustifiedSensitive.join( + ', ' + )} — submit should have blocked this; do not approve until justified.` + : declaredSensitive.length > 0 + ? `Sensitive permissions justified — ${declaredSensitive + .map((scope) => { + const j = justifications[scope]; + return typeof j === 'string' && j.trim().length > 0 ? `${scope}: "${j.trim()}"` : scope; + }) + .join('; ')}. Confirm each rationale is truthful.` + : 'No sensitive permissions requested; confirm the remaining scopes are needed for the stated functionality.'; return [ { id: 'code-diff', @@ -106,8 +164,8 @@ export function getOnsiteReviewChecklist(): ReviewChecklistItem[] { { id: 'scopes', label: 'Requested permissions justified', - hint: 'Every requested scope is needed for the stated functionality.', - status: 'todo', + hint: scopesHint, + status: scopesStatus, }, { id: 'screenshots', diff --git a/src/server/services/__tests__/block-manifest-validator.service.test.ts b/src/server/services/__tests__/block-manifest-validator.service.test.ts index 40530537d7..a0a32c8cc5 100644 --- a/src/server/services/__tests__/block-manifest-validator.service.test.ts +++ b/src/server/services/__tests__/block-manifest-validator.service.test.ts @@ -843,6 +843,192 @@ describe('BlockManifestValidator', () => { }); }); + // ENFORCEMENT: a declared SENSITIVE scope (money / private data / cross-user + // write) now REQUIRES a non-empty justification — this used to be optional + // metadata. Runs in `validate`, so it covers every submit path (CLI + // submit-version + web updateManifest both funnel through validateSubmission → + // validate). `apps:storage:shared:write` / `collections:read:private` are + // sensitive AND SKIP_OAUTH_CHECK, so they need no extra OAuth bit; the + // AI context covers `ai:write:budgeted` (requires AIServicesWrite). + describe('scopeJustifications — ENFORCED for sensitive scopes', () => { + const AI_APP_CTX = { + allowedScopes: TokenScope.ModelsRead | TokenScope.AIServicesWrite, + allowedOrigins: ['https://blocks.civitai.com'], + }; + + it('REJECTS a sensitive scope with NO scopeJustifications (previously accepted)', () => { + // ai:write:budgeted can spend the viewer's Buzz — a justification is now required. + const manifest = { ...VALID_MANIFEST, scopes: ['ai:write:budgeted'] }; + const result = BlockManifestValidator.validate(manifest, AI_APP_CTX); + expect(result.valid).toBe(false); + if (!result.valid) { + expect( + result.errors.some( + (e) => e.includes('sensitive scopes require a justification') && e.includes('ai:write:budgeted') + ) + ).toBe(true); + } + }); + + it('REJECTS a sensitive scope when scopeJustifications is present but omits its key', () => { + const manifest = { + ...VALID_MANIFEST, + scopes: ['models:read:self', 'apps:storage:shared:write'], + // Justifies the non-sensitive scope but not the sensitive one. + scopeJustifications: { 'models:read:self': 'We render the page model.' }, + }; + const result = BlockManifestValidator.validate(manifest, APP_CTX); + expect(result.valid).toBe(false); + if (!result.valid) { + expect( + result.errors.some( + (e) => + e.includes('sensitive scopes require a justification') && + e.includes('apps:storage:shared:write') + ) + ).toBe(true); + } + }); + + it.each<[string, string]>([ + ['', 'empty string'], + [' ', 'whitespace-only'], + ['\t\n', 'tabs/newlines only'], + ])('REJECTS a sensitive scope justified with a %j (%s) value', (value) => { + const manifest = { + ...VALID_MANIFEST, + scopes: ['apps:storage:shared:write'], + scopeJustifications: { 'apps:storage:shared:write': value }, + }; + const result = BlockManifestValidator.validate(manifest, APP_CTX); + expect(result.valid).toBe(false); + if (!result.valid) { + expect(result.errors.some((e) => e.includes('apps:storage:shared:write'))).toBe(true); + } + }); + + it('ACCEPTS a sensitive scope WITH a non-empty justification', () => { + const manifest = { + ...VALID_MANIFEST, + scopes: ['apps:storage:shared:write'], + scopeJustifications: { + 'apps:storage:shared:write': 'We persist shared gallery entries other users can see.', + }, + }; + expect(BlockManifestValidator.validate(manifest, APP_CTX)).toEqual({ valid: true }); + }); + + it('ACCEPTS a NON-sensitive scope with no justification (unchanged)', () => { + const manifest = { ...VALID_MANIFEST, scopes: ['models:read:self'] }; + expect(BlockManifestValidator.validate(manifest, APP_CTX)).toEqual({ valid: true }); + }); + + it('lists ALL unjustified sensitive scopes when several are missing', () => { + const manifest = { + ...VALID_MANIFEST, + scopes: ['collections:read:private', 'apps:storage:shared:write'], + // Neither sensitive scope justified. + }; + const result = BlockManifestValidator.validate(manifest, APP_CTX); + expect(result.valid).toBe(false); + if (!result.valid) { + const err = result.errors.find((e) => + e.includes('sensitive scopes require a justification') + ); + expect(err).toBeDefined(); + expect(err).toContain('collections:read:private'); + expect(err).toContain('apps:storage:shared:write'); + } + }); + + it('rejects naming ONLY the unjustified sensitive scope when some are justified', () => { + const manifest = { + ...VALID_MANIFEST, + scopes: ['collections:read:private', 'apps:storage:shared:write'], + scopeJustifications: { 'collections:read:private': 'We read the viewer’s private collections.' }, + }; + const result = BlockManifestValidator.validate(manifest, APP_CTX); + expect(result.valid).toBe(false); + if (!result.valid) { + const err = result.errors.find((e) => + e.includes('sensitive scopes require a justification') + ); + expect(err).toBeDefined(); + expect(err).toContain('apps:storage:shared:write'); + expect(err).not.toContain('collections:read:private'); + } + }); + + it('still applies the shape rules (keys ⊆ scopes, ≤500, non-empty) alongside enforcement', () => { + const manifest = { + ...VALID_MANIFEST, + scopes: ['apps:storage:shared:write'], + scopeJustifications: { + 'apps:storage:shared:write': 'ok reason', + // A justification for a scope NOT declared — still rejected by the shape rule. + 'user:read:self': 'dangling', + }, + }; + const result = BlockManifestValidator.validate(manifest, APP_CTX); + expect(result.valid).toBe(false); + if (!result.valid) { + expect( + result.errors.some((e) => e.includes('user:read:self') && e.includes('not in the manifest')) + ).toBe(true); + } + }); + + // SUBMIT-vs-APPROVE: the enforcement is SUBMIT-only. The moderator approve + // re-validation passes `enforceSensitiveScopeJustification:false` so a LEGACY + // pending request (sensitive scope, no justification, valid under the old + // rules) stays approvable — but ALL OTHER manifest checks still run on approve. + describe('SUBMIT-only (approve exemption)', () => { + it('SUBMIT context (default) still REJECTS a sensitive scope with no justification', () => { + const manifest = { ...VALID_MANIFEST, scopes: ['apps:storage:shared:write'] }; + // Default (no opts) = genuine submit = enforce. + expect(BlockManifestValidator.validate(manifest, APP_CTX).valid).toBe(false); + }); + + it('APPROVE context (enforceSensitiveScopeJustification:false) does NOT reject on the sensitive-scope rule', () => { + const manifest = { ...VALID_MANIFEST, scopes: ['apps:storage:shared:write'] }; + const result = BlockManifestValidator.validate(manifest, APP_CTX, { + enforceSensitiveScopeJustification: false, + }); + expect(result).toEqual({ valid: true }); + }); + + it('APPROVE context is exempted through validateSubmission too', async () => { + const manifest = { ...VALID_MANIFEST, scopes: ['collections:read:private'] }; + const result = await BlockManifestValidator.validateSubmission(manifest, APP_CTX, { + enforceSensitiveScopeJustification: false, + }); + expect(result).toEqual({ valid: true }); + }); + + it('APPROVE context STILL enforces every OTHER manifest rule (only this one is exempt)', () => { + // A sensitive scope with no justification (would pass the exempted rule) + // but also an UNKNOWN scope + a non-https iframe.src — both must still fail. + const manifest = { + ...VALID_MANIFEST, + scopes: ['apps:storage:shared:write', 'models:read:all'], + iframe: { ...VALID_MANIFEST.iframe, src: 'http://blocks.civitai.com/test' }, + }; + const result = BlockManifestValidator.validate(manifest, APP_CTX, { + enforceSensitiveScopeJustification: false, + }); + expect(result.valid).toBe(false); + if (!result.valid) { + expect(result.errors.some((e) => e.includes('not a known block scope'))).toBe(true); + expect(result.errors.some((e) => e.includes('iframe.src'))).toBe(true); + // The exempted rule did NOT contribute an error. + expect( + result.errors.some((e) => e.includes('sensitive scopes require a justification')) + ).toBe(false); + } + }); + }); + }); + // SUBMISSION gate: validateSubmission = the synchronous shape/security checks // PLUS the accurate ReDoS + input-bound gate on settings-field patterns. This // is what every manifest-SUBMISSION path calls (git-push webhook, developer diff --git a/src/server/services/block-manifest-validator.service.ts b/src/server/services/block-manifest-validator.service.ts index 88867f9696..2ae4dbc1a6 100644 --- a/src/server/services/block-manifest-validator.service.ts +++ b/src/server/services/block-manifest-validator.service.ts @@ -1,5 +1,6 @@ import { isKnownBlockScope, + isSensitiveBlockScope, validateBlockScopesAgainstOauthClient, } from '~/shared/constants/block-scope.constants'; import { isKnownSlotId, isPageSlot } from '~/shared/constants/slot-registry'; @@ -18,6 +19,27 @@ import { SCOPE_JUSTIFICATION_MAX_LENGTH } from '@civitai/auth/token-scope'; type ValidationResult = { valid: true } | { valid: false; errors: string[] }; +/** + * Options controlling WHICH validation rules run. Defaults to full enforcement + * (every rule on); a caller passes this only to RELAX a rule for a specific + * context. Currently the sole knob exempts the sensitive-scope-justification + * enforcement on the moderator APPROVE re-validation. + */ +export type ManifestValidationOptions = { + /** + * Enforce that every declared SENSITIVE scope carries a non-empty + * justification. Default `true` (the genuine submit / new-version paths). The + * moderator APPROVE re-validation passes `false` so a LEGACY pending request — + * submitted before this rule shipped, with a sensitive scope and no + * justification — stays approvable (grandfathered). No bypass is created: a + * post-deploy submission already passed this gate at submit time, so + * re-checking it on approve is redundant, and nothing can reach the approve + * queue post-deploy without first passing the submit gate. ALL OTHER + * validation still runs on approve. + */ + enforceSensitiveScopeJustification?: boolean; +}; + interface RawManifest { blockId?: unknown; version?: unknown; @@ -301,7 +323,11 @@ export class BlockManifestValidator { // Back-compat overload: the existing test suite passes a bitmask number. // Real callers pass the AppContext shape (with allowedOrigins) so the // H8 binding check actually runs. - static validate(manifest: unknown, app: AppContext | number): ValidationResult { + static validate( + manifest: unknown, + app: AppContext | number, + opts?: ManifestValidationOptions + ): ValidationResult { const ctx: AppContext = typeof app === 'number' ? { allowedScopes: app, allowedOrigins: [] } @@ -460,6 +486,49 @@ export class BlockManifestValidator { } } + // ENFORCEMENT (was presentation-only): every declared SENSITIVE scope — the + // subset that can spend/read the viewer's Buzz, read their PRIVATE data, or + // write data other users see (see SENSITIVE_BLOCK_SCOPES) — MUST carry a + // non-empty justification. `scopeJustifications` used to be fully optional; + // for sensitive scopes it is now REQUIRED, so a moderator always sees WHY an + // elevated-risk permission was requested. An entirely-absent + // `scopeJustifications` (previously valid) now fails when any sensitive scope + // is declared. Reuses the single-sourced sensitive set (never re-hardcodes + // it), and runs for every SUBMIT path because both the CLI submit-version and + // the web `updateManifest` funnel through `validateSubmission` → this + // `validate`. + // + // SUBMIT-ONLY (default on): the moderator APPROVE re-validation passes + // `enforceSensitiveScopeJustification:false` so a LEGACY pending request + // (submitted before this shipped, no justification) stays approvable. Only + // THIS rule is exempted on approve — every other check above still runs. + if (opts?.enforceSensitiveScopeJustification !== false && Array.isArray(m.scopes)) { + const justifications = + m.scopeJustifications && + typeof m.scopeJustifications === 'object' && + !Array.isArray(m.scopeJustifications) + ? (m.scopeJustifications as Record) + : {}; + const unjustifiedSensitive = [ + ...new Set( + (m.scopes as unknown[]) + .filter((s): s is string => typeof s === 'string') + .filter((scope) => isSensitiveBlockScope(scope)) + .filter((scope) => { + const raw = justifications[scope]; + return !(typeof raw === 'string' && raw.trim().length > 0); + }) + ), + ]; + if (unjustifiedSensitive.length > 0) { + errors.push( + `sensitive scopes require a justification — add a non-empty scopeJustifications entry for: ${unjustifiedSensitive.join( + ', ' + )}` + ); + } + } + // H8: build the app-bound origin allowlist. Manifest URLs (iframe.src, // assetBundleUrl) must be on origins the registrant actually controls, // not arbitrary HTTPS endpoints. Without this binding, anyone with @@ -718,9 +787,10 @@ export class BlockManifestValidator { */ static async validateSubmission( manifest: unknown, - app: AppContext | number + app: AppContext | number, + opts?: ManifestValidationOptions ): Promise { - const base = this.validate(manifest, app); + const base = this.validate(manifest, app, opts); const errors: string[] = base.valid ? [] : [...base.errors]; const settings = diff --git a/src/server/services/blocks/publish-request.service.ts b/src/server/services/blocks/publish-request.service.ts index b1a6a4b284..6e26439133 100644 --- a/src/server/services/blocks/publish-request.service.ts +++ b/src/server/services/blocks/publish-request.service.ts @@ -2119,7 +2119,16 @@ export async function approveRequest(params: ApproveRequestParams): Promise