Fixes #2028: user attributes could overwrite an indexed object's source offsets

docs/CHANGELOG.md

@@ -4,9 +4,6 @@ An attempt at documenting the changes/new features introduced in each release.
 Whenever a commit is pushed to the `main` branch, within ~5 minutes, it will be released as a docker image with the `:v2` tag, and a binary in the [edge release](https://github.com/silverbulletmd/silverbullet/releases/tag/edge). If you want to live on the bleeding edge of SilverBullet goodness (or regression) this is where to do it.
 
 * [[Space Manager]]: multi-space hosting with multiple accounts is here. A fresh install pointed at an empty folder opens a browser-based first-run **setup wizard** that creates an admin account and your first space, then serves it in place with no restart. One server can host any number of [[Space|spaces]], each bound to a URL prefix or hostname.
-* Fix: `SB_SHELL_BACKEND=local` disabled shell command execution instead of enabling it.
-* Fix: only the first entry of a frontmatter list of wiki links formed a [[Object/relation]] (e.g. an `authors:` list of `[[...]]` items), so every entry after the first was missing from [[Object Graph]], linked mentions and rename refactoring.
-* Fix: writing to a read-only path (anything served from the bundled library, a `SB_READ_ONLY` server) returned a 500, which clients could not tell apart from a temporary server fault — so a syncing client retried it forever. Read-only refusals now return 403, and the sync engine records the path and stops re-attempting it until the local file changes. This most often bit spaces holding a stale copy of a `Library/Std` page that a later release had dropped from the bundle.
 * [[Baked Sections]]: bake `${...}` Lua expressions and widgets into
   HTML-comment-delimited markdown (`<!--#lua EXPR -->` … `<!--/lua-->`).
 * Space Lua: **code complete now shows documentation** (where available), all available via [[API/spacelua]] reflection APIs.
@@ -17,6 +14,10 @@ Whenever a commit is pushed to the `main` branch, within ~5 minutes, it will be
 * [[Frontmatter]] in the editor now has configurable folding: by default long frontmatter blocks fold automatically, and `frontmatterFolding` options let  you disable auto-folding, always fold frontmatter, or change the line threshold. A subtle right-side marker folds or unfolds the whole block, and folded frontmatter previews any `tags` value as tag chips. This is configurable via the [[Configuration Manager]] as well.
 * [[Page Picker]]: type `$` to switch to [[Markdown/Anchor|anchor]] navigation
 * Pulling the "this was experimental card" for the CLI: removed the `sb get` command and the `/.runtime/objects/*` REST API, including their dedicated client-side query bridge. Use `sb query`, `sb eval`, or `sb script` for indexed-object access. This added too much complexity and another query language.
+* Fix: `SB_SHELL_BACKEND=local` disabled shell command execution instead of enabling it.
+* Fix: a `range` (or `pos`) attribute in your own content could overwrite the source offsets an indexed object uses to point back at its place in the page.
+* Fix: only the first entry of a frontmatter list of wiki links formed a [[Object/relation]] (e.g. an `authors:` list of `[[...]]` items), so every entry after the first was missing from [[Object Graph]], linked mentions and rename refactoring.
+* Fix: writing to a read-only path (anything served from the bundled library, a `SB_READ_ONLY` server) returned a 500, which clients could not tell apart from a temporary server fault — so a syncing client retried it forever. Read-only refusals now return 403, and the sync engine records the path and stops re-attempting it until the local file changes. This most often bit spaces holding a stale copy of a `Library/Std` page that a later release had dropped from the bundle.
 * Fix: major typing/navigation slowdown on pages with many internal links in large spaces.
 * Fix: the service worker precached client assets *through* the browser's HTTP cache, so a stale client could be copied into its cache and then served as though it were the current build — leaving a "A new version of SilverBullet client is available." notification that no reload could clear (only a hard reload, which bypasses the service worker, showed the real client; the next normal reload brought the notification back). Precaching now bypasses the HTTP cache.
 * The server now sets `Cache-Control` on client assets, which matters if you run SilverBullet behind a CDN.

plugs/index/data.test.ts

@@ -142,3 +142,24 @@ describe("range covers the YAML content, not the fence markers", () => {
     expect(md.slice(b.range[0], b.range[1])).toContain("Hank");
   });
 });
+
+test("user `pos`/`range` attributes cannot overwrite the block's source offsets", async () => {
+  createMockSystem();
+  const md = [
+    "```#person",
+    "name: Pete",
+    "range: 0, 36",
+    "pos: nope",
+    "```",
+  ].join("\n");
+  const results = await indexDataForTest(md, "Page");
+  const person = results.find((o) => o.tag === "person")!;
+  expect(person).toBeTruthy();
+  expect(person.range.every((n: unknown) => typeof n === "number")).toBe(true);
+  expect(typeof person.pos).toBe("number");
+  expect(person.range[0]).toBe(md.indexOf("name:"));
+  expect(md.slice(person.range[0], person.range[1])).toContain("Pete");
+  expect(md.slice(person.range[0], person.range[1])).not.toContain("```");
+  // The user's own attributes are still indexed as data, just not as offsets.
+  expect(person.name).toBe("Pete");
+});

plugs/index/data.ts

@@ -7,6 +7,7 @@ import {
 import type { TagObject } from "./tags.ts";
 import type { FrontMatter } from "./frontmatter.ts";
 import { updateITags } from "./tags.ts";
+import { stripPositionAttributes } from "./position_attributes.ts";
 import type {
   ObjectValue,
   PageMeta,
@@ -77,7 +78,7 @@ export function indexData(
           itags: ["data"],
           pos: docStart,
           range: [docStart, docEnd] as [number, number],
-          ...doc,
+          ...stripPositionAttributes(doc),
           page: pageMeta.name,
         };
         updateITags(dataObj, frontmatter);

plugs/index/frontmatter.test.ts

@@ -0,0 +1,28 @@
+import { expect, test } from "vitest";
+import { parseMarkdown } from "../../client/markdown_parser/parser.ts";
+import { extractFrontMatter } from "./frontmatter.ts";
+
+test("extracts the frontmatter block's own range", () => {
+  const text = `---\nstuff: things\n---\n\nBody\n`;
+  const fm = extractFrontMatter(parseMarkdown(text));
+  expect(fm.range).toEqual([0, text.indexOf("---\n\nBody") + 3]);
+});
+
+test("a user `range` attribute cannot overwrite the frontmatter's own range", () => {
+  // Regression for #2028: `range` reaches CodeMirror as a position pair
+  // (X-Ray diagnostics, lint), so a user-authored value took the tab down.
+  const text = `---\nstuff: things\nrange: 0, 36\notherstuff: more things\n---\n\nBody\n`;
+  const fm = extractFrontMatter(parseMarkdown(text));
+  expect(Array.isArray(fm.range)).toBe(true);
+  expect(fm.range!.every((n) => typeof n === "number")).toBe(true);
+  expect(text.slice(fm.range![0], fm.range![1])).toContain("stuff: things");
+  // Other frontmatter data is untouched.
+  expect(fm.stuff).toEqual("things");
+  expect(fm.otherstuff).toEqual("more things");
+});
+
+test("a user `pos` attribute cannot overwrite a synthesized position either", () => {
+  const text = `---\npos: not a number\n---\n\nBody\n`;
+  const fm = extractFrontMatter(parseMarkdown(text));
+  expect(fm.pos).toBeUndefined();
+});

plugs/index/frontmatter.ts

@@ -11,6 +11,7 @@ import YAML from "js-yaml";
 import { extractHashtag } from "@silverbulletmd/silverbullet/lib/tags";
 import type { CompleteEvent } from "@silverbulletmd/silverbullet/type/client";
 import { determineTags } from "./cheap_yaml.ts";
+import { stripPositionAttributes } from "./position_attributes.ts";
 import { attributeCompletion } from "./complete.ts";
 
 export type FrontMatter = {
@@ -99,7 +100,10 @@ export function extractFrontMatter(
         const parsedData: any = cleanupJSON(YAML.load(yamlText));
         // console.log("Parsed front matter", parsedData);
         const newData = { ...parsedData };
-        frontmatter = { ...frontmatter, ...parsedData };
+        frontmatter = {
+          ...frontmatter,
+          ...stripPositionAttributes(parsedData),
+        };
         // Make sure we have a tags array
         if (!frontmatter.tags) {
           frontmatter.tags = [];

plugs/index/header.test.ts

@@ -71,3 +71,23 @@ test("header without anchor keeps Page@pos ref shape", async () => {
   expect(headers.length).toEqual(1);
   expect(headers[0].ref).toMatch(/^MyPage@\d+$/);
 });
+
+test("user `range` attribute cannot overwrite a header's source offsets", async () => {
+  createMockSystem();
+  const md = `# Real header [range: 0, 36]`;
+  const tree = parseMarkdown(md);
+  const frontmatter = extractFrontMatter(tree);
+  const pageMeta: PageMeta = {
+    ref: "test",
+    name: "test",
+    tag: "page",
+    created: "",
+    lastModified: "",
+    perm: "rw",
+  };
+  const headers = await indexHeaders(pageMeta, frontmatter, tree);
+  const header = headers.find((h: any) => h.tag === "header")! as any;
+  expect(header).toBeTruthy();
+  expect(header.range).toEqual([0, md.length]);
+  expect(typeof header.pos).toBe("number");
+});

plugs/index/header.ts

@@ -18,6 +18,7 @@ import type { FrontMatter } from "./frontmatter.ts";
 import { cleanAttributes, collectAttributes } from "./attribute.ts";
 import { cleanTags, collectTags } from "./tags.ts";
 import { cleanAnchor, collectAnchor } from "./anchor.ts";
+import { stripPositionAttributes } from "./position_attributes.ts";
 
 type HeaderObject = ObjectValue<
   {
@@ -63,7 +64,7 @@ export function indexHeaders(
       page: pageMeta.name,
       pos: n.from!,
       range: [n.from!, n.to!],
-      ...attributes,
+      ...stripPositionAttributes(attributes),
     });
   }
 

plugs/index/item.test.ts

@@ -151,3 +151,25 @@ test("Test item indexing with nested lists in list items", async () => {
   const regularItem = items.find((i) => i.name === "Regular item");
   expect(regularItem).toBeDefined();
 });
+
+test("user `range` attribute cannot overwrite an item's source offsets", async () => {
+  createMockSystem();
+  const md = `* An item [range: 0, 36]`;
+  const tree = parseMarkdown(md);
+  const frontmatter = extractFrontMatter(tree);
+  const pageMeta: PageMeta = {
+    ref: "test",
+    name: "test",
+    tag: "page",
+    created: "",
+    lastModified: "",
+    perm: "rw",
+  };
+  const items = await indexItems(pageMeta, frontmatter, tree);
+  const item = items.find((i: any) => i.tag === "item")! as any;
+  expect(item).toBeTruthy();
+  expect(Array.isArray(item.range)).toBe(true);
+  expect(item.range.every((n: unknown) => typeof n === "number")).toBe(true);
+  expect(md.slice(item.range[0], item.range[1])).toContain("An item");
+  expect(typeof item.pos).toBe("number");
+});

plugs/index/item.ts

@@ -7,6 +7,7 @@ import {
 } from "@silverbulletmd/silverbullet/lib/tree";
 import { cleanTags, collectTags, updateITags } from "./tags.ts";
 import { cleanAnchor, collectAnchor } from "./anchor.ts";
+import { isPositionAttribute } from "./position_attributes.ts";
 import type { FrontMatter } from "./frontmatter.ts";
 import type {
   ObjectValue,
@@ -177,6 +178,8 @@ export function extractItemFromNode(
   }
 
   for (const [key, value] of Object.entries(attributes)) {
+    // `pos`/`range` are the item's own source offsets, not user data (#2028).
+    if (isPositionAttribute(key)) continue;
     item[key] = value;
   }
 

plugs/index/paragraph.test.ts

@@ -86,3 +86,15 @@ test("paragraph without anchor keeps Page@pos ref", async () => {
   const para = objects.find((o) => o.tag === "paragraph")!;
   expect(para.ref).toMatch(/^MyPage@\d+$/);
 });
+
+test("a `range` attribute does not overwrite a paragraph's source offsets", async () => {
+  // Tagged so it is indexed with the default `index.paragraph.all = false`.
+  const paragraphs = await indexParagraphsForTest(
+    "A #noted paragraph with [range: 0, 36] in it.\n",
+  );
+  const p = paragraphs[0] as any;
+  expect(p).toBeTruthy();
+  expect(Array.isArray(p.range)).toBe(true);
+  expect(p.range.every((n: unknown) => typeof n === "number")).toBe(true);
+  expect(typeof p.pos).toBe("number");
+});

plugs/index/paragraph.ts

@@ -14,6 +14,7 @@ import type {
 } from "@silverbulletmd/silverbullet/type/index";
 import { system } from "@silverbulletmd/silverbullet/syscalls";
 import { cleanAttributes, collectAttributes } from "./attribute.ts";
+import { stripPositionAttributes } from "./position_attributes.ts";
 
 /** ParagraphObject  An index object for the top level text nodes */
 export type ParagraphObject = ObjectValue<
@@ -81,7 +82,7 @@ export async function indexParagraphs(
         page: pageMeta.name,
         pos,
         range: [p.from!, p.to!],
-        ...attrs,
+        ...stripPositionAttributes(attrs),
       };
       if (tags.length > 0) {
         paragraph.tags = [...tags];

plugs/index/position_attributes.ts

@@ -0,0 +1,30 @@
+/**
+ * Internally used attribute names that cannot be overriden by the user.
+ */
+const POSITION_ATTRIBUTES = ["pos", "range"];
+
+/**
+ * Returns `data` without the reserved position attributes, so it is safe to
+ * merge over a synthesized object. Returns the original object untouched when
+ * it carries neither, which is the overwhelmingly common case.
+ */
+export function stripPositionAttributes<T extends Record<string, any>>(
+  data: T,
+): T {
+  if (!POSITION_ATTRIBUTES.some((name) => name in data)) {
+    return data;
+  }
+  const cleaned = { ...data };
+  for (const name of POSITION_ATTRIBUTES) {
+    delete cleaned[name];
+  }
+  return cleaned;
+}
+
+/**
+ * Whether `name` is a reserved position attribute (for attribute-by-attribute
+ * merges).
+ */
+export function isPositionAttribute(name: string): boolean {
+  return POSITION_ATTRIBUTES.includes(name);
+}

plugs/index/table.test.ts

@@ -34,3 +34,33 @@ test("Test table indexing", async () => {
   expect(datas[1].name).toEqual("Fred");
   expect(datas[1].age).toEqual("21");
 });
+
+test("a `range` column does not overwrite the row's source offsets", async () => {
+  createMockSystem();
+  // A table of date ranges is an entirely reasonable thing to write; the
+  // column must not land on the row object's own offsets (#2028).
+  const md = `
+| name | range |
+|----------|----------|
+| Frank | 0, 36 |
+`.trim();
+  const tree = parseMarkdown(md);
+  const frontmatter = extractFrontMatter(tree);
+  const pageMeta: PageMeta = {
+    ref: "test",
+    name: "test",
+    tag: "page",
+    created: "",
+    lastModified: "",
+    perm: "rw",
+  };
+
+  const rows = await indexTables(pageMeta, frontmatter, tree);
+  expect(rows).toHaveLength(1);
+  const row = rows[0] as any;
+  expect(row.name).toEqual("Frank");
+  expect(Array.isArray(row.range)).toBe(true);
+  expect(row.range.every((n: unknown) => typeof n === "number")).toBe(true);
+  expect(md.slice(row.range[0], row.range[1])).toContain("Frank");
+  expect(typeof row.pos).toBe("number");
+});

plugs/index/table.ts

@@ -14,6 +14,7 @@ import type {
 } from "@silverbulletmd/silverbullet/type/index";
 import { extractHashtag } from "@silverbulletmd/silverbullet/lib/tags";
 import type { FrontMatter } from "./frontmatter.ts";
+import { isPositionAttribute } from "./position_attributes.ts";
 
 type TableRowObject = ObjectValue<{
   tableref: string;
@@ -102,7 +103,8 @@ export function indexTables(
         });
         const content = concatChildrenTextsPreserveLinks(c.children!);
         const label = headerLabels[i];
-        tableRow[label!] = content;
+        if (!label || isPositionAttribute(label)) return;
+        tableRow[label] = content;
       });
       result.push(tableRow);
     }