Skip to content

fix(subscriber): keep the POST body across the http to https redirect - #9

Merged
andrewshell merged 1 commit into
mainfrom
fix/i18n-keys-and-protocol-fallback
Aug 7, 2026
Merged

fix(subscriber): keep the POST body across the http to https redirect#9
andrewshell merged 1 commit into
mainfrom
fix/i18n-keys-and-protocol-fallback

Conversation

@andrewshell

@andrewshell andrewshell commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Fixes the Failed No feed for url1. errors showing against many rssCloud servers in the admin screen.

Cause

The redirect, not the payload.

rssCloud endpoints are usually advertised as http in the <cloud> element and answer with a permanent redirect to https. libcurl's default on 301/302/303 is to follow it as a bodyless GET — exactly what curl -L does. The server then sees no url1 and says so.

On FreshRSS 1.29.0 and 1.29.1 this is the whole story: httpGet() sets CURLOPT_FOLLOWLOCATION and hands the following to libcurl, so the body was always lost. Setting CURLOPT_POSTREDIR keeps the POST intact.

Verified against a redirecting echo server:

Config Method after 301 url1
as shipped GET lost
+ CURLOPT_POSTREDIR POST arrives

A note for reviewers

This option looks redundant on main/edge development builds, which follow redirects by hand and incidentally preserve the body. It is not — deleting it breaks every released version. The docblock says so at the call site, and a test pins it, because this is exactly the line someone removes during a cleanup.

The option assembly moved into notifyCurlOptions() purely so the behaviour is reachable from a test; there is no behaviour change beyond CURLOPT_POSTREDIR.

Testing

  • Extension suite green: 24 tests, 68 assertions (2 new).
  • php -l clean.
  • phpstan: one pre-existing finding in Redirects.php:155, confirmed present with these changes stashed. It is the method_exists() version-compat check, which reads as always-true only because phpstan analyses against edge; it is load-bearing on 1.29.x.

Known limitation, unrelated to this PR

WordPress.com's rssCloud cannot notify an https callback at all, and the protocol fallback merged in #8 does not rescue it:

  • https-post is rejected outright (Only http-post notifications are supported at this time.).
  • Falling back to http-post does not change the callback's port, which is still 443. WordPress.com then attempts http://example.com:443/path — plain HTTP against the TLS port — which fails with a different error.

So a FreshRSS instance served over https cannot register with WordPress.com until the fix lands upstream there. Noted so the remaining failures are not mistaken for the bug this PR fixes.

🤖 Generated with Claude Code

rssCloud endpoints are usually advertised as `http` in the `<cloud>` element
and answer with a permanent redirect to `https`. libcurl's default on
301/302/303 is to follow that as a bodyless GET, exactly as `curl -L` does, so
the server received no `url1` and answered `No feed for url1.` — which is what
made many servers show as permanently failed in the admin screen.

On FreshRSS 1.29.0 and 1.29.1 this is the whole story: `httpGet()` sets
`CURLOPT_FOLLOWLOCATION` and lets libcurl follow the redirect, so the body was
always lost. Development builds follow redirects by hand and happen to preserve
it, which is why the bug does not reproduce when tested against `edge` — and
why the option below looks redundant there.

Set `CURLOPT_POSTREDIR` so the POST survives the redirect, and extract the
option assembly into `notifyCurlOptions()` so the behaviour can be pinned by a
test rather than rediscovered.

Verified against a redirecting echo server: without the option the request
arrives as a GET with an empty body; with it, `url1` arrives intact.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

RssCloud_Subscriber now exposes notifyCurlOptions() for notification POST configuration. The helper preserves POST bodies across redirects and limits redirects to 10. subscribe() uses the helper, and tests verify both settings.

Changes

Notification redirect handling

Layer / File(s) Summary
Centralized notification cURL options
RssCloud/Subscriber.php, tests/RssCloud/SubscriberTest.php
Adds notifyCurlOptions() and tests its POST body and redirect settings.
Subscriber integration
RssCloud/Subscriber.php
Updates subscribe() to use the shared cURL option helper.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Poem

I’m a rabbit with a POST in flight,
Through every redirect, the body stays right.
Ten hops are allowed, the options align,
Tests guard the payload each time.
Hop, hop—what a tidy design!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary fix: preserving the POST body across HTTP-to-HTTPS redirects.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/i18n-keys-and-protocol-fallback

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@andrewshell andrewshell changed the title fix: repair rssCloud registration (redirect POST body, protocol fallback, i18n keys) fix(subscriber): keep the POST body across the http to https redirect Aug 7, 2026
@andrewshell
andrewshell merged commit 8684276 into main Aug 7, 2026
4 checks passed
@andrewshell
andrewshell deleted the fix/i18n-keys-and-protocol-fallback branch August 7, 2026 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant