Skip to content

Commit cf7c9db

Browse files
authored
Fixed actor documents mixing http and https URI schemes (#1976)
closes #1178 ## Problem An actor document could be served with mismatched URI schemes: ```json { "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 `id` comes from the stored `ap_id`, which is always created with an https scheme (`AccountService.createInternalAccount`). The `publicKey.id`/`owner` URIs, however, are generated by Fedify from the incoming request URL. That URL is only https if the reverse proxy in front of the service sends `X-Forwarded-Proto: https` — when the header is missing (or arrives as `http`, 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-Proto` is pinned to `https` at the serve boundary, before the existing `behindProxy` decorator 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 when `NODE_ENV` is exactly `staging`/`production` — so deployments with an unset or non-standard `NODE_ENV` are 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 shared `isLocalEnvironment` helper 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 of `app.ts` into a `createServeFetch(environment, fetch)` factory in `src/http/serve-fetch.ts` — `app.ts` boots the server at import time, so the env gate and decorator ordering were untestable there.
1 parent e501003 commit cf7c9db

8 files changed

Lines changed: 224 additions & 20 deletions

File tree

src/app.ts

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ import * as Sentry from '@sentry/node';
4343
import { get } from 'es-toolkit/compat';
4444
import { Hono, type Context as HonoContext, type Next } from 'hono';
4545
import { cors } from 'hono/cors';
46-
import { behindProxy } from 'x-forwarded-fetch';
4746

4847
import type { Account } from '@/account/account.entity';
4948
import { AccountBlockedEvent } from '@/account/events/account-blocked.event';
@@ -103,6 +102,7 @@ import type { FeedUpdateService } from '@/feed/feed-update.service';
103102
import type { FlagService } from '@/flag/flag.service';
104103
import type { GhostPostService } from '@/ghost/ghost-post.service';
105104
import { getTraceContext } from '@/helpers/context-header';
105+
import { isLocalEnvironment } from '@/helpers/environment';
106106
import { AccountController } from '@/http/api/account.controller';
107107
import { BlockController } from '@/http/api/block.controller';
108108
import { BlueskyController } from '@/http/api/bluesky.controller';
@@ -123,6 +123,7 @@ import type { SiteController } from '@/http/api/site.controller';
123123
import { TopicController } from '@/http/api/topic.controller';
124124
import type { WebFingerController } from '@/http/api/webfinger.controller';
125125
import type { WebhookController } from '@/http/api/webhook.controller';
126+
import { createFetchHandler } from '@/http/fetch-handler';
126127
import type { HostDataContextLoader } from '@/http/host-data-context-loader';
127128
import { createDeploymentHeadersMiddleware } from '@/http/middleware/deployment-headers';
128129
import { createHostDataContextMiddleware } from '@/http/middleware/host-data-context';
@@ -1037,16 +1038,9 @@ app.onError((err, c) => {
10371038
return c.text('Internal Server Error', 500);
10381039
});
10391040

1040-
function forceAcceptHeader(fn: (req: Request) => unknown) {
1041-
return (request: Request) => {
1042-
request.headers.set('accept', 'application/activity+json');
1043-
return fn(request);
1044-
};
1045-
}
1046-
10471041
serve(
10481042
{
1049-
fetch: forceAcceptHeader(behindProxy(app.fetch)),
1043+
fetch: createFetchHandler(process.env.NODE_ENV, app.fetch),
10501044
port: Number.parseInt(process.env.PORT || '8080', 10),
10511045
},
10521046
(info) => {
@@ -1084,13 +1078,13 @@ async function gracefulShutdown(signal: 'SIGINT' | 'SIGTERM') {
10841078
}
10851079

10861080
process.on('SIGINT', () => {
1087-
if (['development', 'testing'].includes(process.env.NODE_ENV || '')) {
1081+
if (isLocalEnvironment(process.env.NODE_ENV)) {
10881082
process.exit(0);
10891083
}
10901084
void gracefulShutdown('SIGINT');
10911085
});
10921086
process.on('SIGTERM', () => {
1093-
if (['development', 'testing'].includes(process.env.NODE_ENV || '')) {
1087+
if (isLocalEnvironment(process.env.NODE_ENV)) {
10941088
process.exit(0);
10951089
}
10961090
void gracefulShutdown('SIGTERM');

src/configuration/registrations.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ import { FeedService } from '@/feed/feed.service';
4848
import { FeedUpdateService } from '@/feed/feed-update.service';
4949
import { FlagService } from '@/flag/flag.service';
5050
import { GhostPostService } from '@/ghost/ghost-post.service';
51+
import { isLocalEnvironment } from '@/helpers/environment';
5152
import { getSiteSettings } from '@/helpers/ghost';
5253
import { AccountController } from '@/http/api/account.controller';
5354
import { BlockController } from '@/http/api/block.controller';
@@ -309,14 +310,10 @@ export function registerDependencies(
309310
circuitBreaker: false,
310311
skipSignatureVerification:
311312
process.env.SKIP_SIGNATURE_VERIFICATION === 'true' &&
312-
['development', 'testing'].includes(
313-
process.env.NODE_ENV || '',
314-
),
313+
isLocalEnvironment(process.env.NODE_ENV),
315314
allowPrivateAddress:
316315
process.env.ALLOW_PRIVATE_ADDRESS === 'true' &&
317-
['development', 'testing'].includes(
318-
process.env.NODE_ENV || '',
319-
),
316+
isLocalEnvironment(process.env.NODE_ENV),
320317
firstKnock: 'draft-cavage-http-signatures-12',
321318
});
322319
}).singleton(),

src/helpers/environment.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/**
2+
* Environments that run over plain http (local development and CI). Code that
3+
* follows the request scheme (e.g. the JWKS lookup in the role middleware)
4+
* relies on requests staying http in these environments, and the serve
5+
* boundary must not force the scheme to https for them (see
6+
* `createFetchHandler`).
7+
*
8+
* Every other environment — including an unset or unrecognised `NODE_ENV` —
9+
* is treated as being served over https, matching the canonical account URLs
10+
* which are always created with an https scheme
11+
* (`AccountService.createInternalAccount`).
12+
*/
13+
const LOCAL_ENVIRONMENTS = ['development', 'testing'];
14+
15+
export function isLocalEnvironment(environment: string | undefined): boolean {
16+
return LOCAL_ENVIRONMENTS.includes(environment || '');
17+
}
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { describe, expect, it } from 'vitest';
2+
3+
import { isLocalEnvironment } from './environment';
4+
5+
describe('isLocalEnvironment', () => {
6+
it('should return true for local environments', () => {
7+
expect(isLocalEnvironment('development')).toBe(true);
8+
expect(isLocalEnvironment('testing')).toBe(true);
9+
});
10+
11+
it('should return false for deployed environments', () => {
12+
expect(isLocalEnvironment('staging')).toBe(false);
13+
expect(isLocalEnvironment('production')).toBe(false);
14+
});
15+
16+
it('should return false when the environment is unset or unrecognised', () => {
17+
expect(isLocalEnvironment(undefined)).toBe(false);
18+
expect(isLocalEnvironment('')).toBe(false);
19+
expect(isLocalEnvironment('prod')).toBe(false);
20+
});
21+
});

src/http/fetch-handler.ts

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import { behindProxy } from 'x-forwarded-fetch';
2+
3+
import { isLocalEnvironment } from '@/helpers/environment';
4+
5+
type FetchHandler = (request: Request) => Response | Promise<Response>;
6+
7+
/**
8+
* Decorate a `fetch()` function so that the request is always treated as
9+
* having been made over https.
10+
*
11+
* Canonical URLs are always https (see `AccountEntity.draft` /
12+
* `AccountService.createInternalAccount`), but Fedify derives generated URIs
13+
* (e.g. the actor's `publicKey.id`) from the incoming request URL. A reverse
14+
* proxy that omits the `X-Forwarded-Proto` header would leave the request URL
15+
* as http, producing http URIs that contradict the stored https actor IDs and
16+
* breaking HTTP signature verification on remote servers.
17+
*
18+
* Fedify's `origin` option on `createFederation` would be the first-class way
19+
* to pin generated URIs, but it takes a single static origin — unusable here,
20+
* where the host varies per tenant.
21+
*
22+
* This must wrap a `fetch()` function already decorated with `behindProxy`
23+
* (i.e. run before it), as `behindProxy` is what applies the header to the
24+
* request URL — `createFetchHandler` owns that composition.
25+
*/
26+
function forceHttps(fetch: FetchHandler): FetchHandler {
27+
return (request: Request) => {
28+
request.headers.set('x-forwarded-proto', 'https');
29+
return fetch(request);
30+
};
31+
}
32+
33+
function forceAcceptHeader(fetch: FetchHandler): FetchHandler {
34+
return (request: Request) => {
35+
request.headers.set('accept', 'application/activity+json');
36+
return fetch(request);
37+
};
38+
}
39+
40+
/**
41+
* Build the `fetch()` function passed to `serve()`: applies `X-Forwarded-*`
42+
* headers to the request URL and forces the accept header, and — outside
43+
* local environments, which serve plain http — forces the https scheme so
44+
* generated URIs match the stored https canonical URLs regardless of proxy
45+
* configuration.
46+
*
47+
* @param environment `process.env.NODE_ENV`
48+
* @param fetch The app's `fetch()` function
49+
*/
50+
export function createFetchHandler(
51+
environment: string | undefined,
52+
fetch: FetchHandler,
53+
): FetchHandler {
54+
const proxiedFetch = behindProxy(fetch);
55+
56+
return forceAcceptHeader(
57+
isLocalEnvironment(environment)
58+
? proxiedFetch
59+
: forceHttps(proxiedFetch),
60+
);
61+
}
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
import { describe, expect, it } from 'vitest';
2+
3+
import { createFetchHandler } from './fetch-handler';
4+
5+
async function dispatch(environment: string | undefined, request: Request) {
6+
let receivedRequest: Request | undefined;
7+
8+
const fetch = createFetchHandler(environment, (request: Request) => {
9+
receivedRequest = request;
10+
return new Response();
11+
});
12+
13+
await fetch(request);
14+
15+
if (!receivedRequest) {
16+
throw new Error('Expected the wrapped fetch to be called');
17+
}
18+
19+
return receivedRequest;
20+
}
21+
22+
describe('createFetchHandler', () => {
23+
for (const environment of ['staging', 'production']) {
24+
it(`should force an https request URL in ${environment} when x-forwarded-proto is missing`, async () => {
25+
const request = await dispatch(
26+
environment,
27+
new Request('http://example.com/foo'),
28+
);
29+
30+
expect(request.url).toBe('https://example.com/foo');
31+
});
32+
33+
it(`should force an https request URL in ${environment} when x-forwarded-proto is http`, async () => {
34+
const request = await dispatch(
35+
environment,
36+
new Request('http://example.com/foo', {
37+
headers: {
38+
'x-forwarded-proto': 'http',
39+
},
40+
}),
41+
);
42+
43+
expect(request.url).toBe('https://example.com/foo');
44+
});
45+
}
46+
47+
it('should force an https request URL when NODE_ENV is unset or unrecognised', async () => {
48+
for (const environment of [undefined, '', 'prod']) {
49+
const request = await dispatch(
50+
environment,
51+
new Request('http://example.com/foo'),
52+
);
53+
54+
expect(request.url).toBe('https://example.com/foo');
55+
}
56+
});
57+
58+
for (const environment of ['development', 'testing']) {
59+
it(`should keep an http request URL in ${environment}`, async () => {
60+
const request = await dispatch(
61+
environment,
62+
new Request('http://example.com/foo'),
63+
);
64+
65+
expect(request.url).toBe('http://example.com/foo');
66+
});
67+
68+
it(`should still honour x-forwarded-proto in ${environment}`, async () => {
69+
const request = await dispatch(
70+
environment,
71+
new Request('http://example.com/foo', {
72+
headers: {
73+
'x-forwarded-proto': 'https',
74+
},
75+
}),
76+
);
77+
78+
expect(request.url).toBe('https://example.com/foo');
79+
});
80+
}
81+
82+
it('should apply x-forwarded-host to the request URL', async () => {
83+
const request = await dispatch(
84+
'production',
85+
new Request('http://internal.host/foo', {
86+
headers: {
87+
'x-forwarded-host': 'example.com',
88+
},
89+
}),
90+
);
91+
92+
expect(request.url).toBe('https://example.com/foo');
93+
});
94+
95+
it('should force the accept header in every environment', async () => {
96+
for (const environment of ['development', 'production']) {
97+
const request = await dispatch(
98+
environment,
99+
new Request('http://example.com/foo', {
100+
headers: {
101+
accept: 'text/html',
102+
},
103+
}),
104+
);
105+
106+
expect(request.headers.get('accept')).toBe(
107+
'application/activity+json',
108+
);
109+
}
110+
});
111+
});

src/http/middleware/role-guard.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import type { Context as HonoContext, Next } from 'hono';
44
import jwt from 'jsonwebtoken';
55
import jose from 'node-jose';
66

7+
import { isLocalEnvironment } from '@/helpers/environment';
8+
79
export enum GhostRole {
810
Anonymous = 'Anonymous',
911
Owner = 'Owner',
@@ -21,9 +23,9 @@ function getJwksURL(host: string, ctx: HonoContext) {
2123
const GHOST_JWKS_ENDPOINT = '/ghost/.well-known/jwks.json';
2224

2325
let protocol = 'https';
24-
// We allow insecure requests when not in production for things like testing
26+
// We allow insecure requests in local environments for things like testing
2527
if (
26-
!['staging', 'production'].includes(process.env.NODE_ENV || '') &&
28+
isLocalEnvironment(process.env.NODE_ENV) &&
2729
!ctx.req.raw.url.startsWith('https')
2830
) {
2931
protocol = 'http';

src/lookup-helpers.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { lookupWebFinger } from '@fedify/webfinger';
1010

1111
import type { FedifyContext } from '@/app';
1212
import { error, ok, type Result } from '@/core/result';
13+
import { isLocalEnvironment } from '@/helpers/environment';
1314

1415
export type LookupError = 'no-links-found' | 'no-self-link' | 'lookup-error';
1516

@@ -82,7 +83,7 @@ export async function lookupActorProfile(
8283
const webfingerData = await lookupWebFinger(resource, {
8384
allowPrivateAddress:
8485
process.env.ALLOW_PRIVATE_ADDRESS === 'true' &&
85-
['development', 'testing'].includes(process.env.NODE_ENV || ''),
86+
isLocalEnvironment(process.env.NODE_ENV),
8687
});
8788

8889
if (!webfingerData?.links) {

0 commit comments

Comments
 (0)