fix(opml): stop dynamic OPML categories accumulating redirected feeds - #1
Conversation
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) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe extension adds ChangesRedirect reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OPMLImport
participant FeedBeforeInsert
participant RssCloud_Redirects
participant ExistingFeedStore
OPMLImport->>FeedBeforeInsert: submit imported feed
FeedBeforeInsert->>RssCloud_Redirects: resolve feed URL
RssCloud_Redirects-->>FeedBeforeInsert: return redirect target
FeedBeforeInsert->>ExistingFeedStore: check target URL
ExistingFeedStore-->>FeedBeforeInsert: return existing feed or no match
FeedBeforeInsert-->>OPMLImport: return reconciled or unchanged feed
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extension.php`:
- Around line 389-390: Update the logging statement in the rssCloud redirect
handling to sanitize the `$target` value with
`\SimplePie\Misc::url_remove_credentials()` before including it in the
`Minz_Log::notice` message, while preserving the existing sanitization of `$url`
and the log behavior.
- Around line 82-85: Register the FeedBeforeInsert hook for onFeedBeforeInsert
independently of the isEnabledForOpml() condition in the extension
initialization flow, while keeping FreshrssUserMaintenance registration gated by
OPML support. Ensure manual subscriptions reconcile permanent redirects when
OPML discovery is disabled.
In `@RssCloud/Redirects.php`:
- Around line 197-202: Update the redirect-status handling in follow() so
retryable HTTP failures (408, 429, and 5xx) and 301/308 responses with a missing
or empty Location return false, preventing store() from caching them as
non-redirects. Preserve null for stable non-permanent responses, including
rejected HEAD requests, and continue returning valid Location values for
well-formed 301/308 responses.
- Around line 110-129: Update the permanent-redirect resolution flow around the
hop loop and `permanentLocation` so exactly `self::MAX_HOPS` redirects are
followed and the URL reached by the final hop is probed. After that final probe,
return the target when it has no further permanent redirect, and only
reject/cache failure when the probe reveals another permanent redirect; preserve
existing handling for unusable URLs, loops, and earlier failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be03a810-8050-45e0-ab93-734da56da59a
📒 Files selected for processing (3)
README.mdRssCloud/Redirects.phpextension.php
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) <noreply@anthropic.com>
|
Addressed all four findings in 51f132e. All were valid. Register Scrub credentials from Allow exactly Do not cache transient failures as "no redirect" — the most consequential of the four. Any non-301/308 status returned VerificationExtended the probe harness to cover exactly these cases, against live hosts: The original cases still hold: One aside worth recording: the four-hop assertion failed on its first run and passed on re-run. That was httpbin returning a transient error — which the new code classified as a failure rather than caching it as "did not move", so the flake was the fix demonstrating itself.
|
1 participant
The bug
Seen in production: a dynamic OPML category gradually accumulating duplicates of a feed that appears once in the OPML. Six copies so far, all sharing the URL
https://www.almaren.ch/feed/, while the OPML listshttps://www.almaren.ch/feed.That URL 301s to the trailing-slash form (verified). FreshRSS and OPML then disagree about what identifies a feed:
FreshRSS_Feed::load()treats a feed as the document it resolves to, and rewrites the stored URL on HTTP 301 (Feed.php:677).FreshRSS_Category::refreshDynamicOpml()treats a feed as the exactxmlUrlstring in the list, which does not change (Category.php:236-254).One 301 is enough to make them disagree forever. Every later refresh reads the entry as new and inserts it again, then mutes the drifted copy for having disappeared from the list. Nothing catches the collision:
_feed.urlhas no unique index (pgsql explicitly drops_feed_url_key), andFeedDAO::updateFeed()does not check for one. Copies accumulate at one per refresh — unbounded here, because rssCloud refreshes on notification rather than on a timer.This is a core defect that affects every dynamic OPML user; rssCloud just makes it fire at ping rate instead of once per cache duration. Worth reporting upstream separately.
The fix
FeedBeforeInsertsettles 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, andFeedDAO::addFeedObject()unmutes it if an earlier refresh muted it.New
RssCloud_Redirectsresolves and caches that, one file per URL underdata/rssCloud/redirects/:CURLOPT_FOLLOWLOCATION => false, so every hop is re-checked throughFreshRSS_http_Util::getCurlResolveInfo()— a redirect cannot walk into the private network. Protocols pinned to http/https, loops detected.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 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.
Testing
The FreshRSS classes were stubbed and the real
RssCloud_Redirectsexercised against live hosts (nodata/config.phpin the dev checkout, so FreshRSS cannot boot):almaren.ch/feed/feed/almaren.ch/feed/That caught a real bug: a 302 and a dead host both stored
target=null, so "it definitively did not move" was getting the short retry TTL meant for "we could not tell". Now separated by an explicitfailedflag.php -l, phpstan, phpcs and markdownlint all pass locally.Reviewer notes
FeedBeforeInsertthe feed has no category yet, so the check is "does this redirect to any feed we hold", not "one in this category". If the target sits in a different category,addFeedObject()finds it, unmutes it, and does not insert or move it — no duplicate either way, just less precise than you might expect.Callback.php:83is a non-atomic read-check-write, so concurrent notifications can both pass it and race the non-atomicsearchByUrl→INSERT. Separate issue.🤖 Generated with Claude Code
Summary by CodeRabbit