diff --git a/packages/backend/src/server/ServerService.ts b/packages/backend/src/server/ServerService.ts index 5857b3059e..2d20aa1222 100644 --- a/packages/backend/src/server/ServerService.ts +++ b/packages/backend/src/server/ServerService.ts @@ -7,7 +7,7 @@ import cluster from 'node:cluster'; import * as fs from 'node:fs'; import { fileURLToPath } from 'node:url'; import { Inject, Injectable, OnApplicationShutdown } from '@nestjs/common'; -import Fastify, { FastifyInstance } from 'fastify'; +import Fastify, { type FastifyInstance } from 'fastify'; import fastifyStatic from '@fastify/static'; import fastifyRawBody from 'fastify-raw-body'; import { IsNull } from 'typeorm'; @@ -75,7 +75,7 @@ export class ServerService implements OnApplicationShutdown { } @bindThis - public async launch() { + public async launch(): Promise { const fastify = Fastify({ trustProxy: true, logger: false, @@ -135,8 +135,8 @@ export class ServerService implements OnApplicationShutdown { reply.header('content-type', 'text/plain; charset=utf-8'); reply.header('link', `<${encodeURI(location)}>; rel="canonical"`); done(null, [ - 'Refusing to relay remote ActivityPub object lookup.', - '', + "Refusing to relay remote ActivityPub object lookup.", + "", `Please remove 'application/activity+json' and 'application/ld+json' from the Accept header or fetch using the authoritative URL at ${location}.`, ].join('\n')); }); @@ -304,7 +304,6 @@ export class ServerService implements OnApplicationShutdown { } await fastify.ready(); - return fastify; } @bindThis @@ -313,6 +312,13 @@ export class ServerService implements OnApplicationShutdown { await this.#fastify.close(); } + /** + * Get the Fastify instance for testing. + */ + public get fastify(): FastifyInstance { + return this.#fastify; + } + @bindThis async onApplicationShutdown(signal: string): Promise { await this.dispose(); diff --git a/packages/backend/src/server/api/ApiCallService.ts b/packages/backend/src/server/api/ApiCallService.ts index 1b8d33f9c9..0d2dafd556 100644 --- a/packages/backend/src/server/api/ApiCallService.ts +++ b/packages/backend/src/server/api/ApiCallService.ts @@ -6,11 +6,8 @@ import { randomUUID } from 'node:crypto'; import * as fs from 'node:fs'; import * as stream from 'node:stream/promises'; -import { Transform } from 'node:stream'; -import { type MultipartFile } from '@fastify/multipart'; import { Inject, Injectable } from '@nestjs/common'; import * as Sentry from '@sentry/node'; -import { AttachmentFile } from '@/server/api/endpoint-base.js'; import { DI } from '@/di-symbols.js'; import { getIpHash } from '@/misc/get-ip-hash.js'; import type { MiLocalUser, MiUser } from '@/models/User.js'; @@ -19,7 +16,7 @@ import type Logger from '@/logger.js'; import type { MiMeta, UserIpsRepository } from '@/models/_.js'; import { createTemp } from '@/misc/create-temp.js'; import { bindThis } from '@/decorators.js'; -import { type RolePolicies, RoleService } from '@/core/RoleService.js'; +import { RoleService } from '@/core/RoleService.js'; import type { Config } from '@/config.js'; import { sendRateLimitHeaders } from '@/misc/rate-limit-utils.js'; import { SkRateLimiterService } from '@/server/SkRateLimiterService.js'; @@ -194,6 +191,18 @@ export class ApiCallService implements OnApplicationShutdown { return; } + const [path, cleanup] = await createTemp(); + await stream.pipeline(multipartData.file, fs.createWriteStream(path)); + + // ファイルサイズが制限を超えていた場合 + // なお truncated はストリームを読み切ってからでないと機能しないため、stream.pipeline より後にある必要がある + if (multipartData.file.truncated) { + cleanup(); + reply.code(413); + reply.send(); + return; + } + const fields = {} as Record; for (const [k, v] of Object.entries(multipartData.fields)) { fields[k] = typeof v === 'object' && 'value' in v ? v.value : undefined; @@ -204,13 +213,18 @@ export class ApiCallService implements OnApplicationShutdown { ? request.headers.authorization.slice(7) : fields['i']; if (token != null && typeof token !== 'string') { + cleanup(); reply.code(400); return; } this.authenticateService.authenticate(token).then(([user, app]) => { - this.call(endpoint, user, app, fields, multipartData, request, reply).then((res) => { + this.call(endpoint, user, app, fields, { + name: multipartData.filename, + path: path, + }, request, reply).then((res) => { this.send(reply, res); }).catch((err: ApiError) => { + cleanup(); this.#sendApiError(reply, err); }); @@ -218,6 +232,7 @@ export class ApiCallService implements OnApplicationShutdown { this.logIp(request, user); } }).catch(err => { + cleanup(); this.#sendAuthenticationError(reply, err); }); } @@ -278,7 +293,10 @@ export class ApiCallService implements OnApplicationShutdown { user: MiLocalUser | null | undefined, token: MiAccessToken | null | undefined, data: any, - multipartFile: MultipartFile | null, + file: { + name: string; + path: string; + } | null, request: FastifyRequest<{ Body: Record | undefined, Querystring: Record }>, reply: FastifyReply, ) { @@ -354,37 +372,6 @@ export class ApiCallService implements OnApplicationShutdown { } } - // Cast non JSON input - if ((ep.meta.requireFile || request.method === 'GET') && ep.params.properties) { - for (const k of Object.keys(ep.params.properties)) { - const param = ep.params.properties![k]; - if (['boolean', 'number', 'integer'].includes(param.type ?? '') && typeof data[k] === 'string') { - try { - data[k] = JSON.parse(data[k]); - } catch (e) { - throw new ApiError({ - message: 'Invalid param.', - code: 'INVALID_PARAM', - id: '0b5f1631-7c1a-41a6-b399-cce335f34d85', - }, { - param: k, - reason: `cannot cast to ${param.type}`, - }); - } - } - } - } - - if (token && ((ep.meta.kind && !token.permission.some(p => p === ep.meta.kind)) - || (!ep.meta.kind && (ep.meta.requireCredential || ep.meta.requireModerator || ep.meta.requireAdmin)))) { - throw new ApiError({ - message: 'Your app does not have the necessary permissions to use this endpoint.', - code: 'PERMISSION_DENIED', - kind: 'permission', - id: '1370e5b7-d4eb-4566-bb1d-7748ee6a1838', - }); - } - if ((ep.meta.requireModerator || ep.meta.requireAdmin) && (this.meta.rootUserId !== user!.id)) { const myRoles = await this.roleService.getUserRoles(user!.id); if (ep.meta.requireModerator && !myRoles.some(r => r.isModerator || r.isAdministrator)) { @@ -418,91 +405,49 @@ export class ApiCallService implements OnApplicationShutdown { } } - let attachmentFile: AttachmentFile | null = null; - let cleanup = () => {}; - if (ep.meta.requireFile && request.method === 'POST' && multipartFile) { - const policies = await this.roleService.getUserPolicies(user!.id); - const result = await this.handleAttachmentFile( - Math.min((policies.maxFileSizeMb * 1024 * 1024), this.config.maxFileSize), - multipartFile, - ); - attachmentFile = result.attachmentFile; - cleanup = result.cleanup; + if (token && ((ep.meta.kind && !token.permission.some(p => p === ep.meta.kind)) + || (!ep.meta.kind && (ep.meta.requireCredential || ep.meta.requireModerator || ep.meta.requireAdmin)))) { + throw new ApiError({ + message: 'Your app does not have the necessary permissions to use this endpoint.', + code: 'PERMISSION_DENIED', + kind: 'permission', + id: '1370e5b7-d4eb-4566-bb1d-7748ee6a1838', + }); + } + + // Cast non JSON input + if ((ep.meta.requireFile || request.method === 'GET') && ep.params.properties) { + for (const k of Object.keys(ep.params.properties)) { + const param = ep.params.properties![k]; + if (['boolean', 'number', 'integer'].includes(param.type ?? '') && typeof data[k] === 'string') { + try { + data[k] = JSON.parse(data[k]); + } catch (e) { + throw new ApiError({ + message: 'Invalid param.', + code: 'INVALID_PARAM', + id: '0b5f1631-7c1a-41a6-b399-cce335f34d85', + }, { + param: k, + reason: `cannot cast to ${param.type}`, + }); + } + } + } } // API invoking if (this.config.sentryForBackend) { return await Sentry.startSpan({ name: 'API: ' + ep.name, - }, () => { - return ep.exec(data, user, token, attachmentFile, request.ip, request.headers) - .catch((err: Error) => this.#onExecError(ep, data, err, user?.id)) - .finally(() => cleanup()); - }); + }, () => ep.exec(data, user, token, file, request.ip, request.headers) + .catch((err: Error) => this.#onExecError(ep, data, err, user?.id))); } else { - return await ep.exec(data, user, token, attachmentFile, request.ip, request.headers) - .catch((err: Error) => this.#onExecError(ep, data, err, user?.id)) - .finally(() => cleanup()); + return await ep.exec(data, user, token, file, request.ip, request.headers) + .catch((err: Error) => this.#onExecError(ep, data, err, user?.id)); } } - @bindThis - private async handleAttachmentFile( - fileSizeLimit: number, - multipartFile: MultipartFile, - ) { - function createTooLongError() { - return new ApiError({ - httpStatusCode: 413, - kind: 'client', - message: 'File size is too large.', - code: 'FILE_SIZE_TOO_LARGE', - id: 'ff827ce8-9b4b-4808-8511-422222a3362f', - }); - } - - function createLimitStream(limit: number) { - let total = 0; - - return new Transform({ - transform(chunk, _, callback) { - total += chunk.length; - if (total > limit) { - callback(createTooLongError()); - } else { - callback(null, chunk); - } - }, - }); - } - - const [path, cleanup] = await createTemp(); - try { - await stream.pipeline( - multipartFile.file, - createLimitStream(fileSizeLimit), - fs.createWriteStream(path), - ); - - // ファイルサイズが制限を超えていた場合 - // なお truncated はストリームを読み切ってからでないと機能しないため、stream.pipeline より後にある必要がある - if (multipartFile.file.truncated) { - throw createTooLongError(); - } - } catch (err) { - cleanup(); - throw err; - } - - return { - attachmentFile: { - name: multipartFile.filename, - path, - }, - cleanup, - }; - } - @bindThis public dispose(): void { clearInterval(this.userIpHistoriesClearIntervalId); diff --git a/packages/backend/src/server/api/endpoint-base.ts b/packages/backend/src/server/api/endpoint-base.ts index b063487305..e061aa3a8e 100644 --- a/packages/backend/src/server/api/endpoint-base.ts +++ b/packages/backend/src/server/api/endpoint-base.ts @@ -21,23 +21,23 @@ ajv.addFormat('misskey:id', /^[a-zA-Z0-9]+$/); export type Response = Record | void; -export type AttachmentFile = { +type File = { name: string | null; path: string; }; // TODO: paramsの型をT['params']のスキーマ定義から推論する type Executor = - (params: SchemaType, user: T['requireCredential'] extends true ? MiLocalUser : MiLocalUser | null, token: MiAccessToken | null, file?: AttachmentFile, cleanup?: () => any, ip?: string | null, headers?: Record | null) => - Promise>>; + (params: SchemaType, user: T['requireCredential'] extends true ? MiLocalUser : MiLocalUser | null, token: MiAccessToken | null, file?: File, cleanup?: () => any, ip?: string | null, headers?: Record | null) => + Promise>>; export abstract class Endpoint { - public exec: (params: any, user: T['requireCredential'] extends true ? MiLocalUser : MiLocalUser | null, token: MiAccessToken | null, file?: AttachmentFile, ip?: string | null, headers?: Record | null) => Promise; + public exec: (params: any, user: T['requireCredential'] extends true ? MiLocalUser : MiLocalUser | null, token: MiAccessToken | null, file?: File, ip?: string | null, headers?: Record | null) => Promise; constructor(meta: T, paramDef: Ps, cb: Executor) { const validate = ajv.compile(paramDef); - this.exec = (params: any, user: T['requireCredential'] extends true ? MiLocalUser : MiLocalUser | null, token: MiAccessToken | null, file?: AttachmentFile, ip?: string | null, headers?: Record | null) => { + this.exec = (params: any, user: T['requireCredential'] extends true ? MiLocalUser : MiLocalUser | null, token: MiAccessToken | null, file?: File, ip?: string | null, headers?: Record | null) => { let cleanup: undefined | (() => void) = undefined; if (meta.requireFile) { diff --git a/packages/backend/src/server/api/endpoints/drive/files/create.ts b/packages/backend/src/server/api/endpoints/drive/files/create.ts index 7043f4883a..f4c47d71bf 100644 --- a/packages/backend/src/server/api/endpoints/drive/files/create.ts +++ b/packages/backend/src/server/api/endpoints/drive/files/create.ts @@ -67,6 +67,7 @@ export const meta = { message: 'Cannot upload the file because it exceeds the maximum file size.', code: 'MAX_FILE_SIZE_EXCEEDED', id: 'b9d8c348-33f0-4673-b9a9-5d4da058977a', + httpStatusCode: 413, }, }, } as const; diff --git a/packages/backend/test/e2e/api.ts b/packages/backend/test/e2e/api.ts index f9e65aaa84..49c6a0636b 100644 --- a/packages/backend/test/e2e/api.ts +++ b/packages/backend/test/e2e/api.ts @@ -159,8 +159,8 @@ describe('API', () => { user: { token: application3 }, }, { status: 403, - code: 'PERMISSION_DENIED', - id: '1370e5b7-d4eb-4566-bb1d-7748ee6a1838', + code: 'ROLE_PERMISSION_DENIED', + id: 'c3d38592-54c0-429d-be96-5636b0431a61', }); await failedApiCall({ diff --git a/packages/backend/test/unit/server/api/drive/files/create.ts b/packages/backend/test/unit/server/api/drive/files/create.ts index b98892fa03..9b38f4d744 100644 --- a/packages/backend/test/unit/server/api/drive/files/create.ts +++ b/packages/backend/test/unit/server/api/drive/files/create.ts @@ -3,29 +3,31 @@ * SPDX-License-Identifier: AGPL-3.0-only */ -import { S3Client } from '@aws-sdk/client-s3'; import { Test, TestingModule } from '@nestjs/testing'; -import { mockClient } from 'aws-sdk-client-mock'; import { FastifyInstance } from 'fastify'; import request from 'supertest'; +import { randomString } from '../../../../../utils.js'; import { CoreModule } from '@/core/CoreModule.js'; import { RoleService } from '@/core/RoleService.js'; import { DI } from '@/di-symbols.js'; import { GlobalModule } from '@/GlobalModule.js'; -import { MiRole, UserProfilesRepository, UsersRepository } from '@/models/_.js'; +import { DriveFoldersRepository, MiDriveFolder, MiRole, UserProfilesRepository, UsersRepository } from '@/models/_.js'; import { MiUser } from '@/models/User.js'; import { ServerModule } from '@/server/ServerModule.js'; import { ServerService } from '@/server/ServerService.js'; +import { IdService } from '@/core/IdService.js'; describe('/drive/files/create', () => { let module: TestingModule; let server: FastifyInstance; - const s3Mock = mockClient(S3Client); let roleService: RoleService; + let idService: IdService; let root: MiUser; let role_tinyAttachment: MiRole; + let folder: MiDriveFolder; + beforeAll(async () => { module = await Test.createTestingModule({ imports: [GlobalModule, CoreModule, ServerModule], @@ -33,21 +35,34 @@ describe('/drive/files/create', () => { module.enableShutdownHooks(); const serverService = module.get(ServerService); - server = await serverService.launch(); + await serverService.launch(); + server = serverService.fastify; + + idService = module.get(IdService); const usersRepository = module.get(DI.usersRepository); + await usersRepository.delete({}); root = await usersRepository.insert({ - id: 'root', + id: idService.gen(), username: 'root', usernameLower: 'root', token: '1234567890123456', }).then(x => usersRepository.findOneByOrFail(x.identifiers[0])); const userProfilesRepository = module.get(DI.userProfilesRepository); + await userProfilesRepository.delete({}); await userProfilesRepository.insert({ userId: root.id, }); + const driveFoldersRepository = module.get(DI.driveFoldersRepository); + folder = await driveFoldersRepository.insertOne({ + id: idService.gen(), + name: 'root-folder', + parentId: null, + userId: root.id, + }); + roleService = module.get(RoleService); role_tinyAttachment = await roleService.create({ name: 'test-role001', @@ -65,8 +80,8 @@ describe('/drive/files/create', () => { }); beforeEach(async () => { - s3Mock.reset(); - await roleService.unassign(root.id, role_tinyAttachment.id).catch(() => {}); + await roleService.unassign(root.id, role_tinyAttachment.id).catch(() => { + }); }); afterAll(async () => { @@ -74,35 +89,76 @@ describe('/drive/files/create', () => { await module.close(); }); - test('200 ok', async () => { - const result = await request(server.server) + async function postFile(props: { + name: string, + comment: string, + isSensitive: boolean, + force: boolean, + fileContent: Buffer | string, + }) { + const { name, comment, isSensitive, force, fileContent } = props; + + return await request(server.server) .post('/api/drive/files/create') .set('Content-Type', 'multipart/form-data') - .set('Authorization', `Bearer ${root.token}`) - .attach('file', Buffer.from('a'.repeat(1024 * 1024))); + .attach('file', fileContent) + .field('name', name) + .field('comment', comment) + .field('isSensitive', isSensitive) + .field('force', force) + .field('folderId', folder.id) + .field('i', root.token ?? ''); + } + + test('200 ok', async () => { + const name = randomString(); + const comment = randomString(); + const result = await postFile({ + name: name, + comment: comment, + isSensitive: true, + force: true, + fileContent: Buffer.from('a'.repeat(1000 * 1000)), + }); expect(result.statusCode).toBe(200); + expect(result.body.name).toBe(name + '.unknown'); + expect(result.body.comment).toBe(comment); + expect(result.body.isSensitive).toBe(true); + expect(result.body.folderId).toBe(folder.id); }); test('200 ok(with role)', async () => { await roleService.assign(root.id, role_tinyAttachment.id); - const result = await request(server.server) - .post('/api/drive/files/create') - .set('Content-Type', 'multipart/form-data') - .set('Authorization', `Bearer ${root.token}`) - .attach('file', Buffer.from('a'.repeat(10))); + const name = randomString(); + const comment = randomString(); + const result = await postFile({ + name: name, + comment: comment, + isSensitive: true, + force: true, + fileContent: Buffer.from('a'.repeat(10)), + }); expect(result.statusCode).toBe(200); + expect(result.body.name).toBe(name + '.unknown'); + expect(result.body.comment).toBe(comment); + expect(result.body.isSensitive).toBe(true); + expect(result.body.folderId).toBe(folder.id); }); test('413 too large', async () => { await roleService.assign(root.id, role_tinyAttachment.id); - const result = await request(server.server) - .post('/api/drive/files/create') - .set('Content-Type', 'multipart/form-data') - .set('Authorization', `Bearer ${root.token}`) - .attach('file', Buffer.from('a'.repeat(11))); + const name = randomString(); + const comment = randomString(); + const result = await postFile({ + name: name, + comment: comment, + isSensitive: true, + force: true, + fileContent: Buffer.from('a'.repeat(11)), + }); expect(result.statusCode).toBe(413); - expect(result.body.error.code).toBe('FILE_SIZE_TOO_LARGE'); + expect(result.body.error.code).toBe('MAX_FILE_SIZE_EXCEEDED'); }); }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index fe009057fa..cce229ef5a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -21215,7 +21215,7 @@ snapshots: formidable: 3.5.4 methods: 1.1.2 mime: 2.6.0 - qs: 6.13.0 + qs: 6.14.0 transitivePeerDependencies: - supports-color