fix: make episode identity key collision-resistant and prototype-safe (#226)

chhoumann · web-flow · commit a5683db15ab0 · 2026-06-29T12:01:39.000+02:00
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
diff --git 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 <title> 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
@@ -1,18 +1,80 @@
 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 {
 	if (!episode || !episode.title) {
 		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
@@ -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
@@ -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;
 
