From 203f6cc5bed46d7ac6e8fb5172d2e5832864adaf Mon Sep 17 00:00:00 2001 From: Charis Daniels Date: Sat, 29 Aug 2026 20:07:48 +0000 Subject: [PATCH] fix: rate limiting, filter/sort allowlists, overflow bounds, and bench CI gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the four Stellar Wave issues assigned to DanielCharis1: - #295: rate-limit auth, wallet, and token-validation flows. The RateLimiterModule is now global, the guard emits standard 429 retry metadata (Retry-After + X-RateLimit-* headers, retryAfterSeconds in the body), supports account/wallet-aware keys, and lockout behavior is covered by new service + guard tests. - #277: reject arbitrary sortBy/sortOrder values in puzzle-review via runtime allowlists mapping public keys to known columns (the old code interpolated raw query strings into the ORDER BY clause). - #265: explicit checked arithmetic for question ids, per-level indices, attempts, and last_question_index, failing with a defined ArithmeticOverflow error; min/max and overflow boundary tests added. - #281: wire the previously-unincluded bench/test modules into the contract crate (the CI bench step was producing empty output), fix the duplicated nested bench test, and enforce documented resource-budget baselines (onchain/bench-baselines.json) via a CI check step. Also fixes the stale onchain/Cargo.lock (broke --locked builds) and the un-wired NFT test that referenced undefined bindings. šŸ¤– Generated with Codebuff Co-Authored-By: Codebuff --- .github/workflows/build.yml | 9 +- backend/src/app.module.ts | 2 + .../src/auth/controllers/auth.controller.ts | 15 +- .../controllers/puzzle-review.controller.ts | 26 +- .../services/puzzle-review.service.spec.ts | 81 ++++ .../services/puzzle-review.service.ts | 29 +- .../puzzle-submission.controller.ts | 4 +- .../src/rate-limiter/rate-limit.decorator.ts | 3 +- .../src/rate-limiter/rate-limit.guard.spec.ts | 178 ++++++++ backend/src/rate-limiter/rate-limit.guard.ts | 72 +++- .../src/rate-limiter/rate-limit.interface.ts | 11 + .../src/rate-limiter/rate-limiter.module.ts | 18 +- .../rate-limiter/rate-limiter.service.spec.ts | 91 ++++ .../src/rate-limiter/rate-limiter.service.ts | 40 +- backend/src/wallet/wallet.controller.spec.ts | 6 + backend/src/wallet/wallet.controller.ts | 23 + onchain/Cargo.lock | 2 + onchain/bench-baselines.json | 20 + onchain/contracts/stellar_hunts/src/bench.rs | 68 +-- onchain/contracts/stellar_hunts/src/lib.rs | 81 ++-- onchain/contracts/stellar_hunts/src/test.rs | 405 ++++++++++-------- .../contracts/stellar_hunts_nft/src/lib.rs | 5 +- .../contracts/stellar_hunts_nft/src/test.rs | 8 +- scripts/check-bench-budgets.py | 101 +++++ 24 files changed, 977 insertions(+), 321 deletions(-) create mode 100644 backend/src/puzzle-review/puzzle-review/services/puzzle-review.service.spec.ts create mode 100644 backend/src/rate-limiter/rate-limit.guard.spec.ts create mode 100644 backend/src/rate-limiter/rate-limiter.service.spec.ts create mode 100644 onchain/bench-baselines.json create mode 100644 scripts/check-bench-budgets.py diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 56f8b905..71e16b13 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -121,11 +121,18 @@ jobs: # ā”€ā”€ Resource bench ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ # Bench tests for submit_answer budget (issue #34). Output is # captured as an artifact so budget regressions are visible in - # the CI run summary. + # the CI run summary, and then enforced against the documented + # baselines in onchain/bench-baselines.json (issue #281) ā€” a + # material regression fails the job. - name: Run resource bench working-directory: onchain run: cargo test --workspace --locked -- bench_ --nocapture 2>&1 | tee bench-output.txt + - name: Check bench budget thresholds + # Fails when any measured metric exceeds its baseline or when + # the bench produced no measurements at all (silent regression). + run: python3 scripts/check-bench-budgets.py onchain/bench-output.txt + - name: Upload bench artifact uses: actions/upload-artifact@v4 with: diff --git a/backend/src/app.module.ts b/backend/src/app.module.ts index 71d80d40..2665827b 100644 --- a/backend/src/app.module.ts +++ b/backend/src/app.module.ts @@ -31,6 +31,7 @@ import { PuzzleCategoryModule } from './puzzle-category/puzzle-category.module'; import { PuzzleDependencyModule } from './puzzle-dependency/puzzle-dependency.module'; import { PuzzleModule } from './puzzle/puzzle.module'; import { PuzzleSubmissionModule } from './puzzle-submission/puzzle-submission.module'; +import { RateLimiterModule } from './rate-limiter/rate-limiter.module'; import { PuzzleTranslationModule } from './puzzle-translation/puzzle-translation.module'; import { ReferralModule } from './referral/referral.module'; import { ReportModule } from './report/report.module'; @@ -131,6 +132,7 @@ import { GracefulShutdownService } from './graceful-shutdown.service'; PuzzleModule, PuzzleSubmissionModule, PuzzleTranslationModule, + RateLimiterModule, ReferralModule, ReportModule, RewardShopModule, diff --git a/backend/src/auth/controllers/auth.controller.ts b/backend/src/auth/controllers/auth.controller.ts index 3ba406cf..465e9020 100644 --- a/backend/src/auth/controllers/auth.controller.ts +++ b/backend/src/auth/controllers/auth.controller.ts @@ -21,6 +21,8 @@ import { AuthResponseDto } from '../dto/auth-response.dto'; import { RegisterDto } from '../dto/register.dto'; import { LoginDto } from '../dto/login.dto'; import { JwtAuthGuard } from '../guards/jwt-auth.guard'; +import { RateLimit } from '../../rate-limiter/rate-limit.decorator'; +import { RateLimitGuard } from '../../rate-limiter/rate-limit.guard'; import { User } from '../entities/user.entity'; @ApiTags('Authentication') @@ -30,6 +32,8 @@ export class AuthController { @Post('register') @Auth(AuthType.None) + @UseGuards(RateLimitGuard) + @RateLimit({ ttl: 900, limit: 10 }) @HttpCode(HttpStatus.CREATED) @ApiOperation({ summary: 'Register a new user', @@ -60,6 +64,14 @@ export class AuthController { @Post('login') @Auth(AuthType.None) // Public route + @UseGuards(RateLimitGuard) + // Account-aware throttle: keyed by email so brute-forcing one account + // is limited even when the attacker rotates IPs. Falls back to IP. + @RateLimit({ + ttl: 900, + limit: 10, + keyGenerator: (req) => req.body?.email, + }) @HttpCode(HttpStatus.OK) @ApiOperation({ summary: 'User login', @@ -109,7 +121,8 @@ export class AuthController { } @Post('validate-token') - @UseGuards(JwtAuthGuard) + @UseGuards(JwtAuthGuard, RateLimitGuard) + @RateLimit({ ttl: 60, limit: 30 }) @ApiBearerAuth() @HttpCode(HttpStatus.OK) @ApiOperation({ diff --git a/backend/src/puzzle-review/puzzle-review/controllers/puzzle-review.controller.ts b/backend/src/puzzle-review/puzzle-review/controllers/puzzle-review.controller.ts index c19d74c2..a1f0946e 100644 --- a/backend/src/puzzle-review/puzzle-review/controllers/puzzle-review.controller.ts +++ b/backend/src/puzzle-review/puzzle-review/controllers/puzzle-review.controller.ts @@ -1,4 +1,5 @@ import { + BadRequestException, Controller, Get, Post, @@ -32,6 +33,12 @@ import type { ReviewStats, } from '../interfaces/review.interface'; +// Runtime allowlists (issue #277). TS union types are compile-time only ā€” +// query strings reach the controller as arbitrary values, so they are +// validated here before they can reach the query builder. +const REVIEW_SORT_FIELDS = new Set(['createdAt', 'rating', 'helpfulCount']); +const SORT_ORDERS = new Set(['ASC', 'DESC']); + @ApiTags('Puzzle Reviews') @Controller('puzzle-reviews') export class PuzzleReviewController { @@ -313,8 +320,23 @@ export class PuzzleReviewController { if (minRating && minRating > 0) filters.minRating = minRating; if (maxRating && maxRating > 0) filters.maxRating = maxRating; if (reviewType) filters.reviewType = reviewType; - if (sortBy) filters.sortBy = sortBy; - if (sortOrder) filters.sortOrder = sortOrder; + if (sortBy !== undefined) { + if (!REVIEW_SORT_FIELDS.has(sortBy)) { + throw new BadRequestException( + `Invalid sortBy field: "${sortBy}"`, + ); + } + filters.sortBy = sortBy; + } + if (sortOrder !== undefined) { + const normalized = sortOrder.toUpperCase(); + if (!SORT_ORDERS.has(normalized)) { + throw new BadRequestException( + `Invalid sortOrder: "${sortOrder}" (expected ASC or DESC)`, + ); + } + filters.sortOrder = normalized as 'ASC' | 'DESC'; + } const result = await this.reviewService.getReviews(filters, page, limit); diff --git a/backend/src/puzzle-review/puzzle-review/services/puzzle-review.service.spec.ts b/backend/src/puzzle-review/puzzle-review/services/puzzle-review.service.spec.ts new file mode 100644 index 00000000..8fab2b35 --- /dev/null +++ b/backend/src/puzzle-review/puzzle-review/services/puzzle-review.service.spec.ts @@ -0,0 +1,81 @@ +import { BadRequestException } from '@nestjs/common'; +import { PuzzleReviewService } from './puzzle-review.service'; + +function createMockQueryBuilder() { + const queryBuilder: any = { + andWhere: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + skip: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + getCount: jest.fn().mockResolvedValue(0), + getMany: jest.fn().mockResolvedValue([]), + }; + return queryBuilder; +} + +describe('PuzzleReviewService (filter/sort allowlists)', () => { + let service: PuzzleReviewService; + let repository: any; + + beforeEach(() => { + repository = { + createQueryBuilder: jest.fn(), + }; + service = new PuzzleReviewService(repository); + }); + + it('maps allowlisted sort keys to known database columns', async () => { + const queryBuilder = createMockQueryBuilder(); + repository.createQueryBuilder.mockReturnValue(queryBuilder); + + await service.getReviews({ sortBy: 'helpfulCount', sortOrder: 'ASC' }); + + expect(queryBuilder.orderBy).toHaveBeenCalledWith( + 'review.helpfulCount', + 'ASC', + ); + }); + + it('defaults sorting to createdAt DESC', async () => { + const queryBuilder = createMockQueryBuilder(); + repository.createQueryBuilder.mockReturnValue(queryBuilder); + + await service.getReviews({}); + + expect(queryBuilder.orderBy).toHaveBeenCalledWith( + 'review.createdAt', + 'DESC', + ); + }); + + it('rejects sort fields outside the allowlist', async () => { + const queryBuilder = createMockQueryBuilder(); + repository.createQueryBuilder.mockReturnValue(queryBuilder); + + await expect( + service.getReviews({ sortBy: 'createdAt); DROP TABLE reviews;--' as any }), + ).rejects.toThrow(BadRequestException); + + expect(queryBuilder.orderBy).not.toHaveBeenCalled(); + }); + + it('rejects lowercase or arbitrary sort orders', async () => { + const queryBuilder = createMockQueryBuilder(); + repository.createQueryBuilder.mockReturnValue(queryBuilder); + + await expect( + service.getReviews({ sortOrder: 'desc; DROP TABLE reviews;--' as any }), + ).rejects.toThrow(BadRequestException); + + expect(queryBuilder.orderBy).not.toHaveBeenCalled(); + }); + + it('accepts case-insensitive valid sort orders', async () => { + const queryBuilder = createMockQueryBuilder(); + repository.createQueryBuilder.mockReturnValue(queryBuilder); + + await service.getReviews({ sortBy: 'rating', sortOrder: 'asc' as any }); + + expect(queryBuilder.orderBy).toHaveBeenCalledWith('review.rating', 'ASC'); + }); +}); diff --git a/backend/src/puzzle-review/puzzle-review/services/puzzle-review.service.ts b/backend/src/puzzle-review/puzzle-review/services/puzzle-review.service.ts index 5766eba4..d315451b 100644 --- a/backend/src/puzzle-review/puzzle-review/services/puzzle-review.service.ts +++ b/backend/src/puzzle-review/puzzle-review/services/puzzle-review.service.ts @@ -21,6 +21,17 @@ import type { ReviewValidationResult, } from '../interfaces/review.interface'; +// Allowlist mapping public sort keys to known database columns (issue +// #277). Arbitrary strings are never interpolated into the query builder; +// anything not in this map is rejected with a 400. +const REVIEW_SORT_COLUMNS: Record = { + createdAt: 'review.createdAt', + rating: 'review.rating', + helpfulCount: 'review.helpfulCount', +}; + +const SORT_ORDERS = new Set(['ASC', 'DESC']); + @Injectable() export class PuzzleReviewService { private readonly logger = new Logger(PuzzleReviewService.name); @@ -298,10 +309,22 @@ export class PuzzleReviewService { // Get total count const total = await queryBuilder.getCount(); - // Apply sorting + // Apply sorting. `sortBy` / `sortOrder` are runtime inputs (TS union + // types do not validate them), so map them through the allowlists + // before touching the query builder ā€” never interpolate raw values. const sortBy = filters?.sortBy || 'createdAt'; - const sortOrder = filters?.sortOrder || 'DESC'; - queryBuilder.orderBy(`review.${sortBy}`, sortOrder); + const sortColumn = REVIEW_SORT_COLUMNS[sortBy]; + if (!sortColumn) { + throw new BadRequestException(`Invalid sortBy field: "${sortBy}"`); + } + + const sortOrder = (filters?.sortOrder || 'DESC').toUpperCase(); + if (!SORT_ORDERS.has(sortOrder)) { + throw new BadRequestException( + `Invalid sortOrder: "${sortOrder}" (expected ASC or DESC)`, + ); + } + queryBuilder.orderBy(sortColumn, sortOrder as 'ASC' | 'DESC'); // Apply pagination const reviews = await queryBuilder diff --git a/backend/src/puzzle-submission/puzzle-submission.controller.ts b/backend/src/puzzle-submission/puzzle-submission.controller.ts index 92402d43..27726cde 100644 --- a/backend/src/puzzle-submission/puzzle-submission.controller.ts +++ b/backend/src/puzzle-submission/puzzle-submission.controller.ts @@ -1,7 +1,7 @@ import { Controller, Post, Body, UseGuards } from '@nestjs/common'; import { PuzzleSubmissionService } from './puzzle-submission.service'; -import { RateLimit } from 'src/rate-limiter/rate-limit.decorator'; -import { RateLimitGuard } from 'src/rate-limiter/rate-limit.guard'; +import { RateLimit } from '../rate-limiter/rate-limit.decorator'; +import { RateLimitGuard } from '../rate-limiter/rate-limit.guard'; @Controller('puzzle-submission') export class PuzzleSubmissionController { diff --git a/backend/src/rate-limiter/rate-limit.decorator.ts b/backend/src/rate-limiter/rate-limit.decorator.ts index b06639c1..7e1ad511 100644 --- a/backend/src/rate-limiter/rate-limit.decorator.ts +++ b/backend/src/rate-limiter/rate-limit.decorator.ts @@ -1,5 +1,6 @@ import { SetMetadata } from '@nestjs/common'; import { RATE_LIMIT_KEY } from './rate-limit.guard'; +import type { RateLimitConfig } from './rate-limit.interface'; -export const RateLimit = (config: { ttl: number; limit: number }) => +export const RateLimit = (config: RateLimitConfig) => SetMetadata(RATE_LIMIT_KEY, config); diff --git a/backend/src/rate-limiter/rate-limit.guard.spec.ts b/backend/src/rate-limiter/rate-limit.guard.spec.ts new file mode 100644 index 00000000..9837baff --- /dev/null +++ b/backend/src/rate-limiter/rate-limit.guard.spec.ts @@ -0,0 +1,178 @@ +import { ExecutionContext, HttpException, HttpStatus } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { RateLimitGuard, RATE_LIMIT_KEY } from './rate-limit.guard'; +import { RateLimiterService } from './rate-limiter.service'; + +function buildContext(overrides: { + request?: any; + handlerName?: string; + className?: string; +} = {}): ExecutionContext { + const request = overrides.request || {}; + return { + getHandler: () => ({ name: overrides.handlerName || 'handler' }), + getClass: () => ({ name: overrides.className || 'Controller' }), + switchToHttp: () => ({ + getRequest: () => request, + getResponse: () => request.__response, + }), + } as unknown as ExecutionContext; +} + +describe('RateLimitGuard', () => { + let reflector: Reflector; + let service: RateLimiterService; + let guard: RateLimitGuard; + + beforeEach(() => { + reflector = new Reflector(); + service = new RateLimiterService(); + guard = new RateLimitGuard(reflector, service); + }); + + it('passes when no rate limit metadata is present', () => { + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue(undefined); + + const context = buildContext(); + expect(guard.canActivate(context)).toBe(true); + }); + + it('passes requests within the limit', () => { + const config = { ttl: 60, limit: 5 }; + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue(config); + + const context = buildContext({ + request: { + ip: '1.2.3.4', + headers: {}, + __response: { setHeader: jest.fn() }, + }, + }); + + expect(guard.canActivate(context)).toBe(true); + }); + + it('throws 429 with retry metadata and sets Retry-After when limited', () => { + const config = { ttl: 60, limit: 1 }; + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue(config); + + const setHeader = jest.fn(); + const request = { + ip: '1.2.3.4', + headers: {}, + __response: { setHeader }, + }; + const context = buildContext({ request }); + + guard.canActivate(context); // consumes the only allowed request + + try { + guard.canActivate(context); + throw new Error('expected guard to throw'); + } catch (error) { + expect(error).toBeInstanceOf(HttpException); + const exception = error as HttpException; + expect(exception.getStatus()).toBe(HttpStatus.TOO_MANY_REQUESTS); + const body = exception.getResponse() as any; + expect(body.retryAfterSeconds).toBeGreaterThan(0); + expect(body.message).toContain('Too many requests'); + } + + expect(setHeader).toHaveBeenCalledWith( + 'Retry-After', + expect.any(String), + ); + expect(setHeader).toHaveBeenCalledWith('X-RateLimit-Limit', '1'); + expect(setHeader).toHaveBeenCalledWith('X-RateLimit-Remaining', '0'); + expect(setHeader).toHaveBeenCalledWith( + 'X-RateLimit-Reset', + expect.any(String), + ); + }); + + it('keys by user id when the request is authenticated', () => { + const config = { ttl: 60, limit: 5 }; + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue(config); + + const checkSpy = jest.spyOn(service, 'check'); + const context = buildContext({ + request: { + ip: '1.2.3.4', + user: { id: 'user-42' }, + headers: {}, + }, + }); + + guard.canActivate(context); + + expect(checkSpy).toHaveBeenCalledWith( + 'rate:user:user-42:handler', + 60, + 5, + ); + }); + + it('keys by client IP for anonymous traffic, honoring x-forwarded-for', () => { + const config = { ttl: 60, limit: 5 }; + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue(config); + + const checkSpy = jest.spyOn(service, 'check'); + const context = buildContext({ + request: { + ip: '203.0.113.9', + headers: { 'x-forwarded-for': '198.51.100.7, 10.0.0.1' }, + }, + }); + + guard.canActivate(context); + + expect(checkSpy).toHaveBeenCalledWith( + 'rate:ip:198.51.100.7:handler', + 60, + 5, + ); + }); + + it('uses the custom key generator when provided (account-aware)', () => { + const config = { + ttl: 900, + limit: 10, + keyGenerator: (req: any) => req.body?.email, + }; + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue(config); + + const checkSpy = jest.spyOn(service, 'check'); + const context = buildContext({ + request: { + ip: '1.2.3.4', + body: { email: 'attacker@example.com' }, + headers: {}, + }, + }); + + guard.canActivate(context); + + expect(checkSpy).toHaveBeenCalledWith( + 'rate:attacker@example.com:handler', + 900, + 10, + ); + }); + + it('isolates limits per client IP', () => { + const config = { ttl: 60, limit: 1 }; + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue(config); + + const first = buildContext({ + request: { ip: '1.2.3.4', headers: {}, __response: { setHeader: jest.fn() } }, + }); + const second = buildContext({ + request: { ip: '5.6.7.8', headers: {}, __response: { setHeader: jest.fn() } }, + }); + + guard.canActivate(first); // exhausts 1.2.3.4 + + expect(guard.canActivate(second)).toBe(true); + expect(() => guard.canActivate(first)).toThrow(HttpException); + }); +}); diff --git a/backend/src/rate-limiter/rate-limit.guard.ts b/backend/src/rate-limiter/rate-limit.guard.ts index 604a15f6..d4860d94 100644 --- a/backend/src/rate-limiter/rate-limit.guard.ts +++ b/backend/src/rate-limiter/rate-limit.guard.ts @@ -2,10 +2,12 @@ import { CanActivate, ExecutionContext, Injectable, - ForbiddenException, + HttpException, + HttpStatus, } from '@nestjs/common'; import { Reflector } from '@nestjs/core'; import { RateLimiterService } from './rate-limiter.service'; +import type { RateLimitConfig } from './rate-limit.interface'; export const RATE_LIMIT_KEY = 'rate-limit'; @@ -17,35 +19,65 @@ export class RateLimitGuard implements CanActivate { ) {} canActivate(context: ExecutionContext): boolean { - const config = this.reflector.get<{ ttl: number; limit: number }>( + const config = this.reflector.getAllAndOverride( RATE_LIMIT_KEY, - context.getHandler(), + [context.getHandler(), context.getClass()], ); if (!config) return true; const request = context.switchToHttp().getRequest(); - const ip = - (request.headers['x-forwarded-for'] as string)?.split(',')[0]?.trim() || - request.ip || - request.connection.remoteAddress; - const userId = request.user?.id; - const key = userId - ? `rate:${userId}:${context.getHandler().name}` - : `rate:${ip}:${context.getHandler().name}`; - - const isLimited = this.rateLimiterService.isRateLimited( - key, - config.ttl, - config.limit, - ); + const key = this.buildKey(request, config, context); + const result = this.rateLimiterService.check(key, config.ttl, config.limit); + + if (result.limited) { + const response = context.switchToHttp().getResponse(); + const resetAt = + Math.floor(Date.now() / 1000) + result.retryAfterSeconds; + + response?.setHeader?.('Retry-After', String(result.retryAfterSeconds)); + response?.setHeader?.('X-RateLimit-Limit', String(config.limit)); + response?.setHeader?.('X-RateLimit-Remaining', '0'); + response?.setHeader?.('X-RateLimit-Reset', String(resetAt)); - if (isLimited) { - throw new ForbiddenException( - 'Too many requests. Please try again later.', + throw new HttpException( + { + statusCode: HttpStatus.TOO_MANY_REQUESTS, + message: 'Too many requests. Please try again later.', + retryAfterSeconds: result.retryAfterSeconds, + }, + HttpStatus.TOO_MANY_REQUESTS, ); } return true; } + + private buildKey( + request: any, + config: RateLimitConfig, + context: ExecutionContext, + ): string { + const scope = context.getHandler().name; + + const customKey = config.keyGenerator?.(request); + if (customKey) { + return `rate:${customKey}:${scope}`; + } + + const userId = request.user?.id; + if (userId) { + return `rate:user:${userId}:${scope}`; + } + + return `rate:ip:${this.getClientIp(request)}:${scope}`; + } + + private getClientIp(request: any): string { + const forwarded = request.headers?.['x-forwarded-for']; + if (typeof forwarded === 'string' && forwarded.length > 0) { + return forwarded.split(',')[0].trim(); + } + return request.ip || request.connection?.remoteAddress || 'unknown'; + } } diff --git a/backend/src/rate-limiter/rate-limit.interface.ts b/backend/src/rate-limiter/rate-limit.interface.ts index 60cd42c0..abb813e0 100644 --- a/backend/src/rate-limiter/rate-limit.interface.ts +++ b/backend/src/rate-limiter/rate-limit.interface.ts @@ -1,4 +1,15 @@ +import type { Request } from 'express'; + export interface RateLimitConfig { + /** Window size in seconds. */ ttl: number; + /** Maximum number of requests allowed within the window. */ limit: number; + /** + * Optional per-request key override (e.g. the account email or wallet + * address being targeted). This makes throttling survive IP rotation + * for account-level brute-force protection. Falls back to the + * user/IP-based key when omitted or when the generator returns nothing. + */ + keyGenerator?: (request: Request) => string | undefined; } diff --git a/backend/src/rate-limiter/rate-limiter.module.ts b/backend/src/rate-limiter/rate-limiter.module.ts index b36f4dde..591316fe 100644 --- a/backend/src/rate-limiter/rate-limiter.module.ts +++ b/backend/src/rate-limiter/rate-limiter.module.ts @@ -1,14 +1,10 @@ -import { Module, DynamicModule } from '@nestjs/common'; +import { Module, Global } from '@nestjs/common'; import { RateLimiterService } from './rate-limiter.service'; import { RateLimitGuard } from './rate-limit.guard'; -@Module({}) -export class RateLimiterModule { - static forRoot(): DynamicModule { - return { - module: RateLimiterModule, - providers: [RateLimiterService, RateLimitGuard], - exports: [RateLimiterService, RateLimitGuard], - }; - } -} +@Global() +@Module({ + providers: [RateLimiterService, RateLimitGuard], + exports: [RateLimiterService, RateLimitGuard], +}) +export class RateLimiterModule {} diff --git a/backend/src/rate-limiter/rate-limiter.service.spec.ts b/backend/src/rate-limiter/rate-limiter.service.spec.ts new file mode 100644 index 00000000..43516d67 --- /dev/null +++ b/backend/src/rate-limiter/rate-limiter.service.spec.ts @@ -0,0 +1,91 @@ +import { RateLimiterService } from './rate-limiter.service'; + +describe('RateLimiterService', () => { + let service: RateLimiterService; + + beforeEach(() => { + service = new RateLimiterService(); + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + service.onModuleDestroy(); + }); + + it('allows requests up to the limit', () => { + const ttl = 60; + const limit = 3; + + expect(service.check('key-a', ttl, limit)).toEqual({ + limited: false, + retryAfterSeconds: 0, + }); + expect(service.check('key-a', ttl, limit)).toEqual({ + limited: false, + retryAfterSeconds: 0, + }); + expect(service.check('key-a', ttl, limit)).toEqual({ + limited: false, + retryAfterSeconds: 0, + }); + }); + + it('locks the key out once the limit is hit', () => { + const ttl = 60; + const limit = 2; + + service.check('key-a', ttl, limit); + service.check('key-a', ttl, limit); + + const result = service.check('key-a', ttl, limit); + expect(result.limited).toBe(true); + expect(result.retryAfterSeconds).toBeGreaterThan(0); + }); + + it('reports a positive retry-after while locked out', () => { + const ttl = 60; + const limit = 1; + + service.check('key-a', ttl, limit); + const result = service.check('key-a', ttl, limit); + + expect(result.limited).toBe(true); + expect(result.retryAfterSeconds).toBe(60); + }); + + it('resets the window after the TTL expires', () => { + const ttl = 60; + const limit = 1; + + service.check('key-a', ttl, limit); + expect(service.check('key-a', ttl, limit).limited).toBe(true); + + // Advance time past the window; the key must be admitted again. + jest.advanceTimersByTime(61_000); + + expect(service.check('key-a', ttl, limit)).toEqual({ + limited: false, + retryAfterSeconds: 0, + }); + }); + + it('keeps separate counters for separate keys', () => { + const ttl = 60; + const limit = 1; + + service.check('ip:1.2.3.4', ttl, limit); + + expect(service.check('ip:1.2.3.4', ttl, limit).limited).toBe(true); + expect(service.check('ip:5.6.7.8', ttl, limit).limited).toBe(false); + }); + + it('exposes the backwards-compatible isRateLimited check', () => { + const ttl = 60; + const limit = 1; + + service.check('key-a', ttl, limit); + expect(service.isRateLimited('key-a', ttl, limit)).toBe(true); + expect(service.isRateLimited('key-b', ttl, limit)).toBe(false); + }); +}); diff --git a/backend/src/rate-limiter/rate-limiter.service.ts b/backend/src/rate-limiter/rate-limiter.service.ts index 196d736d..59391cfe 100644 --- a/backend/src/rate-limiter/rate-limiter.service.ts +++ b/backend/src/rate-limiter/rate-limiter.service.ts @@ -1,8 +1,18 @@ import { Injectable, OnModuleInit, OnModuleDestroy } from '@nestjs/common'; +interface RateLimitEntry { + count: number; + expiresAt: number; +} + +export interface RateLimitResult { + limited: boolean; + retryAfterSeconds: number; +} + @Injectable() export class RateLimiterService implements OnModuleInit, OnModuleDestroy { - private requestsMap = new Map(); + private requestsMap = new Map(); private evictionTimer: ReturnType | null = null; onModuleInit() { @@ -13,20 +23,40 @@ export class RateLimiterService implements OnModuleInit, OnModuleDestroy { if (this.evictionTimer) clearInterval(this.evictionTimer); } - isRateLimited(key: string, ttl: number, limit: number): boolean { + /** + * Checks whether `key` has exceeded its limit within the TTL window. + * + * The window is a fixed window: the first request in a window opens it, + * and every request until `ttl` seconds elapse counts toward `limit`. + * Once the limit is hit the key is locked out until the window expires ā€” + * no further requests are admitted, and `retryAfterSeconds` reports how + * long the caller must wait before trying again. + */ + check(key: string, ttl: number, limit: number): RateLimitResult { const now = Date.now(); const entry = this.requestsMap.get(key); if (!entry || now > entry.expiresAt) { this.requestsMap.set(key, { count: 1, expiresAt: now + ttl * 1000 }); - return false; + return { limited: false, retryAfterSeconds: 0 }; } - if (entry.count >= limit) return true; + if (entry.count >= limit) { + const retryAfterSeconds = Math.max( + 1, + Math.ceil((entry.expiresAt - now) / 1000), + ); + return { limited: true, retryAfterSeconds }; + } entry.count += 1; this.requestsMap.set(key, entry); - return false; + return { limited: false, retryAfterSeconds: 0 }; + } + + /** Backwards-compatible boolean check (kept for existing callers). */ + isRateLimited(key: string, ttl: number, limit: number): boolean { + return this.check(key, ttl, limit).limited; } private evictExpired() { diff --git a/backend/src/wallet/wallet.controller.spec.ts b/backend/src/wallet/wallet.controller.spec.ts index dc18d956..56defb08 100644 --- a/backend/src/wallet/wallet.controller.spec.ts +++ b/backend/src/wallet/wallet.controller.spec.ts @@ -1,6 +1,9 @@ import { Test, TestingModule } from '@nestjs/testing'; +import { Reflector } from '@nestjs/core'; import { WalletController } from './wallet.controller'; import { WalletService } from './wallet.service'; +import { RateLimitGuard } from '../rate-limiter/rate-limit.guard'; +import { RateLimiterService } from '../rate-limiter/rate-limiter.service'; describe('WalletController', () => { let controller: WalletController; @@ -10,6 +13,9 @@ describe('WalletController', () => { const module: TestingModule = await Test.createTestingModule({ controllers: [WalletController], providers: [ + Reflector, + RateLimiterService, + RateLimitGuard, { provide: WalletService, useValue: { diff --git a/backend/src/wallet/wallet.controller.ts b/backend/src/wallet/wallet.controller.ts index c680fd23..8bbde15a 100644 --- a/backend/src/wallet/wallet.controller.ts +++ b/backend/src/wallet/wallet.controller.ts @@ -6,21 +6,35 @@ import { Query, HttpCode, HttpStatus, + UseGuards, } from '@nestjs/common'; import { WalletService } from './wallet.service'; import { Wallet } from './entities/wallet.entity'; +import { RateLimit } from '../rate-limiter/rate-limit.decorator'; +import { RateLimitGuard } from '../rate-limiter/rate-limit.guard'; @Controller('wallet') export class WalletController { constructor(private readonly walletService: WalletService) {} @Post('link') + @UseGuards(RateLimitGuard) + @RateLimit({ ttl: 900, limit: 10 }) @HttpCode(HttpStatus.CREATED) async linkWallet(@Body() body: { address: string }): Promise { return this.walletService.linkWallet(body.address); } @Post('verify-signature') + @UseGuards(RateLimitGuard) + // Keyed by wallet address too: limits signature brute-forcing on a + // specific address across IPs, while still throttling anonymous traffic + // by IP. + @RateLimit({ + ttl: 60, + limit: 10, + keyGenerator: (req) => req.body?.address, + }) async verifySignature( @Body() body: { address: string; signature: string; message: string }, ): Promise<{ valid: boolean }> { @@ -33,6 +47,15 @@ export class WalletController { } @Get('verify-signature') + @UseGuards(RateLimitGuard) + @RateLimit({ + ttl: 60, + limit: 10, + keyGenerator: (req) => { + const address = req.query?.address; + return typeof address === 'string' ? address : undefined; + }, + }) async verifySignatureGet( @Query('address') address: string, @Query('signature') signature: string, diff --git a/onchain/Cargo.lock b/onchain/Cargo.lock index 16ed8f8e..05ba5cf4 100644 --- a/onchain/Cargo.lock +++ b/onchain/Cargo.lock @@ -1429,6 +1429,8 @@ name = "stellar-hunts-receiver" version = "0.1.0" dependencies = [ "soroban-sdk", + "stellar-hunts-nft", + "stellar-hunts-types", ] [[package]] diff --git a/onchain/bench-baselines.json b/onchain/bench-baselines.json new file mode 100644 index 00000000..df718d30 --- /dev/null +++ b/onchain/bench-baselines.json @@ -0,0 +1,20 @@ +{ + "description": "Resource-budget baselines for the stellar-hunts contract benchmarks (issue #281). Measured by the bench_* tests in onchain/contracts/stellar_hunts/src/bench.rs and enforced by scripts/check-bench-budgets.py in CI. Raise a baseline only with a documented, justified change to the hot path; any material regression should fail the build.", + "benchmarks": { + "submit_answer_cpu": { + "max": 5000000, + "unit": "instructions", + "notes": "Single submit_answer call (incl. first-call player initialisation). Typical is ~200-500k; 5M is a generous ceiling." + }, + "submit_answer_mem": { + "max": 131072, + "unit": "bytes", + "notes": "Memory cost of a single submit_answer call. 128 KB ceiling." + }, + "ten_submit_answers_avg_cpu": { + "max": 5000000, + "unit": "instructions", + "notes": "Average CPU cost across ten consecutive correct submit_answer calls. Should stay well under the single-call ceiling." + } + } +} diff --git a/onchain/contracts/stellar_hunts/src/bench.rs b/onchain/contracts/stellar_hunts/src/bench.rs index 0ba2cc33..5a181e31 100644 --- a/onchain/contracts/stellar_hunts/src/bench.rs +++ b/onchain/contracts/stellar_hunts/src/bench.rs @@ -12,6 +12,7 @@ use crate::{StellarHunts, StellarHuntsClient}; use soroban_sdk::testutils::Address as _; +use soroban_sdk::testutils::Ledger; use soroban_sdk::{Address, Bytes, Env}; fn b(env: &Env, s: &str) -> Bytes { @@ -28,6 +29,9 @@ fn b(env: &Env, s: &str) -> Bytes { fn bench_submit_answer_cpu_budget() { let env = Env::default(); env.mock_all_auths(); + // Non-zero ledger so the initialised `last_attempt_ledger == 0` does + // not collide with the current ledger (AttemptTooSoon). + env.ledger().set_sequence_number(100_000); let admin = Address::generate(&env); let contract_id = env.register_contract(None, StellarHunts); @@ -54,10 +58,7 @@ fn bench_submit_answer_cpu_budget() { let mem = budget.memory_bytes_cost(); // Log diagnostics when run with --nocapture. - eprintln!( - "submit_answer budget cpu={} mem={} bytes", - cpu, mem - ); + eprintln!("submit_answer budget cpu={} mem={} bytes", cpu, mem); // Budget ceiling: 5M CPU instructions is generous for a single // submit_answer call (typical is ~200-500k). If this ever trips, @@ -83,6 +84,7 @@ fn bench_submit_answer_cpu_budget() { fn bench_ten_submit_answers_amortised() { let env = Env::default(); env.mock_all_auths(); + env.ledger().set_sequence_number(100_000); let admin = Address::generate(&env); let contract_id = env.register_contract(None, StellarHunts); @@ -108,6 +110,10 @@ fn bench_ten_submit_answers_amortised() { let answer = b(&env, &format!("A{}", i)); let ok = client.submit_answer(&player, &((i as u64) + 1), &answer); assert!(ok); + // The contract allows one attempt per ledger (AttemptTooSoon), so + // advance the simulated ledger between submissions. + env.ledger() + .set_sequence_number(env.ledger().sequence() + 1); } let total_cpu = budget.cpu_instruction_cost(); @@ -117,63 +123,9 @@ fn bench_ten_submit_answers_amortised() { "10x submit_answer total_cpu={} avg_cpu={}", total_cpu, avg_cpu ); - assert!( avg_cpu < 5_000_000, "amortised submit_answer CPU budget exceeded: {} avg instructions", avg_cpu ); - - #[test] -fn bench_submit_answer_cpu_budget() { - let env = Env::default(); - env.mock_all_auths(); - - let admin = Address::generate(&env); - let contract_id = env.register_contract(None, StellarHunts); - let client = StellarHuntsClient::new(&env, &contract_id); - client.init(&admin); - - client.set_question_per_level(&1u32); - let level = crate::Levels::Easy; - let question = b(&env, "Bench question"); - let answer = b(&env, "Bench answer"); - let hint = b(&env, "Bench hint"); - client.add_question(&level, &question, &answer, &hint); - - let player = Address::generate(&env); - - // Reset the budget so we only measure the submit_answer call itself. - let mut budget = env.budget(); - budget.reset_default(); - - let ok = client.submit_answer(&player, &1u64, &answer); - assert!(ok); - - let cpu = budget.cpu_instruction_cost(); - let mem = budget.memory_bytes_cost(); - - // Log diagnostics when run with --nocapture. - eprintln!( - "submit_answer budget cpu={} mem={} bytes", - cpu, mem - ); - - // Budget ceiling: 5M CPU instructions is generous for a single - // submit_answer call (typical is ~200-500k). If this ever trips, - // investigate what storage or crypto work is being done on the hot - // path. - assert!( - cpu < 5_000_000, - "submit_answer CPU budget exceeded: {} instructions (max 5_000_000)", - cpu - ); - - // Memory ceiling: 128 KB. - assert!( - mem < 131_072, - "submit_answer memory budget exceeded: {} bytes (max 131_072)", - mem - ); -} } diff --git a/onchain/contracts/stellar_hunts/src/lib.rs b/onchain/contracts/stellar_hunts/src/lib.rs index 6a3648e5..b3922dfc 100644 --- a/onchain/contracts/stellar_hunts/src/lib.rs +++ b/onchain/contracts/stellar_hunts/src/lib.rs @@ -76,11 +76,16 @@ pub enum DataKey { const CURRENT_SCHEMA_VERSION: u32 = 1; fn get_schema_version(e: &Env) -> u32 { - e.storage().persistent().get(&DataKey::SchemaVersion).unwrap_or(0) + e.storage() + .persistent() + .get(&DataKey::SchemaVersion) + .unwrap_or(0) } fn set_schema_version(e: &Env) { - e.storage().persistent().set(&DataKey::SchemaVersion, &CURRENT_SCHEMA_VERSION); + e.storage() + .persistent() + .set(&DataKey::SchemaVersion, &CURRENT_SCHEMA_VERSION); } // --------------------------------------------------------------------- @@ -102,6 +107,7 @@ pub enum Error { MissingNftContract = 9, AttemptTooSoon = 10, LevelImmutable = 11, + ArithmeticOverflow = 12, } // --------------------------------------------------------------------- @@ -139,7 +145,12 @@ impl StellarHunts { .instance() .get(&DataKey::QuestionCount) .unwrap_or(0u64); - let question_id = count + 1; + // Explicit checked arithmetic: at u64::MAX the next question id + // cannot be represented, so the call fails with a defined error + // instead of silently wrapping. + let question_id = count + .checked_add(1) + .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); env.storage() .instance() .set(&DataKey::QuestionCount, &question_id); @@ -172,12 +183,15 @@ impl StellarHunts { panic_with_error!(&env, Error::QuestionPerLevelLimit); } + let next_index = idx + .checked_add(1) + .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); env.storage() .persistent() .set(&DataKey::QuestionsByLevel(level.clone(), idx), &question_id); env.storage() .persistent() - .set(&DataKey::QuestionPerLevelIndex(level.clone()), &(idx + 1)); + .set(&DataKey::QuestionPerLevelIndex(level.clone()), &next_index); env.events() .publish((Symbol::new(&env, "question_added"),), (question_id, level)); @@ -236,14 +250,24 @@ impl StellarHunts { panic_with_error!(&env, Error::QuestionPerLevelLimit); } - env.storage() - .persistent() - .set(&DataKey::QuestionsByLevel(level.clone(), new_idx), &question_id); - env.storage() - .persistent() - .set(&DataKey::QuestionPerLevelIndex(level.clone()), &(new_idx + 1)); + let new_next_index = new_idx + .checked_add(1) + .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); + env.storage().persistent().set( + &DataKey::QuestionsByLevel(level.clone(), new_idx), + &question_id, + ); + env.storage().persistent().set( + &DataKey::QuestionPerLevelIndex(level.clone()), + &new_next_index, + ); - let last_old_idx = old_idx - 1; + // `old_idx` counts the questions in the old level, so it is + // >= 1 whenever the question being moved exists there; a + // checked_sub keeps the underflow behavior explicit anyway. + let last_old_idx = old_idx + .checked_sub(1) + .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); for i in 0..old_idx { let qid: u64 = env .storage() @@ -257,14 +281,13 @@ impl StellarHunts { .persistent() .get(&DataKey::QuestionsByLevel(old_level.clone(), j + 1)) .unwrap_or(0u64); - env.storage().persistent().set( - &DataKey::QuestionsByLevel(old_level.clone(), j), - &next_qid, - ); + env.storage() + .persistent() + .set(&DataKey::QuestionsByLevel(old_level.clone(), j), &next_qid); } - env.storage().persistent().remove( - &DataKey::QuestionsByLevel(old_level.clone(), last_old_idx), - ); + env.storage() + .persistent() + .remove(&DataKey::QuestionsByLevel(old_level.clone(), last_old_idx)); break; } } @@ -310,10 +333,8 @@ impl StellarHunts { env.storage() .persistent() .set(&DataKey::RetiredQuestion(question_id), &true); - env.events().publish( - (Symbol::new(&env, "question_retired"),), - (question_id,), - ); + env.events() + .publish((Symbol::new(&env, "question_retired"),), (question_id,)); } pub fn set_nft_contract_address(env: Env, new_address: Address) { @@ -371,13 +392,19 @@ impl StellarHunts { panic_with_error!(&env, Error::AttemptTooSoon); } lp.last_attempt_ledger = current_ledger; - lp.attempts += 1; + lp.attempts = lp + .attempts + .checked_add(1) + .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); let hashed: BytesN<32> = env.crypto().sha256(&answer).into(); let is_correct = hashed == question.hashed_answer; if is_correct { - lp.last_question_index += 1; + lp.last_question_index = lp + .last_question_index + .checked_add(1) + .unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow)); let per_level: u32 = env .storage() .instance() @@ -643,3 +670,9 @@ fn require_admin(env: &Env) { .unwrap_or_else(|| panic_with_error!(env, Error::NotInitialized)); admin.require_auth(); } + +#[cfg(test)] +mod test; + +#[cfg(test)] +mod bench; diff --git a/onchain/contracts/stellar_hunts/src/test.rs b/onchain/contracts/stellar_hunts/src/test.rs index 3cb1aa45..17d4ca2e 100644 --- a/onchain/contracts/stellar_hunts/src/test.rs +++ b/onchain/contracts/stellar_hunts/src/test.rs @@ -4,8 +4,8 @@ use crate::{StellarHunts, StellarHuntsClient}; // Brings `Address::generate` into scope as an extension trait method. use soroban_sdk::testutils::Address as _; use soroban_sdk::testutils::Ledger; -use soroban_sdk::{Address, Bytes, BytesN, Env}; use soroban_sdk::testutils::{MockAuth, MockAuthInvoke}; +use soroban_sdk::{Address, Bytes, Env, IntoVal}; /// Generate a fresh admin address (distinct from the destructured binding /// returned by `init_with_admin`). @@ -17,7 +17,6 @@ fn user(env: &Env) -> Address { Address::generate(env) } - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct RateLimitConfig { pub max_requests_per_day: u32, @@ -38,7 +37,10 @@ impl Default for RateLimitConfig { impl RateLimitConfig { /// Validates that a proposed update keeps all limits positive. pub fn validate(&self) -> Result<(), &'static str> { - if self.max_requests_per_day == 0 || self.max_value_per_day == 0 || self.chain_daily_limit == 0 { + if self.max_requests_per_day == 0 + || self.max_value_per_day == 0 + || self.chain_daily_limit == 0 + { return Err("rate limit values must be positive"); } Ok(()) @@ -58,12 +60,14 @@ mod tests { #[test] fn rejects_zeroed_limits() { - let cfg = RateLimitConfig { max_requests_per_day: 0, ..RateLimitConfig::default() }; + let cfg = RateLimitConfig { + max_requests_per_day: 0, + ..RateLimitConfig::default() + }; assert!(cfg.validate().is_err()); } } - fn b(env: &Env, s: &str) -> Bytes { Bytes::from_slice(env, s.as_bytes()) } @@ -72,23 +76,39 @@ fn b(env: &Env, s: &str) -> Bytes { // Helper: init contract with selective auth for the `init` call // --------------------------------------------------------------------- -/// Register the contract, grant admin auth specifically for `init`, then -/// call `init`. Returns `(admin, contract_address, client)` so callers -/// can set up further `mock_auths` for subsequent admin/player calls. +/// Register the contract and authorize the admin's `init` call via +/// `mock_all_auths`. Returns `(admin, contract_address, client)` so +/// callers can run subsequent admin/player flows. fn init_with_admin(env: &Env) -> (Address, Address, StellarHuntsClient) { let admin = new_admin(env); - let contract_id: BytesN<32> = env.register_contract(None, StellarHunts); - let contract_address = Address::from_contract_id(env, &contract_id); - let client = StellarHuntsClient::new(env, &contract_id); + // soroban-sdk 22's `register_contract` returns the contract `Address` + // directly (the old `BytesN<32>` + `Address::from_contract_id` pair is + // gone). + let contract_address = env.register_contract(None, StellarHunts); + let client = StellarHuntsClient::new(env, &contract_address); + + env.mock_all_auths(); + client.init(&admin); + (admin, contract_address, client) +} + +/// Register the contract and authorize ONLY the admin's `init` call. +/// Use this in tests that verify admin-gated functions are *not* +/// authorized afterwards (negative auth coverage). The mock entry must +/// carry the exact invocation args of `init` (soroban-sdk 22 matches on +/// them), and each entry authorizes a single call. +fn init_admin_auth_only(env: &Env) -> (Address, Address, StellarHuntsClient) { + let admin = new_admin(env); + let contract_address = env.register_contract(None, StellarHunts); + let client = StellarHuntsClient::new(env, &contract_address); - // Grant admin auth **only** for the `init` call. env.mock_auths(&[MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), + address: &admin, + invoke: &MockAuthInvoke { + contract: &contract_address, fn_name: "init", - args: Vec::new(env), - sub_invokes: Vec::new(env), + args: (&admin,).into_val(env), + sub_invokes: &[], }, }]); @@ -103,19 +123,7 @@ fn init_with_admin(env: &Env) -> (Address, Address, StellarHuntsClient) { #[test] fn test_set_question_per_level_admin_only() { let env = Env::default(); - env.mock_all_auths(); - let (_admin, client) = init_with_admin(&env); - let (admin, contract_address, client) = init_with_admin(&env); - - env.mock_auths(&[MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "set_question_per_level", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }]); + let (_admin, _contract_address, client) = init_with_admin(&env); client.set_question_per_level(&5u32); assert_eq!(client.get_question_per_level(), 5); @@ -128,13 +136,16 @@ fn test_set_question_per_level_admin_only() { #[test] fn test_set_question_per_level_unauthorized() { let env = Env::default(); - let (_admin, _contract_address, client) = init_with_admin(&env); + let (_admin, _contract_address, client) = init_admin_auth_only(&env); // No mock auth for admin + "set_question_per_level" ā†’ require_auth fails. let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { client.set_question_per_level(&5u32); })); - assert!(result.is_err(), "non-admin should not be able to set_question_per_level"); + assert!( + result.is_err(), + "non-admin should not be able to set_question_per_level" + ); } // --------------------------------------------------------------------- @@ -144,37 +155,13 @@ fn test_set_question_per_level_unauthorized() { #[test] fn test_add_and_get_question() { let env = Env::default(); - env.mock_all_auths(); - let (_admin, client) = init_with_admin(&env); - let (admin, contract_address, client) = init_with_admin(&env); + let (_admin, _contract_address, client) = init_with_admin(&env); let level = crate::Levels::Easy; let question = b(&env, "What is the capital of France?"); let answer = b(&env, "Paris"); let hint = b(&env, "It starts with P"); - // Set up admin auth for both admin-only calls. - env.mock_auths(&[ - MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "set_question_per_level", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "add_question", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - ]); - client.set_question_per_level(&5u32); client.add_question(&level, &question, &answer, &hint); @@ -189,7 +176,7 @@ fn test_add_and_get_question() { #[test] fn test_add_question_unauthorized() { let env = Env::default(); - let (_admin, _contract_address, client) = init_with_admin(&env); + let (_admin, _contract_address, client) = init_admin_auth_only(&env); let level = crate::Levels::Easy; let question = b(&env, "Should I be here?"); @@ -199,7 +186,10 @@ fn test_add_question_unauthorized() { let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { client.add_question(&level, &question, &answer, &hint); })); - assert!(result.is_err(), "non-admin should not be able to add_question"); + assert!( + result.is_err(), + "non-admin should not be able to add_question" + ); } // --------------------------------------------------------------------- @@ -209,7 +199,10 @@ fn test_add_question_unauthorized() { #[test] fn test_submit_answer_correct_progresses() { let env = Env::default(); - let (admin, contract_address, client) = init_with_admin(&env); + // Non-zero ledger so the initialised `last_attempt_ledger == 0` does + // not collide with the current ledger (AttemptTooSoon). + env.ledger().set_sequence_number(100_000); + let (_admin, _contract_address, client) = init_with_admin(&env); let player = user(&env); let level = crate::Levels::Easy; @@ -217,37 +210,6 @@ fn test_submit_answer_correct_progresses() { let answer = b(&env, "4"); let hint = b(&env, "basic math"); - // Set up auths for admin (setup) and player (submit_answer). - env.mock_auths(&[ - MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "set_question_per_level", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "add_question", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - MockAuth { - address: player.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "submit_answer", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - ]); - client.set_question_per_level(&1u32); client.add_question(&level, &question, &answer, &hint); @@ -265,7 +227,8 @@ fn test_submit_answer_correct_progresses() { #[test] fn test_submit_answer_incorrect_does_not_progress() { let env = Env::default(); - let (admin, contract_address, client) = init_with_admin(&env); + env.ledger().set_sequence_number(100_000); + let (_admin, _contract_address, client) = init_with_admin(&env); let player = user(&env); let level = crate::Levels::Easy; @@ -274,36 +237,6 @@ fn test_submit_answer_incorrect_does_not_progress() { let wrong = b(&env, "5"); let hint = b(&env, "basic math"); - env.mock_auths(&[ - MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "set_question_per_level", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "add_question", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - MockAuth { - address: player.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "submit_answer", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - ]); - client.set_question_per_level(&1u32); client.add_question(&level, &question, &answer, &hint); @@ -321,7 +254,8 @@ fn test_submit_answer_incorrect_does_not_progress() { #[test] fn test_request_hint_after_initialize() { let env = Env::default(); - let (admin, contract_address, client) = init_with_admin(&env); + env.ledger().set_sequence_number(100_000); + let (_admin, _contract_address, client) = init_with_admin(&env); let player = user(&env); let level = crate::Levels::Easy; @@ -332,45 +266,6 @@ fn test_request_hint_after_initialize() { let a2 = b(&env, "A2"); let h2 = b(&env, "HINT-Y"); - env.mock_auths(&[ - MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "set_question_per_level", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "add_question", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - MockAuth { - address: player.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "submit_answer", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - MockAuth { - address: player.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "request_hint", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }, - ]); - // Two questions per level ā€” answering the first keeps the player on // Easy, so a hint request for question 1 remains valid. client.set_question_per_level(&2u32); @@ -389,20 +284,10 @@ fn test_request_hint_after_initialize() { #[test] fn test_set_nft_contract_address_admin_only() { let env = Env::default(); - let (admin, contract_address, client) = init_with_admin(&env); + let (_admin, _contract_address, client) = init_with_admin(&env); let new_addr = Address::generate(&env); - env.mock_auths(&[MockAuth { - address: admin.clone(), - invoke: MockAuthInvoke { - contract: contract_address.clone(), - fn_name: "set_nft_contract_address", - args: Vec::new(env), - sub_invokes: Vec::new(env), - }, - }]); - client.set_nft_contract_address(&new_addr); assert_eq!(client.get_nft_contract_address(), new_addr); } @@ -414,7 +299,7 @@ fn test_set_nft_contract_address_admin_only() { #[test] fn test_set_nft_contract_address_unauthorized() { let env = Env::default(); - let (_admin, _contract_address, client) = init_with_admin(&env); + let (_admin, _contract_address, client) = init_admin_auth_only(&env); let new_addr = Address::generate(&env); @@ -491,8 +376,7 @@ fn test_claim_level_completion_nft_retry_safe_on_nft_panic() { // Register and initialise the NFT contract, granting the game // contract the minter role. let nft_id = env.register_contract(None, stellar_hunts_nft::StellarHuntsNft); - let nft_client = - stellar_hunts_nft::StellarHuntsNftClient::new(&env, &nft_id); + let nft_client = stellar_hunts_nft::StellarHuntsNftClient::new(&env, &nft_id); nft_client.init( &admin, &contract_id, @@ -526,8 +410,7 @@ fn test_claim_level_completion_nft_retry_safe_on_nft_panic() { // was interrupted before the storage write. env.as_contract(&contract_id, || { let lp_key = crate::DataKey::PlayerLevelProgress(player.clone(), level.clone()); - let mut lp: crate::LevelProgress = - env.storage().persistent().get(&lp_key).unwrap(); + let mut lp: crate::LevelProgress = env.storage().persistent().get(&lp_key).unwrap(); lp.nft_minted = false; env.storage().persistent().set(&lp_key, &lp); }); @@ -572,6 +455,7 @@ fn test_claim_level_completion_nft_retry_safe_on_nft_panic() { fn test_cross_contract_full_happy_path_nft_registered_first() { let env = Env::default(); env.mock_all_auths(); + env.ledger().set_sequence_number(100_000); // Register the NFT contract first, then the game contract ā€” exercises // the "NFT deployed before the game" ordering. This works because @@ -630,6 +514,7 @@ fn test_cross_contract_full_happy_path_nft_registered_first() { fn test_cross_contract_full_happy_path_game_registered_first() { let env = Env::default(); env.mock_all_auths(); + env.ledger().set_sequence_number(100_000); // Reverse order: game contract registered before the NFT contract. let game_admin = new_admin(&env); @@ -683,11 +568,165 @@ fn test_cross_contract_full_happy_path_game_registered_first() { #[test] fn test_schema_version() { let e = Env::default(); - let contract_id = e.register_contract(None, StellarHunts); - let client = StellarHuntsClient::new(&e, &contract_id); - - let admin = new_admin(&e); - client.init(&admin); + let (_admin, _contract_address, client) = init_with_admin(&e); assert_eq!(client.get_schema_version(), 1); } + +// --------------------------------------------------------------------- +// Overflow and boundary behavior (issue #265) +// +// The contract uses checked arithmetic for every counter increment +// (question ids, per-level indices, attempts, last_question_index) and +// panics with `Error::ArithmeticOverflow` (#12) rather than silently +// wrapping. These tests drive each counter to its boundary and assert +// the defined panic behaviour. +// --------------------------------------------------------------------- + +fn panic_text(result: &std::result::Result<(), Box>) -> String { + result + .as_ref() + .err() + .and_then(|e| { + e.downcast_ref::() + .cloned() + .or_else(|| e.downcast_ref::<&str>().map(|s| s.to_string())) + }) + .unwrap_or_default() +} + +#[test] +fn test_add_question_overflows_at_max_question_count() { + let env = Env::default(); + env.mock_all_auths(); + let (_admin, contract_id, client) = init_with_admin(&env); + + // Push QuestionCount to u64::MAX so the next question id cannot be + // represented. + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::DataKey::QuestionCount, &u64::MAX); + }); + + let level = crate::Levels::Easy; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.add_question(&level, &b(&env, "Q"), &b(&env, "A"), &b(&env, "H")); + })); + + assert!(result.is_err(), "add_question must panic on id overflow"); + assert!( + panic_text(&result).contains("Error(Contract, #12)"), + "expected ArithmeticOverflow panic, got: {}", + panic_text(&result) + ); +} + +#[test] +fn test_question_per_level_boundary_respected() { + let env = Env::default(); + env.mock_all_auths(); + let (_admin, _contract_address, client) = init_with_admin(&env); + + let level = crate::Levels::Easy; + client.set_question_per_level(&1u32); + client.add_question(&level, &b(&env, "Q1"), &b(&env, "A1"), &b(&env, "H1")); + + // A second question exceeds the per-level budget and must be rejected + // with QuestionPerLevelLimit rather than overflowing the index. + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.add_question(&level, &b(&env, "Q2"), &b(&env, "A2"), &b(&env, "H2")); + })); + + assert!(result.is_err(), "adding beyond per-level limit must panic"); + assert!( + panic_text(&result).contains("Error(Contract, #8)"), + "expected QuestionPerLevelLimit panic, got: {}", + panic_text(&result) + ); +} + +#[test] +fn test_submit_answer_attempts_overflow_panics() { + let env = Env::default(); + env.mock_all_auths(); + // Non-zero ledger so the initialisation-time `last_attempt_ledger == 0` + // does not collide with the current ledger. + env.ledger().set_sequence_number(100_000); + let (_admin, contract_id, client) = init_with_admin(&env); + + let player = user(&env); + let level = crate::Levels::Easy; + client.set_question_per_level(&5u32); + client.add_question(&level, &b(&env, "Q?"), &b(&env, "A"), &b(&env, "H")); + + // Initialise the player with one (wrong) attempt. + let ok = client.submit_answer(&player, &1u64, &b(&env, "wrong")); + assert!(!ok); + + // Push attempts to u32::MAX directly. + env.as_contract(&contract_id, || { + let key = crate::DataKey::PlayerLevelProgress(player.clone(), level.clone()); + let mut lp: crate::LevelProgress = env.storage().persistent().get(&key).unwrap(); + lp.attempts = u32::MAX; + env.storage().persistent().set(&key, &lp); + }); + + // Advance the ledger so `AttemptTooSoon` does not fire first. + env.ledger() + .set_sequence_number(env.ledger().sequence() + 1); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.submit_answer(&player, &1u64, &b(&env, "wrong")); + })); + + assert!(result.is_err(), "attempts increment must panic at u32::MAX"); + assert!( + panic_text(&result).contains("Error(Contract, #12)"), + "expected ArithmeticOverflow panic, got: {}", + panic_text(&result) + ); +} + +#[test] +fn test_submit_answer_last_question_index_overflow_panics() { + let env = Env::default(); + env.mock_all_auths(); + env.ledger().set_sequence_number(100_000); + let (_admin, contract_id, client) = init_with_admin(&env); + + let player = user(&env); + let level = crate::Levels::Easy; + client.set_question_per_level(&5u32); + client.add_question(&level, &b(&env, "Q?"), &b(&env, "A"), &b(&env, "H")); + + // Initialise the player. + let ok = client.submit_answer(&player, &1u64, &b(&env, "wrong")); + assert!(!ok); + + // Push last_question_index to u32::MAX directly. + env.as_contract(&contract_id, || { + let key = crate::DataKey::PlayerLevelProgress(player.clone(), level.clone()); + let mut lp: crate::LevelProgress = env.storage().persistent().get(&key).unwrap(); + lp.last_question_index = u32::MAX; + env.storage().persistent().set(&key, &lp); + }); + + env.ledger() + .set_sequence_number(env.ledger().sequence() + 1); + + // A correct answer increments last_question_index -> overflow. + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.submit_answer(&player, &1u64, &b(&env, "A")); + })); + + assert!( + result.is_err(), + "correct answer must panic when last_question_index is at max" + ); + assert!( + panic_text(&result).contains("Error(Contract, #12)"), + "expected ArithmeticOverflow panic, got: {}", + panic_text(&result) + ); +} diff --git a/onchain/contracts/stellar_hunts_nft/src/lib.rs b/onchain/contracts/stellar_hunts_nft/src/lib.rs index 5b1e2e13..d9dc2b17 100644 --- a/onchain/contracts/stellar_hunts_nft/src/lib.rs +++ b/onchain/contracts/stellar_hunts_nft/src/lib.rs @@ -5,11 +5,11 @@ extern crate alloc; +use alloc::string::ToString; use soroban_sdk::{ contract, contracterror, contractimpl, contracttype, panic_with_error, Address, Env, String, Symbol, }; -use alloc::string::ToString; // Use the shared `Levels` enum from the types crate so we can compile // standalone without depending on the game contract (which would create @@ -86,8 +86,7 @@ impl StellarHuntsNft { let symbol_text = symbol.to_string(); if base_uri_text.len() > MAX_BASE_URI_LEN - || (!base_uri_text.starts_with("ipfs://") - && !base_uri_text.starts_with("https://")) + || (!base_uri_text.starts_with("ipfs://") && !base_uri_text.starts_with("https://")) { panic_with_error!(&env, Error::InvalidBaseUri); } diff --git a/onchain/contracts/stellar_hunts_nft/src/test.rs b/onchain/contracts/stellar_hunts_nft/src/test.rs index dfb3db41..1e27830e 100644 --- a/onchain/contracts/stellar_hunts_nft/src/test.rs +++ b/onchain/contracts/stellar_hunts_nft/src/test.rs @@ -84,13 +84,7 @@ fn test_double_mint_rejected() { game.mint(&nft_id, &r, &crate::Levels::Easy); // Second mint must fail (already-has-badge error). let should_panic = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - client.init( - &admin, - &game, - &String::from_str(&env, &long_uri), - &String::from_str(&env, "StellarHuntsBadge"), - &String::from_str(&env, "SHB"), - ); + game.mint(&nft_id, &r, &crate::Levels::Easy); })); assert!(should_panic.is_err()); } diff --git a/scripts/check-bench-budgets.py b/scripts/check-bench-budgets.py new file mode 100644 index 00000000..5039a387 --- /dev/null +++ b/scripts/check-bench-budgets.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 +"""Enforce resource-budget baselines for the onchain contract benchmarks. + +Parses the captured `cargo test -- bench_ --nocapture` output (written by +the "Run resource bench" CI step) and fails the job when any measured +metric exceeds its documented baseline in `onchain/bench-baselines.json`, +or when a benchmark produced no measurement at all (which would mean the +bench gate silently stopped running). + +Usage: + python3 scripts/check-bench-budgets.py [path-to-bench-output.txt] +""" + +import json +import re +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +BASELINES_PATH = REPO_ROOT / "onchain" / "bench-baselines.json" +DEFAULT_OUTPUT_PATH = REPO_ROOT / "onchain" / "bench-output.txt" + +# Metrics the bench prints (see onchain/contracts/stellar_hunts/src/bench.rs). +PATTERNS = { + "submit_answer_cpu": re.compile( + r"submit_answer budget\s+cpu=(\d+)\s+mem=(\d+) bytes" + ), + "submit_answer_mem": re.compile( + r"submit_answer budget\s+cpu=(\d+)\s+mem=(\d+) bytes" + ), + "ten_submit_answers_avg_cpu": re.compile( + r"10x submit_answer\s+total_cpu=(\d+)\s+avg_cpu=(\d+)" + ), +} + + +def main() -> int: + output_path = ( + Path(sys.argv[1]) if len(sys.argv) > 1 else DEFAULT_OUTPUT_PATH + ) + + if not output_path.exists(): + print( + f"āŒ Bench output not found at {output_path}. " + "Run the bench first: cargo test --workspace --locked -- bench_ --nocapture", + file=sys.stderr, + ) + return 1 + + baselines = json.loads(BASELINES_PATH.read_text())["benchmarks"] + + measurements: dict[str, int] = {} + for line in output_path.read_text().splitlines(): + cpu_match = PATTERNS["submit_answer_cpu"].search(line) + if cpu_match: + measurements["submit_answer_cpu"] = int(cpu_match.group(1)) + measurements["submit_answer_mem"] = int(cpu_match.group(2)) + continue + + avg_match = PATTERNS["ten_submit_answers_avg_cpu"].search(line) + if avg_match: + measurements["ten_submit_answers_avg_cpu"] = int(avg_match.group(2)) + + failures: list[str] = [] + print(f"{'Metric':<28} {'Measured':>14} {'Max':>14} Status") + print("-" * 70) + for metric, baseline in baselines.items(): + max_value = int(baseline["max"]) + measured = measurements.get(metric) + if measured is None: + failures.append( + f"{metric}: no measurement found in bench output" + ) + print(f"{metric:<28} {'n/a':>14} {max_value:>14} āŒ MISSING") + continue + ok = measured <= max_value + status = "āœ… OK" if ok else "āŒ EXCEEDED" + if not ok: + failures.append( + f"{metric}: {measured} > {max_value} ({baseline.get('unit', '')})" + ) + print(f"{metric:<28} {measured:>14} {max_value:>14} {status}") + print("-" * 70) + + if failures: + print("\nResource budget regression(s) detected:", file=sys.stderr) + for failure in failures: + print(f" - {failure}", file=sys.stderr) + print( + "\nSee onchain/bench-baselines.json for the documented baselines. " + "Investigate the hot path before raising a ceiling.", + file=sys.stderr, + ) + return 1 + + print("\nāœ… All resource budgets within documented baselines.") + return 0 + + +if __name__ == "__main__": + sys.exit(main())