Fixed actor documents mixing http and https URI schemes#1976
Conversation
closes #1178 Canonical URLs are always https (internal account ap_ids are created with a hardcoded https scheme), but Fedify derives generated URIs - notably the actor's publicKey.id and owner - from the incoming request URL. When a reverse proxy in front of the service did not set the X-Forwarded-Proto header (or set it to http), the request URL stayed http and the served actor document contained http key URIs alongside the https actor id, breaking HTTP signature verification on remote servers. Since the service only ever serves https canonical URLs, the request scheme is now pinned to https at the serve boundary by forcing the X-Forwarded-Proto header before the behindProxy decorator applies it to the request URL, rather than trusting the proxy to send it.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe change adds shared environment classification for local HTTP behavior, updates JWKS URL protocol selection, and introduces an environment-aware fetch wrapper. The wrapper applies proxy URL handling, forces the ActivityPub Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b57f815f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
In local setups the service runs over plain http and the role middleware's JWKS lookup follows the request scheme, so forcing https there would break bearer-token authentication. Gate the override on the same environments in which the role middleware already requires https.
The forced https scheme was gated on NODE_ENV being exactly staging or production, but the mismatch it fixes is environment-agnostic — stored actor IDs are always https — so a deployment with NODE_ENV unset or non-standard behind a proxy that omits X-Forwarded-Proto kept serving mixed-scheme actor documents. The gate is now inverted: https is forced everywhere except the known plain-http environments (development, testing), which must keep the request scheme for things like the local JWKS lookup. The environment classification now lives in a single isLocalEnvironment helper shared with the role middleware, so the two sites the previous comment tied together by prose can no longer drift apart. The serve() fetch composition also moved out of app.ts into a createServeFetch factory: app.ts executes serve() at import time, so the env gate and decorator ordering were untestable there — the factory is unit tested through the real production composition.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/http/serve-fetch.ts (1)
24-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve TypeScript return types for fetch wrappers.
Using
unknownas the return type loses theResponse | Promise<Response>signature expected by Hono'sserve. Defining a strict type alias prevents future type mismatches and improves maintainability.♻️ Proposed refactor to preserve types
+type FetchHandler = (req: Request) => Response | Promise<Response>; + -function forceHttps(fetch: (req: Request) => unknown) { +function forceHttps(fetch: FetchHandler): FetchHandler { return (request: Request) => { request.headers.set('x-forwarded-proto', 'https'); return fetch(request); }; } -function forceAcceptHeader(fetch: (req: Request) => unknown) { +function forceAcceptHeader(fetch: FetchHandler): FetchHandler { return (request: Request) => { request.headers.set('accept', 'application/activity+json'); return fetch(request); }; } /** * Build the `fetch()` function passed to `serve()`: applies `X-Forwarded-*` * headers to the request URL and forces the accept header, and — outside * local environments, which serve plain http — forces the https scheme so * generated URIs match the stored https canonical URLs regardless of proxy * configuration. * * `@param` environment `process.env.NODE_ENV` * `@param` fetch The app's `fetch()` function */ export function createServeFetch( environment: string | undefined, - fetch: (request: Request) => Response | Promise<Response>, -) { + fetch: FetchHandler, +): FetchHandler {🤖 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 `@src/http/serve-fetch.ts` around lines 24 - 51, Update the fetch parameter and return types in forceHttps and forceAcceptHeader to preserve the Response | Promise<Response> contract used by createServeFetch and Hono’s serve; replace the unknown wrapper return typing with a shared strict fetch-handler type, and apply it consistently to both wrappers without changing their header behavior.
🤖 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 `@src/http/middleware/role-guard.ts`:
- Around line 26-28: Update the host extraction in the role middleware around
the JWKS URL construction to derive the host from the proxy-resolved request URL
via the request object, rather than reading the raw Host header. Use the
existing ctx.req/request flow so tenant context and JWKS validation share the
same resolved host.
---
Nitpick comments:
In `@src/http/serve-fetch.ts`:
- Around line 24-51: Update the fetch parameter and return types in forceHttps
and forceAcceptHeader to preserve the Response | Promise<Response> contract used
by createServeFetch and Hono’s serve; replace the unknown wrapper return typing
with a shared strict fetch-handler type, and apply it consistently to both
wrappers without changing their header behavior.
🪄 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
Run ID: fbb62cdf-9785-48c3-a829-c3284fe078ad
📒 Files selected for processing (6)
src/app.tssrc/helpers/environment.tssrc/helpers/environment.unit.test.tssrc/http/middleware/role-guard.tssrc/http/serve-fetch.tssrc/http/serve-fetch.unit.test.ts
A shared FetchHandler alias preserves the Response contract through the decorator composition instead of widening to unknown.
|
@sagzy So this means |
The factory builds the fetch handler passed to serve(); naming it after what it produces reads better than naming it after the call site. The module moves to fetch-handler.ts to match.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/fetch-handler.unit.test.ts (1)
5-20: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTest environment
Requestdiffers from productionRequestregarding header mutability.Because this test manually instantiates
new Request(...), theheadersobject has anoneguard according to the Fetch API spec, allowingrequest.headers.set(...)to succeed. However, an incomingRequestprovided by the server in production environments will have animmutableguard, which will causerequest.headers.set(...)inforceHttpsandforceAcceptHeaderto throw aTypeError.You should consider updating the implementation to create a new
Requestinstance (or a clone) to ensure it does not crash on immutable incoming requests.🤖 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 `@src/http/fetch-handler.unit.test.ts` around lines 5 - 20, Update the request handling used by forceHttps and forceAcceptHeader to avoid mutating the incoming Request headers directly; construct a new Request or clone the request with the required header changes, while preserving the existing request data and behavior for mutable test Requests.
🤖 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 `@src/http/fetch-handler.ts`:
- Around line 26-31: Clone the incoming Request before modifying headers in
forceHttps and forceAcceptHeader. For both sites in src/http/fetch-handler.ts
(lines 26-31 and 33-38), create a new request with cloned Headers, update the
clone, and pass it to fetch instead of mutating the original request.
---
Outside diff comments:
In `@src/http/fetch-handler.unit.test.ts`:
- Around line 5-20: Update the request handling used by forceHttps and
forceAcceptHeader to avoid mutating the incoming Request headers directly;
construct a new Request or clone the request with the required header changes,
while preserving the existing request data and behavior for mutable test
Requests.
🪄 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
Run ID: cf7c4199-5895-48f4-a072-e921b7b83270
📒 Files selected for processing (4)
src/app.tssrc/helpers/environment.tssrc/http/fetch-handler.tssrc/http/fetch-handler.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/helpers/environment.ts
Correct, with the fix, self-hosters don't need to send the |
The isLocalEnvironment helper was introduced to give the serve boundary and the role middleware a single definition of "local", so the two couldn't drift. But five other sites still open-coded the same ['development', 'testing'] list — the SIGINT/SIGTERM fast-exit, the Fedify skipSignatureVerification and allowPrivateAddress guards, and the WebFinger allowPrivateAddress guard. Left as-is, adding an environment to the helper's list would change the request scheme and JWKS behavior while silently leaving signature verification and private-address allowance on the old definition — reproducing exactly the scheme/verification mismatch this work set out to prevent. Each replacement is behaviour-identical, so this is a pure refactor that makes the helper the sole source of the classification.
2 participants
closes #1178
Problem
An actor document could be served with mismatched URI schemes:
{ "id": "https://ghost.example/.ghost/activitypub/users/index", "publicKey": { "id": "http://ghost.example/.ghost/activitypub/users/index#main-key", "owner": "http://ghost.example/.ghost/activitypub/users/index", ... } }The actor
idcomes from the storedap_id, which is always created with an https scheme (AccountService.createInternalAccount). ThepublicKey.id/ownerURIs, however, are generated by Fedify from the incoming request URL. That URL is only https if the reverse proxy in front of the service sendsX-Forwarded-Proto: https— when the header is missing (or arrives ashttp, e.g. Caddy behind another TLS-terminating proxy, as reported in the issue), the served document mixes http key URIs with an https actor id, and remote servers fail HTTP signature verification against the actor.Solution
The service only ever serves https canonical URLs, so the request scheme shouldn't be trusted from the proxy at all.
X-Forwarded-Protois pinned tohttpsat the serve boundary, before the existingbehindProxydecorator applies it to the request URL — so every generated URI matches the stored https actor IDs regardless of proxy configuration.Because the mismatch is environment-agnostic (stored actor IDs are https in every environment), the scheme is forced in every environment except the known plain-http ones (
development,testing) — rather than only whenNODE_ENVis exactlystaging/production— so deployments with an unset or non-standardNODE_ENVare covered too. Local environments must keep the request scheme for things like the JWKS lookup in the role middleware; that classification now lives in a single sharedisLocalEnvironmenthelper used by both the serve boundary and the role middleware, so the two can't drift apart.The whole
serve()fetch composition (accept-header forcing, https forcing,behindProxy) moved out ofapp.tsinto acreateServeFetch(environment, fetch)factory insrc/http/serve-fetch.ts—app.tsboots the server at import time, so the env gate and decorator ordering were untestable there.Verification
createServeFetchthrough the real production composition: https forcing instaging/productionand for unset/unrecognisedNODE_ENV, scheme preservation (withX-Forwarded-Protostill honoured) indevelopment/testing,X-Forwarded-Hosthandling, and the forced accept header./.ghost/activitypub/users/indexwith noX-Forwarded-Protoheader now returnspublicKey.idandownerwith the https scheme, matching the actorid.pnpm lint,pnpm test:typesand the full unit suite (75 files, 957 tests) pass.