-
Notifications
You must be signed in to change notification settings - Fork 114
fix(v2): centralize request validation and pagination bounds #540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { BadRequestException, ValidationPipe } from '@nestjs/common'; | ||
| import { describe, expect, it } from '@jest/globals'; | ||
| import { ClaimFeedQueryDto } from './claim-feed-query.dto'; | ||
|
|
||
| describe('ClaimFeedQueryDto', () => { | ||
| const pipe = new ValidationPipe({ | ||
| transform: true, | ||
| whitelist: true, | ||
| forbidNonWhitelisted: true, | ||
| }); | ||
|
|
||
| const validate = async (value: Record<string, unknown>) => | ||
| pipe.transform(value, { | ||
| type: 'query', | ||
| metatype: ClaimFeedQueryDto, | ||
| data: '', | ||
| }); | ||
|
|
||
| it('accepts a valid Ethereum creator address', async () => { | ||
| const result = await validate({ | ||
| creator: '0x1111111111111111111111111111111111111111', | ||
| }); | ||
|
|
||
| expect(result).toBeInstanceOf(ClaimFeedQueryDto); | ||
| expect((result as ClaimFeedQueryDto).creator).toBe( | ||
| '0x1111111111111111111111111111111111111111', | ||
| ); | ||
| }); | ||
|
|
||
| it.each([ | ||
| '', | ||
| 'not-an-address', | ||
| '0x1234', | ||
| '0xgggggggggggggggggggggggggggggggggggggggg', | ||
| ])('rejects an invalid creator address: %s', async (creator) => { | ||
| await expect(validate({ creator })).rejects.toBeInstanceOf( | ||
| BadRequestException, | ||
| ); | ||
| }); | ||
|
|
||
| it('rejects unknown query fields', async () => { | ||
| await expect( | ||
| validate({ | ||
| creator: '0x1111111111111111111111111111111111111111', | ||
| unexpected: 'value', | ||
| }), | ||
| ).rejects.toBeInstanceOf(BadRequestException); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,14 @@ | ||
| import { ApiPropertyOptional } from '@nestjs/swagger'; | ||
| import { IsOptional, IsString, IsInt, Min, Max, IsIn, IsDateString } from 'class-validator'; | ||
| import { | ||
| IsOptional, | ||
| IsString, | ||
| IsInt, | ||
| Min, | ||
| Max, | ||
| IsIn, | ||
| IsDateString, | ||
| IsEthereumAddress, | ||
| } from 'class-validator'; | ||
| import { Type } from 'class-transformer'; | ||
|
|
||
| export const CLAIM_FEED_MAX_LIMIT = 100; | ||
|
|
@@ -35,7 +44,7 @@ export class ClaimFeedQueryDto { | |
|
|
||
| @ApiPropertyOptional({ description: 'Filter by creator wallet address' }) | ||
| @IsOptional() | ||
| @IsString() | ||
| @IsEthereumAddress() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Reject creator addresses with an invalid mixed-case checksum.
🤖 Prompt for AI Agents |
||
| creator?: string; | ||
|
|
||
| @ApiPropertyOptional({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import { describe, expect, it } from '@jest/globals'; | ||
| import { BadRequestException, ValidationPipe } from '@nestjs/common'; | ||
| import { PaginationQueryDto } from './pagination-query.dto'; | ||
|
|
||
| describe('PaginationQueryDto', () => { | ||
| const pipe = new ValidationPipe({ | ||
| transform: true, | ||
| whitelist: true, | ||
| forbidNonWhitelisted: true, | ||
| }); | ||
|
|
||
| const validate = async (value: Record<string, unknown>) => | ||
| pipe.transform(value, { | ||
| type: 'query', | ||
| metatype: PaginationQueryDto, | ||
| data: '', | ||
| }); | ||
|
|
||
| it('uses the default page limit when limit is omitted', async () => { | ||
| const result = await validate({}); | ||
|
|
||
| expect(result).toBeInstanceOf(PaginationQueryDto); | ||
| expect((result as PaginationQueryDto).limit).toBe(20); | ||
| }); | ||
|
|
||
| it('accepts the minimum and maximum page limits', async () => { | ||
| await expect(validate({ limit: '1' })).resolves.toMatchObject({ | ||
| limit: 1, | ||
| }); | ||
|
|
||
| await expect(validate({ limit: '100' })).resolves.toMatchObject({ | ||
| limit: 100, | ||
| }); | ||
| }); | ||
|
|
||
| it.each(['0', '-1', '101'])( | ||
| 'rejects an out-of-range limit: %s', | ||
| async (limit) => { | ||
| await expect(validate({ limit })).rejects.toBeInstanceOf( | ||
| BadRequestException, | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| it('rejects malformed numeric input instead of partially parsing it', async () => { | ||
| await expect(validate({ limit: '20abc' })).rejects.toBeInstanceOf( | ||
| BadRequestException, | ||
| ); | ||
| }); | ||
|
|
||
| it('rejects unknown query fields', async () => { | ||
| await expect( | ||
| validate({ limit: '20', unexpected: 'value' }), | ||
| ).rejects.toBeInstanceOf(BadRequestException); | ||
| }); | ||
|
|
||
| it('accepts an optional string cursor', async () => { | ||
| const result = await validate({ | ||
| cursor: 'opaque-cursor', | ||
| limit: '20', | ||
| }); | ||
|
|
||
| expect(result).toMatchObject({ | ||
| cursor: 'opaque-cursor', | ||
| limit: 20, | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import { ApiPropertyOptional } from '@nestjs/swagger'; | ||
| import { Type } from 'class-transformer'; | ||
| import { IsInt, IsOptional, IsString, Max, Min } from 'class-validator'; | ||
|
|
||
| export const DEFAULT_PAGE_LIMIT = 20; | ||
| export const MAX_PAGE_LIMIT = 100; | ||
|
|
||
| export class PaginationQueryDto { | ||
| @ApiPropertyOptional({ | ||
| description: 'Opaque cursor from a previous response', | ||
| }) | ||
| @IsOptional() | ||
| @IsString() | ||
| cursor?: string; | ||
|
|
||
| @ApiPropertyOptional({ | ||
| default: DEFAULT_PAGE_LIMIT, | ||
| minimum: 1, | ||
| maximum: MAX_PAGE_LIMIT, | ||
| }) | ||
| @IsOptional() | ||
| @Type(() => Number) | ||
| @IsInt() | ||
| @Min(1) | ||
| @Max(MAX_PAGE_LIMIT) | ||
| limit: number = DEFAULT_PAGE_LIMIT; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: DigiNodes/truthbounty-api
Length of output: 5923
🏁 Script executed:
Repository: DigiNodes/truthbounty-api
Length of output: 41294
Assert both parser limits.
The test checks only parser names. It would pass if either
limit: '100kb'option were removed. No other reachable test covers this bound.Mock the Express parser factories and assert their options.
Suggested fix
import { ValidationPipe } from '`@nestjs/common`'; import { describe, expect, it, jest } from '`@jest/globals`'; +import { json, urlencoded } from 'express'; import { configureApp } from './bootstrap'; +jest.mock('express', () => ({ + json: jest.fn().mockReturnValue({ name: 'jsonParser' }), + urlencoded: jest.fn().mockReturnValue({ name: 'urlencodedParser' }), +})); + ... expect(jsonParser).toHaveProperty('name', 'jsonParser'); expect(urlencodedParser).toHaveProperty('name', 'urlencodedParser'); + expect(json).toHaveBeenCalledWith({ limit: '100kb' }); + expect(urlencoded).toHaveBeenCalledWith({ + extended: true, + limit: '100kb', + });🤖 Prompt for AI Agents