Feat/websub hub#36
Merged
Merged
Conversation
Lay out the full WebSub hub implementation plan in TODO.md as ordered TDD vertical slices spanning @rsscloud/core, @rsscloud/express, and apps/server. Settled decisions: async-202 intent verification behind an in-process best-effort VerificationScheduler seam (persisted queue + retry deferred); both thin-publish (re-fetch) and fat-ping content sourcing; honor the requested lease clamped to a configurable range; HMAC-SHA256 signatures. Headline use case: an rssCloud publisher adds <link rel="hub"> and keeps pinging via rssCloud, while WebSub subscribers to the same topic receive full content distribution — which falls out of core's existing resource-keyed fan-out. Each flow gets an e2e acceptance test as its TDD outer loop, and server integration (plugin registration, route mount, config) is spelled out per file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ADR-0002 captures the settled WebSub hub design: async-202 intent verification behind an in-process best-effort VerificationScheduler seam (a persisted queue + retry is a future refactor behind the same seam), plus the lease (honor-requested-clamped) and HMAC-SHA256 signature decisions. CONTEXT.md gains a WebSub vocabulary cluster (Topic vs Resource, Callback vs Subscription.url, Intent verification, VerificationScheduler, Lease, Content distribution, Fat ping, X-Hub-Signature), ties the Hub and Notification entries to their WebSub roles, and adds a dialogue exchange. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add parseSubscribe in protocols/websub-dispatcher.ts: validates hub.mode
(subscribe), hub.callback (a valid absolute URL), and hub.topic
(present), returning {status:400} for anything malformed. A valid
request builds a 'websub' SubscribeRequest directly
(callbackUrl=hub.callback, resourceUrls=[hub.topic]) without
buildSubscribeRequest, which gates on rssCloud-only protocols and
assembles callbacks from port/path/domain.
Internal for now; createWebSubDispatcher and the index export land with
the express factory (S1.4). 100% coverage maintained.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add createWebSubProtocolPlugin (protocols: ['websub']). verify() always performs the WebSub intent-verification GET — never the rssCloud same-domain test-notify, so it ignores diffDomain — appending hub.mode=subscribe / hub.topic / hub.challenge to the callback (preserving any existing query) and requiring a 2xx with an exact challenge echo, else throwing. fetch and the challenge generator are injectable. deliver() is an interim stub reporting failure (it must not throw; the engine's deliverTo does not catch); real content distribution is S2.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Introduce VerificationScheduler — the seam behind WebSub's async-202 intent verification. The default in-process scheduler runs each verify→persist task on the microtask queue (best-effort, one attempt) and routes a rejection to onError; a future persisted queue can satisfy the same interface (ADR-0002). core.acceptSubscription(req) returns immediately and schedules the work via the scheduler. It is a new caller of the unchanged subscribe, so a successful verify persists the subscription and a refusal persists nothing — the synchronous rssCloud subscribe path is untouched. The default scheduler surfaces a thrown task through the existing error event (scope: websub-verification), coercing any non-Error throwable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add createWebSubDispatcher in core: parse the hub.* body and, on a valid
subscribe, hand the built request to core.acceptSubscription and answer
202; a malformed body is 400. Add the thin express websub({ core })
factory mirroring ping/pleaseNotify — it parses the urlencoded body and
copies the dispatcher's status onto the reply, with the hub.* logic
owned by core. Export both, plus createWebSubProtocolPlugin, from their
package indexes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Register createWebSubProtocolPlugin in the core composition root so
core.subscribe accepts the 'websub' protocol, and mount
websub({ core }) at config.webSubPath (default /websub). Add WEBSUB_PATH
and HUB_URL config (hubUrl defaults to domain/port/path; consumed once
content distribution lands). The plugin gets requestTimeoutMs for now;
hubUrl wiring follows with deliver().
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the WebSub subscribe acceptance suite against the running server: a challenge-echoing callback (the existing mock's function responseBody returning req.query['hub.challenge']) is recorded after the async 202, polled via the test API; a refusing callback is never recorded within a bounded timeout; and a malformed hub.* body (missing callback/topic, or an unsupported mode) returns 400. Full suite: 138 e2e passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implement the WebSub plugin's deliver(): POST the changed feed body to each subscriber's callback, relaying the origin Content-Type verbatim (falling back to application/octet-stream when absent) and advertising the hub/self Link rels. Delivery follows 3xx redirects like the rssCloud REST notify path; any non-2xx is a failed delivery. The hub's public URL is injected as a createWebSubProtocolPlugin option and wired from config.hubUrl in apps/server. With this in place a single rssCloud ping already fans content out to WebSub subscribers through the engine's existing resource-keyed fan-out — no new publish path needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a cross-protocol fan-out acceptance test: an rssCloud subscriber and a WebSub subscriber share one topic, and a single ordinary rssCloud /ping fires both — the rssCloud sub gets its notify, the WebSub callback gets a POST carrying the changed feed body, relayed Content-Type, and hub/self Link rels. This is the headline "free WebSub for rssCloud publishers" proof; no hub.mode=publish is involved. Extend the shared mock subscriber with content-capture: a catch-all bodyParser.text records raw, non-urlencoded POST bodies (the WebSub delivery) while leaving rssCloud notify bodies parsed as objects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Authenticate content distribution for subscribers that supply a hub.secret. parseSubscribe carries the secret through as details.secret; the plugin then signs each delivery body with HMAC and adds X-Hub-Signature: <algo>=<hex>. No secret means no header. The HMAC algorithm is a plugin option (default sha256, names both the digest and the header method prefix), wired from a new WEBSUB_SIGNATURE_ALGO env knob in apps/server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an authenticated-distribution suite: subscribe with a hub.secret, fire one rssCloud ping, and recompute HMAC-SHA256(secret, body) over the body the WebSub callback received to confirm it matches X-Hub-Signature. Cover the negative too — no hub.secret means no signature header. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WebSub unsubscribe, like subscribe, must confirm the subscriber's intent before the hub acts — but core.unsubscribe has no verify hook. Add the verified path: - VerifyContext gains an optional `mode`, threaded onto the plugin's challenge GET as hub.mode (defaults to subscribe; rssCloud ignores it). - core.acceptUnsubscription schedules a challenge GET in unsubscribe mode and calls unsubscribe only once confirmed — a no-op when the sub is absent or the callback refuses to echo. - The websub dispatcher branches on hub.mode (subscribe/unsubscribe → 202, anything else → 400) via a shared hub.callback/hub.topic parser; the express factory's core Pick widens to acceptUnsubscription. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Subscribe, then drive hub.mode=unsubscribe: when the callback echoes the unsubscribe-mode challenge the subscription is removed; when it refuses, the subscription survives. Polls the store for removal since verification is async. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add WebSub lease handling. RssCloudConfig gains
webSubLease{Default,Min,Max}Secs; the dispatcher parses hub.lease_seconds
into details, and subscribeOne clamps it to [min, max] (or grants the
default when omitted), records the chosen value in details.leaseSeconds,
and maps it to whenExpires = now + chosen. The chosen lease is threaded
through VerifyContext so the plugin echoes hub.lease_seconds on the
subscribe challenge GET. removeExpired drops a lapsed lease unchanged.
Lease bounds are wired from WEBSUB_LEASE_* env in apps/server.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Subscribe with a below-minimum hub.lease_seconds and assert the chosen lease is clamped up to the bound, recorded in details, and echoed on the verification GET. Separately, force a recorded lease to lapse and confirm removeExpired drops it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Let a pure-WebSub publisher (no rssCloud ping) notify the hub that a topic changed via hub.mode=publish. The dispatcher parses the topic from hub.url (falling back to hub.topic) and calls a new core.acceptPublish, which — per WebSub §7 — acknowledges immediately (202) and re-fetches the topic out of band, reusing ping's existing fetch→payload→fanOut. A failed fetch is surfaced on the error event (scope websub-publish) rather than thrown. The dispatcher and express factory core Picks widen accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A WebSub subscriber subscribes to a topic, then a pure-WebSub publisher POSTs hub.mode=publish for it; poll for the out-of-band delivery and assert the subscriber receives the changed feed body. Also retarget the "unsupported hub.mode" rejection at a bogus mode now that publish is a supported mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The WebSub hub is functionally complete (subscribe, content distribution, HMAC signatures, unsubscribe, leases, native publish). Remove TODO.md now that the roadmap is done — durable decisions live in docs/adr, CONTEXT.md, and git history per CLAUDE.md. Annotate CONTEXT.md's Fat ping entry as out of scope: it is non-standard (a PubSubHubbub-era extension with no WebSub wire format), so the hub only ever does thin publishes. The term is kept solely to explain the naming. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Teach the client harness to speak WebSub alongside rssCloud so the hub's WebSub paths can be exercised by hand. - lib/websub.js: createWebSubClient (subscribe/unsubscribe/publish over hub.* forms) and readVerification for the intent-verification GET. - feed.js: renderCloudFeed optionally advertises the hub via <atom:link rel="hub"> + rel="self"; output is unchanged when omitted. - client.js: a WebSub control set (with lease_seconds/secret inputs), a /websub-callback that echoes the challenge and reports the delivery's X-Hub-Signature with a valid/invalid verdict, and a hub-advertising feed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The README was the only doc page and didn't cover XML-RPC. Break the protocol surface into dedicated pages under apps/server/docs/ and turn the README into an index, so the docs read well both on GitHub and in the app. - New pages: rssCloud over REST, rssCloud over XML-RPC, WebSub (incl. how to advertise a hub from a feed — Link header primary, atom:link backup), and a cross-protocol page on the unified fan-out. - markdown-doc: rewrite relative .md links to in-app routes (preserving #anchors) and add GitHub-style heading ids, so the same source links work as files on GitHub and as routes in the rendered docs. - controllers: DOC_PAGES table; /docs index plus /docs/:page (404 unknown). - home nav and e2e static coverage updated for the new pages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds WebSub hub support across core, server, client, docs, and tests, with async verification scheduling, WebSub request parsing and delivery, lease handling, SSRF-safe fetch guards, and updated documentation and harness wiring. ChangesWebSub Protocol Implementation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 11
🤖 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 `@apps/server/config.js`:
- Around line 19-20: The WEBSUB_PATH configuration variable is being used
without normalization, which causes issues when the value lacks a leading slash
(e.g., setting it to 'websub' results in malformed URLs like
'http://host:portwebsub'). Normalize the WEBSUB_PATH value at its point of
assignment to ensure it always has a leading slash prefix, so that both the
route mounting at the webSubPath variable and the subsequent hubUrl composition
at line 36 produce correctly formatted paths and URLs regardless of how
WEBSUB_PATH is configured.
In `@CONTEXT.md`:
- Line 210: There is a blank line inside the blockquote (quoted example) at line
210 in CONTEXT.md that violates markdownlint rule MD028. Remove this internal
blank line to keep the blockquote contiguous, ensuring all lines in the
blockquote are consecutive with no empty lines between them.
In `@packages/core/src/config.test.ts`:
- Around line 12-15: The default-config test in config.test.ts is checking
literal values but not validating the ordering invariant required by lease
clamping logic. Add an assertion after the configuration section that verifies
the relationship webSubLeaseMinSecs <= webSubLeaseDefaultSecs <=
webSubLeaseMaxSecs holds true. This will catch future misconfigurations of these
three lease-related settings earlier during testing.
In `@packages/core/src/engine/create-core.test.ts`:
- Around line 994-1028: Add three new test cases to the lease suite after the
existing tests to cover edge cases with non-finite and fractional lease values.
Create tests that call subscribeWebSub with leaseSeconds values of NaN,
Infinity, and a fractional number (like 123.456), and verify that each case
safely normalizes to a valid integer value within the allowed range, ensuring
sub?.details contains a valid integer leaseSeconds and sub?.whenExpires is a
valid Date object.
In `@packages/core/src/engine/create-core.ts`:
- Around line 109-116: The clampLease function does not validate that the
requested lease value is finite before clamping it, allowing NaN and Infinity to
pass through since typeof NaN returns 'number'. This causes invalid dates to be
persisted at the point where whenExpires is set. Fix the type check in
clampLease to also verify that the requested value is finite using
Number.isFinite(), returning config.webSubLeaseDefaultSecs if the value is not
finite, ensuring only valid numeric values proceed to the Math.min and Math.max
clamping logic.
In `@packages/core/src/engine/verification-scheduler.ts`:
- Around line 33-35: The current implementation in the queueMicrotask callback
only handles promise rejections via .catch(), but does not handle synchronous
throws that might occur before the task returns a promise. Although async
functions convert synchronous throws to rejections, the interface accepts any ()
=> Promise<void>, so synchronous exceptions could be thrown. Wrap the task()
invocation in a try-catch block to catch both synchronous throws and promise
rejections, ensuring both error types are passed to options.onError to fully
honor the non-throwing contract of the schedule method.
In `@packages/core/src/protocols/websub-dispatcher.test.ts`:
- Around line 50-58: Following the pattern of the existing test that validates
hub.callback rejection for invalid URLs, add additional test cases in the
parseSubscribe test suite to verify that malformed hub.topic and hub.url
parameters are also properly rejected with a 400 status code. Create test cases
similar to the shown test but with invalid non-absolute URLs for the hub.topic
and hub.url fields to ensure the parser consistently enforces URL shape
validation across all URL parameters.
In `@packages/core/src/protocols/websub-dispatcher.ts`:
- Around line 51-53: The validation for hub.topic and hub.url (around lines
51-53 and 63-71 respectively) currently only checks if they are non-empty
strings, but does not validate that they are valid URLs. This allows malformed
WebSub requests to pass initial parsing with a 202 response and fail later
during async verification instead of returning a synchronous 400 error. Add URL
validation logic for both the topic variable (after the non-empty string check)
and the url variable to ensure they are properly formatted URLs before returning
null, rejecting invalid requests immediately at parse time.
In `@packages/core/src/protocols/websub-plugin.test.ts`:
- Around line 394-427: The websub-plugin test suite is missing regression tests
for two critical invariants in the deliver functionality. Add two new test cases
to the deliver test suite in websub-plugin.test.ts: first, create a test that
verifies the redirect-loop cap is enforced (that redirect following has a
bounded limit and fails or stops when exceeded), and second, create a test that
verifies explicit failure occurs when the hubUrl is missing from the plugin
configuration. Both tests should follow the pattern of the existing 'follows a
3xx redirect' test case by setting up a fakeFetch mock and asserting on the
result behavior.
In `@packages/core/src/protocols/websub-plugin.ts`:
- Around line 93-97: The Link header construction in the websub-plugin.ts
headers object interpolates hubUrl without validating it is defined, which
produces invalid metadata like <undefined> when hubUrl is missing. Add a guard
check before the headers object initialization to validate that hubUrl exists
and is not null or undefined, and either throw an error or return early if it is
missing, ensuring the delivery path only proceeds with valid hub metadata.
- Around line 119-123: The distribute function recursively follows HTTP
redirects without any depth limit, which can cause infinite loops if a callback
URL redirects to itself. Add a redirect depth parameter to the distribute
function signature to track the number of hops, then before the recursive
distribute call (where it handles the location header), check if the current
depth has exceeded a maximum limit (e.g., 5 hops). If the limit is reached, log
a warning or error and return early instead of making the recursive call. This
bounds the redirect chain and prevents indefinite delivery task execution.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: cd787834-1333-4827-a6b8-a72efcb01c3b
📒 Files selected for processing (24)
CONTEXT.mdTODO.mdapps/e2e/test/mock.jsapps/e2e/test/websub.jsapps/server/config.jsapps/server/controllers/index.jsapps/server/core.jsdocs/adr/0002-websub-async-intent-verification-seam.mdpackages/core/src/config.test.tspackages/core/src/config.tspackages/core/src/engine/core.tspackages/core/src/engine/create-core.test.tspackages/core/src/engine/create-core.tspackages/core/src/engine/plugin.tspackages/core/src/engine/verification-scheduler.test.tspackages/core/src/engine/verification-scheduler.tspackages/core/src/index.tspackages/core/src/protocols/websub-dispatcher.test.tspackages/core/src/protocols/websub-dispatcher.tspackages/core/src/protocols/websub-plugin.test.tspackages/core/src/protocols/websub-plugin.tspackages/express/src/index.tspackages/express/src/websub-middleware.test.tspackages/express/src/websub-middleware.ts
💤 Files with no reviewable changes (1)
- TODO.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/services/markdown-doc.js (1)
62-69: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMinor: guard
envinheading_openfor parity withlink_open.
heading_opendereferencesenv.usedSlugsdirectly whilelink_openguardsenv && env.docLinks. It is safe today becauserenderMarkdownDocalways callsmd.render(content, { docLinks }), but the asymmetry would throw if this sharedmdinstance were ever rendered without an env object.♻️ Optional guard
md.renderer.rules.heading_open = function(tokens, idx, options, env, self) { - if (!env.usedSlugs) { + if (!env) { + return self.renderToken(tokens, idx, options); + } + if (!env.usedSlugs) { env.usedSlugs = new Map(); }🤖 Prompt for 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. In `@apps/server/services/markdown-doc.js` around lines 62 - 69, Guard the env object in md.renderer.rules.heading_open, since it currently assumes env always exists while link_open already checks env && env.docLinks. Update heading_open to initialize and use env.usedSlugs only when env is present, keeping behavior the same for renderMarkdownDoc and preventing a throw if md.render is ever called without an env object.
🤖 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.
Nitpick comments:
In `@apps/server/services/markdown-doc.js`:
- Around line 62-69: Guard the env object in md.renderer.rules.heading_open,
since it currently assumes env always exists while link_open already checks env
&& env.docLinks. Update heading_open to initialize and use env.usedSlugs only
when env is present, keeping behavior the same for renderMarkdownDoc and
preventing a throw if md.render is ever called without an env object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42b61118-bff3-4d1b-8cb7-73b7f4e9cf8a
📒 Files selected for processing (17)
apps/client/README.mdapps/client/client.jsapps/client/lib/feed.jsapps/client/lib/feed.test.jsapps/client/lib/index.jsapps/client/lib/websub.jsapps/client/lib/websub.test.jsapps/e2e/test/static.jsapps/server/README.mdapps/server/controllers/index.jsapps/server/docs/cross-protocol.mdapps/server/docs/rsscloud-rest.mdapps/server/docs/rsscloud-xml-rpc.mdapps/server/docs/websub.mdapps/server/services/markdown-doc.jsapps/server/services/markdown-doc.test.jsapps/server/views/home.handlebars
✅ Files skipped from review due to trivial changes (4)
- apps/server/docs/rsscloud-rest.md
- apps/client/README.md
- apps/server/README.md
- apps/server/docs/cross-protocol.md
vite 8.0.14 (a dev/test-only transitive dep via vitest) is in range for two advisories, both Windows-only dev-server issues: - GHSA-fx2h-pf6j-xcff (high): server.fs.deny bypass on Windows alt paths - GHSA-v6wh-96g9-6wx3 (medium): launch-editor NTLMv2 hash disclosure Raise the pnpm override floor from >=6.4.2 to >=8.0.16; resolves to 8.1.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Subscriber- and publisher-supplied URLs (hub.callback, hub.topic/url) are fetched and delivered to with no host check. Because WebSub content distribution relays a fetched body to the callback, that is a full-read SSRF: name an internal hub.topic, receive its response at a callback you control. Add createSafeFetch: a fetch that refuses non-http(s) schemes and screens the destination against classifyBlockedAddress (loopback, private, link-local/metadata, unique-local, CGNAT; IPv4-mapped decoded). It is rebinding-safe -- a custom DNS lookup pins the connection to the validated address, and a custom connector screens IP-literal hosts (which skip DNS entirely), both re-firing on every redirect hop. createCidrAllowList exempts operator-listed ranges. Adds undici (Agent + connector, to pin the socket) and ipaddr.js (range classification) as core deps. 100% coverage maintained. See ADR-0003. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Build one createSafeFetch and inject it as the fetch for the engine's topic re-fetch and every protocol plugin's deliveries and verification GETs, so the REST/XML-RPC notify, the WebSub challenge GET, and WebSub content distribution are all guarded -- and the pre-existing rssCloud blind fetch is hardened along the way. On by default via WEBSUB_SSRF_PROTECTION; WEBSUB_FETCH_ALLOW_CIDRS exempts a private LAN that legitimately hosts feeds. The e2e suite runs with protection on and the Docker private ranges allowlisted, so the guarded path is exercised end to end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clears the remaining Dependabot alerts: - ws ^8.20.1 -> ^8.21.0 (high; GHSA-96hv-2xvq-fx4p, memory-exhaustion DoS) - markdown-it ^14.1.1 -> ^14.2.0 (moderate; GHSA-6v5v-wf23-fmfq, quadratic smartquotes DoS) -- both direct apps/server deps - form-data >=4.0.6 via pnpm override (high; GHSA-hmw2-7cc7-3qxx, CRLF injection) -- transitive through superagent/chai-http Lockfile resolves to ws 8.21.0, markdown-it 14.2.0, form-data 4.0.6. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/e2e/docker-compose.yml`:
- Around line 12-15: Remove the loopback CIDR from the E2E SSRF allowlist in the
WEBSUB_FETCH_ALLOW_CIDRS setting so only the Docker-private ranges remain
permitted. Update the docker-compose configuration used by the E2E suite and
keep the existing guarded fetch behavior intact, referencing the
WEBSUB_FETCH_ALLOW_CIDRS entry in apps/e2e/docker-compose.yml.
In `@docs/adr/0003-ssrf-egress-guard-on-outbound-fetch.md`:
- Around line 34-45: The outbound allowlist is being shared between trusted
topic fetches and attacker-controlled callback deliveries, which reopens SSRF
for `hub.callback` targets. Update the
`createSafeFetch`/`WEBSUB_FETCH_ALLOW_CIDRS` policy so private CIDR exemptions
apply only to trusted feed/topic fetch paths, and use a separate stricter policy
for callback delivery and verification GETs in the WebSub and rssCloud injection
points. Locate the fix where the guarded fetch is wired into the engine and
protocol plugins, and split the outbound call types so untrusted callbacks never
inherit the private-range allowlist.
In `@packages/core/src/safe-fetch.ts`:
- Around line 157-160: Normalize bracketed IPv6 hostnames before the IP check in
safe-fetch so the SSRF guard still runs for literal IPv6 hosts. In the logic
that reads options.hostname and calls isIP(), strip surrounding brackets from
values like [::1] before evaluating blockedReason and the allowlist path. Add a
regression test around the safe-fetch request flow for http://[::1]/ to verify
the IP-literal check is not bypassed.
🪄 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: ce3c79ae-e828-4ada-b048-91fee143262d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
CONTEXT.mdapps/e2e/docker-compose.ymlapps/server/config.jsapps/server/core.jsapps/server/docs/websub.mddocs/adr/0003-ssrf-egress-guard-on-outbound-fetch.mdpackage.jsonpackages/core/package.jsonpackages/core/src/index.tspackages/core/src/safe-fetch.test.tspackages/core/src/safe-fetch.ts
✅ Files skipped from review due to trivial changes (2)
- package.json
- apps/server/docs/websub.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/index.ts
- CONTEXT.md
- apps/server/config.js
A WEBSUB_PATH set without a leading slash (e.g. "websub") produced a malformed hubUrl (http://host:portwebsub) — the URL advertised to subscribers in the Link rel="hub" header — and an off-root route mount. Normalize it at assignment so the mount and hubUrl stay well-formed regardless of how it is configured; absolute paths are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Docker e2e reaches the mock servers and the app over Docker service names, which resolve to private (RFC1918) addresses — loopback is never a legitimate egress target here. Keep only the Docker-private ranges so the suite would catch an accidental loopback-egress regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an invariant check that webSubLeaseMinSecs <= webSubLeaseDefaultSecs <= webSubLeaseMaxSecs, so a future change to the lease defaults that breaks the ordering the clamp relies on is caught even if the literal-value assertions are updated to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hub.topic (subscribe/unsubscribe) and hub.url/hub.topic (publish) were only checked for a non-empty string, so a malformed value passed parsing, got a 202, and failed later in the async fetch. Validate them as absolute URLs with the existing isAbsoluteUrl helper (matching hub.callback) so a bad URL is a synchronous 400 at parse time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
distribute() followed 3xx redirects recursively with no hop limit, so a self-redirecting callback looped the delivery task forever (one POST per hop), never settling that subscription's fan-out. Cap the chain at MAX_REDIRECTS (5); when exhausted, throw so deliver() reports a failed delivery rather than looping or silently succeeding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The in-process scheduler routed async rejections to onError but let a synchronous throw from a non-async task escape as an uncaught microtask exception. Wrap the task() call in try/catch so both error kinds reach onError, honoring the scheduler's documented non-throwing/absorb contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sendNotify() followed 3xx redirects recursively with no hop limit, so a self-redirecting subscriber callback looped forever (one POST per hop). Cap the chain at MAX_REDIRECTS (5) — matching the WebSub delivery fix — throwing when exhausted so it reports a failed notification rather than looping. This bounds both deliver() and the same-domain test-notify in verify(), which share sendNotify. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…strict A single guarded fetch (one allowlist) was injected into both the engine's topic re-fetch and every plugin's callback delivery/verification GET. So an operator exempting a private CIDR for their feeds also exempted attacker-chosen hub.callback targets in that range, reopening SSRF on the callback path. Split the guard by trust: - topic-fetch path (engine) honors WEBSUB_FETCH_ALLOW_CIDRS; - callback path (delivery + verification, both to attacker-supplied callbacks) is strict by default and honors only the new WEBSUB_CALLBACK_ALLOW_CIDRS. No @rsscloud/core change: createSafeFetch already takes a per-instance allow, so this is wiring plus a second config knob. Default config is unchanged (both empty = strict everywhere). The e2e sets both (mock topic+callback live on private Docker IPs); docs and ADR-0003 updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
distribute() interpolated hubUrl into the Link rel="hub" header unconditionally, so a plugin built without hubUrl (the type allows it — hubUrl?: string) would POST a malformed `<undefined>; rel="hub"` header to subscribers. Guard at the top of distribute and throw when hubUrl is missing so deliver() reports a failed delivery instead of emitting bad metadata; a host always injects hubUrl (see apps/server). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add scripts/docker-build-push.sh (multi-platform buildx build+push to andrewshell/rsscloud-server, version from apps/server/package.json) plus docker:build-push / docker:dry-run pnpm wrappers. Document the server port with EXPOSE 5337 and ship examples/dockge/compose.yaml for hosting, with persistence via a named volume mount rather than a Dockerfile VOLUME. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
202, and topic-based content distribution with optionalX-Hub-Signatureauthentication.