test(server): add validation test of api:notes/create (#10090)

* fix(server): notes/createのバリデーションが効いていない
Fix #10079

Co-Authored-By: mei23 <m@m544.net>

* anyOf内にバリデーションを書いても最初の一つしかチェックされない

* ✌️

* wip

* wip

* ✌️

* RequiredProp

* Revert "RequiredProp"

This reverts commit 7469390011.

* add api:notes/create

* fix lint

* text

* ✌️

* improve readability

---------

Co-authored-by: mei23 <m@m544.net>
Co-authored-by: syuilo <Syuilotan@yahoo.co.jp>

.vscode/settings.json

@@ -2,5 +2,8 @@
     "search.exclude": {
         "**/node_modules": true
     },
-    "typescript.tsdk": "node_modules/typescript/lib"
+    "typescript.tsdk": "node_modules/typescript/lib",
+    "files.associations": {
+        "*.test.ts": "typescript"
+    }
 }

CONTRIBUTING.md

@@ -299,6 +299,27 @@ pnpm dlx typeorm migration:generate -d ormconfig.js -o <migration name>
 - 生成後、ファイルをmigration下に移してください
 - 作成されたスクリプトは不必要な変更を含むため除去してください
 
+### JSON SchemaのobjectでanyOfを使うとき
+JSON Schemaで、objectに対してanyOfを使う場合、anyOfの中でpropertiesを定義しないこと。  
+バリデーションが効かないため。（SchemaTypeもそのように作られており、objectのanyOf内のpropertiesは捨てられます）  
+https://github.com/misskey-dev/misskey/pull/10082
+
+テキストhogeおよびfugaについて、片方を必須としつつ両方の指定もありうる場合:
+
+```
+export const paramDef = {
+	type: 'object',
+	properties: {
+		hoge: { type: 'string', minLength: 1 },
+		fuga: { type: 'string', minLength: 1 },
+	},
+	anyOf: [
+		{ required: ['hoge'] },
+		{ required: ['fuga'] },
+	],
+} as const;
+```
+
 ### コネクションには`markRaw`せよ
 **Vueのコンポーネントのdataオプションとして**misskey.jsのコネクションを設定するとき、必ず`markRaw`でラップしてください。インスタンスが不必要にリアクティブ化されることで、misskey.js内の処理で不具合が発生するとともに、パフォーマンス上の問題にも繋がる。なお、Composition APIを使う場合はこの限りではない(リアクティブ化はマニュアルなため)。
 

packages/backend/.eslintrc.cjs

@@ -1,7 +1,7 @@
 module.exports = {
 	parserOptions: {
 		tsconfigRootDir: __dirname,
-		project: ['./tsconfig.json'],
+		project: ['./tsconfig.json', './test/tsconfig.json'],
 	},
 	extends: [
 		'../shared/.eslintrc.js',

packages/backend/jest.config.cjs

@@ -20,7 +20,7 @@ module.exports = {
 	// collectCoverage: false,
 
 	// An array of glob patterns indicating a set of files for which coverage information should be collected
-	collectCoverageFrom: ['src/**/*.ts'],
+	collectCoverageFrom: ['src/**/*.ts', '!src/**/*.test.ts'],
 
 	// The directory where Jest should output its coverage files
 	coverageDirectory: "coverage",
@@ -159,6 +159,7 @@ module.exports = {
 	// The glob patterns Jest uses to detect test files
 	testMatch: [
 		"<rootDir>/test/unit/**/*.ts",
+		"<rootDir>/src/**/*.test.ts",
 		//"<rootDir>/test/e2e/**/*.ts"
 	],
 

packages/backend/src/misc/schema.ts

@@ -116,10 +116,10 @@ export type Obj = Record<string, Schema>;
 // https://github.com/misskey-dev/misskey/issues/8535
 // To avoid excessive stack depth error,
 // deceive TypeScript with UnionToIntersection (or more precisely, `infer` expression within it).
-export type ObjType<s extends Obj, RequiredProps extends keyof s> =
+export type ObjType<s extends Obj, RequiredProps extends ReadonlyArray<keyof s>> =
 	UnionToIntersection<
 		{ -readonly [R in RequiredPropertyNames<s>]-?: SchemaType<s[R]> } &
-		{ -readonly [R in RequiredProps]-?: SchemaType<s[R]> } &
+		{ -readonly [R in RequiredProps[number]]-?: SchemaType<s[R]> } &
 		{ -readonly [P in keyof s]?: SchemaType<s[P]> }
 	>;
 
@@ -136,18 +136,19 @@ type PartialIntersection<T> = Partial<UnionToIntersection<T>>;
 // https://github.com/misskey-dev/misskey/pull/8144#discussion_r785287552
 // To get union, we use `Foo extends any ? Hoge<Foo> : never`
 type UnionSchemaType<a extends readonly any[], X extends Schema = a[number]> = X extends any ? SchemaType<X> : never;
-type UnionObjectSchemaType<a extends readonly any[], X extends Schema = a[number]> = X extends any ? ObjectSchemaType<X> : never;
+//type UnionObjectSchemaType<a extends readonly any[], X extends Schema = a[number]> = X extends any ? ObjectSchemaType<X> : never;
+type UnionObjType<s extends Obj, a extends readonly any[], X extends ReadonlyArray<keyof s> = a[number]> = X extends any ? ObjType<s, X> : never;
 type ArrayUnion<T> = T extends any ? Array<T> : never;
 
 type ObjectSchemaTypeDef<p extends Schema> =
 	p['ref'] extends keyof typeof refs ? Packed<p['ref']> :
 	p['properties'] extends NonNullable<Obj> ?
-		p['anyOf'] extends ReadonlyArray<Schema> ?
-			ObjType<p['properties'], NonNullable<p['required']>[number]> & UnionObjectSchemaType<p['anyOf']> & PartialIntersection<UnionObjectSchemaType<p['anyOf']>>
+		p['anyOf'] extends ReadonlyArray<Schema> ? p['anyOf'][number]['required'] extends ReadonlyArray<keyof p['properties']> ?
+			UnionObjType<p['properties'], NonNullable<p['anyOf'][number]['required']>> & ObjType<p['properties'], NonNullable<p['required']>>
+			: never
+			: ObjType<p['properties'], NonNullable<p['required']>>
 	:
-			ObjType<p['properties'], NonNullable<p['required']>[number]>
-	:
-	p['anyOf'] extends ReadonlyArray<Schema> ? UnionObjectSchemaType<p['anyOf']> & PartialIntersection<UnionObjectSchemaType<p['anyOf']>> :
+	p['anyOf'] extends ReadonlyArray<Schema> ? never : // see CONTRIBUTING.md
 	p['allOf'] extends ReadonlyArray<Schema> ? UnionToIntersection<UnionSchemaType<p['allOf']>> :
 	any
 

packages/backend/src/server/api/endpoints/admin/drive/show-file.ts

@@ -138,19 +138,13 @@ export const meta = {
 
 export const paramDef = {
 	type: 'object',
-	anyOf: [
-		{
 	properties: {
 		fileId: { type: 'string', format: 'misskey:id' },
-			},
-			required: ['fileId'],
-		},
-		{
-			properties: {
 		url: { type: 'string' },
 	},
-			required: ['url'],
-		},
+	anyOf: [
+		{ required: ['fileId'] },
+		{ required: ['url'] },
 	],
 } as const;
 

packages/backend/src/server/api/endpoints/drive/files/show.ts

@@ -39,19 +39,13 @@ export const meta = {
 
 export const paramDef = {
 	type: 'object',
-	anyOf: [
-		{
 	properties: {
 		fileId: { type: 'string', format: 'misskey:id' },
-			},
-			required: ['fileId'],
-		},
-		{
-			properties: {
 		url: { type: 'string' },
 	},
-			required: ['url'],
-		},
+	anyOf: [
+		{ required: ['fileId'] },
+		{ required: ['url'] },
 	],
 } as const;
 

packages/backend/src/server/api/endpoints/notes/create.test.ts

@@ -0,0 +1,248 @@
+process.env.NODE_ENV = 'test';
+
+import { readFile } from 'node:fs/promises';
+import { fileURLToPath } from 'node:url';
+import { dirname } from 'node:path';
+import { describe, test, expect } from '@jest/globals';
+import { getValidator } from '../../../../../test/prelude/get-api-validator.js';
+import { paramDef } from './create.js';
+
+const _filename = fileURLToPath(import.meta.url);
+const _dirname = dirname(_filename);
+
+const VALID = true;
+const INVALID = false;
+
+describe('api:notes/create', () => {
+	describe('validation', () => {
+		const v = getValidator(paramDef);
+		const tooLong = readFile(_dirname + '/../../../../../test/resources/misskey.svg', 'utf-8');
+
+		test('reject empty', () => {
+			const valid = v({ });
+			expect(valid).toBe(INVALID);
+		});
+
+		describe('text', () => {
+			test('simple post', () => {
+				expect(v({ text: 'Hello, world!' }))
+					.toBe(VALID);
+			});
+
+			test('null post', () => {
+				expect(v({ text: null }))
+					.toBe(INVALID);
+			});
+
+			test('0 characters post', () => {
+				expect(v({ text: '' }))
+					.toBe(INVALID);
+			});
+
+			test('over 3000 characters post', async () => {
+				expect(v({ text: await tooLong }))
+					.toBe(INVALID);
+			});
+		});
+
+		describe('cw', () => {
+			test('simple cw', () => {
+				expect(v({ text: 'Hello, world!', cw: 'Hello, world!' }))
+					.toBe(VALID);
+			});
+
+			test('null cw', () => {
+				expect(v({ text: 'Body', cw: null }))
+					.toBe(VALID);
+			});
+
+			test('0 characters cw', () => {
+				expect(v({ text: 'Body', cw: '' }))
+					.toBe(VALID);
+			});
+
+			test('reject only cw', () => {
+				expect(v({ cw: 'Hello, world!' }))
+					.toBe(INVALID);
+			});
+
+			test('over 100 characters cw', async () => {
+				expect(v({ text: 'Body', cw: await tooLong }))
+					.toBe(INVALID);
+			});
+		});
+
+		describe('visibility', () => {
+			test('public', () => {
+				expect(v({ text: 'Hello, world!', visibility: 'public' }))
+					.toBe(VALID);
+			});
+
+			test('home', () => {
+				expect(v({ text: 'Hello, world!', visibility: 'home' }))
+					.toBe(VALID);
+			});
+
+			test('followers', () => {
+				expect(v({ text: 'Hello, world!', visibility: 'followers' }))
+					.toBe(VALID);
+			});
+
+			test('reject only visibility', () => {
+				expect(v({ visibility: 'public' }))
+					.toBe(INVALID);
+			});
+
+			test('reject invalid visibility', () => {
+				expect(v({ text: 'Hello, world!', visibility: 'invalid' }))
+					.toBe(INVALID);
+			});
+
+			test('reject null visibility', () => {
+				expect(v({ text: 'Hello, world!', visibility: null }))
+					.toBe(INVALID);
+			});
+
+			describe('visibility:specified', () => {
+				test('specified without visibleUserIds', () => {
+					expect(v({ text: 'Hello, world!', visibility: 'specified' }))
+						.toBe(VALID);
+				});
+
+				test('specified with empty visibleUserIds', () => {
+					expect(v({ text: 'Hello, world!', visibility: 'specified', visibleUserIds: [] }))
+						.toBe(VALID);
+				});
+
+				test('reject specified with non unique visibleUserIds', () => {
+					expect(v({ text: 'Hello, world!', visibility: 'specified', visibleUserIds: ['1', '1', '2'] }))
+						.toBe(INVALID);
+				});
+
+				test('reject specified with null visibleUserIds', () => {
+					expect(v({ text: 'Hello, world!', visibility: 'specified', visibleUserIds: null }))
+						.toBe(INVALID);
+				});
+			});
+		});
+
+		describe('fileIds', () => {
+			test('only fileIds', () => {
+				expect(v({ fileIds: ['1', '2', '3'] }))
+					.toBe(VALID);
+			});
+
+			test('text and fileIds', () => {
+				expect(v({ text: 'Hello, world!', fileIds: ['1', '2', '3'] }))
+					.toBe(VALID);
+			});
+
+			test('reject null fileIds', () => {
+				expect(v({ fileIds: null }))
+					.toBe(INVALID);
+			});
+
+			test('reject text and null fileIds （複合的なanyOfのバリデーションが正しく動作する）', () => {
+				expect(v({ text: 'Hello, world!', fileIds: null }))
+					.toBe(INVALID);
+			});
+
+			test('reject 0 files', () => {
+				expect(v({ fileIds: [] }))
+					.toBe(INVALID);
+			});
+
+			test('reject non unique', () => {
+				expect(v({ fileIds: ['1', '1', '2'] }))
+					.toBe(INVALID);
+			});
+
+			test('reject invalid id', () => {
+				expect(v({ fileIds: ['あ'] }))
+					.toBe(INVALID);
+			});
+
+			test('reject over 17 files', () => {
+				const valid = v({ text: 'Hello, world!', fileIds: ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14', '15', '16', '17', '18'] });
+				expect(valid).toBe(INVALID);
+			});
+		});
+
+		describe('poll', () => {
+			test('note with poll', () => {
+				expect(v({ text: 'Hello, world!', poll: { choices: ['a', 'b', 'c'] } }))
+					.toBe(VALID);
+			});
+
+			test('null poll', () => {
+				expect(v({ text: 'Hello, world!', poll: null }))
+					.toBe(VALID);
+			});
+
+			test('allow only poll', () => {
+				expect(v({ poll: { choices: ['a', 'b', 'c'] } }))
+					.toBe(VALID);
+			});
+
+			test('poll with expiresAt', async () => {
+				expect(v({ poll: { choices: ['a', 'b', 'c'], expiresAt: 1 } }))
+					.toBe(VALID);
+			});
+
+			test('poll with expiredAfter', async () => {
+				expect(v({ poll: { choices: ['a', 'b', 'c'], expiredAfter: 1 } }))
+					.toBe(VALID);
+			});
+
+			test('reject poll without choices', () => {
+				expect(v({ poll: { } }))
+					.toBe(INVALID);
+			});
+
+			test('reject poll with empty choices', () => {
+				expect(v({ poll: { choices: [] } }))
+					.toBe(INVALID);
+			});
+
+			test('reject poll with null choices', () => {
+				expect(v({ poll: { choices: null } }))
+					.toBe(INVALID);
+			});
+
+			test('reject poll with 1 choice', () => {
+				expect(v({ poll: { choices: ['a'] } }))
+					.toBe(INVALID);
+			});
+
+			test('reject poll with too long choice', async () => {
+				expect(v({ poll: { choices: [await tooLong, '2'] } }))
+					.toBe(INVALID);
+			});
+
+			test('reject poll with too many choices', () => {
+				expect(v({ poll: { choices: ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k'] } }))
+					.toBe(INVALID);
+			});
+
+			test('reject poll with non unique choices', () => {
+				expect(v({ poll: { choices: ['a', 'a', 'b', 'c'] } }))
+					.toBe(INVALID);
+			});
+
+			test('reject poll with expiredAfter 0', async () => {
+				expect(v({ poll: { choices: ['a', 'b', 'c'], expiredAfter: 0 } }))
+					.toBe(INVALID);
+			});
+		});
+
+		test('text, fileIds and poll', () => {
+			expect(v({ text: 'Hello, world!', fileIds: ['1', '2', '3'], poll: { choices: ['a', 'b', 'c'] } }))
+				.toBe(VALID);
+		});
+
+		test('text, invalid fileIds and invalid poll', () => {
+			expect(v({ text: 'Hello, world!', fileIds: ['あ'], poll: { choices: ['a'] } }))
+				.toBe(INVALID);
+		});
+	});
+});

packages/backend/src/server/api/endpoints/notes/create.ts

@@ -101,19 +101,17 @@ export const paramDef = {
 		noExtractHashtags: { type: 'boolean', default: false },
 		noExtractEmojis: { type: 'boolean', default: false },
 		replyId: { type: 'string', format: 'misskey:id', nullable: true },
+		renoteId: { type: 'string', format: 'misskey:id', nullable: true },
 		channelId: { type: 'string', format: 'misskey:id', nullable: true },
+
+		// anyOf内にバリデーションを書いても最初の一つしかチェックされない
+		// See https://github.com/misskey-dev/misskey/pull/10082
+		text: {
+			type: 'string',
+			minLength: 1,
+			maxLength: MAX_NOTE_TEXT_LENGTH,
+			nullable: false
 		},
-	anyOf: [
-		{
-			// (re)note with text, files and poll are optional
-			properties: {
-				text: { type: 'string', minLength: 1, maxLength: MAX_NOTE_TEXT_LENGTH, nullable: false },
-			},
-			required: ['text'],
-		},
-		{
-			// (re)note with files, text and poll are optional
-			properties: {
 		fileIds: {
 			type: 'array',
 			uniqueItems: true,
@@ -121,27 +119,13 @@ export const paramDef = {
 			maxItems: 16,
 			items: { type: 'string', format: 'misskey:id' },
 		},
-			},
-			required: ['fileIds'],
-		},
-		{
-			// (re)note with files, text and poll are optional
-			properties: {
 		mediaIds: {
-					deprecated: true,
-					description: 'Use `fileIds` instead. If both are specified, this property is discarded.',
 			type: 'array',
 			uniqueItems: true,
 			minItems: 1,
 			maxItems: 16,
 			items: { type: 'string', format: 'misskey:id' },
 		},
-			},
-			required: ['mediaIds'],
-		},
-		{
-			// (re)note with poll, text and files are optional
-			properties: {
 		poll: {
 			type: 'object',
 			nullable: true,
@@ -160,15 +144,12 @@ export const paramDef = {
 			required: ['choices'],
 		},
 	},
-			required: ['poll'],
-		},
-		{
-			// pure renote
-			properties: {
-				renoteId: { type: 'string', format: 'misskey:id', nullable: true },
-			},
-			required: ['renoteId'],
-		},
+	// (re)note with text, files and poll are optional
+	anyOf: [
+		{ required: ['text'] },
+		{ required: ['fileIds'] },
+		{ required: ['mediaIds'] },
+		{ required: ['poll'] },
 	],
 } as const;
 

packages/backend/src/server/api/endpoints/notes/search-by-tag.ts

@@ -36,16 +36,8 @@ export const paramDef = {
 		sinceId: { type: 'string', format: 'misskey:id' },
 		untilId: { type: 'string', format: 'misskey:id' },
 		limit: { type: 'integer', minimum: 1, maximum: 100, default: 10 },
-	},
-	anyOf: [
-		{
-			properties: {
+
 		tag: { type: 'string', minLength: 1 },
-			},
-			required: ['tag'],
-		},
-		{
-			properties: {
 		query: {
 			type: 'array',
 			description: 'The outer arrays are chained with OR, the inner arrays are chained with AND.',
@@ -60,8 +52,9 @@ export const paramDef = {
 			minItems: 1,
 		},
 	},
-			required: ['query'],
-		},
+	anyOf: [
+		{ required: ['tag'] },
+		{ required: ['query'] },
 	],
 } as const;
 

packages/backend/src/server/api/endpoints/pages/show.ts

@@ -29,20 +29,14 @@ export const meta = {
 
 export const paramDef = {
 	type: 'object',
-	anyOf: [
-		{
 	properties: {
 		pageId: { type: 'string', format: 'misskey:id' },
-			},
-			required: ['pageId'],
-		},
-		{
-			properties: {
 		name: { type: 'string' },
 		username: { type: 'string' },
 	},
-			required: ['name', 'username'],
-		},
+	anyOf: [
+		{ required: ['pageId'] },
+		{ required: ['name', 'username'] },
 	],
 } as const;
 

packages/backend/src/server/api/endpoints/users/followers.ts

@@ -46,16 +46,8 @@ export const paramDef = {
 		sinceId: { type: 'string', format: 'misskey:id' },
 		untilId: { type: 'string', format: 'misskey:id' },
 		limit: { type: 'integer', minimum: 1, maximum: 100, default: 10 },
-	},
-	anyOf: [
-		{
-			properties: {
+
 		userId: { type: 'string', format: 'misskey:id' },
-			},
-			required: ['userId'],
-		},
-		{
-			properties: {
 		username: { type: 'string' },
 		host: {
 			type: 'string',
@@ -63,8 +55,9 @@ export const paramDef = {
 			description: 'The local host is represented with `null`.',
 		},
 	},
-			required: ['username', 'host'],
-		},
+	anyOf: [
+		{ required: ['userId'] },
+		{ required: ['username', 'host'] },
 	],
 } as const;
 

packages/backend/src/server/api/endpoints/users/following.ts

@@ -46,16 +46,8 @@ export const paramDef = {
 		sinceId: { type: 'string', format: 'misskey:id' },
 		untilId: { type: 'string', format: 'misskey:id' },
 		limit: { type: 'integer', minimum: 1, maximum: 100, default: 10 },
-	},
-	anyOf: [
-		{
-			properties: {
+
 		userId: { type: 'string', format: 'misskey:id' },
-			},
-			required: ['userId'],
-		},
-		{
-			properties: {
 		username: { type: 'string' },
 		host: {
 			type: 'string',
@@ -63,8 +55,9 @@ export const paramDef = {
 			description: 'The local host is represented with `null`.',
 		},
 	},
-			required: ['username', 'host'],
-		},
+	anyOf: [
+		{ required: ['userId'] },
+		{ required: ['username', 'host'] },
 	],
 } as const;
 

packages/backend/src/server/api/endpoints/users/search-by-username-and-host.ts

@@ -31,20 +31,13 @@ export const paramDef = {
 	properties: {
 		limit: { type: 'integer', minimum: 1, maximum: 100, default: 10 },
 		detail: { type: 'boolean', default: true },
-	},
-	anyOf: [
-		{
-			properties: {
+
 		username: { type: 'string', nullable: true },
-			},
-			required: ['username'],
-		},
-		{
-			properties: {
 		host: { type: 'string', nullable: true },
 	},
-			required: ['host'],
-		},
+	anyOf: [
+		{ required: ['username'] },
+		{ required: ['host'] },
 	],
 } as const;
 

packages/backend/src/server/api/endpoints/users/show.ts

@@ -54,23 +54,11 @@ export const meta = {
 
 export const paramDef = {
 	type: 'object',
-	anyOf: [
-		{
 	properties: {
 		userId: { type: 'string', format: 'misskey:id' },
-			},
-			required: ['userId'],
-		},
-		{
-			properties: {
 		userIds: { type: 'array', uniqueItems: true, items: {
 			type: 'string', format: 'misskey:id',
 		} },
-			},
-			required: ['userIds'],
-		},
-		{
-			properties: {
 		username: { type: 'string' },
 		host: {
 			type: 'string',
@@ -78,8 +66,10 @@ export const paramDef = {
 			description: 'The local host is represented with `null`.',
 		},
 	},
-			required: ['username'],
-		},
+	anyOf: [
+		{ required: ['userId'] },
+		{ required: ['userIds'] },
+		{ required: ['username'] },
 	],
 } as const;
 

packages/backend/test/prelude/get-api-validator.ts

@@ -0,0 +1,11 @@
+import { Schema } from '@/misc/schema';
+import Ajv from 'ajv';
+
+export const getValidator = (paramDef: Schema) => {
+    const ajv = new Ajv({
+        useDefaults: true,
+    });
+    ajv.addFormat('misskey:id', /^[a-zA-Z0-9]+$/);		
+
+    return ajv.compile(paramDef);
+}

packages/backend/test/tsconfig.json

@@ -33,11 +33,12 @@
 		"lib": [
 			"esnext"
 		],
-		"types": ["jest"]
+		"types": ["jest", "node"]
 	},
 	"compileOnSave": false,
 	"include": [
 		"./**/*.ts",
+		"../src/**/*.test.ts",
 		"../src/@types/**/*.ts",
 	]
 }

packages/backend/tsconfig.json

@@ -26,9 +26,7 @@
 		"rootDir": "./src",
 		"baseUrl": "./",
 		"paths": {
-			"@/*": [
-				"./src/*"
-			]
+			"@/*": ["./src/*"]
 		},
 		"outDir": "./built",
 		"types": [
@@ -46,4 +44,7 @@
 	"include": [
 		"./src/**/*.ts"
 	],
+	"exclude": [
+		"./src/**/*.test.ts"
+	]
 }