diff --git a/src/lib/feature/feeds/feed.svelte.ts b/src/lib/feature/feeds/feed.svelte.ts index e33f63d9..f8261b15 100644 --- a/src/lib/feature/feeds/feed.svelte.ts +++ b/src/lib/feature/feeds/feed.svelte.ts @@ -164,6 +164,9 @@ export interface FeedTypes { post: CovesPostView comments: Promise focused: { uri: AtUri; rkey: string; parentUri?: AtUri } + // The focused comment's author, which names the repo the comment record + // lives in — the commenter segment of its canonical permalink. + commentAuthor: ThreadViewComment['comment']['author'] params: { postUri: string; comments: GetCommentsParams } }, ] diff --git a/src/lib/feature/post/fresh-post.test.ts b/src/lib/feature/post/fresh-post.test.ts new file mode 100644 index 00000000..cc3a5127 --- /dev/null +++ b/src/lib/feature/post/fresh-post.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it, vi } from 'vitest' + +// --------------------------------------------------------------------------- +// `buildFreshPostView` reads the signed-in viewer off `profile.current`, whose +// real module touches localStorage at import time. Stub it to the one shape +// the builder needs: an authenticated viewer. +// --------------------------------------------------------------------------- + +// Literals, not constants: the factory is hoisted above every declaration. +vi.mock('$lib/app/state/auth.svelte', () => ({ + profile: { + current: { + type: 'authenticated', + did: 'did:plc:author', + handle: 'mari.local.coves.dev', + avatar: undefined, + }, + }, +})) + +const VIEWER_DID = 'did:plc:author' + +import type { AtUri, CID, CreatePostOutput } from '$lib/api/coves/types' +import type { DID, Handle } from '$lib/types/atproto' +import { buildFreshPostView } from './fresh-post' + +const RKEY = '3lrkey' + +const output: CreatePostOutput = { + uri: `at://${VIEWER_DID}/social.coves.community.postv2/${RKEY}` as AtUri, + cid: 'bafyreigh2akiscaildc' as CID, +} + +// The community the create form was submitted against, exactly as the AppView +// serves it: a DNS handle *and* the structured `name` + `origin` pair. +const community = { + did: 'did:plc:comm' as DID, + handle: 'c-gardening.coves.social' as Handle, + name: 'gardening', + origin: 'coves.social', +} + +describe('buildFreshPostView', () => { + it('carries the community origin onto the optimistic view', () => { + // The origin is what decides the community's canonical route param, so a + // view that drops it addresses a different URL than the one the create + // flow redirects to — and the one-shot stash is lost to that redirect. + const view = buildFreshPostView({ output, community }) + + expect(view?.community.origin).toBe('coves.social') + }) + + it('still carries the identifying community fields alongside it', () => { + const view = buildFreshPostView({ output, community }) + + expect(view?.community).toMatchObject({ + did: 'did:plc:comm', + handle: 'c-gardening.coves.social', + name: 'gardening', + }) + }) +}) diff --git a/src/lib/feature/post/fresh-post.ts b/src/lib/feature/post/fresh-post.ts index 9107a4af..dd09f6c1 100644 --- a/src/lib/feature/post/fresh-post.ts +++ b/src/lib/feature/post/fresh-post.ts @@ -39,7 +39,13 @@ export function takeFreshPost(rkey: string): PostView | undefined { */ export function buildFreshPostView(args: { output: CreatePostOutput - community: { did: DID; handle?: Handle; name: string; avatar?: string } + community: { + did: DID + handle?: Handle + name: string + avatar?: string + origin?: string + } title?: string content?: string url?: string @@ -69,6 +75,9 @@ export function buildFreshPostView(args: { handle: community.handle, name: community.name, avatar: community.avatar, + // The origin decides the community's canonical route param, so a view + // that drops it is not addressed by the URL the create flow redirects to. + origin: community.origin, }, record: { $type: 'social.coves.community.post', diff --git a/src/lib/feature/post/helpers.test.ts b/src/lib/feature/post/helpers.test.ts index 6946e329..07f53c2b 100644 --- a/src/lib/feature/post/helpers.test.ts +++ b/src/lib/feature/post/helpers.test.ts @@ -13,6 +13,8 @@ import { bestImageURL, buildLegacyPostAtUri, buildPostAtUri, + canonicalCommentPath, + canonicalPostPath, commentLink, decodeCrosspostDraft, encodeCrosspostDraft, @@ -30,6 +32,7 @@ import { streamableEmbedUrl, } from './helpers' import { EMBED_FRAME_ORIGINS } from '$lib/app/util/embed-hosts' +import { INVALID_HANDLE } from '$lib/types/atproto' vi.mock('$env/dynamic/public', () => ({ env: { PUBLIC_INSTANCE_URL: 'https://coves.social' }, @@ -994,6 +997,218 @@ describe('commentLink', () => { }) }) +// --------------------------------------------------------------------------- +// canonicalPostPath() / canonicalCommentPath() +// +// The route-canonicalisation rule, kept beside the link builders it mirrors: +// given the hydrated record and the DECODED route params SvelteKit hands a +// loader, return the path to redirect to, or null when the URL is already the +// one the link builder would emit. The returned path is percent-encoded (it is +// a URL); the params compared against it are not. +// --------------------------------------------------------------------------- + +const OWNED_URI = 'at://did:plc:author/social.coves.community.postv2/3lrkey' +const AUTHOR = { did: 'did:plc:author', handle: 'mari.local.coves.dev' } +const GARDENING = { + did: 'did:plc:comm', + handle: 'gardening.local.coves.dev', + name: 'gardening', +} + +describe('canonicalPostPath', () => { + const post = { uri: OWNED_URI, community: GARDENING, author: AUTHOR } + + it('returns null when both params already match the link postLink emits', () => { + expect( + canonicalPostPath(post, { + handle: 'gardening.local.coves.dev', + owner: 'mari.local.coves.dev', + }), + ).toBeNull() + }) + + it('returns the canonical path when the community segment names another community', () => { + expect( + canonicalPostPath(post, { + handle: 'cooking.local.coves.dev', + owner: 'mari.local.coves.dev', + }), + ).toBe(postLink(post)) + }) + + it('returns the canonical path for a DID-form community param', () => { + // The DID is matcher-valid and resolves, so it is a working alias — but + // the handle/name form is the one link builders emit. + expect( + canonicalPostPath(post, { + handle: 'did:plc:comm', + owner: 'mari.local.coves.dev', + }), + ).toBe('/c/gardening.local.coves.dev/post/mari.local.coves.dev/3lrkey') + }) + + it('returns the canonical path for a DID-form owner param when the author has a handle', () => { + expect( + canonicalPostPath(post, { + handle: 'gardening.local.coves.dev', + owner: 'did:plc:author', + }), + ).toBe('/c/gardening.local.coves.dev/post/mari.local.coves.dev/3lrkey') + }) + + it('accepts a DID owner param when the post carries no author ref', () => { + // Nothing proves a handle for that repo, so the DID is canonical. + expect( + canonicalPostPath( + { uri: OWNED_URI, community: GARDENING }, + { handle: 'gardening.local.coves.dev', owner: 'did:plc:author' }, + ), + ).toBeNull() + }) + + it('accepts a DID owner param for a legacy community-owned post', () => { + // The record lives in the community's repo; the author is a different + // DID, so substituting their handle would address a record that does not + // exist. The authority DID stays canonical. + const legacy = { + uri: 'at://did:plc:comm/social.coves.community.post/3lrkey', + community: GARDENING, + author: AUTHOR, + } + expect( + canonicalPostPath(legacy, { + handle: 'gardening.local.coves.dev', + owner: 'did:plc:comm', + }), + ).toBeNull() + }) + + it('canonicalises a remote community to its name@origin form', () => { + // A bridged Lemmy community: its DNS handle resolves and the matcher + // accepts it, but `name@origin` is the address link builders emit. + const remote = { + uri: OWNED_URI, + community: { + did: 'did:plc:comm', + handle: 'gaming.lemmy-world.tdpl.io', + name: 'gaming', + origin: 'lemmy.world', + }, + author: AUTHOR, + } + const canonical = '/c/gaming@lemmy.world/post/mari.local.coves.dev/3lrkey' + + expect( + canonicalPostPath(remote, { + handle: 'gaming@lemmy.world', + owner: 'mari.local.coves.dev', + }), + ).toBeNull() + expect( + canonicalPostPath(remote, { + handle: 'gaming.lemmy-world.tdpl.io', + owner: 'mari.local.coves.dev', + }), + ).toBe(canonical) + expect( + canonicalPostPath(remote, { + handle: 'did:plc:comm', + owner: 'mari.local.coves.dev', + }), + ).toBe(canonical) + }) + + it('canonicalises a local-origin community to its bare name', () => { + // PUBLIC_INSTANCE_URL is pinned to coves.social at the top of this file. + const local = { + uri: OWNED_URI, + community: { + did: 'did:plc:comm', + handle: 'c-gaming.coves.social', + name: 'gaming', + origin: 'coves.social', + }, + author: AUTHOR, + } + expect( + canonicalPostPath(local, { + handle: 'gaming', + owner: 'mari.local.coves.dev', + }), + ).toBeNull() + expect( + canonicalPostPath(local, { + handle: 'gaming.coves.social', + owner: 'mari.local.coves.dev', + }), + ).toBe('/c/gaming/post/mari.local.coves.dev/3lrkey') + }) +}) + +describe('canonicalCommentPath', () => { + const post = { uri: OWNED_URI, community: GARDENING, author: AUTHOR } + const comment = { + uri: 'at://did:plc:cmt/social.coves.community.comment/3lc' as AtUri, + author: { did: 'did:plc:cmt', handle: 'ravi.local.coves.dev' }, + } + const canonicalParams = { + handle: 'gardening.local.coves.dev', + owner: 'mari.local.coves.dev', + commenter: 'ravi.local.coves.dev', + } + + it('returns null when all three params match the link commentLink emits', () => { + expect(canonicalCommentPath(post, comment, canonicalParams)).toBeNull() + }) + + it('returns the canonical comment path when the community segment is wrong', () => { + expect( + canonicalCommentPath(post, comment, { + ...canonicalParams, + handle: 'cooking.local.coves.dev', + }), + ).toBe(commentLink(post, comment.uri, comment.author)) + }) + + it('returns the canonical comment path for a DID-form commenter param', () => { + expect( + canonicalCommentPath(post, comment, { + ...canonicalParams, + commenter: 'did:plc:cmt', + }), + ).toBe( + '/c/gardening.local.coves.dev/post/mari.local.coves.dev/3lrkey/comment/ravi.local.coves.dev/3lc', + ) + }) +}) + +describe('unresolved author handles (handle.invalid)', () => { + // `handle.invalid` is what ATProto serves for a handle that no longer + // resolves. Emitting it as the owner segment would send a working DID URL + // to a 404, so both the link builder and the canonicalisation rule must + // treat it as no handle at all. + const post = { + uri: OWNED_URI, + community: GARDENING, + author: { did: 'did:plc:author', handle: INVALID_HANDLE }, + } + + it('postLink emits the DID owner segment for an unresolved author handle', () => { + expect(postLink(post)).toBe( + '/c/gardening.local.coves.dev/post/did%3Aplc%3Aauthor/3lrkey', + ) + }) + + it('canonicalPostPath accepts the DID owner param for an unresolved author handle', () => { + expect( + canonicalPostPath(post, { + handle: 'gardening.local.coves.dev', + owner: 'did:plc:author', + }), + ).toBeNull() + }) +}) + // --------------------------------------------------------------------------- // encodeCrosspostDraft / decodeCrosspostDraft // --------------------------------------------------------------------------- diff --git a/src/lib/feature/post/helpers.ts b/src/lib/feature/post/helpers.ts index cf7b335f..6d6a63b1 100644 --- a/src/lib/feature/post/helpers.ts +++ b/src/lib/feature/post/helpers.ts @@ -1,7 +1,8 @@ import type { AtUri, PostEmbed } from '$lib/api/coves/types' import { parseAtUri } from '$lib/api/coves/types' import { isImage, isVideo, isWebUrl } from '$lib/app/util/url' -import { communityLink } from '$lib/app/util/links' +import { communityLink, communityRouteParam } from '$lib/app/util/links' +import { usableHandle } from '$lib/types/atproto' import { STREAMABLE_EMBED_ORIGIN } from '$lib/app/util/embed-hosts' import { type ImagePreset, @@ -156,13 +157,15 @@ export interface PostLinkRef { * The permalink segment naming the repo a record lives in. Defaults to the * record's AT-URI authority DID; the prettier handle is substituted only when * `ref` proves it belongs to that same repo, so the segment always addresses - * the record that actually exists. + * the record that actually exists. An unresolved `handle.invalid` counts as no + * handle (see {@link usableHandle}) — emitting it would route to a dead end. */ function repoSegment( authority: string, ref: { did: string; handle?: string } | undefined, ): string { - return ref?.did === authority && ref.handle ? ref.handle : authority + const handle = ref?.did === authority ? usableHandle(ref.handle) : undefined + return handle ?? authority } /** @@ -224,6 +227,61 @@ export function commentLink( return `${postLink(post)}/comment/${encodeURIComponent(segment)}/${encodeURIComponent(rkey)}` } +/** + * The path a post URL should be redirected to, or null when it is already the + * canonical one {@link postLink} emits. + * + * A post is reachable through several working aliases — the community's DID or + * DNS handle in place of its canonical `name` / `name@origin` param, the + * author's DID in place of their handle — and every alias resolves to the same + * record. Only one of them is the link builders' output, so loaders redirect + * the rest to it, keeping one URL per post for sharing, history and crawlers. + * + * Callers must redirect with 302, never 301. The canonical community segment + * depends on `LOCAL_INSTANCE_DOMAIN`, which is deployment config + * (`PUBLIC_INSTANCE_DOMAIN`, or the hostname of `PUBLIC_INSTANCE_URL`), and + * browsers cache 301s indefinitely by default. A 301 issued under a + * misconfigured domain would keep bouncing visitors to the wrong form — and, + * once the config is fixed, into a cached loop — with no way to clear it + * server-side. + * + * @param params - The route params as SvelteKit hands them to a loader, which + * is to say fully decoded. They are compared against the unencoded segment + * values {@link postLink} builds from, never against the encoded path. + */ +export function canonicalPostPath( + post: PostLinkRef, + params: { handle: string; owner: string }, +): string | null { + const { did } = parseAtUri(post.uri as AtUri) + const canonical = + params.handle === communityRouteParam(post.community) && + params.owner === repoSegment(did, post.author) + return canonical ? null : postLink(post) +} + +/** + * The path a comment permalink should be redirected to, or null when it is + * already the canonical one {@link commentLink} emits. The comment-page + * counterpart of {@link canonicalPostPath}: the post's two segments must match + * on top of the commenter segment, so a stale community or owner alias in a + * comment URL is corrected the same way it is on the post page. + * + * @param params - The decoded route params, compared against the unencoded + * segment values, exactly as in {@link canonicalPostPath}. + */ +export function canonicalCommentPath( + post: PostLinkRef, + comment: { uri: AtUri; author?: { did: string; handle?: string } }, + params: { handle: string; owner: string; commenter: string }, +): string | null { + const { did } = parseAtUri(comment.uri) + const canonical = + canonicalPostPath(post, params) === null && + params.commenter === repoSegment(did, comment.author) + return canonical ? null : commentLink(post, comment.uri, comment.author) +} + export type MediaType = 'video' | 'image' | 'iframe' | 'embed' | 'none' export type IframeType = 'youtube' | 'streamable' | 'video' | 'none' diff --git a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/+page.ts b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/+page.ts index 4cfb3e68..318e9e9c 100644 --- a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/+page.ts +++ b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/+page.ts @@ -1,4 +1,4 @@ -import { error } from '@sveltejs/kit' +import { error, redirect } from '@sveltejs/kit' import { coves } from '$lib/api/client.svelte' import { type AtUri, @@ -20,7 +20,7 @@ import { type FeedTypes, } from '$lib/feature/feeds/feed.svelte' import { takeFreshPost } from '$lib/feature/post/fresh-post' -import { buildPostAtUri } from '$lib/feature/post/helpers' +import { buildPostAtUri, canonicalPostPath } from '$lib/feature/post/helpers' import { addressesRecord, fetchExactPost, @@ -181,6 +181,11 @@ export async function load({ params, url, fetch, route }) { ? Promise.resolve([]) : client.getComments(comments).then((r) => r.comments) + // The canonical redirect below may abandon this promise; claim its + // rejection now so it cannot escape as an unhandled one. The page still + // receives the original promise, so its `{#await}` sees the real failure. + void commentsPromise.catch(() => undefined) + return { post: result, comments: commentsPromise, @@ -207,10 +212,24 @@ export async function load({ params, url, fetch, route }) { }), ) + // The owner and rkey segments are checked against the record itself, but the + // community segment is not, so one post is reachable through every community + // slug the matcher admits. Once the post has hydrated it names its own + // canonical URL; send the browser there so a post has one address. + // 302, not 301: see canonicalPostPath. + // + // An unavailable post carries no community ref to canonicalise against, so + // it renders its "post removed" state on whatever slug was asked for. + const hydrated = loaded.value?.post + if (hydrated) { + const canonical = canonicalPostPath(hydrated, params) + if (canonical) redirect(302, `${canonical}${url.search}`) + } + // The community ref rides along on the post. When the post is unavailable // there's nothing to populate the card with, so fall back to the default // sidebar (omitting the slot) rather than rendering a broken CommunityCard. - const community = loaded.value?.post?.community + const community = hydrated?.community return { data: loaded, diff --git a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/acceptance.test.ts b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/acceptance.test.ts index 2f1f28bd..6fa2313b 100644 --- a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/acceptance.test.ts +++ b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/acceptance.test.ts @@ -56,6 +56,12 @@ vi.mock('$lib/api/coves/sort', () => ({ mapSort: () => ({ sort: 'hot' }), })) +// Pinned so `LOCAL_INSTANCE_DOMAIN` is a real domain rather than null: the +// canonical community segment depends on it whenever a ref carries an origin. +vi.mock('$env/dynamic/public', () => ({ + env: { PUBLIC_INSTANCE_URL: 'https://coves.social' }, +})) + import { postLink } from '$lib/feature/post/helpers' import { load } from './+page' @@ -73,6 +79,10 @@ const LEGACY_URI = `at://${AUTHOR_DID}/${LEGACY_POST_COLLECTION}/${RKEY}` const EXPECTED_LINK = `/c/${COMMUNITY_HANDLE}/post/${AUTHOR_HANDLE}/${RKEY}` +// A different, perfectly valid community slug: the post is not in it, so a +// permalink built on it is an alias that must not render. +const OTHER_COMMUNITY_HANDLE = 'cooking.local.coves.dev' + // An author-owned post: the URI authority is the author's DID, not the // community's. Assigned to a variable (not passed as a fresh object literal) // so the extra `PostView` fields do not trip excess-property checking. @@ -196,4 +206,29 @@ describe('owner-carrying post permalink (acceptance)', () => { expect(value.post).toEqual(post) expect(value.unavailable).toBeUndefined() }) + + it('redirects a wrong community segment to the canonical permalink, which then loads without bouncing', async () => { + const canonicalPath = postLink(post) + const search = '?sort=top' + + // Same owner and rkey — only the community segment is an alias. + const aliasPath = `/c/${OTHER_COMMUNITY_HANDLE}/post/${AUTHOR_HANDLE}/${RKEY}` + expect(aliasPath).not.toBe(canonicalPath) + + await expect( + load(makeArgs(routeParamsFromLink(aliasPath), aliasPath + search)), + ).rejects.toMatchObject({ + status: 302, + location: canonicalPath + search, + }) + + // ...and the target of that redirect is stable: it resolves to the post + // instead of bouncing on to another location. + const result = await load( + makeArgs(routeParamsFromLink(canonicalPath), canonicalPath + search), + ) + const value = loadedValue(result) + expect(value.post).toEqual(post) + expect(value.unavailable).toBeUndefined() + }) }) diff --git a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/+page.ts b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/+page.ts index dc928695..b02b13bd 100644 --- a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/+page.ts +++ b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/+page.ts @@ -1,4 +1,4 @@ -import { error } from '@sveltejs/kit' +import { error, redirect } from '@sveltejs/kit' import { coves } from '$lib/api/client.svelte' import { type AtUri, @@ -13,7 +13,7 @@ import { ReactiveState } from '$lib/app/util/reactive.svelte' import { MAX_INLINE_DEPTH } from '$lib/feature/comment/comments.svelte' import CommunityCard from '$lib/feature/community/CommunityCard.svelte' import { feed } from '$lib/feature/feeds/feed.svelte' -import { buildPostAtUri } from '$lib/feature/post/helpers' +import { buildPostAtUri, canonicalCommentPath } from '$lib/feature/post/helpers' import { addressesRecord, fetchExactPost, @@ -145,6 +145,11 @@ export async function load({ params, url, fetch, route }) { // Already resolved, but kept as a promise so the page shares the post // page's {#await}/reload shape. comments: Promise.resolve(subtree.comments), + // The focused comment's author, which supplies the handle form of the + // commenter segment the canonical permalink is built from. It rides + // beside `focused` rather than inside it because `focused` describes the + // comment the page renders, not the repo the URL addresses it by. + commentAuthor: root.comment.author, focused: { uri: root.comment.uri, rkey: parseAtUri(root.comment.uri).rkey, @@ -169,13 +174,28 @@ export async function load({ params, url, fetch, route }) { }), ) + // The commenter and crkey segments are checked against the record above, and + // an unknown comment has already 404ed inside the init, so by here the post + // and the focused comment both exist. What is still unchecked is which of + // their many working aliases the URL used — the community slug, and the DID + // form of either actor segment — so send the browser to the one URL + // `commentLink` emits and a comment has one address. + // 302, not 301: see canonicalPostPath. + const { post, focused, commentAuthor } = loaded.value + const canonical = canonicalCommentPath( + post, + { uri: focused.uri, author: commentAuthor }, + params, + ) + if (canonical) redirect(302, `${canonical}${url.search}`) + return { data: loaded, communityHandle, slots: { sidebar: { component: CommunityCard, - props: { community: loaded.value.post.community }, + props: { community: post.community }, }, }, } diff --git a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/page.cache.test.ts b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/page.cache.test.ts index d91848f2..e6854435 100644 --- a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/page.cache.test.ts +++ b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/page.cache.test.ts @@ -58,7 +58,6 @@ import { feeds } from '$lib/feature/feeds/feed.svelte' import { load } from './+page' const OWNER_DID = 'did:plc:author' -const OWNER_HANDLE = 'mari.local.coves.dev' const COMMUNITY_DID = 'did:plc:comm' const COMMUNITY_HANDLE = 'gardening.local.coves.dev' const COMMENTER_DID = 'did:plc:commenter456' @@ -91,6 +90,10 @@ function makeArgs(): Parameters[0] { } as unknown as Parameters[0] } +// Both fixtures below are deliberately author-less / handle-less: nothing +// proves a handle for either repo, so the DID actor segments are canonical. +// That keeps the URL free of a redirect and the file free of any identity +// resolution — the cache is what these tests are about. function hydratedPost(uri: string) { return { uri, @@ -98,7 +101,6 @@ function hydratedPost(uri: string) { rkey: RKEY, indexedAt: '2026-01-01T00:00:00.000Z', createdAt: '2026-01-01T00:00:00.000Z', - author: { did: OWNER_DID, handle: OWNER_HANDLE }, community: { did: COMMUNITY_DID, handle: COMMUNITY_HANDLE, @@ -128,7 +130,7 @@ function subtree(postUri: string) { }, createdAt: '2026-01-02T00:00:00.000Z', }, - author: { did: COMMENTER_DID, handle: 'commenter.coves.test' }, + author: { did: COMMENTER_DID }, post: { uri: postUri, cid: 'bafyreigh2akiscaildc' }, stats: { upvotes: 0, downvotes: 0, score: 0, replyCount: 0 }, }, diff --git a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/page.test.ts b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/page.test.ts index 0bc74903..ea5838f1 100644 --- a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/page.test.ts +++ b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/comment/[commenter=actor]/[crkey]/page.test.ts @@ -57,7 +57,14 @@ vi.mock('$lib/api/coves/sort', () => ({ mapSort: () => ({ sort: 'hot' }), })) +// Pinned so `LOCAL_INSTANCE_DOMAIN` is a real domain rather than null: the +// canonical community segment depends on it whenever a ref carries an origin. +vi.mock('$env/dynamic/public', () => ({ + env: { PUBLIC_INSTANCE_URL: 'https://coves.social' }, +})) + import { XrpcError } from '$lib/api/coves/xrpc' +import { INVALID_HANDLE } from '$lib/types/atproto' import CommunityCard from '$lib/feature/community/CommunityCard.svelte' import { load } from './+page' @@ -94,9 +101,12 @@ function makeArgs(overrides?: { query?: string }): Parameters[0] { const handle = overrides?.handle ?? COMMUNITY_HANDLE - const owner = overrides?.owner ?? OWNER_DID + // The canonical actor forms for the default fixture: both the post author + // and the comment author carry handles and own their records, so a DID in + // either segment is now a redirecting alias. + const owner = overrides?.owner ?? OWNER_HANDLE const rkey = overrides?.rkey ?? RKEY - const commenter = overrides?.commenter ?? COMMENTER_DID + const commenter = overrides?.commenter ?? COMMENTER_HANDLE const crkey = overrides?.crkey ?? CRKEY const query = overrides?.query ?? '' return { @@ -174,6 +184,70 @@ function subtree( } } +/** + * The same two fixtures with an author whose handle no longer resolves — + * `handle.invalid` is what ATProto serves for one, and it is the shape the + * API types actually admit (the handle field is not optional). + * + * `repoSegment` treats it as absent, so nothing proves a handle for either + * repo and the URI authority DIDs are the canonical actor segments. That is + * what lets a DID-segment test load without redirecting, and it exercises the + * unresolved-handle guard on the way through. + */ +function unresolvedAuthorPost(uri: string): ReturnType { + return { + ...hydratedPost(uri), + author: { did: OWNER_DID, handle: INVALID_HANDLE }, + } +} + +function unresolvedCommenterSubtree( + crkey: string, + parentUri?: string, + postUri: string = POSTV2_URI, +): ReturnType { + const tree = subtree(crkey, parentUri, postUri) + return { + ...tree, + comments: tree.comments.map((entry) => ({ + ...entry, + comment: { + ...entry.comment, + author: { did: COMMENTER_DID, handle: INVALID_HANDLE }, + }, + })), + } +} + +/** The one URL this fixture's comment is canonically addressed by. */ +const CANONICAL_COMMENT_PATH = + `/c/${COMMUNITY_HANDLE}/post/${OWNER_HANDLE}/${RKEY}` + + `/comment/${COMMENTER_HANDLE}/${CRKEY}` + +const COMMENT_PERMALINK_PATTERN = + /^\/c\/([^/]+)\/post\/([^/]+)\/([^/]+)\/comment\/([^/]+)\/([^/]+)$/ + +/** + * Turns a redirect `location` back into loader args, standing in for the + * router. Used to prove a redirect target does not itself redirect. + */ +function argsFromLocation(location: string): Parameters[0] { + const [path, query] = location.split('?') + const match = COMMENT_PERMALINK_PATTERN.exec(path) + if (!match) { + throw new Error(`redirect location is not a comment permalink: ${location}`) + } + const [, handle, owner, rkey, commenter, crkey] = match + return makeArgs({ + handle: decodeURIComponent(handle), + owner: decodeURIComponent(owner), + rkey: decodeURIComponent(rkey), + commenter: decodeURIComponent(commenter), + crkey: decodeURIComponent(crkey), + query: query ? `?${query}` : '', + }) +} + /** * Answers `getPosts` from a URI→entry table, preserving request order and * filling every unknown URI with the notFound sentinel — the batch endpoint's @@ -265,15 +339,20 @@ describe('comment permalink loader', () => { // ------------------------------------------------------------------------- it('returns the post, focused comment, subtree, and a CommunityCard slot on the happy path', async () => { - const post = hydratedPost(POSTV2_URI) - const tree = subtree(CRKEY, POSTV2_URI) + // Neither ref carries a usable handle, so both DID segments are canonical + // here and the load runs to completion with no profile lookup at all. + const post = unresolvedAuthorPost(POSTV2_URI) + const tree = unresolvedCommenterSubtree(CRKEY, POSTV2_URI) + serve({ [POSTV2_URI]: post }) mockCovesMethods.getComments.mockResolvedValue(tree) // Hold the probe open so the two requests can be told apart in time. const probe = deferred<{ posts: unknown[] }>() mockCovesMethods.getPosts.mockReturnValue(probe.promise) - const pending = load(makeArgs()) + const pending = load( + makeArgs({ owner: OWNER_DID, commenter: COMMENTER_DID }), + ) await flush() // The subtree is asked for on the postv2 URI without waiting for the @@ -320,11 +399,19 @@ describe('comment permalink loader', () => { sidebar: { component: unknown; props: { community: unknown } } } expect(slots.sidebar.component).toBe(CommunityCard) - expect(slots.sidebar.props.community).toEqual(post.community) + expect(slots.sidebar.props.community).toEqual( + hydratedPost(POSTV2_URI).community, + ) }) it('resolves a handle owner through getProfile and probes with its DID', async () => { - await load(makeArgs({ owner: OWNER_HANDLE })) + // A DID commenter over an unresolved comment author, so the one profile + // lookup this asserts is unambiguously the owner's. + mockCovesMethods.getComments.mockResolvedValue( + unresolvedCommenterSubtree(CRKEY, POSTV2_URI), + ) + + await load(makeArgs({ owner: OWNER_HANDLE, commenter: COMMENTER_DID })) expect(mockCovesMethods.getProfile).toHaveBeenCalledTimes(1) expect(mockCovesMethods.getProfile).toHaveBeenCalledWith({ @@ -549,4 +636,73 @@ describe('comment permalink loader', () => { expect(probes()).toEqual([[POSTV2_URI, LEGACY_URI]]) expect(commentedOn()).toBe(POSTV2_URI) }) + + // ------------------------------------------------------------------------- + // Canonical URL enforcement + // + // Same rule as the post page, extended to the commenter segment: once the + // post and the focused comment are in hand, a non-canonical segment + // redirects to the single URL `commentLink` emits. Ordering is unchanged — + // the 404s above still fire first, so a bad crkey never becomes a redirect. + // ------------------------------------------------------------------------- + + it('redirects a wrong community slug to the canonical comment permalink', async () => { + const location = `${CANONICAL_COMMENT_PATH}?sort=top` + + await expect( + load(makeArgs({ handle: 'cooking.local.coves.dev', query: '?sort=top' })), + ).rejects.toMatchObject({ status: 302, location }) + + // The target is stable: the second hop renders the comment. + const result = await load(argsFromLocation(location)) + expect(loadedValue(result).focused).toMatchObject({ rkey: CRKEY }) + }) + + it('redirects a DID-form commenter segment to the handle form', async () => { + await expect( + load(makeArgs({ commenter: COMMENTER_DID })), + ).rejects.toMatchObject({ + status: 302, + location: CANONICAL_COMMENT_PATH, + }) + + const result = await load(argsFromLocation(CANONICAL_COMMENT_PATH)) + expect(loadedValue(result).focused).toMatchObject({ rkey: CRKEY }) + }) + + it('redirects a DID-form owner segment to the author handle form', async () => { + await expect(load(makeArgs({ owner: OWNER_DID }))).rejects.toMatchObject({ + status: 302, + location: CANONICAL_COMMENT_PATH, + }) + + const result = await load(argsFromLocation(CANONICAL_COMMENT_PATH)) + expect(loadedValue(result).focused).toMatchObject({ rkey: CRKEY }) + }) + + it('does not redirect a comment URL that is already canonical', async () => { + const result = await load(makeArgs({ query: '?sort=top' })) + + expect(loadedValue(result).focused).toEqual({ + uri: `at://${COMMENTER_DID}/${COMMENT_COLLECTION}/${CRKEY}`, + rkey: CRKEY, + parentUri: undefined, + }) + }) + + it('404s a missing comment under a wrong slug instead of redirecting', async () => { + // The comment is what the page is; an unknown crkey is a dead URL whether + // or not the community segment happens to be wrong, so the 404 wins. + mockCovesMethods.getComments.mockResolvedValue({ + post: hydratedPost(POSTV2_URI), + comments: [], + }) + + await expect( + load(makeArgs({ handle: 'cooking.local.coves.dev' })), + ).rejects.toMatchObject({ + status: 404, + body: { message: 'couldnt_find_comment' }, + }) + }) }) diff --git a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/page.cache.test.ts b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/page.cache.test.ts index 8ef5ce79..e3f71cf7 100644 --- a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/page.cache.test.ts +++ b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/page.cache.test.ts @@ -58,7 +58,6 @@ import { feeds } from '$lib/feature/feeds/feed.svelte' import { load } from './+page' const OWNER_DID = 'did:plc:author' -const OWNER_HANDLE = 'mari.local.coves.dev' const COMMUNITY_DID = 'did:plc:comm' const COMMUNITY_HANDLE = 'gardening.local.coves.dev' const RKEY = 'abc123' @@ -80,6 +79,10 @@ function makeArgs(): Parameters[0] { } as unknown as Parameters[0] } +// Deliberately author-less: nothing proves a handle for the repo the record +// lives in, so the DID owner segment is this post's canonical one. That keeps +// the URL free of a redirect and the file free of any identity resolution — +// the cache is what these tests are about. function hydratedPost(uri: string) { return { uri, @@ -87,7 +90,6 @@ function hydratedPost(uri: string) { rkey: RKEY, indexedAt: '2026-01-01T00:00:00.000Z', createdAt: '2026-01-01T00:00:00.000Z', - author: { did: OWNER_DID, handle: OWNER_HANDLE }, community: { did: COMMUNITY_DID, handle: COMMUNITY_HANDLE, diff --git a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/page.test.ts b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/page.test.ts index 42c176a1..c02f2b7f 100644 --- a/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/page.test.ts +++ b/src/routes/c/[handle=handle]/post/[owner=actor]/[rkey]/page.test.ts @@ -62,8 +62,31 @@ vi.mock('$lib/api/coves/sort', () => ({ mapSort: () => ({ sort: 'hot' }), })) +// Pinned so `LOCAL_INSTANCE_DOMAIN` is a real domain rather than null: the +// canonical community segment depends on it whenever a ref carries an origin. +vi.mock('$env/dynamic/public', () => ({ + env: { PUBLIC_INSTANCE_URL: 'https://coves.social' }, +})) + +// `buildFreshPostView` reads the signed-in viewer off `profile.current`, whose +// real module touches localStorage at import time. +vi.mock('$lib/app/state/auth.svelte', () => ({ + profile: { + current: { + type: 'authenticated', + did: 'did:plc:author', + handle: 'mari.local.coves.dev', + avatar: undefined, + }, + }, +})) + +import type { AtUri, CID, CreatePostOutput } from '$lib/api/coves/types' +import type { DID, Handle } from '$lib/types/atproto' import { XrpcError } from '$lib/api/coves/xrpc' import CommunityCard from '$lib/feature/community/CommunityCard.svelte' +import { buildFreshPostView, stashFreshPost } from '$lib/feature/post/fresh-post' +import { createdPostLink } from '$lib/feature/post/owner' import { load } from './+page' const OWNER_DID = 'did:plc:author' @@ -88,7 +111,9 @@ function makeArgs(overrides?: { query?: string }): Parameters[0] { const handle = overrides?.handle ?? COMMUNITY_HANDLE - const owner = overrides?.owner ?? OWNER_DID + // The canonical owner form for the default fixture: its author carries a + // handle and owns the record, so a DID here would now be a redirecting alias. + const owner = overrides?.owner ?? OWNER_HANDLE const rkey = overrides?.rkey ?? RKEY const query = overrides?.query ?? '' return { @@ -118,6 +143,39 @@ function hydratedPost(uri: string) { } } +/** + * The same fixture with no author ref — nothing proves a handle for the repo + * the record lives in, so the URI authority DID is its canonical owner segment. + */ +function authorlessPost(uri: string): Record { + const { author: _author, ...rest } = hydratedPost(uri) + return rest +} + +/** The one URL this fixture's post is canonically addressed by. */ +const CANONICAL_PATH = `/c/${COMMUNITY_HANDLE}/post/${OWNER_HANDLE}/${RKEY}` + +const PERMALINK_PATTERN = /^\/c\/([^/]+)\/post\/([^/]+)\/([^/]+)$/ + +/** + * Turns a redirect `location` back into loader args, standing in for the + * router. Used to prove a redirect target does not itself redirect. + */ +function argsFromLocation(location: string): Parameters[0] { + const [path, query] = location.split('?') + const match = PERMALINK_PATTERN.exec(path) + if (!match) { + throw new Error(`redirect location is not a post permalink: ${location}`) + } + const [, handle, owner, rkey] = match + return makeArgs({ + handle: decodeURIComponent(handle), + owner: decodeURIComponent(owner), + rkey: decodeURIComponent(rkey), + query: query ? `?${query}` : '', + }) +} + /** * Answers `getPosts` from a URI→entry table, preserving request order and * filling every unknown URI with the notFound sentinel — the batch endpoint's @@ -180,14 +238,19 @@ describe('post loader', () => { // ------------------------------------------------------------------------- it('probes both collections for a DID owner, with no profile lookup', async () => { - const result = await load(makeArgs()) + // The post carries no author ref, so the DID owner segment is canonical + // here and the load runs to completion instead of redirecting. + const post = authorlessPost(POSTV2_URI) + serve({ [POSTV2_URI]: post }) + + const result = await load(makeArgs({ owner: OWNER_DID })) expect(mockCovesMethods.getProfile).not.toHaveBeenCalled() expect(mockCovesMethods.getCommunity).not.toHaveBeenCalled() expect(probes()).toEqual([[POSTV2_URI, LEGACY_URI]]) const value = loadedValue(result) - expect(value.post).toEqual(hydratedPost(POSTV2_URI)) + expect(value.post).toEqual(post) expect(value.unavailable).toBeUndefined() expect(commentedOn()).toBe(POSTV2_URI) @@ -389,4 +452,203 @@ describe('post loader', () => { vi.useRealTimers() } }) + + // ------------------------------------------------------------------------- + // Canonical URL enforcement + // + // Owner and rkey are checked against the record itself; the community + // segment was not, so one post had unlimited working aliases. Once the post + // hydrates, any non-canonical segment redirects to the single URL the link + // builders emit, carrying the query string across untouched. + // ------------------------------------------------------------------------- + + it('redirects a wrong community slug to the canonical permalink', async () => { + const location = `${CANONICAL_PATH}?sort=top` + + await expect( + load(makeArgs({ handle: 'cooking.local.coves.dev', query: '?sort=top' })), + ).rejects.toMatchObject({ status: 302, location }) + + // The target is stable: the second hop renders instead of bouncing on. + const result = await load(argsFromLocation(location)) + expect(loadedValue(result).post).toEqual(hydratedPost(POSTV2_URI)) + }) + + it('preserves a ?uri= param across the canonical redirect', async () => { + // The create flow's one-shot hand-off rides in the query string; dropping + // it on the redirect would cost the fresh post its retry budget. + serve({ [LEGACY_URI]: hydratedPost(LEGACY_URI) }) + const query = `?uri=${encodeURIComponent(LEGACY_URI)}` + const location = `${CANONICAL_PATH}${query}` + + await expect( + load(makeArgs({ handle: 'cooking.local.coves.dev', query })), + ).rejects.toMatchObject({ status: 302, location }) + + const result = await load(argsFromLocation(location)) + expect(loadedValue(result).post).toEqual(hydratedPost(LEGACY_URI)) + }) + + it('redirects a DID-form community slug to the handle form', async () => { + // The DID resolves and the matcher accepts it, so it is a working alias + // rather than an error — which is exactly why it needs canonicalising. + await expect( + load(makeArgs({ handle: COMMUNITY_DID })), + ).rejects.toMatchObject({ status: 302, location: CANONICAL_PATH }) + + const result = await load(argsFromLocation(CANONICAL_PATH)) + expect(loadedValue(result).post).toEqual(hydratedPost(POSTV2_URI)) + }) + + it('redirects a DID-form owner segment to the author handle form', async () => { + await expect(load(makeArgs({ owner: OWNER_DID }))).rejects.toMatchObject({ + status: 302, + location: CANONICAL_PATH, + }) + + const result = await load(argsFromLocation(CANONICAL_PATH)) + expect(loadedValue(result).post).toEqual(hydratedPost(POSTV2_URI)) + }) + + it('does not redirect a URL that is already canonical', async () => { + const result = await load(makeArgs({ query: '?sort=top' })) + + const value = loadedValue(result) + expect(value.post).toEqual(hydratedPost(POSTV2_URI)) + expect(value.unavailable).toBeUndefined() + }) + + it('renders a just-created post on the URL the create flow redirects to', async () => { + // The create flow writes the record, stashes an optimistic view, and + // navigates to `createdPostLink`. That link is built from the community + // the form was submitted against — which carries `origin`, so its + // canonical segment is the bare name. If the stashed view canonicalises + // to anything else the very first load redirects, and the one-shot stash + // is spent on a URL nobody renders. + const community = { + did: COMMUNITY_DID as DID, + handle: 'c-gardening.coves.social' as Handle, + name: 'gardening', + origin: 'coves.social', + } + const output: CreatePostOutput = { + uri: POSTV2_URI as AtUri, + cid: 'bafyreigh2akiscaildc' as CID, + } + + const view = buildFreshPostView({ output, community }) + if (!view) throw new Error('buildFreshPostView returned no view') + stashFreshPost(view) + + const link = createdPostLink({ ...output, community, post: view }) + const [path, query] = link.split('?') + expect(path).toBe(`/c/gardening/post/${OWNER_HANDLE}/${RKEY}`) + + const result = await load( + makeArgs({ handle: 'gardening', query: `?${query}` }), + ) + + expect(loadedValue(result).post).toEqual(view) + // The stash answered the load outright; nothing was fetched. + expect(mockCovesMethods.getPosts).not.toHaveBeenCalled() + }) + + it('does not leak an unhandled rejection when it redirects away from an alias', async () => { + // The feed init fires the comments request and hands the promise back for + // the page to stream. A redirect throws past that return value, so nobody + // is left to observe a failure — and in Node an unobserved rejection is a + // process-level warning (a crash under --unhandled-rejections=strict). + mockCovesMethods.getComments.mockRejectedValue(new Error('comments down')) + + const unhandled: unknown[] = [] + const onUnhandled = (reason: unknown): void => { + unhandled.push(reason) + } + process.on('unhandledRejection', onUnhandled) + + try { + await expect( + load( + makeArgs({ handle: 'cooking.local.coves.dev', query: '?sort=top' }), + ), + ).rejects.toMatchObject({ + status: 302, + location: `${CANONICAL_PATH}?sort=top`, + }) + + // Node reports an unobserved rejection only once the microtask queue + // has drained, so give it a turn of the event loop before asserting. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(unhandled).toEqual([]) + } finally { + process.off('unhandledRejection', onUnhandled) + } + }) + + it('still hands the page a rejecting comments promise on a canonical URL', async () => { + // The guard above must not become a blanket swallow: when the page does + // render, a failed comments fetch has to reach its {#await} as an error + // rather than an empty thread. + mockCovesMethods.getComments.mockRejectedValue(new Error('comments down')) + + const result = await load(makeArgs()) + + await expect(loadedValue(result).comments).rejects.toThrow('comments down') + }) + + it('redirects a remote community to its name@origin form, @ left literal', async () => { + // A bridged Lemmy community. `@` is a legal path character, and encoding + // it would make the canonical URL unreadable — and would not survive the + // round trip back through the router as the same param. + const remotePost = { + ...hydratedPost(POSTV2_URI), + community: { + did: COMMUNITY_DID, + handle: 'gaming.lemmy-world.tdpl.io', + name: 'gaming', + origin: 'lemmy.world', + }, + } + serve({ [POSTV2_URI]: remotePost }) + const location = `/c/gaming@lemmy.world/post/${OWNER_HANDLE}/${RKEY}` + + await expect( + load(makeArgs({ handle: 'gaming.lemmy-world.tdpl.io' })), + ).rejects.toMatchObject({ status: 302, location }) + + // Replayed through the router, the `@` decodes back to itself, so the + // target is canonical and does not bounce. + const result = await load(argsFromLocation(location)) + expect(loadedValue(result).post).toEqual(remotePost) + }) + + it('does not redirect a community-owned legacy post addressed by the community DID', async () => { + // The record lives in the community's repo, so the author's handle would + // address a record that does not exist. The authority DID is canonical + // here, and nothing needs resolving to know it. + const communityOwnedUri = `at://${COMMUNITY_DID}/${LEGACY_POST_COLLECTION}/${RKEY}` + const legacy = hydratedPost(communityOwnedUri) + serve({ [communityOwnedUri]: legacy }) + + const result = await load(makeArgs({ owner: COMMUNITY_DID })) + + const value = loadedValue(result) + expect(value.post).toEqual(legacy) + expect(value.unavailable).toBeUndefined() + expect(mockCovesMethods.getProfile).not.toHaveBeenCalled() + }) + + it('does not redirect an unavailable post, whatever the slug', async () => { + // There is no hydrated community ref to canonicalise against, so the + // "post removed" state must render rather than bounce. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + serve({}) + + const result = await load(makeArgs({ handle: 'cooking.local.coves.dev' })) + + expect(loadedValue(result).unavailable).toBe('notFound') + + warn.mockRestore() + }) })