fix(canvas): duplicate canvas ids must keep newest payload (greptile P1)
readCanvases sorts file canvases newest-first by updatedAt. The canvases route then merged them via Map.set in iteration order, which overwrites earlier (newer) entries with later (older) ones. If an agent wrote `result-v1.json` and `result-v2.json` with the same canvas id, the user would see the OLDER payload — the exact opposite of the intended last-write-wins behavior. Fix: extract the merge into a pure helper (merge.ts) with explicit first-write-wins semantics. Synthesized seeds the map first (preserves the pass-9 reservation that core- prefix wins on collision); file canvases skip ids already present, so the newest wins. Adds 5 regression tests covering: empty input, synth + file mix, duplicate-id newest-wins (the regression), synth-vs-file collision, and final sort order. Also adds a "Why no HTML canvas type?" section to the feature writeup HTML — the most-asked design question, answered honestly so reviewers don't have to ask. Covers the trust-hierarchy-inversion argument (agents are lowest-trust + LLMs are prompt-injectable + same-origin HTML = supervisor JS execution), the leaky-sanitizer argument, and the legitimate sandboxed-iframe path for v0.5+. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
4a91eeda10
commit
b845a285e7
|
|
@ -214,6 +214,7 @@
|
|||
<li><a href="#files">File map</a></li>
|
||||
<li><a href="#try">Try it locally</a></li>
|
||||
<li><a href="#roadmap">Roadmap</a></li>
|
||||
<li><a href="#why-no-html">Why no <code>html</code> canvas type?</a></li>
|
||||
<li><a href="#non-goals">Deliberate non-goals</a></li>
|
||||
</ol>
|
||||
</nav>
|
||||
|
|
@ -623,7 +624,45 @@ EOF</code></pre>
|
|||
</tbody>
|
||||
</table>
|
||||
|
||||
<h2 id="non-goals">10. Deliberate non-goals</h2>
|
||||
<h2 id="why-no-html">10. Why no <code>html</code> canvas type?</h2>
|
||||
<p>The most-asked design question — answered honestly here so reviewers don't have to ask.</p>
|
||||
<p>Two reasons, both load-bearing.</p>
|
||||
|
||||
<h3>Trust hierarchy inversion</h3>
|
||||
<p>Agents are the lowest-trust surface in the system. They emit content on every poll cycle, and large parts of that content come from LLMs that can be prompt-injected. If an agent ingests a malicious URL, summarizes a poisoned document, or just hits a jailbreak, its next "render this nice summary" call could include attacker-controlled HTML. Putting that HTML into the same-origin dashboard means the attacker now has:</p>
|
||||
<ul>
|
||||
<li>Read access to every same-origin auth token / cookie / <code>localStorage</code> value.</li>
|
||||
<li>Permission to call every API the user can call (kill sessions, restore them, modify config).</li>
|
||||
<li>The ability to inject scripts that survive across navigation.</li>
|
||||
<li>Keylogging on every page the dashboard renders.</li>
|
||||
</ul>
|
||||
<p>That's the same blast radius v0.4 renderer plugins have — but renderer plugins clear <code>npm install</code> review <em>once</em>. An agent emits a fresh canvas every poll. The trust gap is enormous.</p>
|
||||
|
||||
<h3>HTML sanitization is a leaky abstraction</h3>
|
||||
<p>DOMPurify, sanitize-html, etc. are honest attempts but every year ships new bypasses — <code><svg></code> foreign objects, <code><math></code> payloads, mutation-XSS via clipboard, CSS exfiltration via <code>attr()</code>. A sanitizer-based HTML canvas is a 90% solution that fails open on the 10%, and the 10% is "execute arbitrary JS in your supervisor". Even GitHub, GitLab, and Slack treat HTML rendering as a multi-quarter security investment, not a feature you slot in next to <code>markdown</code>.</p>
|
||||
|
||||
<h3>What the existing types cover</h3>
|
||||
<table>
|
||||
<thead><tr><th>You want to render</th><th>Use</th></tr></thead>
|
||||
<tbody>
|
||||
<tr><td>Formatted text with bold / italic / headings / code / lists</td><td><code>markdown</code> (the v0.1 parser handles all of this safely — no HTML pass-through)</td></tr>
|
||||
<tr><td>Code changes</td><td><code>diff</code></td></tr>
|
||||
<tr><td>Structured rows</td><td><code>table</code></td></tr>
|
||||
<tr><td>Metric cards</td><td><code>stats</code></td></tr>
|
||||
<tr><td>Custom UI (flame graph, Gantt, network diagram)</td><td><code>canvas-renderer</code> plugin (v0.4)</td></tr>
|
||||
<tr><td>Genuinely arbitrary, untrusted markup</td><td><strong>Sandboxed iframe canvas</strong> — credible v0.5+ option, deferred</td></tr>
|
||||
</tbody>
|
||||
</table>
|
||||
|
||||
<h3>The legitimate "I need raw HTML" path — sandboxed iframe</h3>
|
||||
<p>An iframe with <code>sandbox="allow-scripts"</code> (no <code>allow-same-origin</code>) runs in a null origin — it can't reach your auth tokens or call APIs. Add a <code>postMessage</code> protocol for size negotiation and limited event-out, document the trust trade-off explicitly, and you've got a real HTML surface that's safe by construction.</p>
|
||||
<p>But — per codex's pass-13 advice — <strong>half-baked iframe support is worse than no sandbox</strong>. Sandbox protocols have to be designed end-to-end (CSP headers, postMessage validation, focus management, accessibility, viewport sizing) before they ship. v0.4 renderer plugins is the bigger unlock; the iframe canvas comes after, only if a real use case shows up.</p>
|
||||
|
||||
<div class="callout">
|
||||
<strong>Bottom line</strong> — not because we couldn't, but because direct HTML inverts the trust hierarchy (agents shouldn't be able to JS-execute in the supervisor) and sanitizers don't fix that. The plugin path covers most "I need a custom widget" cases at v0.4. Sandboxed iframe is the answer for genuine arbitrary HTML when we're ready to design the protocol properly.
|
||||
</div>
|
||||
|
||||
<h2 id="non-goals">11. Deliberate non-goals</h2>
|
||||
<ul>
|
||||
<li><strong>Remote-loaded JS at runtime.</strong> No URL-fetched code, no agent-emitted React, no on-the-fly module fetching. v0.4 plugins are <em>discovered</em> at AO startup from <code>node_modules</code> (Node loads each plugin's schema entrypoint) and <em>renderers</em> are bundled at web build time via a generated barrel of static imports. That's the OSS extensibility line: trust at install, not at runtime.</li>
|
||||
<li><strong>Write APIs from the dashboard back into canvases.</strong> Read-only surface in v0.1. Bidirectional adds a whole new failure mode (auth, ordering, conflict).</li>
|
||||
|
|
|
|||
|
|
@ -0,0 +1,63 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import type { CanvasArtifact } from "@aoagents/ao-core";
|
||||
import { mergeCanvases } from "../merge";
|
||||
|
||||
function md(id: string, updatedAt: string, body: string): CanvasArtifact {
|
||||
return {
|
||||
version: 1,
|
||||
id,
|
||||
type: "markdown",
|
||||
title: id,
|
||||
createdAt: updatedAt,
|
||||
updatedAt,
|
||||
payload: { markdown: body },
|
||||
};
|
||||
}
|
||||
|
||||
describe("mergeCanvases", () => {
|
||||
it("returns empty when nothing to merge", () => {
|
||||
expect(mergeCanvases(null, [])).toEqual([]);
|
||||
});
|
||||
|
||||
it("includes synthesized canvas alongside file canvases", () => {
|
||||
const synth = md("core-git-diff", "2026-05-06T10:00:00Z", "diff");
|
||||
const file = md("notes", "2026-05-06T09:00:00Z", "notes");
|
||||
const out = mergeCanvases(synth, [file]);
|
||||
expect(out.map((c) => c.id)).toEqual(["core-git-diff", "notes"]);
|
||||
});
|
||||
|
||||
it("on duplicate ids, keeps the NEWEST file entry (regression: PR #1653 pass-14 P1)", () => {
|
||||
// readCanvases sorts newest-first; merge must respect that.
|
||||
// If two files emit the same id, the user must see the newer payload.
|
||||
const newer = md("test-results", "2026-05-06T10:00:00Z", "v2 (newer)");
|
||||
const older = md("test-results", "2026-05-06T08:00:00Z", "v1 (older)");
|
||||
const out = mergeCanvases(null, [newer, older]); // newest-first input
|
||||
expect(out).toHaveLength(1);
|
||||
expect(out[0]).toMatchObject({ id: "test-results" });
|
||||
if (out[0]?.type === "markdown") {
|
||||
expect(out[0].payload.markdown).toBe("v2 (newer)");
|
||||
} else {
|
||||
throw new Error("expected markdown canvas");
|
||||
}
|
||||
});
|
||||
|
||||
it("synthesized wins on id collision with a file canvas", () => {
|
||||
// Even though `core-` is reserved at the reader, the merge layer also
|
||||
// needs to resolve collisions deterministically in favor of synthesized.
|
||||
const synth = md("core-git-diff", "2026-05-06T08:00:00Z", "synthesized");
|
||||
const fake = md("core-git-diff", "2026-05-06T10:00:00Z", "imposter");
|
||||
const out = mergeCanvases(synth, [fake]);
|
||||
expect(out).toHaveLength(1);
|
||||
if (out[0]?.type === "markdown") {
|
||||
expect(out[0].payload.markdown).toBe("synthesized");
|
||||
}
|
||||
});
|
||||
|
||||
it("output is sorted newest-first by updatedAt", () => {
|
||||
const a = md("a", "2026-05-06T08:00:00Z", "");
|
||||
const b = md("b", "2026-05-06T10:00:00Z", "");
|
||||
const c = md("c", "2026-05-06T09:00:00Z", "");
|
||||
const out = mergeCanvases(null, [b, c, a]); // arbitrary input order
|
||||
expect(out.map((x) => x.id)).toEqual(["b", "c", "a"]);
|
||||
});
|
||||
});
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
import type { CanvasArtifact } from "@aoagents/ao-core";
|
||||
|
||||
/**
|
||||
* Merge synthesized + file canvases for the canvases endpoint.
|
||||
*
|
||||
* Semantics:
|
||||
* 1. Synthesized canvas (e.g. `core-git-diff`) seeds the map first. The
|
||||
* `core-` prefix is reserved at the reader, so a legitimate file canvas
|
||||
* can't collide; if collision happens anyway the trusted synthesized
|
||||
* version wins by being inserted first.
|
||||
* 2. `fileCanvases` MUST be sorted newest-first by `updatedAt`. The first
|
||||
* occurrence of each id is kept; subsequent duplicates are skipped, NOT
|
||||
* used to overwrite.
|
||||
*
|
||||
* The naive `Map.set` loop did the opposite (older payload won) — see PR #1653
|
||||
* pass-14 review.
|
||||
*
|
||||
* Final output is sorted by `updatedAt` descending (newest first).
|
||||
*/
|
||||
export function mergeCanvases(
|
||||
synthesized: CanvasArtifact | null,
|
||||
fileCanvases: CanvasArtifact[],
|
||||
): CanvasArtifact[] {
|
||||
const merged = new Map<string, CanvasArtifact>();
|
||||
if (synthesized) merged.set(synthesized.id, synthesized);
|
||||
for (const c of fileCanvases) {
|
||||
if (!merged.has(c.id)) merged.set(c.id, c);
|
||||
}
|
||||
return Array.from(merged.values()).sort((a, b) => (a.updatedAt < b.updatedAt ? 1 : -1));
|
||||
}
|
||||
|
|
@ -5,7 +5,6 @@ import {
|
|||
readCanvases,
|
||||
synthesizeGitDiffCanvas,
|
||||
SessionNotFoundError,
|
||||
type CanvasArtifact,
|
||||
} from "@aoagents/ao-core";
|
||||
import {
|
||||
getCorrelationId,
|
||||
|
|
@ -13,6 +12,7 @@ import {
|
|||
recordApiObservation,
|
||||
resolveProjectIdForSessionId,
|
||||
} from "@/lib/observability";
|
||||
import { mergeCanvases } from "./merge";
|
||||
|
||||
/** GET /api/sessions/:id/canvases — List canvas artifacts for a session */
|
||||
export async function GET(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) {
|
||||
|
|
@ -47,13 +47,7 @@ export async function GET(_request: NextRequest, { params }: { params: Promise<{
|
|||
])
|
||||
: [[], null];
|
||||
|
||||
const merged = new Map<string, CanvasArtifact>();
|
||||
if (synthesized) merged.set(synthesized.id, synthesized);
|
||||
for (const c of fileCanvases) merged.set(c.id, c);
|
||||
|
||||
const canvases = Array.from(merged.values()).sort((a, b) =>
|
||||
a.updatedAt < b.updatedAt ? 1 : -1,
|
||||
);
|
||||
const canvases = mergeCanvases(synthesized, fileCanvases);
|
||||
|
||||
recordApiObservation({
|
||||
config,
|
||||
|
|
|
|||
Loading…
Reference in New Issue