diff --git a/packages/shared/src/artifact-shapes.ts b/packages/shared/src/artifact-shapes.ts index 945c342be..d72260b1b 100644 --- a/packages/shared/src/artifact-shapes.ts +++ b/packages/shared/src/artifact-shapes.ts @@ -149,11 +149,10 @@ export function resolveLegacyShape( case 'result': { const d = data ?? {}; const hasSummary = typeof d.summary === 'string' && d.summary.length > 0; - const hasUrl = - typeof d.url === 'string' || - typeof d.pr_url === 'string' || - typeof d.prUrl === 'string' || - typeof d.review_url === 'string'; + // findLinkUrl recognizes any *_url field (pr_url, merged_pr_url, + // review_url, …), so the post-approval merge audit (merged_pr_url) routes + // to a link like other URL-bearing results. + const hasUrl = findLinkUrl(d) !== null; // A summary is the important payload (QA outcome / completion note) and // must stay visible to decision readers, so prefer decision when present. // Only a URL-only result becomes a pure link. @@ -168,6 +167,26 @@ export function isArtifactShape(value: unknown): value is ArtifactShape { return typeof value === 'string' && (ARTIFACT_SHAPES as readonly string[]).includes(value); } +/** + * Find a URL-bearing field in legacy data. Pre-shape producers stored the URL + * under various keys (`url`, `pr_url`, `prUrl`, `review_url`, `merged_pr_url`, + * `image_url`, `external_url`, …), so match any key that is `url` or ends in + * `_url`/`Url`. Returns the first non-empty string value found, else null. + */ +function findLinkUrl(data: Record | undefined): string | null { + if (!data) return null; + for (const [key, value] of Object.entries(data)) { + if ( + (key === 'url' || key.endsWith('_url') || key.endsWith('Url')) && + typeof value === 'string' && + value + ) { + return value; + } + } + return null; +} + /** * For a legacy row being treated as a `link`, copy the URL-bearing field onto * `data.url` so link readers (which key off `data.url`) find it. Returns a new @@ -175,10 +194,7 @@ export function isArtifactShape(value: unknown): value is ArtifactShape { */ export function normalizeLinkData(data: Record): Record { if (typeof data.url === 'string' && data.url) return data; - const url = - (typeof data.pr_url === 'string' && data.pr_url) || - (typeof data.prUrl === 'string' && data.prUrl) || - (typeof data.review_url === 'string' && data.review_url); + const url = findLinkUrl(data); if (!url) return data; return { ...data, url }; } @@ -259,11 +275,26 @@ export function validateArtifactShape( data: Record ): ArtifactValidation { switch (shape) { - case 'link': + case 'link': { if (!nonEmptyString(data.url)) { return { ok: false, error: "shape 'link' requires data.url (the URL)." }; } + // Defense-in-depth: link URLs are agent-controlled and rendered as + // clickable anchors, so restrict storage to http(s). This prevents + // `javascript:` / custom-scheme URLs from ever reaching a renderer. + try { + const parsed = new URL(data.url as string); + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + return { + ok: false, + error: `shape 'link' requires an http(s) URL (got '${parsed.protocol}').`, + }; + } + } catch { + return { ok: false, error: "shape 'link' requires a valid http(s) URL." }; + } return { ok: true }; + } case 'check': if (!nonEmptyString(data.name)) { return { ok: false, error: "shape 'check' requires data.name (the check identity)." }; diff --git a/packages/shared/tests/artifact-shapes.test.ts b/packages/shared/tests/artifact-shapes.test.ts index 161e29154..78e83b059 100644 --- a/packages/shared/tests/artifact-shapes.test.ts +++ b/packages/shared/tests/artifact-shapes.test.ts @@ -77,10 +77,18 @@ describe('artifact-shapes: deriveArtifactKey (identity rules)', () => { describe('artifact-shapes: validateArtifactShape', () => { test('link requires data.url', () => { expect(validateArtifactShape('link', { url: 'https://x' })).toEqual({ ok: true }); + expect(validateArtifactShape('link', { url: 'http://x.example' })).toEqual({ ok: true }); const bad = validateArtifactShape('link', { title: 'no url' }); expect(bad.ok).toBe(false); }); + test('link rejects non-http(s) URLs (defense-in-depth against agent-controlled schemes)', () => { + expect(validateArtifactShape('link', { url: 'javascript:alert(1)' }).ok).toBe(false); + expect(validateArtifactShape('link', { url: 'data:text/html,