From b5c361089f03f4d459fa1cdc49ff66dee736af12 Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Tue, 9 Mar 2021 14:01:44 +0100 Subject: [PATCH] Fix 404 AP status codes --- server/helpers/requests.ts | 18 +++-- server/lib/activitypub/actor.ts | 22 +++--- server/lib/activitypub/playlist.ts | 17 +++-- server/lib/activitypub/videos.ts | 19 ++--- .../job-queue/handlers/activitypub-cleaner.ts | 51 +++++++------ server/tests/api/activitypub/security.ts | 73 ++++++++++++------- server/tools/peertube-import-videos.ts | 5 +- 7 files changed, 119 insertions(+), 86 deletions(-) diff --git a/server/helpers/requests.ts b/server/helpers/requests.ts index aee8f6673..5eb69486d 100644 --- a/server/helpers/requests.ts +++ b/server/helpers/requests.ts @@ -1,5 +1,5 @@ import { createWriteStream, remove } from 'fs-extra' -import got, { CancelableRequest, Options as GotOptions } from 'got' +import got, { CancelableRequest, Options as GotOptions, RequestError } from 'got' import { join } from 'path' import { CONFIG } from '../initializers/config' import { ACTIVITY_PUB, PEERTUBE_VERSION, WEBSERVER } from '../initializers/constants' @@ -7,6 +7,11 @@ import { pipelinePromise } from './core-utils' import { processImage } from './image-utils' import { logger } from './logger' +export interface PeerTubeRequestError extends Error { + statusCode?: number + responseBody?: any +} + const httpSignature = require('http-signature') type PeerTubeRequestOptions = { @@ -180,14 +185,15 @@ function buildGotOptions (options: PeerTubeRequestOptions) { } } -function buildRequestError (error: any) { - const newError = new Error(error.message) +function buildRequestError (error: RequestError) { + const newError: PeerTubeRequestError = new Error(error.message) newError.name = error.name newError.stack = error.stack - if (error.response?.body) { - error.responseBody = error.response.body + if (error.response) { + newError.responseBody = error.response.body + newError.statusCode = error.response.statusCode } - return error + return newError } diff --git a/server/lib/activitypub/actor.ts b/server/lib/activitypub/actor.ts index 52b6c1f56..3c9a7ba02 100644 --- a/server/lib/activitypub/actor.ts +++ b/server/lib/activitypub/actor.ts @@ -14,7 +14,7 @@ import { isActivityPubUrlValid } from '../../helpers/custom-validators/activityp import { retryTransactionWrapper, updateInstanceWithAnother } from '../../helpers/database-utils' import { logger } from '../../helpers/logger' import { createPrivateAndPublicKeys } from '../../helpers/peertube-crypto' -import { doJSONRequest } from '../../helpers/requests' +import { doJSONRequest, PeerTubeRequestError } from '../../helpers/requests' import { getUrlFromWebfinger } from '../../helpers/webfinger' import { MIMETYPES, WEBSERVER } from '../../initializers/constants' import { sequelizeTypescript } from '../../initializers/database' @@ -279,16 +279,7 @@ async function refreshActorIfNeeded ( updater: (url: string, newUrl: string) => Promise, deleter: (url: string) => Promise ): Promise<{ data: T, status: 'deleted' | 'updated' } | null> { - const { statusCode, body } = await doJSONRequest(url, { activityPub: true }) - - // Does not exist anymore, remove entry - if (statusCode === HttpStatusCode.NOT_FOUND_404) { + const on404OrTombstone = async () => { logger.info('Removing remote AP object %s.', url) const data = await deleter(url) - return { status: 'deleted', data } + return { status: 'deleted' as 'deleted', data } } - // If not same id, check same host and update - if (!body || !body.id || !bodyValidator(body)) throw new Error(`Body or body id of ${url} is invalid`) + try { + const { body } = await doJSONRequest(url, { activityPub: true }) - if (body.type === 'Tombstone') { - logger.info('Removing remote AP object %s.', url) - const data = await deleter(url) + // If not same id, check same host and update + if (!body || !body.id || !bodyValidator(body)) throw new Error(`Body or body id of ${url} is invalid`) - return { status: 'deleted', data } - } - - const newUrl = body.id - if (newUrl !== url) { - if (checkUrlsSameHost(newUrl, url) !== true) { - throw new Error(`New url ${newUrl} has not the same host than old url ${url}`) + if (body.type === 'Tombstone') { + return on404OrTombstone() } - logger.info('Updating remote AP object %s.', url) - const data = await updater(url, newUrl) + const newUrl = body.id + if (newUrl !== url) { + if (checkUrlsSameHost(newUrl, url) !== true) { + throw new Error(`New url ${newUrl} has not the same host than old url ${url}`) + } - return { status: 'updated', data } + logger.info('Updating remote AP object %s.', url) + const data = await updater(url, newUrl) + + return { status: 'updated', data } + } + + return null + } catch (err) { + // Does not exist anymore, remove entry + if ((err as PeerTubeRequestError).statusCode === HttpStatusCode.NOT_FOUND_404) { + return on404OrTombstone() + } + + throw err } - - return null } function rateOptionsFactory () { diff --git a/server/tests/api/activitypub/security.ts b/server/tests/api/activitypub/security.ts index 26b4545ac..9745052a3 100644 --- a/server/tests/api/activitypub/security.ts +++ b/server/tests/api/activitypub/security.ts @@ -79,9 +79,12 @@ describe('Test ActivityPub security', function () { Digest: buildDigest({ hello: 'coucou' }) } - const { statusCode } = await makePOSTAPRequest(url, body, baseHttpSignature(), headers) - - expect(statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + try { + await makePOSTAPRequest(url, body, baseHttpSignature(), headers) + expect(true, 'Did not throw').to.be.false + } catch (err) { + expect(err.statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + } }) it('Should fail with an invalid date', async function () { @@ -89,9 +92,12 @@ describe('Test ActivityPub security', function () { const headers = buildGlobalHeaders(body) headers['date'] = 'Wed, 21 Oct 2015 07:28:00 GMT' - const { statusCode } = await makePOSTAPRequest(url, body, baseHttpSignature(), headers) - - expect(statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + try { + await makePOSTAPRequest(url, body, baseHttpSignature(), headers) + expect(true, 'Did not throw').to.be.false + } catch (err) { + expect(err.statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + } }) it('Should fail with bad keys', async function () { @@ -101,9 +107,12 @@ describe('Test ActivityPub security', function () { const body = activityPubContextify(getAnnounceWithoutContext(servers[1])) const headers = buildGlobalHeaders(body) - const { statusCode } = await makePOSTAPRequest(url, body, baseHttpSignature(), headers) - - expect(statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + try { + await makePOSTAPRequest(url, body, baseHttpSignature(), headers) + expect(true, 'Did not throw').to.be.false + } catch (err) { + expect(err.statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + } }) it('Should reject requests without appropriate signed headers', async function () { @@ -123,8 +132,12 @@ describe('Test ActivityPub security', function () { for (const badHeaders of badHeadersMatrix) { signatureOptions.headers = badHeaders - const { statusCode } = await makePOSTAPRequest(url, body, signatureOptions, headers) - expect(statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + try { + await makePOSTAPRequest(url, body, signatureOptions, headers) + expect(true, 'Did not throw').to.be.false + } catch (err) { + expect(err.statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + } } }) @@ -133,7 +146,6 @@ describe('Test ActivityPub security', function () { const headers = buildGlobalHeaders(body) const { statusCode } = await makePOSTAPRequest(url, body, baseHttpSignature(), headers) - expect(statusCode).to.equal(HttpStatusCode.NO_CONTENT_204) }) @@ -150,9 +162,12 @@ describe('Test ActivityPub security', function () { const body = activityPubContextify(getAnnounceWithoutContext(servers[1])) const headers = buildGlobalHeaders(body) - const { statusCode } = await makePOSTAPRequest(url, body, baseHttpSignature(), headers) - - expect(statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + try { + await makePOSTAPRequest(url, body, baseHttpSignature(), headers) + expect(true, 'Did not throw').to.be.false + } catch (err) { + expect(err.statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + } }) }) @@ -183,9 +198,12 @@ describe('Test ActivityPub security', function () { const headers = buildGlobalHeaders(signedBody) - const { statusCode } = await makePOSTAPRequest(url, signedBody, baseHttpSignature(), headers) - - expect(statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + try { + await makePOSTAPRequest(url, signedBody, baseHttpSignature(), headers) + expect(true, 'Did not throw').to.be.false + } catch (err) { + expect(err.statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + } }) it('Should fail with an altered body', async function () { @@ -204,9 +222,12 @@ describe('Test ActivityPub security', function () { const headers = buildGlobalHeaders(signedBody) - const { statusCode } = await makePOSTAPRequest(url, signedBody, baseHttpSignature(), headers) - - expect(statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + try { + await makePOSTAPRequest(url, signedBody, baseHttpSignature(), headers) + expect(true, 'Did not throw').to.be.false + } catch (err) { + expect(err.statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + } }) it('Should succeed with a valid signature', async function () { @@ -221,7 +242,6 @@ describe('Test ActivityPub security', function () { const headers = buildGlobalHeaders(signedBody) const { statusCode } = await makePOSTAPRequest(url, signedBody, baseHttpSignature(), headers) - expect(statusCode).to.equal(HttpStatusCode.NO_CONTENT_204) }) @@ -243,9 +263,12 @@ describe('Test ActivityPub security', function () { const headers = buildGlobalHeaders(signedBody) - const { statusCode } = await makePOSTAPRequest(url, signedBody, baseHttpSignature(), headers) - - expect(statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + try { + await makePOSTAPRequest(url, signedBody, baseHttpSignature(), headers) + expect(true, 'Did not throw').to.be.false + } catch (err) { + expect(err.statusCode).to.equal(HttpStatusCode.FORBIDDEN_403) + } }) }) diff --git a/server/tools/peertube-import-videos.ts b/server/tools/peertube-import-videos.ts index 9be0834ba..915995031 100644 --- a/server/tools/peertube-import-videos.ts +++ b/server/tools/peertube-import-videos.ts @@ -202,10 +202,7 @@ async function uploadVideoOnPeerTube (parameters: { if (videoInfo.thumbnail) { thumbnailfile = join(cwd, sha256(videoInfo.thumbnail) + '.jpg') - await doRequestAndSaveToFile({ - method: 'GET', - uri: videoInfo.thumbnail - }, thumbnailfile) + await doRequestAndSaveToFile(videoInfo.thumbnail, thumbnailfile) } const originallyPublishedAt = buildOriginallyPublishedAt(videoInfo)