Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a rate limiting feature for notes containing URLs as a configurable role policy. The feature aims to help prevent spam by limiting how many notes with URLs a user can post within a specified time period.
Key Changes:
- Added three new role policies:
noteWithUrlLimit(count),noteWithUrlLimitDuration(period in ms), andnoteWithUrlLimitPublicOnly(boolean flag) - Implemented URL detection from MFM-parsed note content (text, CW, and poll choices)
- Added rate limit checking in the note creation flow using Redis-backed counters
- Provided admin UI controls for configuring these policies per role
- Added localized error messages in Japanese and English
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| plans/url-note-rate-limit-design.md | Design document describing the architecture, implementation status, and future test plans for the URL rate limiting feature |
| packages/misskey-js/src/consts.ts | Added three new role policy constants to the exported array |
| packages/i18n/src/autogen/locale.ts | Auto-generated TypeScript types for the new localization keys |
| packages/frontend/src/pages/admin/roles.vue | Added UI controls in the admin roles overview page for the three new policies |
| packages/frontend/src/pages/admin/roles.editor.vue | Added detailed editor UI for configuring the new rate limit policies when editing a role |
| packages/frontend/src/components/MkPostForm.vue | Added error handling to display a message when the URL rate limit is exceeded |
| packages/backend/src/server/api/endpoints/notes/create.ts | Added error definition and catch handler for the new rate limit error |
| packages/backend/src/models/json-schema/role.ts | Added JSON schema definitions for the three new role policy fields |
| packages/backend/src/misc/extract-urls-from-mfm.ts | New utility module to extract URLs from MFM tokens and check if URLs exist |
| packages/backend/src/core/RoleService.ts | Extended RolePolicies type with three new fields, added default values, and implemented policy aggregation logic |
| packages/backend/src/core/NoteCreateService.ts | Integrated URL detection and rate limit checking into the note creation flow; added custom checkRateLimit method |
| locales/ja-JP.yml | Added Japanese error message and policy descriptions |
| locales/en-US.yml | Added English error message and policy descriptions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private async checkRateLimit(key: string, duration: number, max: number): Promise<{ remaining: number; total: number; reset: number }> { | ||
| return new Promise<{ remaining: number; total: number; reset: number }>((resolve, reject) => { | ||
| const limiter = new Limiter({ | ||
| id: key, | ||
| duration: duration, | ||
| max: max, | ||
| db: this.redisClient, | ||
| }); | ||
|
|
||
| limiter.get((err, info) => { | ||
| if (err) { | ||
| return reject(err); | ||
| } | ||
| resolve({ | ||
| remaining: info.remaining, | ||
| total: info.total, | ||
| reset: info.reset, | ||
| }); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The design document mentions that RateLimiterService.limit() was considered for use (section 3.2, line 212), but the actual implementation creates a separate checkRateLimit() method in NoteCreateService instead. This creates code duplication and inconsistency. Consider using the existing RateLimiterService to maintain consistency with other rate limiting in the codebase, or update the design document to reflect the actual implementation approach.
| const urls: string[] = []; | ||
|
|
||
| function extractFromNode(node: mfm.MfmNode): void { | ||
| if (node.type === 'url') { | ||
| urls.push(node.props.url); | ||
| } else if (node.type === 'link') { | ||
| urls.push(node.props.url); | ||
| } else if ('children' in node && node.children) { | ||
| for (const child of node.children) { | ||
| extractFromNode(child); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const node of nodes) { | ||
| extractFromNode(node); | ||
| } | ||
|
|
||
| return urls; |
There was a problem hiding this comment.
The extractUrlsFromMfm function manually traverses the MFM tree structure, which is inconsistent with other similar extraction functions in the codebase (extractHashtags, extractMentions, extractCustomEmojisFromMfm) that use mfm.extract(). Consider refactoring to use mfm.extract() for consistency and to leverage the tested utility function already provided by the mfm-js library.
| const urls: string[] = []; | |
| function extractFromNode(node: mfm.MfmNode): void { | |
| if (node.type === 'url') { | |
| urls.push(node.props.url); | |
| } else if (node.type === 'link') { | |
| urls.push(node.props.url); | |
| } else if ('children' in node && node.children) { | |
| for (const child of node.children) { | |
| extractFromNode(child); | |
| } | |
| } | |
| } | |
| for (const node of nodes) { | |
| extractFromNode(node); | |
| } | |
| return urls; | |
| const urlNodes = mfm.extract(nodes, node => node.type === 'url' || node.type === 'link'); | |
| return urlNodes.map(node => (node as any).props.url); |
| /* | ||
| * SPDX-FileCopyrightText: syuilo and misskey-project | ||
| * SPDX-License-Identifier: AGPL-3.0-only | ||
| */ | ||
|
|
||
| import * as mfm from 'mfm-js'; | ||
|
|
||
| /** | ||
| * MFMトークンからURLを抽出 | ||
| * @param nodes MFMノードの配列 | ||
| * @returns URL文字列の配列 | ||
| */ | ||
| export function extractUrlsFromMfm(nodes: mfm.MfmNode[]): string[] { | ||
| const urls: string[] = []; | ||
|
|
||
| function extractFromNode(node: mfm.MfmNode): void { | ||
| if (node.type === 'url') { | ||
| urls.push(node.props.url); | ||
| } else if (node.type === 'link') { | ||
| urls.push(node.props.url); | ||
| } else if ('children' in node && node.children) { | ||
| for (const child of node.children) { | ||
| extractFromNode(child); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const node of nodes) { | ||
| extractFromNode(node); | ||
| } | ||
|
|
||
| return urls; | ||
| } | ||
|
|
||
| /** | ||
| * ノートにURLが含まれるかチェック | ||
| * @param tokens MFMトークン | ||
| * @returns URLが含まれる場合true | ||
| */ | ||
| export function hasUrl(tokens: mfm.MfmNode[]): boolean { | ||
| return extractUrlsFromMfm(tokens).length > 0; | ||
| } |
There was a problem hiding this comment.
Tests for the URL rate limit feature are missing. According to the design document (line 340-343), unit tests and integration tests should be created to verify the URL detection logic, rate limit enforcement, public-only option behavior, and error handling. Consider adding tests similar to packages/backend/test/unit/extract-mentions.ts for the extractUrlsFromMfm function, and integration tests for the rate limiting behavior.
| if (err.id === '2e86dcc0-592c-49f6-99bf-4fcd262ad972') { | ||
| os.alert({ | ||
| type: 'error', | ||
| text: '一定期間に可能なURL付きノートの投稿回数を超えました。しばらく時間をおいてから再度お試しください。', |
There was a problem hiding this comment.
The error message is hardcoded in Japanese instead of using the i18n translation system. This breaks internationalization and makes the error message appear in Japanese even when the user interface is in English or other languages. Replace the hardcoded string with i18n.ts._errors.noteWithUrlRateLimitExceeded to properly support multiple languages.
| text: '一定期間に可能なURL付きノートの投稿回数を超えました。しばらく時間をおいてから再度お試しください。', | |
| text: i18n.ts._errors.noteWithUrlRateLimitExceeded, |
| const limitInfo = await this.checkRateLimit( | ||
| `noteWithUrl:${user.id}`, | ||
| policies.noteWithUrlLimitDuration, | ||
| policies.noteWithUrlLimit, | ||
| ); | ||
|
|
||
| if (limitInfo.remaining === 0) { | ||
| throw new IdentifiableError( | ||
| '2e86dcc0-592c-49f6-99bf-4fcd262ad972', | ||
| 'Note with URL rate limit exceeded', | ||
| ); | ||
| } |
There was a problem hiding this comment.
The rate limit check consumes the limit counter even when the request fails. The limiter.get() method both checks AND decrements the counter. When remaining reaches 0, an error is thrown, but the counter has already been consumed. This means if a user has exactly 1 remaining post allowed, calling limiter.get() will decrement it to 0, return remaining: 0, and throw an error - consuming the user's last allowed post without actually posting the note. The check should verify remaining BEFORE decrementing (or check if remaining is less than 1 before the call), or the counter should only be decremented after successful post creation.
| if (limitInfo.remaining === 0) { | ||
| throw new IdentifiableError( | ||
| '2e86dcc0-592c-49f6-99bf-4fcd262ad972', | ||
| 'Note with URL rate limit exceeded', |
There was a problem hiding this comment.
The error thrown does not include rate limit information (such as reset time) that would be useful for the frontend to display to users. The limitInfo object contains remaining, total, and reset fields that should be passed to the IdentifiableError as the third parameter so that the API can return this information to the client.
| 'Note with URL rate limit exceeded', | |
| 'Note with URL rate limit exceeded', | |
| limitInfo, |
1 participant
close #67