oficina / docs Public

Download this file

1	# Plan 7 — images, links and tables: what a pasted document keeps
2
3	Brief, from Paul, 2026-09-02: *"make a plan to handle images, links, and
4	tables. Images can be rendered immediately and uploaded async to object
5	storage"* — after pasting a Google Doc and finding the table flattened
6	and the formatting gone.
7
8	Spec §11.2 already commits to all three: *"The schema covers headings and
9	named styles, lists, tables, images, links, headers and footers, page
10	setup and comments."* So none of this is a new decision about scope. What
11	it is, is one decision about a safety property that two of the three
12	depend on, and a storage seam the app does not yet have.
13
14	*Revision 2, after Fable's review. It changed two decisions — D4's
15	ordering and D6's expectation — and corrected several facts, the largest
16	being that revision 1 proposed writing an escaper that already exists.*
17
18	## What is actually broken today
19
20	Verified against `origin/main` by running Google-Docs clipboard HTML
21	through the shipped schema and the real wazero core.
22
23	**Tables are not in the schema at all.** A pasted table becomes one
24	top-level paragraph per cell, in row-major order, the grid gone. The
25	When/What/Who table in Paul's rollout doc came out as six consecutive
26	paragraphs. `core/src/fragment.rs`, `internal/export/markdown.go` and
27	`internal/export/pdf.go` know nothing about tables either.
28
29	**Links and images are half-present, which is worse than absent.**
30	`link` and `image` come from `prosemirror-schema-basic`, so they are in
31	the *browser* schema. A paste writes them into the CRDT. Then
32	`fragment.rs` drops them: `link` is not in `MARKS` (`fragment.rs:77-87`),
33	so the read view renders the anchor text with no anchor, and `image` hits
34	the unknown-element arm (`fragment.rs:203-204`) and renders as **nothing
35	at all**. Both exporters are equally blind. This is the module comment's
36	own "silence here is data loss", happening now.
37
38	## D1 — the safety property, and what it costs to spend it
39
40	`fragment_html`'s output is returned as `template.HTML` and written
41	unescaped by `views/show.html`. It is safe today because of a property
42	the module states and maintains: **no byte from a document reaches
43	attribute position**. Marks are attribute-free and render as fixed
44	literals; `paragraph.align` is only ever a key into a closed table whose
45	right-hand side is a literal; `heading.level` is parsed as an integer and
46	clamped.
47
48	**The escaper already exists** — `fragment.rs:25` escapes `&`, `<`, `>`,
49	`"` and `'`, ampersand first. What is missing is everything about
50	*which* values are allowed to reach an attribute at all.
51
52	**Tables do not spend the property.** `colspan` and `rowspan` are
53	integers, clamped exactly as `heading.level` already is.
54
55	**Links and images do.** So this plan buys it back deliberately:
56
57	**1. A positive scheme grammar, not a denylist.** A rejected-characters
58	rule is the right instinct and the wrong shape: browsers strip ASCII tab
59	and newline anywhere in a URL and strip leading C0 controls, but not
60	U+00A0 or U+2028, and a value with no colon at all is a third case. One
61	rule covers all of them — the value must match
62	`^[A-Za-z][A-Za-z0-9+.-]*:`, and that scheme, case-folded, must be one of
63	`http`, `https`, `mailto`. Everything else renders as text: no colon,
64	`//evil`, `data:`, `blob:`, `java\tscript:`, `\u{0}javascript:`.
65	`blob:` matters more than it looks — it is what this very feature
66	manipulates in the browser, and it must never reach the CRDT.
67
68	**2. `src` is matched against its literal shape, not merely "app-relative".**
69	`//evil.example/x.png` begins with `/`. The rule is
70	`^/d/[A-Za-z0-9]+/blobs/[0-9a-f]{64}$` and nothing else. It does not
71	check that the sqid is *this* document — `fragment_html(doc)` has no sqid
72	— because the GET route does that by construction (D4).
73
74	**3. The allowlist runs once, in the shared path, not per renderer.**
75	This is the correction that matters most. If the check lived only in
76	`node_html`, then `markdown.go` would emit `[text](javascript:…)` and the
77	PDF's `/Link` annotation would carry the same, unless each exporter
78	re-implemented the check — a second copy of a security table, which is
79	the exact coupling this plan claims to avoid. Instead a rejected `href`
80	means the run is emitted **with no `link` mark at all**, in
81	`fragment_json`/`text_json` as much as in `fragment_html`. No consumer
82	can then see a rejected URL, because none is ever produced.
83
84	**4. Accepted image types are sniffed, and SVG is not one of them.** If
85	`/d/{sqid}/blobs/{hash}` can serve `image/svg+xml`, navigating to it runs
86	script on the app's own origin. JPEG, PNG, GIF and WebP only, sniffed
87	from the bytes with `http.DetectContentType` and never taken from the
88	client's header, served with `X-Content-Type-Options: nosniff` and
89	`Content-Security-Policy: sandbox`. This is a safety rule, not an
90	implementation detail of the upload route.
91
92	**5. Adversarial tests in `core/tests/fragment.rs`**, covering every item
93	in 1 and 2 plus `"><script>` in `title` and `alt`.
94
95	## D2 — tables are `prosemirror-tables`, not a hand-rolled node set
96
97	`prosemirror-tables` (1.8.5) is the same team's package, is what TipTap's
98	table extension wraps, and brings the parts that are genuinely hard: cell
99	selection, the column and row commands, and the fixups that keep a table
100	rectangular after every edit. Hand-rolling those is how you get a model
101	that can represent a broken grid.
102
103	It contributes `table`, `table_row`, `table_cell`, `table_header`, with
104	`colspan`, `rowspan` and `colwidth` on cells. Its dependency ranges all
105	dedupe onto the versions `fixtures/package.json` already pins — check
106	with `npm ls prosemirror-model` after installing, because two copies of
107	`prosemirror-model` break ProseMirror silently. It exports commands and
108	the `tableEditing()` plugin but **no keymap**, so Tab and Shift-Tab are
109	ours to bind, and it needs `.selectedCell` CSS for cell selection to be
110	visible at all.
111
112	**Column resizing is off in this slice.** `columnResizing` is what
113	populates `colwidth`, and `colwidth` is an *array* — a different shape
114	from anything on the wire today. Off, it stays null, and y-prosemirror
115	skips null attributes entirely. Google Docs' own cell widths arrive as
116	`style="width:…"` rather than `data-colwidth`, so a paste leaves it null
117	too. Resizing can come later with no migration.
118
119	`colspan` and `rowspan` are **not** null: they default to `1`, so every
120	cell carries both as numeric attributes. That is the one way the wire
121	gets busier. Clamp both to `1..=1000` on the Rust side, the way
122	`heading.level` is clamped. `table_header` renders `<th scope="col">`, a
123	literal. No new closed set of *values* — these are node names, which
124	every consumer already dispatches on by name.
125
126	Google Docs tables contain no `<th>`, so a paste produces zero
127	`table_header` rows. `<tbody>` and `<colgroup>` are transparent to
128	ProseMirror's DOMParser.
129
130	## D3 — a link is a mark with an href, and the read view renders it
131
132	`link` is already in the browser schema and already round-trips. What is
133	missing is the reader. Once D1 exists this is a row in `MARKS` — except
134	`MARKS` is a table of literal open/close strings and `link` must compute
135	its open tag, so the table grows a variant for a valued mark rather than
136	gaining a second table beside it.
137
138	Rendered as `<a href="…" rel="noreferrer">`. Not `noopener`, which does
139	nothing without `target`; `noreferrer` is the one that earns its place,
140	because it stops the document's own URL — sqid and all — reaching the
141	link target. No `target`: a document is a page, and opening a new tab is
142	the reader's choice, not the author's.
143
144	`internal/export/document.go` has to grow with this: its `Mark` is
145	`{Type string}` and its `Attrs` is closed to `Level` and `Align`, so both
146	need widening and `text_json` needs to emit mark attributes.
147
148	## D4 — the image is on screen at once; the node lands when the bytes do
149
150	Paul's brief is that an image renders immediately and uploads in the
151	background. Revision 1 satisfied that by inserting the node with its
152	final `src` up front — hashing the bytes in the browser so the URL was
153	known before the upload. **That was the wrong half of the trick**, and
154	this revision reverses it.
155
156	The reason is what the other windows see. A node inserted before its
157	bytes exist reaches every other tab within ~250ms, where the GET 404s and
158	the reader gets a broken image; a no-JavaScript reader gets the same; and
159	a permanently failed upload leaves a dangling `src` in a CRDT every tab
160	already holds, so "replace the node with its alt text" becomes cross-tab
161	node surgery with no obvious owner. It did not even buy what it claimed:
162	`crypto.subtle.digest` is async and `transformPasted` is synchronous, and
163	the Google Docs path has no bytes at paste time at all, so a placeholder
164	was always required.
165
166	**So: render by decoration, insert on success.** §6.1's own rule is that
167	placeholders are decorations, never nodes. The local tab shows the
168	`blob:` preview as a decoration — ProseMirror's own upload example — and
169	the node is inserted into the CRDT only when the upload returns 2xx, at
170	the decoration's mapped position. The decoration set maps through every
171	intervening transaction; if an undo removed it, the result is discarded.
172	Paul's brief is met exactly — instant on screen, upload async — and the
173	entire dangling-`src` failure class disappears: no other tab and no
174	reader ever sees a `src` that cannot resolve, and retry-then-drop becomes
175	"remove the decoration and say so". Given the save queue's debounce,
176	collaborators would not have seen the image sooner the other way round.
177
178	Once insertion follows the upload, **the server computes the hash and
179	returns the URL**. The browser never hashes, which also disposes of
180	`crypto.subtle`'s secure-context requirement and of the client/server
181	mismatch case.
182
183	**The mapping table is still needed.** `blobs.Sealed.Put` runs
184	`crypto.SealSym`, which draws a random IV, so the stored address is the
185	ciphertext's hash and is not derivable from the plaintext (§11.5 says as
186	much). `Store` has no put-under-key. So:
187
188	document_blobs(document_id, plain_hash, blob_hash, content_type,
189	plain_size, created_at)
190	unique (document_id, plain_hash)
191
192	`plain_hash` is what the URL carries and the CRDT holds; `blob_hash` is
193	the `blobs.Ref.Hash` that `Store.Get` wants. `content_type` is a column
194	because §11.5 requires the *stored* type to be
195	`application/octet-stream`, so the true type cannot be recovered from the
196	store at serve time. `plain_size` is the plaintext size, not `Ref.Size`,
197	which is the sealed one. The unique constraint short-circuits a repeated
198	paste of the same image before `Put`, so re-pasting does not mint a fresh
199	ciphertext object every time.
200
201	Authorisation falls out of placement: `GET /d/{sqid}/blobs/{hash}` sits
202	inside the existing owner-scoped group in `app.go`, and the handler
203	resolves the document through `Store.Get` **before** looking up the row,
204	so an image is exactly as reachable as the document referencing it — by
205	construction, not by a second check somebody has to remember.
206
207	**A sealing key does not exist yet, and that is a decision, not a line.**
208	Nothing in `internal/` or `cmd/` calls `SealSym` today; this is the first
209	sealing the app does. It needs a key with the same fail-closed-outside-
210	development shape as `InstanceKey`, and a stated relationship to the
211	eventual per-document DEK. **This needs Paul's call before Task 6.**
212
213	Two things to add to the spec's §2.1 leak list while doing it: the
214	plaintext SHA-256 sits in a plaintext column and in every access-log line
215	for the URL (confirm-a-guess only, but new), and `alt` text rides in
216	`doc_updates.update_bin`, which is itself still plaintext.
217
218	**Copying an image between documents** gives document B an
219	`<img src="/d/A/blobs/…">` that B's readers may not be able to fetch. The
220	paste handler should recognise a same-origin `/d/…/blobs/…` src from
221	another document, fetch it (same origin, no CORS) and re-upload under B.
222
223	## D5 — what each renderer does with an image
224
225	`image` in `prosemirror-schema-basic` is **inline**, and Google Docs
226	pastes images inside `<p>`, so it stays inline. That is the path of least
227	resistance and it means the PDF breaks the line around an image rather
228	than laying it out as a block.
229
230	- **Read view** (`fragment.rs`): `<img src="…" alt="…">`, both escaped,
231	`src` matched against D1's literal shape. No dimensions in this slice,
232	so the page reflows as images load.
233	- **Markdown** (`markdown.go`): `` — and this is a
234	**dead link the moment the `.md` leaves the app**. Say so in the export
235	rather than calling it lossless.
236	- **PDF** (`pdf.go` and `internal/pdf`): JPEG embeds as `DCTDecode` and
237	PNG decodes with `image/png` and re-deflates — no rasterizer, so
238	§11.4's `internal/raster` is **not** pulled in. Three details revision 1
239	missed: `internal/pdf`'s `addStream` Flate-compresses every stream
240	unconditionally and needs an uncompressed variant for JPEG; the
241	`/Resources` dict it builds carries only `/Font` and needs `/XObject`;
242	and PNG alpha needs a separate `/SMask` XObject or the image renders on
243	black, with 16-bit and palette PNGs decoded to 8-bit RGB first. CMYK
244	JPEGs need `/Decode [1 0 1 0 1 0 1 0]` or should take the decode path.
245	- **A blob the store does not have** renders as the alt text — in the Go
246	exporters, which can ask the store. `fragment.rs` cannot: it is a pure
247	function of the CRDT with no store access. The browser covers the
248	residual race by retrying on `img.onerror` with bounded backoff.
249
250	## D6 — the Google Docs image, and what to test first
251
252	A Google Docs paste gives a `googleusercontent.com` URL, not bytes.
253	Revision 1 assumed fetching it from the browser would be CORS-blocked and
254	planned around that. **The evidence points the other way**: a
255	`googleusercontent.com` fetch with a cross-origin `Origin` header returns
256	`access-control-allow-origin: *` and `cross-origin-resource-policy:
257	cross-origin`, from the same `fife` infrastructure that serves the
258	`lh7-rt.googleusercontent.com/docsz/…` URLs in the clipboard HTML — which
259	are signed-and-expiring rather than cookie-gated, so "credentialed" was
260	probably wrong too.
261
262	That is one observation on one URL, not proof for `docsz`. So the test
263	stands, but it moves: **fetch a real Docs image URL cross-origin before
264	building any of Task 7**, because the expected answer is now "it works",
265	and that changes the deliverable rather than confirming a fallback.
266
267	The real risk is **expiry**, not CORS: the URL dies after some window, so
268	a paste sitting in a retry loop can go from fetchable to 403. Fetch
269	early, once, and fail loudly.
270
271	Server-side fetching stays out. It is an SSRF surface with its own threat
272	model and should not arrive as an implementation detail of a paste.
273
274	**"Degrade loudly" needs something to degrade into, and it does not
275	exist.** There is no paste report anywhere in the codebase, and §11.2's
276	import report is unbuilt. Both D4's failure path and D6's depend on it,
277	so it is a task with UI, catalog keys and a copy review — not a sentence.
278
279	## D7 — order, and why
280
281	**Tables, then links, then images.** Tables need no new safety property
282	and no new storage. Links buy D1 and pay for it with adversarial tests.
283	Images need D1, a new table, a new route, a sealing key, an upload path
284	and PDF work — and carry D6.
285
286	D1 is **not** its own branch. Revision 1 proposed landing it with no
287	caller "so it can be reviewed on its own"; AGENTS.md's own record is
288	against that — `devauth.go` was deleted for having zero readers. It is
289	the first commit of the links branch instead, reviewed as a commit.
290
291	## The shape of the work
292
293	### Task 1 — tables in the schema and on the wire
294	`fixtures/editor-entry.js` (the four node types, `tableEditing()`, Tab
295	and Shift-Tab bound by hand), `fixtures/package.json`,
296	`fixtures/tables.test.mjs`, the gate. Measure the bundle: roughly 40KB
297	minified against the current 320KB, and visible in the diff because §6.5
298	forbids a build step.
299
300	### Task 2 — tables in the three renderers
301	`core/src/fragment.rs`: the four arms, `colspan`/`rowspan` parsed and
302	clamped, `table_header` → `<th scope="col">`. `node_text` needs a ruling:
303	cells contain paragraphs, which already push their own entries into
304	`blocks`, so either add a mode or accept one block per cell in the
305	plain-text preview — the latter is fine for a list preview and should be
306	stated rather than discovered. `internal/export/markdown.go`: GFM tables,
307	whose delimiter row is mandatory and whose first row **is** the header —
308	so a Google Docs table with no `table_header` needs its first row
309	promoted. Pipes escape as `\|`. `colspan`, `rowspan`, multi-paragraph
310	cells and lists in cells are outside GFM entirely and the export must say
311	so rather than claiming losslessness. `internal/export/pdf.go`: **bound
312	this explicitly or it swallows the branch** — equal column widths from
313	the text column, cells wrap, a row that will not fit starts a new page,
314	no cell merging across pages, `colspan` rendered, `rowspan` degraded to
315	repeated cells.
316
317	### Task 3 — tables on screen
318	`views/show.html`: table CSS in the `[docs-text]` vocabulary, borders
319	that read as a document rather than a spreadsheet, `overflow-x: auto` on
320	a wrapper so a wide table scrolls inside the page, and `.selectedCell`.
321	`fixtures/toolbar.mjs`: insert table, add/remove row and column. WCAG 2.2
322	AA, every string a catalog key.
323
324	### Task 4 — new wire fixtures
325	`fixtures/generate-fragment.mjs` hand-builds its vectors from
326	y-prosemirror's described behaviour. That is fine for tables (element
327	names and numeric attrs, the same shape as `aligned.bin`) and **not fine
328	for `link`**, whose wire value is a `mark.attrs` object under the mark
329	name — a shape no existing fixture has. Generate `links.bin` through
330	y-prosemirror's own `prosemirrorToYXmlFragment` so Rust is tested against
331	the real encoding rather than a transcription of it. `tables.bin` and
332	`images.bin` alongside. Existing fixtures do not change; these are
333	additions, and they are the gate for the new types.
334
335	### Task 5 — the allowlist, then links
336	First commit: D1's grammar, the reject-to-text path in the shared
337	`fragment_json`/`text_json` path, and the adversarial tests. Second:
338	`MARKS`' valued variant, `document.go`'s `Mark` and `Attrs`,
339	`markdown.go`'s `[text](href)`, `pdf.go`'s link styling and `/Link`
340	annotation, `toolbar.mjs`'s add/remove link.
341
342	### Task 6 — the sealing key, the blob store and the route
343	**Blocked on Paul's key decision.** Then `models.go` plus a migration for
344	`document_blobs`; `blobs.Sealed` over `blobs.S3FromEnv()` in
345	`cmd/docs/main.go`, falling back to `blobs.Dir` for local runs;
346	`POST /d/{sqid}/blobs` and `GET /d/{sqid}/blobs/{hash}` in the
347	owner-scoped group, with the size cap, D1's sniffed type list and its
348	response headers. §12's plaintext-grep gate already covers bucket
349	objects.
350
351	### Task 7 — the paste report
352	The thing every "degrade loudly" path needs and none of them has. UI,
353	catalog keys, copy review.
354
355	### Task 8 — images from a file
356	`fixtures/image-upload.mjs` (decoration, upload, retry-then-drop, reusing
357	save-queue's shape) and its test; insertion on success at the mapped
358	position; `fragment.rs`, `markdown.go` per D5.
359
360	### Task 9 — images from Google Docs
361	Only after D6's fetch test. The cross-origin fetch, its expiry handling,
362	and the report entry when it fails.
363
364	### Task 10 — images in the PDF
365	`internal/pdf`'s uncompressed stream variant, `/XObject` in `/Resources`,
366	`/SMask` for alpha, the decode paths. Its own task because it is its own
367	body of work.
368
369	### Task 11 — copying an image between documents
370	Recognise a same-origin `/d/…/blobs/…` src from another document, fetch
371	and re-upload under this one. Or an explicit "not in this slice" line —
372	but not silence.
373
374	## Risks
375
376	| Risk | Where | What to do |
377	|---|---|---|
378	| The Docs fetch behaves differently from the probe | D6 | Test a real Doc image **first**. It sets the shape of Tasks 9 and 7. |
379	| A Docs URL expires mid-retry | D6 | Fetch early, once, fail loudly. |
380	| No sealing key decision | D4, Task 6 | Paul's call. Blocks Task 6. |
381	| PDF tables run away | Task 2 | The explicit bound above. |
382	| PDF images touch `internal/pdf`'s stream and resource assumptions | Task 10 | Its own task, not a corner of another. |
383	| `validate: "string"` on `image.src`/`link.href` | All | A raw update with a non-string attr makes y-prosemirror **delete the type**. Pre-existing, but links and images widen it from "attrs the editor never writes" to "attrs every paste writes". Not this plan's to fix; recorded so it is not rediscovered as a mystery. |
384	| Two `prosemirror-model` copies | Task 1 | `npm ls prosemirror-model` after install. Breaks silently. |
385	| Two branches inventing the same vocabulary | All | Discussion #20 (`worktree-import-documents`) builds DOCX import, which produces these same nodes. Agree names and attrs there first. |
386
387	## What this plan does not do
388
389	Headers and footers, page setup, comments, footnotes and tracked changes.
390	Column resizing (D2). Image dimensions and lazy loading (D5).
391	Server-side image fetching (D6). Underline, strikethrough, colour and
392	highlight, which are a separate question about the attribute-free `MARKS`
393	discipline rather than about these three nodes.
394