diff --git a/src/lib/router.ts b/src/lib/router.ts index bf3bb1816..392907389 100644 --- a/src/lib/router.ts +++ b/src/lib/router.ts @@ -425,16 +425,6 @@ export function extractBaseUrl(hostAndPort: string): BaseUrl { // encoded shape never reaches link building or anything user-visible. Only // identifiers are decoded: revision and path segments are deliberately kept // encoded, because branch and tag lookups are keyed on the encoded name. -// Decoding throws on a stray `%` in a repo alias, which is operator-supplied -// and unvalidated, so an undecodable segment is passed through untouched. -function decodeIdentifier(segment: string): string { - try { - return decodeURIComponent(segment); - } catch { - return segment; - } -} - function urlToRoute(url: URL): Route | null { // Normalize a trailing slash (`/guides/protocol/` → `/guides/protocol`) so // both the strict marketing resolver and the lenient repo resolver agree. @@ -454,13 +444,17 @@ function urlToRoute(url: URL): Route | null { if (id === "users") { const did = segments.shift(); if (did) { - return { resource: "users", baseUrl, did: decodeIdentifier(did) }; + return { + resource: "users", + baseUrl, + did: utils.safeDecodeURIComponent(did), + }; } return null; } else if (id) { return resolveRepoRoute( baseUrl, - decodeIdentifier(id), + utils.safeDecodeURIComponent(id), segments, url.search, ); diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 90dbd1c00..c0d90879b 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -305,6 +305,17 @@ export function formatObjectId(id: string): string { return id.substring(0, 7); } +// Percent-decoding throws on a stray `%`, which branch names, tags and repo +// aliases are all free to contain, so a segment that is not a well-formed +// encoding is returned untouched rather than taking down whatever renders it. +export function safeDecodeURIComponent(segment: string): string { + try { + return decodeURIComponent(segment); + } catch { + return segment; + } +} + export function formatQualifiedRefname( refname: string, peer: string | undefined, diff --git a/src/views/repos/History.svelte b/src/views/repos/History.svelte index 6bf883384..a978dc7ca 100644 --- a/src/views/repos/History.svelte +++ b/src/views/repos/History.svelte @@ -10,7 +10,11 @@ import config from "@app/lib/config"; import { HttpdClient } from "@http-client"; - import { baseUrlToString, formatQualifiedRefname } from "@app/lib/utils"; + import { + baseUrlToString, + formatQualifiedRefname, + safeDecodeURIComponent, + } from "@app/lib/utils"; import { groupCommits } from "@app/lib/commit"; import Button from "@app/components/Button.svelte"; @@ -37,8 +41,13 @@ export let nodeId: string; export let nodeAvatarUrl: string | undefined; + // The revision carries the encoded branch name after an in-app navigation, + // and this refname reaches the user as part of the clone popover's archive + // command. $: currentRefname = formatQualifiedRefname( - revision || repo.payloads["xyz.radicle.project"].data.defaultBranch, + revision + ? safeDecodeURIComponent(revision) + : repo.payloads["xyz.radicle.project"].data.defaultBranch, peer, ); diff --git a/src/views/repos/Source.svelte b/src/views/repos/Source.svelte index f39bf0b40..8f62ac1ec 100644 --- a/src/views/repos/Source.svelte +++ b/src/views/repos/Source.svelte @@ -4,7 +4,10 @@ import { HttpdClient } from "@http-client"; - import { formatQualifiedRefname } from "@app/lib/utils"; + import { + formatQualifiedRefname, + safeDecodeURIComponent, + } from "@app/lib/utils"; import BlobComponent from "./Source/Blob.svelte"; import Button from "@app/components/Button.svelte"; @@ -48,8 +51,13 @@ }); }; + // The revision carries the encoded branch name after an in-app navigation, + // and this refname reaches the user as part of the clone popover's archive + // command. $: currentRefname = formatQualifiedRefname( - revision || repo.payloads["xyz.radicle.project"].data.defaultBranch, + revision + ? safeDecodeURIComponent(revision) + : repo.payloads["xyz.radicle.project"].data.defaultBranch, peer, ); diff --git a/src/views/repos/Source/PeerBranchSelector.svelte b/src/views/repos/Source/PeerBranchSelector.svelte index 39b7c5aa8..1639bc614 100644 --- a/src/views/repos/Source/PeerBranchSelector.svelte +++ b/src/views/repos/Source/PeerBranchSelector.svelte @@ -12,6 +12,7 @@ getBranchesFromRefs, getTagsFromRefs, gravatarURL, + safeDecodeURIComponent, } from "@app/lib/utils"; import Badge from "@app/components/Badge.svelte"; @@ -207,6 +208,14 @@ // rather than claiming there are none. $: tagsTabDisabled = !hasTags && peers !== undefined; + // Links to a branch encode its name so a slash cannot be mistaken for a path + // separator, which leaves the encoded form in the route after an in-app + // navigation. Only the label is decoded: the comparisons above match against + // both forms and rely on the route value as it stands. + $: selectedBranchLabel = selectedBranch + ? safeDecodeURIComponent(selectedBranch) + : undefined; + $: isSelectedBranchCanonical = (() => { if (onCanonical) return true; if (!selectedBranch || peer) return false; @@ -368,7 +377,7 @@ {:else if selectedBranch} - {selectedBranch} + {selectedBranchLabel} {#if isSelectedBranchCanonical} diff --git a/tests/e2e/repo.spec.ts b/tests/e2e/repo.spec.ts index bdf831cb4..d802b7260 100644 --- a/tests/e2e/repo.spec.ts +++ b/tests/e2e/repo.spec.ts @@ -438,6 +438,59 @@ test("peer and branch switching", async ({ page }) => { } }); +// Links to a canonical branch encode its name so a slash cannot be mistaken +// for a path separator, which leaves the encoded form in the route after an +// in-app navigation. The label has to show the branch name, not the encoding. +// Only the canonical list encodes: the peer list covered above passes branch +// names through untouched, so the branch has to be canonical to reproduce it. +test("canonical branch with a slash in the name", async ({ page, peer }) => { + const { rid, repoFolder } = await createRepo(peer, { + name: "slash-branch", + }); + + // A branch is only resolved by delegate quorum when a canonical-ref rule + // matches it. Without this the branch is listed under the peer instead, + // where names are never encoded. + await peer.rad( + [ + "id", + "update", + "--title", + "Add canonical ref rule", + "--description", + "", + "--payload", + "xyz.radicle.crefs", + "rules", + JSON.stringify({ + "refs/heads/feature/*": { + allow: [`did:key:${peer.nodeId}`], + threshold: 1, + }, + }), + "--no-confirm", + ], + { cwd: repoFolder }, + ); + + await peer.git(["checkout", "-b", "feature/branch"], { cwd: repoFolder }); + await peer.git( + ["commit", "--allow-empty", "--message", "branch off the default"], + { cwd: repoFolder }, + ); + await peer.git(["push", "rad", "feature/branch"], { cwd: repoFolder }); + + await page.goto(peer.ridUrl(rid)); + const trigger = page.locator('[title="Change branch"]:visible').first(); + await trigger.click(); + await page.getByRole("button", { name: /feature\/branch Canonical/ }).click(); + + await expect(trigger).toHaveText(/feature\/branch/); + await expect(trigger).not.toHaveText(/%2F/); + // The URL keeps the encoded form; only what the user reads is decoded. + await expect(page).toHaveURL(/tree\/feature%2Fbranch/); +}); + test("only one modal can be open at a time", async ({ page }) => { await page.goto(sourceBrowsingUrl); diff --git a/tests/unit/utils.test.ts b/tests/unit/utils.test.ts index e9f6fdce4..ae90db125 100644 --- a/tests/unit/utils.test.ts +++ b/tests/unit/utils.test.ts @@ -352,3 +352,26 @@ describe("repoSegment", () => { expect(utils.repoSegment(repo)).toEqual(rid); }); }); + +describe("safeDecodeURIComponent", () => { + test("decodes an encoded branch name", () => { + expect(utils.safeDecodeURIComponent("releaseprep%2Fv27.1")).toEqual( + "releaseprep/v27.1", + ); + }); + + test("leaves an already decoded name untouched", () => { + expect(utils.safeDecodeURIComponent("releaseprep/v27.1")).toEqual( + "releaseprep/v27.1", + ); + }); + + // Branch names, tags and repo aliases may all contain a bare `%`, which is + // not a well-formed encoding and would otherwise throw. + test("returns an undecodable segment unchanged", () => { + expect(utils.safeDecodeURIComponent("100%")).toEqual("100%"); + expect(utils.safeDecodeURIComponent("wip/50%-done")).toEqual( + "wip/50%-done", + ); + }); +});