From 58c0ac6c8986194d735071f17c008850c28b2064 Mon Sep 17 00:00:00 2001 From: dakkar Date: Mon, 21 Apr 2025 14:44:19 +0100 Subject: [PATCH 1/6] check signatures with and without query - fix #1036 @Oneric explained: > Spec says query params must be included in the signature; Mastodon > being Mastodon used to always exclude it though and for > compatibility everyone followed this. At some point GtS decided to > follow spec instead which caused interop issues, but succeeded in > getting Mastodon (and others like *oma) to accept incoming requests > with (and also still without) query params though outgoing requests > remaing query-param-free. Some still only accept query-param-less > requests though and GtS uses a retry mechanism to resend any request > failing with 401 with an query-parama-less signature once. (Also > see: > https://docs.gotosocial.org/en/latest/federation/http_signatures/ ) > > So for incoming requests both versions need to be checked. For > outgoing requests, unless you want to jump through retry hoops like > GtS, omitting query-params is the safer bet for now (presumably this > will only change if Mastodon ever decides to send out requests > signed with query params) --- .../src/server/ActivityPubServerService.ts | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index ea534af458..f40fbe8245 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -177,7 +177,7 @@ export class ActivityPubServerService { this is also inspired by FireFish's `checkFetch` */ - let signature; + let signature: httpSignature.IParsedSignature; try { signature = httpSignature.parseRequest(request.raw, { @@ -230,14 +230,38 @@ export class ActivityPubServerService { return `${logPrefix} signer is suspended: refuse`; } - let httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); + // some fedi implementations include the query (`?foo=bar`) in the + // signature, some don't, so we have to handle both cases + function verifyWithOrWithoutQuery() { + const httpSignatureValidated = httpSignature.verifySignature(signature, authUser!.key!.keyPem); + if (httpSignatureValidated) return true; + + const requestUrl = new URL(`http://whatever${request.raw.url}`); + if (! requestUrl.search) return false; + + // verification failed, the request URL contained a query, let's try without + const semiRawRequest = request.raw; + semiRawRequest.url = requestUrl.pathname; + + // no need for try/catch, if the original request parsed, this + // one will, too + const signatureWithoutQuery = httpSignature.parseRequest(semiRawRequest, { + headers: ['(request-target)', 'host', 'date'], + authorizationHeaderName: 'signature', + }); + + return httpSignature.verifySignature(signatureWithoutQuery, authUser!.key!.keyPem); + } + + console.warn('starting verification'); + let httpSignatureValidated = verifyWithOrWithoutQuery(); // maybe they changed their key? refetch it // TODO rate-limit this using lastFetchedAt if (!httpSignatureValidated) { authUser.key = await this.apDbResolverService.refetchPublicKeyForApId(authUser.user); if (authUser.key != null) { - httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); + httpSignatureValidated = verifyWithOrWithoutQuery(); } } From fda71c414727b98ed52eb6a9195b3bbdcfda9054 Mon Sep 17 00:00:00 2001 From: dakkar Date: Mon, 21 Apr 2025 14:58:22 +0100 Subject: [PATCH 2/6] make `toPuny` work better in testing --- packages/backend/src/core/UtilityService.ts | 13 ++++++++++-- packages/backend/test/unit/UtilityService.ts | 21 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/core/UtilityService.ts b/packages/backend/src/core/UtilityService.ts index 81eaa5f95d..9ed8138ffe 100644 --- a/packages/backend/src/core/UtilityService.ts +++ b/packages/backend/src/core/UtilityService.ts @@ -106,13 +106,22 @@ export class UtilityService { @bindThis public toPuny(host: string): string { - return domainToASCII(host.toLowerCase()); + // domainToASCII will return an empty string if we give it a + // string like `name:123`, but `host` may well be in that form + // (e.g. when testing locally, you'll get `localhost:3000`); split + // the port off, and add it back later + const hostParts = host.toLowerCase().match(/^(.+?)(:.+)?$/); + if (!hostParts) return ''; + const hostname = hostParts[1]; + const port = hostParts[2] ?? ''; + + return domainToASCII(hostname) + port; } @bindThis public toPunyNullable(host: string | null | undefined): string | null { if (host == null) return null; - return domainToASCII(host.toLowerCase()); + return this.toPuny(host); } @bindThis diff --git a/packages/backend/test/unit/UtilityService.ts b/packages/backend/test/unit/UtilityService.ts index d86e794f2f..cb010ff1f9 100644 --- a/packages/backend/test/unit/UtilityService.ts +++ b/packages/backend/test/unit/UtilityService.ts @@ -22,6 +22,12 @@ describe('UtilityService', () => { test('japanese', () => { assert.equal(utilityService.punyHost('http://www.新聞.com'), 'www.xn--efvv70d.com'); }); + test('simple, with port', () => { + assert.equal(utilityService.punyHost('http://www.foo.com:3000'), 'www.foo.com:3000'); + }); + test('japanese, with port', () => { + assert.equal(utilityService.punyHost('http://www.新聞.com:3000'), 'www.xn--efvv70d.com:3000'); + }); }); describe('punyHostPSLDomain', () => { @@ -31,6 +37,12 @@ describe('UtilityService', () => { test('japanese', () => { assert.equal(utilityService.punyHostPSLDomain('http://www.新聞.com'), 'xn--efvv70d.com'); }); + test('simple, with port', () => { + assert.equal(utilityService.punyHostPSLDomain('http://www.foo.com:3000'), 'foo.com:3000'); + }); + test('japanese, with port', () => { + assert.equal(utilityService.punyHostPSLDomain('http://www.新聞.com:3000'), 'xn--efvv70d.com:3000'); + }); test('lower', () => { assert.equal(utilityService.punyHostPSLDomain('http://foo.github.io'), 'foo.github.io'); assert.equal(utilityService.punyHostPSLDomain('http://foo.bar.github.io'), 'bar.github.io'); @@ -40,4 +52,13 @@ describe('UtilityService', () => { assert.equal(utilityService.punyHostPSLDomain('http://foo.bar.masto.host'), 'bar.masto.host'); }); }); + + describe('toPuny', () => { + test('without port ', () => { + assert.equal(utilityService.toPuny('www.foo.com'), 'www.foo.com'); + }); + test('with port ', () => { + assert.equal(utilityService.toPuny('www.foo.com:3000'), 'www.foo.com:3000'); + }); + }); }); From ec404fd3ce123b8c8dac84504009d8516b89599f Mon Sep 17 00:00:00 2001 From: dakkar Date: Wed, 30 Apr 2025 20:30:52 +0100 Subject: [PATCH 3/6] remove leftover debug line --- packages/backend/src/server/ActivityPubServerService.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index f40fbe8245..d27c4c9177 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -253,7 +253,6 @@ export class ActivityPubServerService { return httpSignature.verifySignature(signatureWithoutQuery, authUser!.key!.keyPem); } - console.warn('starting verification'); let httpSignatureValidated = verifyWithOrWithoutQuery(); // maybe they changed their key? refetch it From 581cc2b5137d335d010a4b05b39cd5fdefcfd160 Mon Sep 17 00:00:00 2001 From: Marie Date: Mon, 5 May 2025 13:00:31 +0000 Subject: [PATCH 4/6] remove http/https protocol --- packages/backend/src/server/api/mastodon/endpoints/instance.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/server/api/mastodon/endpoints/instance.ts b/packages/backend/src/server/api/mastodon/endpoints/instance.ts index d6ee92b466..866a3acd44 100644 --- a/packages/backend/src/server/api/mastodon/endpoints/instance.ts +++ b/packages/backend/src/server/api/mastodon/endpoints/instance.ts @@ -50,7 +50,7 @@ export class ApiInstanceMastodon { const roles = await this.roleService.getUserPolicies(me?.id ?? null); const response: MastodonEntity.Instance = { - uri: this.config.url, + uri: this.config.url.replace(/^https?:\/\//, ''), title: this.meta.name || 'Sharkey', description: this.meta.description || 'This is a vanilla Sharkey Instance. It doesn\'t seem to have a description.', email: instance.email || '', From e2be44fb99f567a1169e4dd6929ea634e1e0cf03 Mon Sep 17 00:00:00 2001 From: Marie Date: Mon, 5 May 2025 13:03:39 +0000 Subject: [PATCH 5/6] change regex to include a zero-length match --- packages/backend/src/server/api/mastodon/endpoints/instance.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/server/api/mastodon/endpoints/instance.ts b/packages/backend/src/server/api/mastodon/endpoints/instance.ts index 866a3acd44..7ed8d52313 100644 --- a/packages/backend/src/server/api/mastodon/endpoints/instance.ts +++ b/packages/backend/src/server/api/mastodon/endpoints/instance.ts @@ -50,7 +50,7 @@ export class ApiInstanceMastodon { const roles = await this.roleService.getUserPolicies(me?.id ?? null); const response: MastodonEntity.Instance = { - uri: this.config.url.replace(/^https?:\/\//, ''), + uri: this.config.url.replace(/^(https?:|)\/\//, ''), title: this.meta.name || 'Sharkey', description: this.meta.description || 'This is a vanilla Sharkey Instance. It doesn\'t seem to have a description.', email: instance.email || '', From cb3f5f598da7eaee9818e17cab98a9dd81ed90a3 Mon Sep 17 00:00:00 2001 From: Marie Date: Mon, 5 May 2025 17:33:27 +0000 Subject: [PATCH 6/6] Update instance.ts --- packages/backend/src/server/api/mastodon/endpoints/instance.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/server/api/mastodon/endpoints/instance.ts b/packages/backend/src/server/api/mastodon/endpoints/instance.ts index 7ed8d52313..6ecd36970d 100644 --- a/packages/backend/src/server/api/mastodon/endpoints/instance.ts +++ b/packages/backend/src/server/api/mastodon/endpoints/instance.ts @@ -50,7 +50,7 @@ export class ApiInstanceMastodon { const roles = await this.roleService.getUserPolicies(me?.id ?? null); const response: MastodonEntity.Instance = { - uri: this.config.url.replace(/^(https?:|)\/\//, ''), + uri: this.config.host, title: this.meta.name || 'Sharkey', description: this.meta.description || 'This is a vanilla Sharkey Instance. It doesn\'t seem to have a description.', email: instance.email || '',