From 25052164c0971497f9177f88446b110e8ca91ce2 Mon Sep 17 00:00:00 2001 From: "饺子w (Yumechi)" <35571479+eternal-flame-AD@users.noreply.github.com> Date: Sun, 23 Feb 2025 04:21:34 -0600 Subject: Merge commit from fork * fix(backend): Fix an issue where the origin of ActivityPub lookup response was not validated correctly. [GHSA-6w2c-vf6f-xf26](https://github.com/misskey-dev/misskey/security/advisories/GHSA-6w2c-vf6f-xf26) Signed-off-by: eternal-flame-AD * Enhance: Add configuration option to disable all external redirects when responding to an ActivityPub lookup (config.disallowExternalApRedirect) Signed-off-by: eternal-flame-AD * fixup! fix(backend): Fix an issue where the origin of ActivityPub lookup response was not validated correctly. * docs & one edge case Signed-off-by: eternal-flame-AD * apply suggestions Signed-off-by: eternal-flame-AD * remove stale frontend reference to _responseInvalidIdHostNotMatch Signed-off-by: eternal-flame-AD * apply suggestions Signed-off-by: eternal-flame-AD --------- Signed-off-by: eternal-flame-AD --- packages/backend/src/core/HttpRequestService.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'packages/backend/src/core/HttpRequestService.ts') diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 083153940a..8085bbf961 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -16,7 +16,7 @@ import type { Config } from '@/config.js'; import { StatusError } from '@/misc/status-error.js'; import { bindThis } from '@/decorators.js'; import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js'; -import { assertActivityMatchesUrls } from '@/core/activitypub/misc/check-against-url.js'; +import { assertActivityMatchesUrls, FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js'; import type { IObject } from '@/core/activitypub/type.js'; import type { Response } from 'node-fetch'; import type { URL } from 'node:url'; @@ -215,7 +215,7 @@ export class HttpRequestService { } @bindThis - public async getActivityJson(url: string, isLocalAddressAllowed = false): Promise { + public async getActivityJson(url: string, isLocalAddressAllowed = false, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise { const res = await this.send(url, { method: 'GET', headers: { @@ -232,7 +232,7 @@ export class HttpRequestService { const finalUrl = res.url; // redirects may have been involved const activity = await res.json() as IObject; - assertActivityMatchesUrls(activity, [finalUrl]); + assertActivityMatchesUrls(url, activity, [finalUrl], allowSoftfail); return activity; } -- cgit v1.2.3-freya From 495db2743318d43961f2a7fea0084ccae1cd9dea Mon Sep 17 00:00:00 2001 From: おさむのひと <46447427+samunohito@users.noreply.github.com> Date: Wed, 26 Feb 2025 10:48:38 +0900 Subject: fix(backend): カスタム絵文字の一括インポートをした時にHTTPプロキシの除外設定が効かないのを修正 (#15431) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * pxory * fix * fix CHANGELOG.md * allow localAddress --------- Co-authored-by: syuilo <4439005+syuilo@users.noreply.github.com> --- CHANGELOG.md | 1 + packages/backend/src/core/DownloadService.ts | 4 +-- packages/backend/src/core/HttpRequestService.ts | 47 +++++++++++++++++++++---- 3 files changed, 43 insertions(+), 9 deletions(-) (limited to 'packages/backend/src/core/HttpRequestService.ts') diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b5579db4c..8ffce0beee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ - Enhance: 成り済まし対策として、ActivityPub照会された時にリモートのリダイレクトを拒否できるように (config.disallowExternalApRedirect) - Fix: `following/invalidate`でフォロワーを解除しようとしているユーザーの情報を返すように - Fix: オブジェクトストレージの設定でPrefixを設定していなかった場合nullまたは空文字になる問題を修正 +- Fix: HTTPプロキシとその除外設定を行った状態でカスタム絵文字の一括インポートをしたとき、除外設定が効かないのを修正( #8766 ) - Fix: pgroongaでの検索時にはじめのキーワードのみが検索に使用される問題を修正 (Cherry-picked from https://activitypub.software/TransFem-org/Sharkey/-/merge_requests/886) - Fix: メールアドレスの形式が正しくなければ以降の処理を行わないように diff --git a/packages/backend/src/core/DownloadService.ts b/packages/backend/src/core/DownloadService.ts index 2e78e6d877..a2b74d1ab2 100644 --- a/packages/backend/src/core/DownloadService.ts +++ b/packages/backend/src/core/DownloadService.ts @@ -60,8 +60,8 @@ export class DownloadService { request: operationTimeout, // whole operation timeout }, agent: { - http: this.httpRequestService.httpAgent, - https: this.httpRequestService.httpsAgent, + http: this.httpRequestService.getAgentForHttp(urlObj, true), + https: this.httpRequestService.getAgentForHttps(urlObj, true), }, http2: false, // default retry: { diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 8085bbf961..13d8f7f43b 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -115,32 +115,32 @@ export class HttpRequestService { /** * Get http non-proxy agent (without local address filtering) */ - private httpNative: http.Agent; + private readonly httpNative: http.Agent; /** * Get https non-proxy agent (without local address filtering) */ - private httpsNative: https.Agent; + private readonly httpsNative: https.Agent; /** * Get http non-proxy agent */ - private http: http.Agent; + private readonly http: http.Agent; /** * Get https non-proxy agent */ - private https: https.Agent; + private readonly https: https.Agent; /** * Get http proxy or non-proxy agent */ - public httpAgent: http.Agent; + public readonly httpAgent: http.Agent; /** * Get https proxy or non-proxy agent */ - public httpsAgent: https.Agent; + public readonly httpsAgent: https.Agent; constructor( @Inject(DI.config) @@ -197,7 +197,8 @@ export class HttpRequestService { /** * Get agent by URL * @param url URL - * @param bypassProxy Allways bypass proxy + * @param bypassProxy Always bypass proxy + * @param isLocalAddressAllowed */ @bindThis public getAgentByUrl(url: URL, bypassProxy = false, isLocalAddressAllowed = false): http.Agent | https.Agent { @@ -214,6 +215,38 @@ export class HttpRequestService { } } + /** + * Get agent for http by URL + * @param url URL + * @param isLocalAddressAllowed + */ + @bindThis + public getAgentForHttp(url: URL, isLocalAddressAllowed = false): http.Agent { + if ((this.config.proxyBypassHosts ?? []).includes(url.hostname)) { + return isLocalAddressAllowed + ? this.httpNative + : this.http; + } else { + return this.httpAgent; + } + } + + /** + * Get agent for https by URL + * @param url URL + * @param isLocalAddressAllowed + */ + @bindThis + public getAgentForHttps(url: URL, isLocalAddressAllowed = false): https.Agent { + if ((this.config.proxyBypassHosts ?? []).includes(url.hostname)) { + return isLocalAddressAllowed + ? this.httpsNative + : this.https; + } else { + return this.httpsAgent; + } + } + @bindThis public async getActivityJson(url: string, isLocalAddressAllowed = false, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise { const res = await this.send(url, { -- cgit v1.2.3-freya From e5d117dc98b725f908402638754f8643bbbeef90 Mon Sep 17 00:00:00 2001 From: "饺子w (Yumechi)" <35571479+eternal-flame-AD@users.noreply.github.com> Date: Wed, 12 Mar 2025 12:39:24 +0000 Subject: fix(backend): tighten an overly relaxed criteria and remove capability of matching multiple final URLs in URL authority checking (#15655) Signed-off-by: eternal-flame-AD --- CHANGELOG.md | 2 +- packages/backend/src/core/HttpRequestService.ts | 4 +- .../src/core/activitypub/ApRequestService.ts | 4 +- .../src/core/activitypub/misc/check-against-url.ts | 22 ++++--- packages/backend/test/unit/ap-request.ts | 75 +++++++++------------- 5 files changed, 49 insertions(+), 58 deletions(-) (limited to 'packages/backend/src/core/HttpRequestService.ts') diff --git a/CHANGELOG.md b/CHANGELOG.md index d02b37cbdb..a8b7f47a29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ ### Server - Fix: プロフィール追加情報で無効なURLに入力された場合に照会エラーを出るのを修正 - +- Fix: ActivityPubリクエストURLチェック実装は仕様に従っていないのを修正 ## 2025.3.1 diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 13d8f7f43b..3ddfe52045 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -16,7 +16,7 @@ import type { Config } from '@/config.js'; import { StatusError } from '@/misc/status-error.js'; import { bindThis } from '@/decorators.js'; import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js'; -import { assertActivityMatchesUrls, FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js'; +import { assertActivityMatchesUrl, FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js'; import type { IObject } from '@/core/activitypub/type.js'; import type { Response } from 'node-fetch'; import type { URL } from 'node:url'; @@ -265,7 +265,7 @@ export class HttpRequestService { const finalUrl = res.url; // redirects may have been involved const activity = await res.json() as IObject; - assertActivityMatchesUrls(url, activity, [finalUrl], allowSoftfail); + assertActivityMatchesUrl(url, activity, finalUrl, allowSoftfail); return activity; } diff --git a/packages/backend/src/core/activitypub/ApRequestService.ts b/packages/backend/src/core/activitypub/ApRequestService.ts index 6c29cce325..61d328ccac 100644 --- a/packages/backend/src/core/activitypub/ApRequestService.ts +++ b/packages/backend/src/core/activitypub/ApRequestService.ts @@ -17,7 +17,7 @@ import { LoggerService } from '@/core/LoggerService.js'; import { bindThis } from '@/decorators.js'; import type Logger from '@/logger.js'; import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js'; -import { assertActivityMatchesUrls, FetchAllowSoftFailMask as FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js'; +import { assertActivityMatchesUrl, FetchAllowSoftFailMask as FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js'; import type { IObject } from './type.js'; type Request = { @@ -258,7 +258,7 @@ export class ApRequestService { const finalUrl = res.url; // redirects may have been involved const activity = await res.json() as IObject; - assertActivityMatchesUrls(url, activity, [finalUrl], allowSoftfail); + assertActivityMatchesUrl(url, activity, finalUrl, allowSoftfail); return activity; } diff --git a/packages/backend/src/core/activitypub/misc/check-against-url.ts b/packages/backend/src/core/activitypub/misc/check-against-url.ts index dfcfb1943e..bbfe57f9fa 100644 --- a/packages/backend/src/core/activitypub/misc/check-against-url.ts +++ b/packages/backend/src/core/activitypub/misc/check-against-url.ts @@ -75,7 +75,7 @@ function normalizeSynonymousSubdomain(url: URL | string): URL { return new URL(urlParsed.toString().replace(host, normalizedHost)); } -export function assertActivityMatchesUrls(requestUrl: string | URL, activity: IObject, candidateUrls: (string | URL)[], allowSoftfail: FetchAllowSoftFailMask): FetchAllowSoftFailMask { +export function assertActivityMatchesUrl(requestUrl: string | URL, activity: IObject, finalUrl: string | URL, allowSoftfail: FetchAllowSoftFailMask): FetchAllowSoftFailMask { // must have a unique identifier to verify authority if (!activity.id) { throw new Error('bad Activity: missing id field'); @@ -95,26 +95,32 @@ export function assertActivityMatchesUrls(requestUrl: string | URL, activity: IO const requestUrlParsed = normalizeSynonymousSubdomain(requestUrl); const idParsed = normalizeSynonymousSubdomain(activity.id); - const candidateUrlsParsed = candidateUrls.map(it => normalizeSynonymousSubdomain(it)); + const finalUrlParsed = normalizeSynonymousSubdomain(finalUrl); + + // mastodon sends activities with hash in the URL + // currently it only happens with likes, deletes etc. + // but object ID never has hash + requestUrlParsed.hash = ''; + finalUrlParsed.hash = ''; const requestUrlSecure = requestUrlParsed.protocol === 'https:'; - const finalUrlSecure = candidateUrlsParsed.every(it => it.protocol === 'https:'); + const finalUrlSecure = finalUrlParsed.protocol === 'https:'; if (requestUrlSecure && !finalUrlSecure) { throw new Error(`bad Activity: id(${activity.id}) is not allowed to have http:// in the url`); } // Compare final URL to the ID - if (!candidateUrlsParsed.some(it => it.href === idParsed.href)) { - requireSoftfail(FetchAllowSoftFailMask.NonCanonicalId, `bad Activity: id(${activity.id}) does not match response url(${candidateUrlsParsed.map(it => it.toString())})`); + if (finalUrlParsed.href !== idParsed.href) { + requireSoftfail(FetchAllowSoftFailMask.NonCanonicalId, `bad Activity: id(${activity.id}) does not match response url(${finalUrlParsed.toString()})`); // at lease host need to match exactly (ActivityPub requirement) - if (!candidateUrlsParsed.some(it => idParsed.host === it.host)) { - throw new Error(`bad Activity: id(${activity.id}) does not match response host(${candidateUrlsParsed.map(it => it.host)})`); + if (idParsed.host !== finalUrlParsed.host) { + throw new Error(`bad Activity: id(${activity.id}) does not match response host(${finalUrlParsed.host})`); } } // Compare request URL to the ID - if (!requestUrlParsed.href.includes(idParsed.href)) { + if (requestUrlParsed.href !== idParsed.href) { requireSoftfail(FetchAllowSoftFailMask.NonCanonicalId, `bad Activity: id(${activity.id}) does not match request url(${requestUrlParsed.toString()})`); // if cross-origin lookup is allowed, we can accept some variation between the original request URL to the final object ID (but not between the final URL and the object ID) diff --git a/packages/backend/test/unit/ap-request.ts b/packages/backend/test/unit/ap-request.ts index 0426de8e19..f8b2a697f2 100644 --- a/packages/backend/test/unit/ap-request.ts +++ b/packages/backend/test/unit/ap-request.ts @@ -8,7 +8,7 @@ import httpSignature from '@peertube/http-signature'; import { genRsaKeyPair } from '@/misc/gen-key-pair.js'; import { ApRequestCreator } from '@/core/activitypub/ApRequestService.js'; -import { assertActivityMatchesUrls, FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js'; +import { assertActivityMatchesUrl, FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js'; import { IObject } from '@/core/activitypub/type.js'; export const buildParsedSignature = (signingString: string, signature: string, algorithm: string) => { @@ -66,23 +66,26 @@ describe('ap-request', () => { }); test('rejects non matching domain', () => { - assert.doesNotThrow(() => assertActivityMatchesUrls( + assert.doesNotThrow(() => assertActivityMatchesUrl( 'https://alice.example.com/abc', { id: 'https://alice.example.com/abc' } as IObject, - [ - 'https://alice.example.com/abc', - ], + 'https://alice.example.com/abc', FetchAllowSoftFailMask.Strict, ), 'validation should pass base case'); - assert.throws(() => assertActivityMatchesUrls( + assert.throws(() => assertActivityMatchesUrl( 'https://alice.example.com/abc', { id: 'https://bob.example.com/abc' } as IObject, - [ - 'https://alice.example.com/abc', - ], + 'https://alice.example.com/abc', FetchAllowSoftFailMask.Any, ), 'validation should fail no matter what if the response URL is inconsistent with the object ID'); + assert.doesNotThrow(() => assertActivityMatchesUrl( + 'https://alice.example.com/abc#test', + { id: 'https://alice.example.com/abc' } as IObject, + 'https://alice.example.com/abc', + FetchAllowSoftFailMask.Strict, + ), 'validation should pass with hash in request URL'); + // fix issues like threads // https://github.com/misskey-dev/misskey/issues/15039 const withOrWithoutWWW = [ @@ -97,89 +100,71 @@ describe('ap-request', () => { ), withOrWithoutWWW, ).forEach(([[a, b], c]) => { - assert.doesNotThrow(() => assertActivityMatchesUrls( + assert.doesNotThrow(() => assertActivityMatchesUrl( a, { id: b } as IObject, - [ - c, - ], + c, FetchAllowSoftFailMask.Strict, ), 'validation should pass with or without www. subdomain'); }); }); test('cross origin lookup', () => { - assert.doesNotThrow(() => assertActivityMatchesUrls( + assert.doesNotThrow(() => assertActivityMatchesUrl( 'https://alice.example.com/abc', { id: 'https://bob.example.com/abc' } as IObject, - [ - 'https://bob.example.com/abc', - ], + 'https://bob.example.com/abc', FetchAllowSoftFailMask.CrossOrigin | FetchAllowSoftFailMask.NonCanonicalId, ), 'validation should pass if the response is otherwise consistent and cross-origin is allowed'); - assert.throws(() => assertActivityMatchesUrls( + assert.throws(() => assertActivityMatchesUrl( 'https://alice.example.com/abc', { id: 'https://bob.example.com/abc' } as IObject, - [ - 'https://bob.example.com/abc', - ], + 'https://bob.example.com/abc', FetchAllowSoftFailMask.Strict, ), 'validation should fail if the response is otherwise consistent and cross-origin is not allowed'); }); test('rejects non-canonical ID', () => { - assert.throws(() => assertActivityMatchesUrls( + assert.throws(() => assertActivityMatchesUrl( 'https://alice.example.com/@alice', { id: 'https://alice.example.com/users/alice' } as IObject, - [ - 'https://alice.example.com/users/alice' - ], + 'https://alice.example.com/users/alice', FetchAllowSoftFailMask.Strict, ), 'throws if the response ID did not exactly match the expected ID'); - assert.doesNotThrow(() => assertActivityMatchesUrls( + assert.doesNotThrow(() => assertActivityMatchesUrl( 'https://alice.example.com/@alice', { id: 'https://alice.example.com/users/alice' } as IObject, - [ - 'https://alice.example.com/users/alice', - ], + 'https://alice.example.com/users/alice', FetchAllowSoftFailMask.NonCanonicalId, ), 'does not throw if non-canonical ID is allowed'); }); test('origin relaxed alignment', () => { - assert.doesNotThrow(() => assertActivityMatchesUrls( + assert.doesNotThrow(() => assertActivityMatchesUrl( 'https://alice.example.com/abc', { id: 'https://ap.alice.example.com/abc' } as IObject, - [ - 'https://ap.alice.example.com/abc', - ], + 'https://ap.alice.example.com/abc', FetchAllowSoftFailMask.MisalignedOrigin | FetchAllowSoftFailMask.NonCanonicalId, ), 'validation should pass if response is a subdomain of the expected origin'); - assert.throws(() => assertActivityMatchesUrls( + assert.throws(() => assertActivityMatchesUrl( 'https://alice.multi-tenant.example.com/abc', { id: 'https://alice.multi-tenant.example.com/abc' } as IObject, - [ - 'https://bob.multi-tenant.example.com/abc', - ], + 'https://bob.multi-tenant.example.com/abc', FetchAllowSoftFailMask.MisalignedOrigin | FetchAllowSoftFailMask.NonCanonicalId, ), 'validation should fail if response is a disjoint domain of the expected origin'); - assert.throws(() => assertActivityMatchesUrls( + assert.throws(() => assertActivityMatchesUrl( 'https://alice.example.com/abc', { id: 'https://ap.alice.example.com/abc' } as IObject, - [ - 'https://ap.alice.example.com/abc', - ], + 'https://ap.alice.example.com/abc', FetchAllowSoftFailMask.Strict, ), 'throws if relaxed origin is forbidden'); }); test('resist HTTP downgrade', () => { - assert.throws(() => assertActivityMatchesUrls( + assert.throws(() => assertActivityMatchesUrl( 'https://alice.example.com/abc', { id: 'https://alice.example.com/abc' } as IObject, - [ - 'http://alice.example.com/abc', - ], + 'http://alice.example.com/abc', FetchAllowSoftFailMask.Strict, ), 'throws if HTTP downgrade is detected'); }); -- cgit v1.2.3-freya