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 <yume@yumechi.jp>
This commit is contained in:
parent
4a73feb041
commit
e5d117dc98
|
@ -15,7 +15,7 @@
|
|||
|
||||
### Server
|
||||
- Fix: プロフィール追加情報で無効なURLに入力された場合に照会エラーを出るのを修正
|
||||
|
||||
- Fix: ActivityPubリクエストURLチェック実装は仕様に従っていないのを修正
|
||||
|
||||
## 2025.3.1
|
||||
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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');
|
||||
});
|
||||
|
|
Loading…
Reference in New Issue