From 440d91c83b11f134142d3ba12b0ece811e2379cd Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 29 Jun 2026 09:23:51 +0200 Subject: [PATCH] fix: make episode identity key collision-resistant and prototype-safe getEpisodeKey built an episode's identity as `${podcastName}::${title}` with no escaping, so distinct (podcastName, title) pairs collided: ("A", "B::C") and ("A::B", "C") both mapped to "A::B::C", and a boundary colon (("A:", "B") vs ("A", ":B")) collided too. Both fields come verbatim from RSS, so a malicious feed could alias another subscribed feed's played-state / dedup key. Keep the plain `name::title` format for ordinary pairs (byte-identical, so existing data.json keys resolve with no migration) and escape only the rare delimiter-forging pair into a NUL-prefixed, colon-free, injective form that is disjoint from both the plain composite keys and the legacy title-only keys. The manual escape never throws (unlike encodeURIComponent on lone surrogates), so a hostile value cannot turn key generation into a crash. findPlayedEpisodesInFeeds grouped played episodes onto a plain object keyed by the feed-controlled podcastName, so names like "__proto__" / "constructor" resolved to inherited prototype members and crashed `.push`, rejecting the returned promise. Group with a Map so any feed-controlled name is a safe key. deepsec: other-key-collision, other-unsafe-object-key --- src/utility/episodeKey.test.ts | 92 +++++++++++++++++++++++- src/utility/episodeKey.ts | 64 ++++++++++++++++- src/utility/findPlayedEpisodes.test.ts | 96 ++++++++++++++++++++++++++ src/utility/findPlayedEpisodes.ts | 23 +++--- 4 files changed, 265 insertions(+), 10 deletions(-) create mode 100644 src/utility/findPlayedEpisodes.test.ts diff --git a/src/utility/episodeKey.test.ts b/src/utility/episodeKey.test.ts index 783781bf..0fdb7f90 100644 --- a/src/utility/episodeKey.test.ts +++ b/src/utility/episodeKey.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import type { Episode } from "src/types/Episode"; -import { isSameStoredEpisode } from "./episodeKey"; +import { getEpisodeKey, isSameStoredEpisode } from "./episodeKey"; function ep(title: string, podcastName?: string): Episode { return { @@ -14,6 +14,91 @@ function ep(title: string, podcastName?: string): Episode { } as unknown as Episode; } +describe("getEpisodeKey", () => { + it("keeps the plain podcastName::title format for ordinary episodes (backward compatible)", () => { + expect(getEpisodeKey(ep("Episode 1", "My Podcast"))).toBe( + "My Podcast::Episode 1", + ); + }); + + it("keeps the plain format when names/titles carry single colons", () => { + // Single colons cannot forge the `::` delimiter, so these stay verbatim and + // need no migration. + expect(getEpisodeKey(ep("Ch 5: The Return", "Serial: A Show"))).toBe( + "Serial: A Show::Ch 5: The Return", + ); + }); + + it("falls back to the title for legacy episodes without a podcastName", () => { + expect(getEpisodeKey(ep("Legacy Title"))).toBe("Legacy Title"); + }); + + it("returns an empty string when episode or title is missing", () => { + expect(getEpisodeKey(undefined)).toBe(""); + expect(getEpisodeKey(null)).toBe(""); + expect(getEpisodeKey(ep(""))).toBe(""); + }); + + it("does not collide when the delimiter appears inside a component (#other-key-collision)", () => { + // Both pairs used to map to "A::B::C". + const a = getEpisodeKey(ep("B::C", "A")); // podcastName "A", title "B::C" + const b = getEpisodeKey(ep("C", "A::B")); // podcastName "A::B", title "C" + expect(a).not.toBe(b); + }); + + it("does not collide when a name ends with / a title starts with the delimiter colon", () => { + // Both pairs used to map to "A:::B". + const a = getEpisodeKey(ep("B", "A:")); // ("A:", "B") + const b = getEpisodeKey(ep(":B", "A")); // ("A", ":B") + expect(a).not.toBe(b); + }); + + it("keeps escaped keys disjoint from plain keys (no `::`)", () => { + const encoded = getEpisodeKey(ep("B::C", "A")); + expect(encoded.includes("::")).toBe(false); + expect(encoded).not.toBe(getEpisodeKey(ep("Plain", "Podcast"))); + }); + + it("keeps escaped keys disjoint from legacy title-only keys", () => { + // A feed with an empty produces podcastName="" -> a raw-title key. + // The escaped form minus its (NUL) prefix is an ordinary title a feed could + // carry, so a legacy episode with that title must NOT alias another feed's + // escaped composite key. + const escaped = getEpisodeKey(ep("a", ":")); // podcastName=":", title="a" + expect(escaped.charCodeAt(0)).toBe(0); // the NUL prefix is present + const realisticTitle = escaped.slice(1); // an ordinary, NUL-free title + expect(getEpisodeKey(ep(realisticTitle))).not.toBe(escaped); + }); + + it("maps a batch of delimiter-adjacent pairs to distinct keys (injective)", () => { + const pairs: Array<[name: string, title: string]> = [ + ["A", "B::C"], + ["A::B", "C"], + ["A:", "B"], + ["A", ":B"], + ["A::B::C", "D"], + ["A", "B"], + ["A:", ":B"], + [":A", "B:"], + ]; + const keys = pairs.map(([name, title]) => getEpisodeKey(ep(title, name))); + expect(new Set(keys).size).toBe(pairs.length); + }); + + it("treats __proto__/constructor names as ordinary string keys", () => { + expect(getEpisodeKey(ep("Ep", "__proto__"))).toBe("__proto__::Ep"); + expect(typeof getEpisodeKey(ep("Ep", "constructor"))).toBe("string"); + }); + + it("never throws on a delimiter-forging value containing a lone surrogate", () => { + // encodeURIComponent would throw URIError here; the manual escape must not, + // so a hostile feed value cannot crash key generation. + expect(() => getEpisodeKey(ep("::\uD800", "Pod"))).not.toThrow(); + const key = getEpisodeKey(ep("::\uD800", "Pod")); + expect(key.includes("::")).toBe(false); + }); +}); + describe("isSameStoredEpisode (#214)", () => { it("matches the same podcast + title (composite key)", () => { expect(isSameStoredEpisode(ep("E1", "Pod"), ep("E1", "Pod"))).toBe(true); @@ -34,6 +119,11 @@ describe("isSameStoredEpisode (#214)", () => { expect(isSameStoredEpisode(ep("E1"), ep("E2", "Pod"))).toBe(false); }); + it("does not conflate two feeds that forge the `::` delimiter (#other-key-collision)", () => { + // ("A", "B::C") and ("A::B", "C") used to share the key "A::B::C". + expect(isSameStoredEpisode(ep("B::C", "A"), ep("C", "A::B"))).toBe(false); + }); + it("returns false for null/undefined inputs", () => { expect(isSameStoredEpisode(undefined, ep("E1", "Pod"))).toBe(false); expect(isSameStoredEpisode(ep("E1", "Pod"), null)).toBe(false); diff --git a/src/utility/episodeKey.ts b/src/utility/episodeKey.ts index 6ee44f3f..eb7d30cb 100644 --- a/src/utility/episodeKey.ts +++ b/src/utility/episodeKey.ts @@ -1,10 +1,62 @@ import type { Episode } from "src/types/Episode"; +const KEY_DELIMITER = "::"; + +/** + * Joining a (podcastName, title) pair with a raw `::` is only unambiguous when + * neither component can forge or straddle the delimiter. The mapping breaks + * (two distinct pairs produce one key) when a component contains `::`, or the + * name ends with `:` / the title starts with `:` - those merge with the + * delimiter into `:::`, which can be split two ways. Concretely + * ("A", "B::C") and ("A::B", "C") both yield "A::B::C", and ("A:", "B") and + * ("A", ":B") both yield "A:::B". Both components come verbatim from a + * (potentially malicious) RSS feed, so such a pair can be crafted to collide + * with another subscribed feed's episode. Pairs flagged here are encoded + * instead so the key stays injective. + */ +function compositeKeyIsAmbiguous(podcastName: string, title: string): boolean { + return ( + podcastName.includes(KEY_DELIMITER) || + title.includes(KEY_DELIMITER) || + podcastName.endsWith(":") || + title.startsWith(":") + ); +} + +/** + * A NUL byte cannot appear in XML feed text (nor in the parsed PocketCasts + * titles), so the escaped form below is prefixed with it. That keeps the escaped + * key disjoint from the legacy title-only keys, which are raw, arbitrary titles + * (a feed with an empty `<title>` yields `podcastName === ""` and so a raw-title + * key - see feedParser/pocketCastsParser). Without the prefix a crafted legacy + * title could equal an escaped composite key and re-introduce a cross-feed + * collision. + */ +const ESCAPED_KEY_PREFIX = "\u0000"; + +/** + * Escapes a key component so it contains no `:` at all: the escape char is + * doubled (`\` -> `\\`) and every colon becomes `\c`. The result is therefore + * colon-free and the mapping is injective. Unlike `encodeURIComponent` this + * never throws (e.g. on lone surrogates), so a hostile feed value cannot turn + * key generation into a crash. + */ +function escapeKeyComponent(value: string): string { + return value.replace(/\\/g, "\\\\").replace(/:/g, "\\c"); +} + /** * Generates a unique key for an episode. * Uses podcastName + title to avoid collisions between episodes with the same title * from different podcasts. * + * The common case keeps the plain `podcastName::title` format so existing + * `data.json` keys (and the dedup heuristics that look for `::`) stay valid with + * no migration. Only the rare delimiter-forging pair (see + * {@link compositeKeyIsAmbiguous}) is escaped, which makes the key collision- + * resistant: a feed cannot choose a name/title that maps onto another feed's + * episode key. + * * Falls back to title-only for backwards compatibility with episodes that don't have podcastName. */ export function getEpisodeKey(episode: Episode | null | undefined): string { @@ -12,7 +64,17 @@ export function getEpisodeKey(episode: Episode | null | undefined): string { return ""; } if (episode.podcastName) { - return `${episode.podcastName}::${episode.title}`; + const { podcastName, title } = episode; + if (compositeKeyIsAmbiguous(podcastName, title)) { + // Collision-resistant form for the rare delimiter-forging pair. Each + // escaped component is colon-free, so joining with a single `:` yields a + // key with exactly one colon and never the `::` delimiter, keeping it + // disjoint from the plain composite keys below; the NUL prefix keeps it + // disjoint from the legacy title-only keys. The encoding is injective, so + // no two distinct pairs can share a key. + return `${ESCAPED_KEY_PREFIX}${escapeKeyComponent(podcastName)}:${escapeKeyComponent(title)}`; + } + return `${podcastName}${KEY_DELIMITER}${title}`; } // Fallback for legacy episodes without podcastName return episode.title; diff --git a/src/utility/findPlayedEpisodes.test.ts b/src/utility/findPlayedEpisodes.test.ts new file mode 100644 index 00000000..d361e647 --- /dev/null +++ b/src/utility/findPlayedEpisodes.test.ts @@ -0,0 +1,96 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import type { Episode } from "src/types/Episode"; +import type { PlayedEpisode } from "src/types/PlayedEpisode"; +import type { PodcastFeed } from "src/types/PodcastFeed"; + +// Feed parsing hits the network, so stub it with a per-url lookup the tests fill in. +const { feedEpisodes } = vi.hoisted(() => ({ + feedEpisodes: new Map<string, Episode[]>(), +})); + +vi.mock("src/parser/feedParser", () => ({ + default: class { + private feed: PodcastFeed | undefined; + constructor(feed?: PodcastFeed) { + this.feed = feed; + } + async getEpisodes(url: string): Promise<Episode[]> { + return feedEpisodes.get(url) ?? []; + } + }, +})); + +import findPlayedEpisodesInFeeds from "./findPlayedEpisodes"; + +function playedEp(title: string, podcastName: string): PlayedEpisode { + return { title, podcastName, time: 0, duration: 0, finished: true }; +} + +function episode(title: string, podcastName: string): Episode { + return { + title, + podcastName, + streamUrl: "", + url: "", + description: "", + content: "", + }; +} + +function feed(title: string, url: string): PodcastFeed { + return { title, url, artworkUrl: "" }; +} + +describe("findPlayedEpisodesInFeeds", () => { + beforeEach(() => { + feedEpisodes.clear(); + }); + + it("does not crash when a feed-controlled podcast name shadows Object.prototype (#other-unsafe-object-key)", async () => { + // "__proto__"/"constructor"/"hasOwnProperty" resolve to inherited members on + // a plain object accumulator and would throw on `.push`; a Map keys safely. + const played = [ + playedEp("Ep A", "__proto__"), + playedEp("Ep B", "constructor"), + playedEp("Ep C", "hasOwnProperty"), + ]; + + await expect(findPlayedEpisodesInFeeds(played, [])).resolves.toEqual([]); + }); + + it("returns the feed episodes whose title matches a played episode, including for a __proto__ feed", async () => { + feedEpisodes.set("https://a.example/feed", [ + episode("Ep A", "Pod A"), + episode("Ep Z", "Pod A"), + ]); + feedEpisodes.set("https://proto.example/feed", [ + episode("Ep P", "__proto__"), + ]); + + const played = [ + playedEp("Ep A", "Pod A"), + playedEp("Missing", "Pod A"), + playedEp("Ep P", "__proto__"), + ]; + const feeds = [ + feed("Pod A", "https://a.example/feed"), + feed("__proto__", "https://proto.example/feed"), + ]; + + const result = await findPlayedEpisodesInFeeds(played, feeds); + + expect(result.map((e) => e.title)).toEqual(["Ep A", "Ep P"]); + }); + + it("skips played episodes whose podcast is not among the feeds", async () => { + feedEpisodes.set("https://a.example/feed", [episode("Ep A", "Pod A")]); + + const result = await findPlayedEpisodesInFeeds( + [playedEp("Ep A", "Pod A"), playedEp("Ep B", "Unsubscribed")], + [feed("Pod A", "https://a.example/feed")], + ); + + expect(result.map((e) => e.title)).toEqual(["Ep A"]); + }); +}); diff --git a/src/utility/findPlayedEpisodes.ts b/src/utility/findPlayedEpisodes.ts index 7c24fe51..fe3d4ab7 100644 --- a/src/utility/findPlayedEpisodes.ts +++ b/src/utility/findPlayedEpisodes.ts @@ -7,17 +7,24 @@ export default async function findPlayedEpisodesInFeeds( playedEpisodes: PlayedEpisode[], feeds: PodcastFeed[], ): Promise<Episode[]> { - const episodesByPodcast = playedEpisodes.reduce((acc: { [podcastName: string]: PlayedEpisode[] }, episode) => { - const podcastName = episode.podcastName; - const episodes = acc[podcastName] || []; - episodes.push(episode); - acc[podcastName] = episodes; - return acc; - }, {}); + // Group by podcast name with a Map, not a plain object. The name comes + // verbatim from a feed's `<title>`, so a crafted value like "__proto__" or + // "constructor" would resolve `acc[name]` to an inherited prototype member + // (truthy, no `.push`) and throw, rejecting this Promise. A Map keys on the + // string itself, so any name is handled safely. + const episodesByPodcast = new Map<string, PlayedEpisode[]>(); + for (const episode of playedEpisodes) { + const episodes = episodesByPodcast.get(episode.podcastName); + if (episodes) { + episodes.push(episode); + } else { + episodesByPodcast.set(episode.podcastName, [episode]); + } + } const playedEpisodesInFeeds: Episode[] = []; - for (const [podcastName, episodes] of Object.entries(episodesByPodcast)) { + for (const [podcastName, episodes] of episodesByPodcast) { const feed = feeds.find(feed => feed.title === podcastName); if (!feed) continue;