From cb968e7ce3f2ace29aaccd20920354f8fd2fdd2f Mon Sep 17 00:00:00 2001 From: Andrew Shell Date: Mon, 3 Aug 2026 18:08:08 -0500 Subject: [PATCH 1/2] fix(opml): stop dynamic OPML categories accumulating redirected feeds FreshRSS and OPML disagree about what identifies a feed. FreshRSS_Feed::load() treats a feed as the document it resolves to and rewrites the stored URL when the feed answers HTTP 301; FreshRSS_Category::refreshDynamicOpml() treats it as the exact xmlUrl in the list, which does not change. One 301 is enough to make them disagree forever. Every later refresh then reads the entry as new and inserts it again, and mutes the drifted copy for having disappeared from the list. Nothing catches the collision: `_feed`.url has no unique index and FeedDAO::updateFeed() does not check for one. Copies accumulate at one per refresh -- unbounded here, since rssCloud refreshes on notification rather than on a timer. Settle it at the import step, before core does its matching. A feed whose URL is not already subscribed is resolved to wherever it permanently moved; if that is a feed we hold, the import is addressed to it instead. Core then recognises it as existing, so it is neither inserted nor muted, and addFeedObject() unmutes it if an earlier refresh muted it. Resolution is a HEAD, so a large first import costs one cheap request per entry rather than downloading every feed twice, and answers are cached on disk. Hops are walked by hand with CURLOPT_FOLLOWLOCATION off so that every one is re-checked against the IP allowlist, which keeps a redirect from reaching the private network. Only 301 and 308 are followed: a temporary redirect says the resource has not moved, so following it would merge two feeds the publisher considers distinct. FeedBeforeInsert fires on every import path, so this also covers refreshes driven by cron or the CLI, and keeps a manual subscription from duplicating a feed already held under its post-redirect URL. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 37 +++++++- RssCloud/Redirects.php | 204 +++++++++++++++++++++++++++++++++++++++++ extension.php | 72 ++++++++++++++- 3 files changed, 309 insertions(+), 4 deletions(-) create mode 100644 RssCloud/Redirects.php diff --git a/README.md b/README.md index c9f9b85..1b593ad 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,7 @@ Deliberately mirrors core's WebSub layout under `PSHB_PATH`: ```text data/rssCloud/resources//!cloud.json subscription state data/rssCloud/resources//.txt one marker per interested user +data/rssCloud/redirects/.json where a URL permanently moved to ``` Subscriptions are instance-wide (the cloud server knows one callback), but resources are per-user, @@ -81,6 +82,10 @@ Feeds additionally carry an `rssCloud` attribute holding the resource URL they a is what makes the polling decision possible *before* a feed is fetched — `FreshRSS_Feed::selfUrl()` is only populated during the SimplePie parse and is not persisted. +Redirect resolutions are cached one file per URL, so that concurrent notifications cannot lose each +other's writes. An answer is trusted for 30 days; a resolution that *failed* — as opposed to one that +found no move — is retried after 6 hours. + Logs go to `data/users/_/log_rsscloud.txt`. ## Hooks used @@ -88,14 +93,44 @@ Logs go to `data/users/_/log_rsscloud.txt`. | Hook | Purpose | | --- | --- | | `ApiMisc` | serve the callback | -| `SimplepieAfterInit` | discover a feed's cloud, subscribe, persist the resource attribute | +| `SimplepieAfterInit` | discover a feed's cloud, subscribe, persist the resource attribute; pin dynamic OPML feed URLs | | `FeedsListBeforeActualize` | renew feed subscriptions (capped per cycle) | | `FeedBeforeActualize` | skip polling a feed with a healthy subscription | | `FreshrssUserMaintenance` | discover and renew dynamic OPML subscriptions | +| `FeedBeforeInsert` | reconcile a redirected feed against one already subscribed | Renewal has to happen in `FeedsListBeforeActualize` rather than at discovery time, because a feed whose polling is skipped never reaches `SimplepieAfterInit` and would otherwise never renew. +### Redirected feeds in dynamic OPML categories + +FreshRSS and OPML disagree about what identifies a feed: + +* `FreshRSS_Feed::load()` treats a feed as the document it resolves to, and rewrites the stored URL + when the feed answers HTTP 301. +* `FreshRSS_Category::refreshDynamicOpml()` treats a feed as the exact `xmlUrl` string in the list, + which does not change. + +One 301 is enough to make them disagree forever. Every later refresh then reads the entry as new and +inserts it again, and mutes the drifted copy for having disappeared from the list. Nothing catches +the collision: `_feed.url` has no unique index, and `FeedDAO::updateFeed()` does not check for one. +Copies accumulate at one per refresh — unbounded here, because rssCloud refreshes on notification +rather than on a timer. + +`FeedBeforeInsert` settles it at the import step, before core does its matching. A feed whose URL is +not already subscribed is resolved to wherever it permanently moved; if *that* is a feed we hold, the +import is addressed to it instead. Core then recognises it as existing, so it is neither inserted nor +muted, and `FeedDAO::addFeedObject()` unmutes it if an earlier refresh muted it. + +Only 301 and 308 are followed. A temporary redirect deliberately says the resource has *not* moved, +so following it would merge two feeds the publisher considers distinct. A feed that moved onto +nothing we hold is left exactly as the list gives it, and core canonicalises it on first fetch as +usual. + +The hook fires on every import path, so this covers refreshes driven by cron or the CLI as well as by +a notification, and keeps a manual subscription from duplicating a feed already held under its +post-redirect URL. + ## Known gaps These are deliberate scaffold-level limitations, not oversights: diff --git a/RssCloud/Redirects.php b/RssCloud/Redirects.php new file mode 100644 index 0000000..812ee82 --- /dev/null +++ b/RssCloud/Redirects.php @@ -0,0 +1,204 @@ +.json {"url":…,"target":…,"failed":…,"time":…} + * ``` + * + * One file per URL rather than one shared map, so that concurrent notifications cannot lose each + * other's writes. `target` is null for a URL that resolves to itself, which is cached too: the + * common case is a URL that has not moved, and it should not be probed again on every refresh. + * `failed` separates "it did not move" from "we could not tell", so only the latter is retried soon. + */ +final class RssCloud_Redirects { + + /** How long a completed resolution is trusted. Permanent redirects rarely stop being permanent. */ + public const TTL_SECONDS = 30 * 86400; + + /** How long to wait before probing a URL whose resolution failed, e.g. because the host was down. */ + public const TTL_FAILED_SECONDS = 6 * 3600; + + /** Redirect hops to follow before giving up. Matches the default cURL limit core applies. */ + public const MAX_HOPS = 4; + + public function __construct( + private readonly string $basePath, + ) { + } + + /** + * The URL that `$url` permanently moved to, or `$url` itself if it did not move or could not be + * checked. Never throws: a failure to resolve has to leave the caller with the status quo. + * + * @param array $attributes the feed attributes, for `curl_params` and `timeout` + */ + public function resolve(string $url, array $attributes = []): string { + $cached = $this->load($url); + if ($cached !== null) { + return $cached; + } + + $target = $this->follow($url, $attributes); + $this->store($url, $target); + return $target ?? $url; + } + + /** The cached target for `$url`, or null if there is no fresh entry. */ + private function load(string $url): ?string { + $json = @file_get_contents($this->filename($url)); + if (!is_string($json) || $json === '') { + return null; + } + $entry = json_decode($json, true); + if (!is_array($entry)) { + return null; + } + $time = is_numeric($entry['time'] ?? null) ? (int)$entry['time'] : 0; + $target = is_string($entry['target'] ?? null) ? $entry['target'] : null; + // "It did not move" is an answer and is trusted for as long as a move is; "we could not tell" + // is not, and is retried sooner. Both store a missing target, so they are told apart by flag. + $ttl = ($entry['failed'] ?? false) === true ? self::TTL_FAILED_SECONDS : self::TTL_SECONDS; + if ($time < time() - $ttl) { + return null; + } + return $target ?? $url; + } + + private function store(string $url, ?string $target): void { + $directory = $this->basePath . '/redirects'; + if (!@is_dir($directory) && !@mkdir($directory, 0770, true)) { + Minz_Log::error('rssCloud: cannot create ' . $directory, RSSCLOUD_LOG); + return; + } + $entry = [ + 'url' => $url, + 'target' => $target === null || $target === $url ? null : $target, + 'failed' => $target === null, + 'time' => time(), + ]; + @file_put_contents($this->filename($url), json_encode($entry)); + } + + private function filename(string $url): string { + return $this->basePath . '/redirects/' . sha1($url) . '.json'; + } + + /** + * Walk the chain of permanent redirects, or null if any hop could not be checked. + * + * Only 301 and 308 are followed. A temporary redirect deliberately says the resource has *not* + * moved, so following it would merge two feeds that the publisher considers distinct — and it is + * also what core declines to follow when it rewrites a feed URL, via SimplePie's permanent URL. + * + * @param array $attributes + */ + private function follow(string $url, array $attributes): ?string { + $current = $url; + $seen = [$current => true]; + + for ($hop = 0; $hop < self::MAX_HOPS; $hop++) { + $location = self::permanentLocation($current, $attributes); + if ($location === false) { + return null; + } + if ($location === null) { + return $current; + } + $absolute = \SimplePie\Misc::absolutize_url($location, $current); + $next = is_string($absolute) ? (FreshRSS_http_Util::checkUrl($absolute, fixScheme: false) ?: '') : ''; + if ($next === '' || isset($seen[$next])) { + // Unusable or looping: stop where we are rather than report a move we cannot trust. + return $current; + } + $seen[$next] = true; + $current = $next; + } + + Minz_Log::warning('rssCloud: too many permanent redirects from ' . $url, RSSCLOUD_LOG); + return null; + } + + /** + * Issue one HEAD and report the `Location` of a permanent redirect. + * + * @param array $attributes + * @return string|false|null the location; null if this is not a permanent redirect; false if the + * request could not be made or failed, which is not the same answer and must not be cached as one + */ + private static function permanentLocation(string $url, array $attributes): string|false|null { + if ($url === '') { + return false; + } + + // Re-checked at every hop, so a redirect cannot walk into the private network. + $resolve = FreshRSS_http_Util::getCurlResolveInfo($url); + if (!is_array($resolve)) { + // null: the host's IP is not in the allowlist. false: the host did not resolve. + return false; + } + + $ch = curl_init(); + if ($ch === false) { + return false; + } + + $limits = FreshRSS_Context::systemConf()->limits; + $timeout = is_numeric($attributes['timeout'] ?? null) && (int)$attributes['timeout'] > 0 ? + (int)$attributes['timeout'] : (int)($limits['timeout'] ?? 10); + + curl_setopt_array($ch, [ + CURLOPT_URL => $url, + // Only the status line and Location are wanted, so never ask for a body. + CURLOPT_NOBODY => true, + // Hops are walked by hand above, so that each one is re-checked against the allowlist. + CURLOPT_FOLLOWLOCATION => false, + CURLOPT_RETURNTRANSFER => true, + CURLOPT_USERAGENT => FRESHRSS_USERAGENT, + CURLOPT_CONNECTTIMEOUT => $timeout, + CURLOPT_TIMEOUT => $timeout, + ]); + if ($resolve !== []) { + curl_setopt($ch, CURLOPT_RESOLVE, $resolve); // Prevent DNS rebinding + } + if (defined('CURLOPT_PROTOCOLS_STR') && is_int(CURLOPT_PROTOCOLS_STR)) { + curl_setopt($ch, CURLOPT_PROTOCOLS_STR, 'http,https'); + } elseif (defined('CURLOPT_PROTOCOLS') && defined('CURLPROTO_HTTP') && defined('CURLPROTO_HTTPS')) { + curl_setopt($ch, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS); + } + + // Instance-wide options carry the proxy configuration, so they must not be skipped. + curl_setopt_array($ch, FreshRSS_Context::systemConf()->curl_options); + if (is_array($attributes['curl_params'] ?? null)) { + curl_setopt_array($ch, FreshRSS_http_Util::sanitizeCurlParams($attributes['curl_params'])); + } + // Reassert what the options above are not allowed to undo. + curl_setopt($ch, CURLOPT_FOLLOWLOCATION, false); + + curl_exec($ch); + $status = (int)curl_getinfo($ch, CURLINFO_HTTP_CODE); + $location = curl_getinfo($ch, CURLINFO_REDIRECT_URL); + $error = curl_error($ch); + + if ($error !== '') { + Minz_Log::debug('rssCloud: cannot check ' . $url . ' for a permanent redirect: ' . $error, RSSCLOUD_LOG); + return false; + } + // A server that rejects HEAD tells us nothing about redirection, which is the status quo, not + // a failure — retrying it in six hours would not produce a different answer. + if (!in_array($status, [301, 308], true)) { + return null; + } + return is_string($location) && $location !== '' ? $location : null; + } +} diff --git a/extension.php b/extension.php index c7af565..43f7f4a 100644 --- a/extension.php +++ b/extension.php @@ -44,6 +44,7 @@ final class RssCloudExtension extends Minz_Extension { private const FEED_ATTRIBUTE = 'rssCloud'; private ?RssCloud_Registry $registry = null; + private ?RssCloud_Redirects $redirects = null; private ?RssCloud_Subscriber $subscriber = null; /** Whether {@see self::subscriber()} has run, so a failure is diagnosed and logged only once. */ @@ -80,6 +81,7 @@ public function init(): void { } if ($this->isEnabledForOpml()) { $this->registerHook(Minz_HookType::FreshrssUserMaintenance, [$this, 'onUserMaintenance']); + $this->registerHook(Minz_HookType::FeedBeforeInsert, [$this, 'onFeedBeforeInsert']); } } @@ -88,12 +90,15 @@ public function install() { // Note: install() runs *before* Minz_ExtensionManager enables the extension, so the // autoloader registered above is not active yet. Nothing here may touch an RssCloud_* class. $resources = RSSCLOUD_PATH . '/resources'; - if (!@is_dir($resources) && !@mkdir($resources, 0770, true)) { - return 'Cannot create ' . $resources; + $redirects = RSSCLOUD_PATH . '/redirects'; + foreach ([$resources, $redirects] as $directory) { + if (!@is_dir($directory) && !@mkdir($directory, 0770, true)) { + return 'Cannot create ' . $directory; + } } // `data/.htaccess` denies all, but add the same directory-listing guards core ships under // `data/PubSubHubbub/` for servers that do not read .htaccess. - foreach ([RSSCLOUD_PATH, $resources] as $directory) { + foreach ([RSSCLOUD_PATH, $resources, $redirects] as $directory) { if (!file_exists($directory . '/index.html')) { @file_put_contents($directory . '/index.html', ''); } @@ -176,6 +181,10 @@ private function registry(): RssCloud_Registry { return $this->registry ??= new RssCloud_Registry(RSSCLOUD_PATH); } + private function redirects(): RssCloud_Redirects { + return $this->redirects ??= new RssCloud_Redirects(RSSCLOUD_PATH); + } + private function subscriber(): ?RssCloud_Subscriber { if ($this->subscriberResolved) { return $this->subscriber; @@ -325,6 +334,63 @@ public function onUserMaintenance(): void { } } + /** + * Address a feed being imported by the URL it has permanently moved to, when that is a feed we + * already hold. Otherwise leave it exactly as the list gives it. + * + * This is what stops a dynamic OPML category accumulating duplicates. The two sides disagree + * about what identifies a feed: + * + * * `FreshRSS_Feed::load()` rewrites a feed's stored URL to wherever it moved on HTTP 301. + * * `FreshRSS_Category::refreshDynamicOpml()` matches what it holds against the OPML by exact URL. + * + * So one 301 is enough to make every later refresh read the entry as new and insert it again, + * then mute the drifted copy for having vanished from the list. Nothing catches the collision: + * `_feed.url` carries no unique index, and `FeedDAO::updateFeed()` does not check for one. The + * copies therefore accumulate at one per refresh — unbounded under rssCloud, where refreshes + * follow notifications rather than a timer. + * + * Resolving the redirect here settles the disagreement in core's favour, at the import step and + * before core does its matching: the feed is recognised as one we already hold, so it is neither + * inserted nor muted, and `FeedDAO::addFeedObject()` unmutes it if an earlier refresh muted it. + * + * The hook fires on every import path, so this also covers refreshes driven by cron or the CLI + * rather than by a notification, and keeps a manual subscription from duplicating a feed already + * held under its post-redirect URL. + * + * The feed is always returned: the hook can cancel an import by returning null, but a URL this + * extension failed to reconcile is not a reason to drop a subscription the list asked for. + */ + public function onFeedBeforeInsert(FreshRSS_Feed $feed): FreshRSS_Feed { + $url = $feed->url(); + $feedDAO = FreshRSS_Factory::createFeedDao(); + if ($url === '' || $feedDAO->searchByUrl($url) !== null) { + // Already held under this exact URL, so there is nothing to reconcile — and no reason to + // spend a request finding that out. + return $feed; + } + + $target = $this->redirects()->resolve($url, $feed->attributes()); + if ($target === $url) { + return $feed; + } + if ($feedDAO->searchByUrl($target) === null) { + // It moved, but not onto anything we hold. Genuinely new, so let core add it under the + // URL the list gives and rewrite that itself on the first fetch. + return $feed; + } + + try { + $feed->_url($target); + } catch (FreshRSS_BadUrl_Exception $e) { + Minz_Log::warning('rssCloud: ' . $e->getMessage(), RSSCLOUD_LOG); + return $feed; + } + Minz_Log::notice('rssCloud: ' . \SimplePie\Misc::url_remove_credentials($url) . + ' permanently moved to a feed already subscribed as ' . $target . ', reusing it', RSSCLOUD_LOG); + return $feed; + } + // /** From 51f132ef679200f8860b0c5aec03a4b83be79abc Mon Sep 17 00:00:00 2001 From: Andrew Shell Date: Mon, 3 Aug 2026 18:18:57 -0500 Subject: [PATCH 2/2] fix(opml): harden redirect resolution against review findings Four issues from review, all in the redirect reconciliation added by the parent commit. Register FeedBeforeInsert unconditionally. It was gated on opml_enabled, but that switch governs whether rssCloud subscribes to a resource, not whether the duplicates exist: the dynamic OPML refresh that creates them runs from cron and the CLI regardless. Gating it also contradicted the documented behaviour for manual subscriptions. Do not cache a transient HTTP failure as "did not move". Any status that was not 301 or 308 -- including 5xx, 429 and 408 -- resolved to null, which follow() read as a confirmed non-redirect and store() then trusted for 30 days. A briefly failing origin could therefore resume creating duplicates for a month. Those statuses, and a permanent redirect carrying no Location, now report a failure instead, which is retried after 6 hours. A rejected HEAD stays a settled answer, since it would answer the same way later. Probe the target reached by the last allowed hop. The loop followed MAX_HOPS redirects but exited before checking where the last one landed, so a chain of exactly MAX_HOPS was reported unresolvable. Core counts the same way in FreshRSS_http_Util::httpGet(). Scrub credentials from every logged URL. A redirect Location can carry them, and the target and both Redirects.php messages were written unsanitised. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 4 +++- RssCloud/Redirects.php | 33 +++++++++++++++++++++++++-------- extension.php | 9 +++++++-- 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 1b593ad..705a4d4 100644 --- a/README.md +++ b/README.md @@ -129,7 +129,9 @@ usual. The hook fires on every import path, so this covers refreshes driven by cron or the CLI as well as by a notification, and keeps a manual subscription from duplicating a feed already held under its -post-redirect URL. +post-redirect URL. It is registered regardless of the two configuration switches: those govern +whether rssCloud *subscribes* to a resource, while these duplicates are created by the dynamic OPML +refresh itself, which runs either way. ## Known gaps diff --git a/RssCloud/Redirects.php b/RssCloud/Redirects.php index 812ee82..5fc33b6 100644 --- a/RssCloud/Redirects.php +++ b/RssCloud/Redirects.php @@ -107,7 +107,10 @@ private function follow(string $url, array $attributes): ?string { $current = $url; $seen = [$current => true]; - for ($hop = 0; $hop < self::MAX_HOPS; $hop++) { + // One probe more than the hop limit: the URL reached by the last allowed hop still has to be + // checked, or a chain of exactly MAX_HOPS would be reported as unresolvable despite being + // within the limit — which is also how core counts, in `FreshRSS_http_Util::httpGet()`. + for ($hop = 0; $hop <= self::MAX_HOPS; $hop++) { $location = self::permanentLocation($current, $attributes); if ($location === false) { return null; @@ -115,6 +118,9 @@ private function follow(string $url, array $attributes): ?string { if ($location === null) { return $current; } + if ($hop === self::MAX_HOPS) { + break; + } $absolute = \SimplePie\Misc::absolutize_url($location, $current); $next = is_string($absolute) ? (FreshRSS_http_Util::checkUrl($absolute, fixScheme: false) ?: '') : ''; if ($next === '' || isset($seen[$next])) { @@ -125,7 +131,8 @@ private function follow(string $url, array $attributes): ?string { $current = $next; } - Minz_Log::warning('rssCloud: too many permanent redirects from ' . $url, RSSCLOUD_LOG); + Minz_Log::warning('rssCloud: too many permanent redirects from ' . + \SimplePie\Misc::url_remove_credentials($url), RSSCLOUD_LOG); return null; } @@ -191,14 +198,24 @@ private static function permanentLocation(string $url, array $attributes): strin $error = curl_error($ch); if ($error !== '') { - Minz_Log::debug('rssCloud: cannot check ' . $url . ' for a permanent redirect: ' . $error, RSSCLOUD_LOG); + Minz_Log::debug('rssCloud: cannot check ' . \SimplePie\Misc::url_remove_credentials($url) . + ' for a permanent redirect: ' . $error, RSSCLOUD_LOG); return false; } - // A server that rejects HEAD tells us nothing about redirection, which is the status quo, not - // a failure — retrying it in six hours would not produce a different answer. - if (!in_array($status, [301, 308], true)) { - return null; + + if (in_array($status, [301, 308], true)) { + // A permanent redirect with nowhere to go is malformed, and says nothing either way. + return is_string($location) && $location !== '' ? $location : false; + } + + // Transient: the same request may well answer differently later, so this must not be recorded + // as a lasting "did not move" — that would let duplicates resume for the whole cache lifetime. + if ($status === 0 || $status === 408 || $status === 429 || $status >= 500) { + return false; } - return is_string($location) && $location !== '' ? $location : null; + + // Anything else is a stable answer of "this has not permanently moved", including a server + // that rejects HEAD outright: retrying that in six hours would not produce a different answer. + return null; } } diff --git a/extension.php b/extension.php index 43f7f4a..612307f 100644 --- a/extension.php +++ b/extension.php @@ -81,8 +81,12 @@ public function init(): void { } if ($this->isEnabledForOpml()) { $this->registerHook(Minz_HookType::FreshrssUserMaintenance, [$this, 'onUserMaintenance']); - $this->registerHook(Minz_HookType::FeedBeforeInsert, [$this, 'onFeedBeforeInsert']); } + + // Deliberately not gated on either switch. Those govern whether rssCloud *subscribes* to a + // resource, whereas the duplicates this guards against are created by the dynamic OPML + // refresh itself — which cron and the CLI perform whatever this extension is set to do. + $this->registerHook(Minz_HookType::FeedBeforeInsert, [$this, 'onFeedBeforeInsert']); } #[\Override] @@ -387,7 +391,8 @@ public function onFeedBeforeInsert(FreshRSS_Feed $feed): FreshRSS_Feed { return $feed; } Minz_Log::notice('rssCloud: ' . \SimplePie\Misc::url_remove_credentials($url) . - ' permanently moved to a feed already subscribed as ' . $target . ', reusing it', RSSCLOUD_LOG); + ' permanently moved to a feed already subscribed as ' . + \SimplePie\Misc::url_remove_credentials($target) . ', reusing it', RSSCLOUD_LOG); return $feed; }