amadan

oficina / docs Public

Core: a paragraph's alignment, through a closed allowlist

The first block-level formatting. Alignment cannot be a mark — it
belongs to the block, not the run — so it arrives as a real Yjs XML
attribute carrying a value, which is the one thing the inline
vocabulary was built never to have.

fragment_html's output goes into the page unescaped (store.go's
template.HTML), and that is safe today only because no document byte
ever reaches attribute position. ALIGNMENTS keeps it that way: the
value is used only as a key, and what gets emitted is the table's
right-hand side, a literal in this binary. center and right are the
whole set; left, an absent attribute and anything unrecognised all
render identically, with no class and no attrs.

fragment_json reports the same two through the same allowlist so the
exporters can see an alignment they cannot derive from markup — and
reaches that answer independently, the way the heading clamp does, so
a paragraph cannot read as centred on screen and export flush left.

The naive version of this (interpolate the value, emit attrs on every
paragraph) was written first and watched failing: it rendered
`<p class="docs-align-left" onmouseover="alert(1)" data-x="<script>">`
for the hostile fixture, and changed the output of every existing
document. Both are now tests.

aligned.bin is the fixture, generated through Yjs itself, so what the
Rust side reads back is what an aligned document actually contains on
the wire rather than anyone's description of it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev b16381ee233ae7eacb0b2ac7bfa36bff371ced53 parent 8777e55
5 files changed, +257 −3
  • core/src/fragment.rs +79 −2
  • core/tests/fragment.rs +153 −0
  • fixtures/generate-fragment.mjs +25 −1
  • fixtures/vectors/fragment/aligned.bin binary
  • internal/core/testdata/docs_core.wasm binary

Intent

full ledger: worktree-align

diff --git a/core/src/fragment.rs b/core/src/fragment.rs
index 382f371..216fd04 100644
--- a/core/src/fragment.rs
+++ b/core/src/fragment.rs
@@ -12,7 +12,7 @@
use std::fmt::Write as _;
use yrs::types::xml::{XmlFragment, XmlOut, XmlTextRef};
-use yrs::{Doc, GetString, ReadTxn, Text, Transact, Xml, XmlFragmentRef};
+use yrs::{Doc, GetString, ReadTxn, Text, Transact, Xml, XmlElementRef, XmlFragmentRef};
/// The CRDT root name. y-prosemirror's default, and part of the wire
/// contract — changing it silently orphans every existing document.
@@ -53,6 +53,12 @@ fn escape(s: &str, out: &mut String) {
/// attribute escaping and (for `href`) a scheme allowlist, not a row in
/// this table.
///
+/// Paragraph alignment (ALIGNMENTS below) is that same discipline
+/// applied to a property that genuinely cannot be a mark: it belongs to
+/// the block, so it arrives as an attribute WITH a value — and that
+/// value is used only as a key into a table whose right-hand side is a
+/// literal in this binary.
+///
/// The font and size marks are ordered ahead of the semantic ones so a
/// run carrying both nests as `<span class="…"><strong>…</strong></span>`
/// rather than the other way round. Serif and "normal" carry no mark at
@@ -70,6 +76,60 @@ const MARKS: &[(&str, &str, &str)] = &[
("code", "<code>", "</code>"),
];
+/// Paragraph alignments this build knows: `(attribute value, class
+/// name)`. The first block-level formatting, and the first thing in
+/// this file that reads a document attribute whose value is not a
+/// number.
+///
+/// That is why it is a table and not an interpolation. Alignment cannot
+/// be a mark — it belongs to the block, not the run — but it has to
+/// keep the property the MARKS table above exists to protect: nothing
+/// from a document may reach attribute position in `fragment_html`,
+/// whose output `internal/docs/store.go` returns as `template.HTML` and
+/// `views/show.html` writes into the page unescaped. So the value is
+/// only ever a key looked up here, and what gets emitted is the
+/// right-hand side, which is a literal in this binary.
+///
+/// Closed, therefore, in the strong sense: a value that is not in this
+/// table — a typo, a hand-crafted update through
+/// `POST /d/{sqid}/updates`, or a future version's `justify` — is
+/// dropped entirely. Never escaped and emitted, never passed through.
+/// Adding `justify` here later is one row plus one CSS rule plus the
+/// arithmetic in `internal/export/pdf.go`; letting an unknown value
+/// through would be a decision about attribute escaping, which is the
+/// decision this shape refuses to have to make.
+///
+/// `left` is deliberately absent. It is what a paragraph is when nobody
+/// has said otherwise, so it spends no class and no `attrs` — the same
+/// reasoning that keeps serif and "normal" out of MARKS, and what makes
+/// a document written before alignment existed render byte-identically
+/// to one whose author chose left on purpose.
+const ALIGNMENTS: &[(&str, &str)] = &[
+ ("center", "docs-align-center"),
+ ("right", "docs-align-right"),
+];
+
+/// The alignment value, if this build knows it. `None` for left, for an
+/// absent attribute, and for anything unrecognised — the three cases
+/// that all render the same way.
+fn alignment<T: ReadTxn>(txn: &T, e: &XmlElementRef) -> Option<&'static str> {
+ let value = e.get_attribute(txn, "align")?;
+ ALIGNMENTS
+ .iter()
+ .find(|(name, _)| *name == value)
+ .map(|(name, _)| *name)
+}
+
+/// The class `fragment_html` puts on an aligned paragraph, from the
+/// same table.
+fn align_class<T: ReadTxn>(txn: &T, e: &XmlElementRef) -> Option<&'static str> {
+ let value = e.get_attribute(txn, "align")?;
+ ALIGNMENTS
+ .iter()
+ .find(|(name, _)| *name == value)
+ .map(|(_, class)| *class)
+}
+
fn text_html<T: ReadTxn>(txn: &T, t: &XmlTextRef, out: &mut String) {
// Diff gives runs with their attributes, which is where marks live.
for chunk in t.diff(txn, yrs::types::text::YChange::identity) {
@@ -99,7 +159,12 @@ fn node_html<T: ReadTxn>(txn: &T, node: &XmlOut, out: &mut String) {
XmlOut::Element(e) => {
let name = e.tag().to_string();
let (open, close) = match name.as_str() {
- "paragraph" => ("<p>".to_string(), "</p>".to_string()),
+ "paragraph" => match align_class(txn, e) {
+ // `class` is a literal from ALIGNMENTS, never the
+ // attribute's own value — see that table.
+ Some(class) => (format!("<p class=\"{class}\">"), "</p>".to_string()),
+ None => ("<p>".to_string(), "</p>".to_string()),
+ },
"heading" => {
// Clamp: a corrupt or hostile level must not emit
// <h0> or <h99>.
@@ -378,6 +443,18 @@ fn node_json<T: ReadTxn>(txn: &T, node: &XmlOut, out: &mut String, first: &mut b
.clamp(1, 6);
let _ = write!(out, ",\"attrs\":{{\"level\":{level}}}");
}
+ if name == "paragraph" {
+ // Same allowlist, same drop, and — like the heading
+ // clamp above — arrived at independently in both
+ // renderers, so a paragraph cannot read as centred on
+ // screen and export flush left. `align` is the
+ // allowlist's own key, a literal in this binary, so
+ // nothing here needs escaping and nothing from the
+ // document can appear.
+ if let Some(align) = alignment(txn, e) {
+ let _ = write!(out, ",\"attrs\":{{\"align\":\"{align}\"}}");
+ }
+ }
out.push_str(",\"content\":[");
let mut inner = true;
for child in e.children(txn) {
diff --git a/core/tests/fragment.rs b/core/tests/fragment.rs
index 6bae207..2c7656f 100644
--- a/core/tests/fragment.rs
+++ b/core/tests/fragment.rs
@@ -349,3 +349,156 @@ fn a_hard_break_does_not_split_the_block_it_is_in() {
}
assert_eq!(fragment_text(&doc), "one\ntwo\n\nthree");
}
+
+// --- paragraph alignment ---
+//
+// The first block-level formatting. Unlike a mark it belongs to the
+// block and so lives in a real Yjs XML attribute, which is the whole
+// reason these tests are worth their length: MARKS can be a closed set
+// because a mark's presence IS its meaning, whereas an attribute
+// arrives carrying a value, and POST /d/{sqid}/updates accepts any Yjs
+// bytes a client cares to send. So the value never reaches the output
+// — it is only ever looked up in an allowlist whose right-hand side is
+// a literal in this binary.
+
+#[test]
+fn renders_paragraph_alignment_as_a_class() {
+ let doc = doc_from(include_bytes!(
+ "../../fixtures/vectors/fragment/aligned.bin"
+ ));
+ assert_eq!(
+ fragment_html(&doc),
+ concat!(
+ "<p>Left by default.</p>",
+ "<p class=\"docs-align-center\">Centred.</p>",
+ "<p class=\"docs-align-right\">Right.</p>",
+ // "left" said out loud is still the default, and the
+ // default spends no class — the same decision the MARKS
+ // table's own comment makes for serif and "normal", for the
+ // same reason: it would put an attribute on most of the
+ // blocks in most documents.
+ "<p>Left, said out loud.</p>",
+ )
+ );
+}
+
+// The exporters cannot see the CRDT, only this JSON — so alignment has
+// to be reported here or a centred paragraph exports flush left. It is
+// reported ONLY for the two alignments that are not the default, for
+// the same reason the HTML emits no class for left: an `attrs` object
+// on every paragraph would change the output for every document that
+// has never been aligned.
+#[test]
+fn json_reports_paragraph_alignment() {
+ let doc = doc_from(include_bytes!(
+ "../../fixtures/vectors/fragment/aligned.bin"
+ ));
+ assert_eq!(
+ fragment_json(&doc),
+ concat!(
+ "{\"type\":\"doc\",\"content\":[",
+ "{\"type\":\"paragraph\",\"content\":[",
+ "{\"type\":\"text\",\"text\":\"Left by default.\"}]},",
+ "{\"type\":\"paragraph\",\"attrs\":{\"align\":\"center\"},\"content\":[",
+ "{\"type\":\"text\",\"text\":\"Centred.\"}]},",
+ "{\"type\":\"paragraph\",\"attrs\":{\"align\":\"right\"},\"content\":[",
+ "{\"type\":\"text\",\"text\":\"Right.\"}]},",
+ "{\"type\":\"paragraph\",\"content\":[",
+ "{\"type\":\"text\",\"text\":\"Left, said out loud.\"}]}]}",
+ )
+ );
+}
+
+// The one that matters. fragment_html's output is written into the page
+// UNESCAPED (internal/docs/store.go's template.HTML), which is safe
+// only while no byte of a document reaches attribute position. An
+// alignment this build does not know — a typo, a future version's
+// `justify`, or the hand-crafted value below — is dropped whole. Never
+// escaped and emitted, never passed through, never interpolated: the
+// allowlist's right-hand side is the only thing that can appear.
+#[test]
+fn an_unknown_alignment_is_dropped_from_every_renderer() {
+ let hostile = "left\" onmouseover=\"alert(1)\" data-x=\"<script>";
+ let doc = Doc::with_options(Options::with_guid_and_client_id("align".into(), 1));
+ {
+ let f = doc.get_or_insert_xml_fragment("prosemirror");
+ let mut txn = doc.transact_mut();
+ let p = f.push_back(&mut txn, yrs::XmlElementPrelim::empty("paragraph"));
+ p.insert_attribute(&mut txn, "align", hostile);
+ p.push_back(&mut txn, yrs::XmlTextPrelim::new("Words."));
+ let q = f.push_back(&mut txn, yrs::XmlElementPrelim::empty("paragraph"));
+ q.insert_attribute(&mut txn, "align", "justify");
+ q.push_back(&mut txn, yrs::XmlTextPrelim::new("Later."));
+ }
+
+ // The words survive. Dropping an alignment must never drop the
+ // text — the module comment's rule, and the whole reason unknown
+ // nodes render their children.
+ let html = fragment_html(&doc);
+ assert_eq!(html, "<p>Words.</p><p>Later.</p>");
+
+ // Said again as a search rather than as a comparison, because the
+ // comparison above would still pass if the value leaked into some
+ // other renderer, and because this is the assertion that actually
+ // names the defect: no fragment of the attacker's string may appear
+ // anywhere in the output.
+ for fragment in [hostile, "onmouseover", "alert(1)", "<script", "justify"] {
+ assert!(
+ !html.contains(fragment),
+ "{fragment:?} reached the read view's HTML: {html}"
+ );
+ }
+ let json = fragment_json(&doc);
+ for fragment in ["onmouseover", "alert(1)", "script", "justify", "attrs"] {
+ assert!(
+ !json.contains(fragment),
+ "{fragment:?} reached the structured renderer: {json}"
+ );
+ }
+ // And the plain-text rendering is untouched by any of it.
+ assert_eq!(fragment_text(&doc), "Words.\n\nLater.");
+}
+
+// Alignment is opt-in, and this is what says so in bytes rather than in
+// prose: fixtures written before alignment existed render exactly the
+// markup and exactly the JSON they rendered before, to the byte. The
+// failure it guards is the plausible one — emitting an `attrs` object
+// or a `class` on every paragraph — which changes nothing a reader can
+// see, changes every document's output, and which the aligned fixture
+// above would not notice at all.
+#[test]
+fn a_document_with_no_alignment_renders_exactly_as_it_did_before() {
+ let simple = doc_from(include_bytes!("../../fixtures/vectors/fragment/simple.bin"));
+ assert_eq!(
+ fragment_html(&simple),
+ "<p>First paragraph.</p><p>Second paragraph.</p>"
+ );
+ assert_eq!(
+ fragment_json(&simple),
+ concat!(
+ "{\"type\":\"doc\",\"content\":[",
+ "{\"type\":\"paragraph\",\"content\":[{\"type\":\"text\",\"text\":\"First paragraph.\"}]},",
+ "{\"type\":\"paragraph\",\"content\":[{\"type\":\"text\",\"text\":\"Second paragraph.\"}]}]}",
+ )
+ );
+
+ let styled = doc_from(include_bytes!("../../fixtures/vectors/fragment/styled.bin"));
+ assert_eq!(
+ fragment_json(&styled),
+ concat!(
+ "{\"type\":\"doc\",\"content\":[",
+ "{\"type\":\"paragraph\",\"content\":[",
+ "{\"type\":\"text\",\"text\":\"sans\",\"marks\":[{\"type\":\"font_sans\"}]},",
+ "{\"type\":\"text\",\"text\":\" mono\",\"marks\":[{\"type\":\"font_mono\"}]},",
+ "{\"type\":\"text\",\"text\":\" large\",\"marks\":[{\"type\":\"size_lg\"}]},",
+ "{\"type\":\"text\",\"text\":\" both\",\"marks\":[{\"type\":\"font_sans\"},{\"type\":\"strong\"}]}]},",
+ "{\"type\":\"blockquote\",\"content\":[{\"type\":\"paragraph\",\"content\":[",
+ "{\"type\":\"text\",\"text\":\"Quoted.\"}]}]},",
+ "{\"type\":\"ordered_list\",\"content\":[",
+ "{\"type\":\"list_item\",\"content\":[{\"type\":\"paragraph\",\"content\":[",
+ "{\"type\":\"text\",\"text\":\"First\"}]}]},",
+ "{\"type\":\"list_item\",\"content\":[{\"type\":\"paragraph\",\"content\":[",
+ "{\"type\":\"text\",\"text\":\"Second\"}]}]}]}]}",
+ )
+ );
+}
diff --git a/fixtures/generate-fragment.mjs b/fixtures/generate-fragment.mjs
index 1a213e8..7e24efa 100644
--- a/fixtures/generate-fragment.mjs
+++ b/fixtures/generate-fragment.mjs
@@ -75,6 +75,30 @@ writeFileSync('vectors/fragment/styled.bin', docWith(f => {
])
}))
+// aligned.bin covers the first block-level formatting: a paragraph's
+// `align` attribute. Unlike a mark, alignment belongs to the block, so
+// it is a real Yjs XML attribute on the element — which is exactly why
+// it needs a fixture of its own. y-prosemirror stores node attributes
+// this way (createTypeFromElementNode: setAttribute for every non-null
+// attr), so this is what an aligned document ACTUALLY contains on the
+// wire, and core/src/fragment.rs has to read it back from there rather
+// than from anyone's description of it.
+//
+// The first paragraph carries no attribute at all, which is the whole
+// point of left being the default: the editor's schema stores null,
+// y-prosemirror skips null, and an unaligned document is byte-identical
+// to one written before alignment existed. The last one says "left" out
+// loud, which a paste or a future build can produce and which must
+// render as no class rather than as a class of its own.
+writeFileSync('vectors/fragment/aligned.bin', docWith(f => {
+ f.insert(0, [
+ el('paragraph', null, text('Left by default.')),
+ el('paragraph', { align: 'center' }, text('Centred.')),
+ el('paragraph', { align: 'right' }, text('Right.')),
+ el('paragraph', { align: 'left' }, text('Left, said out loud.')),
+ ])
+}))
+
// --- trap-containment vectors ---
//
// core/tests/wasm_trap_containment.rs mutation-fuzzes real fixtures and
@@ -189,4 +213,4 @@ emitTrap('concurrent', (doc) => {
Y.applyUpdate(doc, Y.encodeStateAsUpdate(a))
})
-console.log('wrote 4 fragment vectors + 4 trap-containment vectors')
+console.log('wrote 5 fragment vectors + 4 trap-containment vectors')
diff --git a/fixtures/vectors/fragment/aligned.bin b/fixtures/vectors/fragment/aligned.bin
new file mode 100644
index 0000000..6796471
Binary files /dev/null and b/fixtures/vectors/fragment/aligned.bin differ
diff --git a/internal/core/testdata/docs_core.wasm b/internal/core/testdata/docs_core.wasm
index 61d79e7..4dc432c 100755
Binary files a/internal/core/testdata/docs_core.wasm and b/internal/core/testdata/docs_core.wasm differ