oficina / slides Public

Reviewed 6c757a4. All source locations below refer to that commit. The worktree advanced to eca23fd during the review; its changes are outside this assessment.

1. P1 — Uploaded assets remain writable after verification. §2.7; internal/slides/assets.go:176, :186, :367; internal/slides/thumbs.go:244. A member can request a PUT URL for a client-chosen ref, including an already registered object. Registration records verification, but subsequent thumbnail reads trust that record and read the object again. The pinned store implementation signs an unconditional PUT with an unsigned payload (rastrillo@v0.26.0/blobs/s3.go:194). Consequently, replacement bytes can invalidate verification and alter pictures in other decks without a document update. Registration also does not perform the rehash promised by its comment. Instead: upload into isolated staging keys, verify and hash the bytes, then promote them into immutable final objects. Never issue browser PUT capabilities for verified objects. I verified the source path, not a live bucket exploit.

1. P1 — Registration turns knowledge of an asset ref into permission to read it. §2.7, §5; internal/slides/assets.go:279, :315, :367, :512. Registration checks access to the destination deck, then fetches any supplied ref and creates that deck’s verification record. It requires neither an upload receipt nor access to a source deck. A member who knows another deck’s image ref can register it in their own deck and satisfy both serving checks. This defeats the stated rule that knowing a content address is insufficient authorization. Instead: require a completed upload tied to the requesting principal and deck, or an explicitly authorized copy from a readable source deck.

1. P1 — The write gate validates the server’s projection, not the document the editor consumes. §2.6, §3.3; internal/slides/live.go:331; internal/slides/deck/deck.go:173; core/src/schema.rs:203; fixtures/stage.mjs:174. An unknown formatting attribute disappears from the Rust JSON view, so parsing that view succeeds. The underlying Yjs document still contains it. ProseMirror encounters the unknown mark and deletes the affected XmlText; the installed, lockfile-pinned y-prosemirror 1.3.7 does this at src/plugins/sync-plugin.js:839. Reproduced: the committed wasm accepted text marked unknown_mark, returned the words normally, and ProseMirror’s conversion deleted those words. Instead: validate the actual shared text structure and marks before persistence, and make browser handling of unsupported content non-destructive. Keep the gate before the transaction; strengthen what it checks.

1. P1 — Layout conversion loses concurrent text edits. §2.3, §3.3; core/src/layout.rs:109; fixtures/layout.mjs:103. When canvas-to-flow conversion changes reading order, it clones every block, deletes the original array contents, and inserts the copies. Concurrent text edits address the original shared objects, which have been deleted. This is content loss beyond the disclosed loss of placement. Reproduced: one client converted two out-of-order canvas blocks while another appended text. After exchanging updates, both clients lost the appended text. Instead: keep block objects stable and store ordering separately, such as ordered IDs referencing shared block maps. Add concurrent conversion/edit tests across both implementations.

1. P1 — Following a presentation escapes share-link revocation. §3.4, §5; internal/slides/share.go:342; internal/slides/presentation.go:568, :181; internal/slides/static/present.js:89. Following a share link exposes the presentation session ID and uses /p/{session} for subsequent reads. Those reads require only a live presentation, not the original share token. Revoking the share link therefore leaves the recipient able to fetch fresh deck content until the presentation ends or expires. There is also a permission mismatch: members may create these public presentation capabilities, while ordinary share-link creation is owner-only (internal/slides/app.go:138). Instead: keep audience requests behind their originating share capability, including streams, fragments, thumbnails and assets. Decide explicitly who may publish an independently usable presentation link.

1. P1 — Removing a person from the allowlist leaves their existing authority active. §6.1, §7.1; internal/slides/auth.go:257, :357; internal/slides/agents.go:184, :204; internal/slides/decks.go:164. The allowlist gates session creation. Existing sessions resolve their user row without checking current admission, and agent authentication requires only an unrevoked token and an existing operator row. The pinned auth.RequireSession also performs no admission recheck (rastrillo@v0.26.0/auth/handlers.go:204). Removing an address therefore does not remove its existing access, including agent writes to team decks. Instead: define offboarding in the lone-instance design and enforce current membership for sessions and agent operators. Explicitly address outstanding presentation capabilities as part of that lifecycle.

1. P2 — The “block tree” permits cycles and multiple parents. §0.1, §2.4; core/src/ops.rs:605; fixtures/ops-model.mjs:62; internal/slides/render/render.go:392, :432. Child validation rejects self-reference and missing IDs, but permits longer cycles and shared children. Rendering first marks every referenced block as claimed, then starts only from unclaimed blocks. A cycle can therefore make its entire connected content disappear without producing an error. Reproduced: both JS operations and the committed Rust wasm accepted A → B, B → A, with a text child under A. The rendering traversal has no root from which to reach them. Instead: enforce unique IDs, one parent and acyclicity on accepted merged documents, with a deterministic policy for concurrent conflicting assignments.

1. P2 — Flow conversion does not preserve the template’s placement, and round trips discard regions. §2.3; core/src/layout.rs:29, :51, :91; internal/slides/static/slides.css:299, :318, :348. Flow-to-canvas uses one fixed region table without consulting the template or measuring content. A centred title and a full-frame image therefore cannot receive their existing boxes. Canvas-to-flow never assigns aside, so a two-column slide loses that region after a round trip. Instead: obtain conversion geometry from the actual template layout. Specify which placement information survives repeated switching, and preserve recoverable placement metadata where practical. A generic “lossy” warning does not explain these concrete changes.

1. P2 — The renderer and stylesheet disagree about when the phone layer applies. §2.3, §2.5, §3.2; internal/slides/viewer.go:160; internal/slides/render/render.go:138; internal/slides/static/slides.css:647. Canvas reflow requires an explicit Phone render, but ordinary viewer URLs default to Desktop even when opened on a phone. Meanwhile, CSS applies phone overrides to any slide container below 600px, including a narrow desktop editor stage. The same rule enlarges canvas text with minimum font sizes while retaining fractional boxes, defeating scale-to-fit geometry. Instead: define one effective viewport policy shared by HTML and CSS. Preserve explicit desktop previews, make ordinary phone viewing honour canvas reflow, and separate reflow typography from scaled-canvas typography. These conclusions are source-derived; I did not run browser layout checks.

1. P2 — Phone reading order is visual only. §2.5, §8; internal/slides/render/render.go:464, :601; internal/slides/static/slides.css:711. Phone order becomes CSS order, leaving document and keyboard order unchanged. Readers using assistive technology can consequently encounter a different sequence from sighted readers. Canvas reflow also starts with stacking order, so bringing an object forward can change its reading position. Instead: distinguish semantic reading order from stacking order and render the effective phone sequence into the document structure. Test ordered text and links with keyboard and assistive-technology navigation.

1. P2 — Supported content has materially different meanings across renderers. §0.1, §2.4, §2.7, §4. Multiple renderers do not themselves create a second deck, but the supported vocabulary needs a stronger common contract. At this commit, WebP is accepted (internal/slides/assets.go:427) but omitted by the SVG inline allowlist (internal/slides/svg/svg.go:309) and lacks an export decoder (internal/slides/export.go:29, :248). Nested SVG images explicitly ignore fit (internal/slides/svg/svg.go:354). Arbitrary font stacks reach HTML while layout substitutes embedded faces (internal/slides/render/render.go:342; internal/slides/layout/layout.go:89). Instead: normalize accepted images into formats every surface supports, honour the same fitting rules, and offer fonts the exporter can reproduce. Add cross-surface checks for these cases; separate golden files do not establish agreement.

1. P2 — Finished-PDF limits do not bound export’s working memory. §4.3; slice-3 global constraints; internal/slides/export.go:120, :165; internal/slides/pdfcore/pdfcore.go:181, :239. Export retains all SVG pages and cached image bytes before conversion. The PDF host creates another framed copy and copies it into wasm. At this commit neither wasm runtime sets an application memory limit (internal/slides/pdfcore/pdfcore.go:105; internal/slides/core/core.go:135). A cap applied to the finished PDF cannot contain these earlier allocations or concurrent jobs. Instead: set guest limits, bound aggregate intermediate bytes and concurrent exports, and budget decoding and notes generation. Measure image-heavy decks and simultaneous jobs, not only text-only throughput. I did not measure peak memory.

1. P2 — Out-of-order slide fetches leave the projector permanently behind. §3.4; internal/slides/static/present.js:76. The script records the announced slide before its fetch succeeds, then installs every response without checking whether a newer request superseded it. If slide 3 returns before slide 2, slide 2 overwrites it. Another event announcing slide 3 is ignored because the recorded index already says 3. Failed fetches have the same premature-success problem. Reproduced: reversing two responses left the displayed content at slide 2, the recorded index at 3, and a repeated slide-3 event triggered no fetch. Instead: track requested and displayed state separately, discard obsolete responses, and advance displayed state only after successful installation.

1. P2 — The design does not settle what editing a deck during a presentation means. §3.3–§3.4; internal/slides/presentation.go:568, :592, :1240; internal/slides/static/present.js:80. Presentation state stores an index into the current deck. Inserting or moving slides before that index changes which slide it names. Content edits at the same index do not cause the presentation script to fetch again, so existing projectors and newly opened audience pages can show different content. Instead: choose explicitly between presenting a pinned revision and presenting live edits. For live presentation, track stable slide IDs and document sequence, define deletion behaviour, and notify viewers when the displayed slide changes without changing position.

1. P2 — Anonymous remote commands are attributed to the session creator. §6.2, ruling 7; internal/slides/presentation.go:1140, :1219. A remote command authenticates a transferable capability, but its logged actor is always p.Actor. The server cannot establish that the capability holder is the person or agent who created the session. The unqualified “every write names its real actor” rule therefore does not hold for these writes. Instead: record a remote-capability principal and its issuer separately, with an explicit exception for anonymous presentation control. If named human attribution is required, pairing must establish that identity.

Verification limits: The focused suite passed 35 tests. The additional reproductions used unchanged JS files and the committed core wasm. I did not run the full build gate, a live deployment, a bucket exploit, or browser/accessibility checks. At 6c757a4, neither router contains the export job endpoints (internal/slides/app.go:99; internal/slides/api.go:91), so I could not verify the claimed one-hour result cache, its caps, or job/download authorization at that revision. I made no repository changes.