From 3975ef7146cb590918b3d0865c00ad50c720d6a8 Mon Sep 17 00:00:00 2001 From: Elian Doran Date: Fri, 21 Aug 2026 13:39:02 +0300 Subject: [PATCH] fix(share): encode and escape the share id when building links A shareAlias is free-form text, so a share id can carry spaces, quotes and path separators. buildShareLink() percent-encodes it into a single path segment the /share/:shareId route decodes back, the share info bar escapes the link it renders, and the share index escapes the href it builds for each child. Also implements the SBranch TODO in buildShareNote() so a mocked note's children reach getChildNotes(), which the index test needs. Co-Authored-By: Claude Opus 5 (1M context) --- apps/client/src/services/share_link.spec.ts | 16 ++++++++++++ apps/client/src/services/share_link.ts | 10 ++++--- apps/client/src/widgets/shared_info.spec.tsx | 26 +++++++++++++++++++ apps/client/src/widgets/shared_info.tsx | 10 +++++-- .../server/src/share/content_renderer.spec.ts | 23 ++++++++++++++++ apps/server/src/share/content_renderer.ts | 2 +- apps/server/src/test/shaca_mocking.ts | 13 ++++++++-- 7 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 apps/client/src/widgets/shared_info.spec.tsx diff --git a/apps/client/src/services/share_link.spec.ts b/apps/client/src/services/share_link.spec.ts index b19cde5e85..e81db9761c 100644 --- a/apps/client/src/services/share_link.spec.ts +++ b/apps/client/src/services/share_link.spec.ts @@ -35,4 +35,20 @@ describe("buildShareLink", () => { setGlob({ httpBaseUrl: "http://127.0.0.1:37742" }); expect(buildShareLink("", undefined)).toBe("http://127.0.0.1:37742/share/"); }); + + it("percent-encodes a shareId carrying characters that are not path-safe", () => { + // A shareAlias is free-form text, so it can hold spaces, quotes, separators and anything + // else a title can. Each branch must produce one path segment the /share/:shareId route + // decodes back to the alias. + const shareId = `my alias"/?x`; + const encoded = "my%20alias%22%2F%3Fx"; + const syncServerHost = "https://notes.example.com"; + + setGlob({ httpBaseUrl: "http://127.0.0.1:37742" }); + expect(buildShareLink(shareId, syncServerHost)).toBe(`${syncServerHost}/share/${encoded}`); + expect(buildShareLink(shareId, undefined)).toBe(`http://127.0.0.1:37742/share/${encoded}`); + + setGlob({}); + expect(buildShareLink(shareId, null)).toBe(`http://localhost:3000/share/${encoded}`); + }); }); diff --git a/apps/client/src/services/share_link.ts b/apps/client/src/services/share_link.ts index e72b5278bb..a7c26ecd8f 100644 --- a/apps/client/src/services/share_link.ts +++ b/apps/client/src/services/share_link.ts @@ -8,13 +8,17 @@ * origin (which is correct for the server build and for a browser hitting the desktop). */ export function buildShareLink(shareId: string, syncServerHost: string | null | undefined): string { + // A shareId is either a noteId or a free-form shareAlias, so it can hold spaces, quotes and + // path separators. Encoding keeps it a single segment the /share/:shareId route decodes back. + const encodedShareId = encodeURIComponent(shareId); + if (syncServerHost) { - return new URL(`/share/${shareId}`, syncServerHost).href; + return new URL(`/share/${encodedShareId}`, syncServerHost).href; } if (window.glob.httpBaseUrl) { - return new URL(`/share/${shareId}`, window.glob.httpBaseUrl).href; + return new URL(`/share/${encodedShareId}`, window.glob.httpBaseUrl).href; } - return `${location.protocol}//${location.host}${location.pathname}share/${shareId}`; + return `${location.protocol}//${location.host}${location.pathname}share/${encodedShareId}`; } diff --git a/apps/client/src/widgets/shared_info.spec.tsx b/apps/client/src/widgets/shared_info.spec.tsx new file mode 100644 index 0000000000..89d354cfdc --- /dev/null +++ b/apps/client/src/widgets/shared_info.spec.tsx @@ -0,0 +1,26 @@ +import { describe, expect, it } from "vitest"; + +import { buildShareLinkHtml } from "./shared_info"; + +describe("buildShareLinkHtml", () => { + it("keeps the whole link in the anchor's href and text, whatever characters it carries", () => { + const link = `http://host/share/my alias"x`; + const anchor = parseAnchor(buildShareLinkHtml(link)); + + expect(anchor.getAttribute("href")).toBe(link); + expect(anchor.textContent).toBe(link); + expect(anchor.getAttributeNames().sort()).toEqual([ "class", "href" ]); + }); +}); + +function parseAnchor(html: string) { + const container = document.createElement("div"); + container.innerHTML = html; + + const anchor = container.querySelector("a"); + if (!anchor) { + throw new Error(`No anchor in ${html}`); + } + + return anchor; +} diff --git a/apps/client/src/widgets/shared_info.tsx b/apps/client/src/widgets/shared_info.tsx index fba2369c8a..ef05b578eb 100644 --- a/apps/client/src/widgets/shared_info.tsx +++ b/apps/client/src/widgets/shared_info.tsx @@ -6,7 +6,7 @@ import FNote from "../entities/fnote"; import attributes from "../services/attributes"; import { t } from "../services/i18n"; import { buildShareLink } from "../services/share_link"; -import { isElectron } from "../services/utils"; +import { escapeHtml, isElectron } from "../services/utils"; import HelpButton from "./react/HelpButton"; import { useNoteContext, useTriliumEvent, useTriliumOption } from "./react/hooks"; import InfoBar from "./react/InfoBar"; @@ -43,7 +43,7 @@ export function useShareInfo(note: FNote | null | undefined) { const link = buildShareLink(getShareId(note), syncServerHost); - setLink(`${link}`); + setLink(buildShareLinkHtml(link)); setLinkHref(link); } @@ -63,6 +63,12 @@ export function useShareInfo(note: FNote | null | undefined) { }; } +export function buildShareLinkHtml(link: string) { + const escapedLink = escapeHtml(link); + + return `${escapedLink}`; +} + function getShareId(note: FNote) { if (note.hasOwnedLabel("shareRoot")) { return ""; diff --git a/apps/server/src/share/content_renderer.spec.ts b/apps/server/src/share/content_renderer.spec.ts index bd1b6bb63a..ae94dc9b2d 100644 --- a/apps/server/src/share/content_renderer.spec.ts +++ b/apps/server/src/share/content_renderer.spec.ts @@ -4,6 +4,7 @@ import { beforeAll, describe, expect, it, vi } from "vitest"; import { buildShareNote, buildShareNotes } from "../test/shaca_mocking.js"; import { getContent, renderCode, type Result, shouldSyntaxHighlight } from "./content_renderer.js"; +import shareRoot from "./share_root.js"; describe("content_renderer", () => { beforeAll(() => { @@ -533,4 +534,26 @@ describe("content_renderer", () => { expect(shouldSyntaxHighlight("x".repeat(50_001))).toBe(false); }); }); + + describe("Share index", () => { + it("points each index entry at the child's shareId, whatever characters it carries", () => { + buildShareNote({ + id: shareRoot.SHARE_ROOT_NOTE_ID, + children: [ + { "id": "child1", "title": "Child", "#shareAlias": `my alias"x` } + ] + }); + const note = buildShareNote({ + "id": "indexNote", + "content": "

Index

", + "#shareIndex": "" + }); + + const result = getContent(note); + const anchor = parse(String(result.content)).querySelector("#index a"); + + expect(anchor?.getAttribute("href")).toBe(`./my alias"x`); + expect(Object.keys(anchor?.attributes ?? {}).sort()).toEqual([ "class", "href" ]); + }); + }); }); diff --git a/apps/server/src/share/content_renderer.ts b/apps/server/src/share/content_renderer.ts index 0233fef4bc..4da4130fcb 100644 --- a/apps/server/src/share/content_renderer.ts +++ b/apps/server/src/share/content_renderer.ts @@ -340,7 +340,7 @@ function renderIndex(result: Result) { for (const childNote of rootNote.getChildNotes()) { const isExternalLink = childNote.hasLabel("shareExternalLink"); const rawHref = childNote.getLabelValue("shareExternalLink") ?? ""; - const href = isExternalLink ? escapeHtml(sanitize.sanitizeUrl(rawHref)) : `./${childNote.shareId}`; + const href = escapeHtml(isExternalLink ? sanitize.sanitizeUrl(rawHref) : `./${childNote.shareId}`); const target = isExternalLink ? `target="_blank" rel="noopener noreferrer"` : ""; result.content += `
  • ${childNote.escapedTitle}
    • `; } diff --git a/apps/server/src/test/shaca_mocking.ts b/apps/server/src/test/shaca_mocking.ts index 08c77e4a0a..8ec3741f59 100644 --- a/apps/server/src/test/shaca_mocking.ts +++ b/apps/server/src/test/shaca_mocking.ts @@ -1,6 +1,7 @@ import utils from "../services/utils.js"; import SAttachment from "../share/shaca/entities/sattachment.js"; import SAttribute from "../share/shaca/entities/sattribute.js"; +import SBranch from "../share/shaca/entities/sbranch.js"; import SNote from "../share/shaca/entities/snote.js"; import shaca from "../share/shaca/shaca.js"; @@ -87,12 +88,20 @@ export function buildShareNote(noteDef: NoteDefinition) { } } - // Handle children. + // Handle children. The SBranch constructor is what links the two notes, so getChildNotes() + // and getVisibleChildNotes() only see a child once its branch exists. if (noteDef.children) { for (const childDef of noteDef.children) { const childNote = buildShareNote(childDef); - // TODO: Create corresponding SBranch. + new SBranch([ + utils.randomString(12), + childNote.noteId, + note.noteId, + "", // prefix + "", // isExpanded + false // utcDateModified + ]); } }