diff --git a/backend/src/main.ts b/backend/src/main.ts index 6370dd8a..5defaf29 100644 --- a/backend/src/main.ts +++ b/backend/src/main.ts @@ -5,6 +5,7 @@ import { DocumentBuilder, SwaggerModule } from '@nestjs/swagger'; import helmet from 'helmet'; import { AppModule } from './app.module'; +import { securityHeadersConfig } from './security-headers'; /** * Hard limit (ms) we allow the graceful shutdown sequence to take before @@ -55,23 +56,7 @@ async function bootstrap(): Promise { configService.get('appConfig.cors.credentials') ?? true, }); - app.use(helmet({ - contentSecurityPolicy: { - directives: { - defaultSrc: ["'self'"], - scriptSrc: ["'self'", "'unsafe-eval'", "'unsafe-inline'"], - styleSrc: ["'self'", "'unsafe-inline'"], - imgSrc: ["'self'", "data:", "blob:", "https:"], - fontSrc: ["'self'"], - connectSrc: ["'self'", "https://soroban-testnet.stellar.org"], - frameAncestors: ["'none'"], - baseUri: ["'self'"], - formAction: ["'self'"], - }, - }, - hsts: { maxAge: 63072000, includeSubDomains: true, preload: true }, - referrerPolicy: { policy: 'strict-origin-when-cross-origin' }, - })); + app.use(helmet(securityHeadersConfig)); // Global request validation (#335). `whitelist` strips properties that are // not declared on the request DTO, and `forbidNonWhitelisted` turns any diff --git a/backend/src/security-headers.ts b/backend/src/security-headers.ts new file mode 100644 index 00000000..68f48a09 --- /dev/null +++ b/backend/src/security-headers.ts @@ -0,0 +1,28 @@ +import type { HelmetOptions } from 'helmet'; + +/** + * Centralised Helmet configuration shared between `bootstrap()` in + * `main.ts` and the security-headers regression test + * (`backend/test/security-headers.e2e-spec.ts`). + * + * Keeping the config here (rather than inline in `main.ts`) lets the e2e + * suite lock the exact production header contract so a future change to + * CSP/HSTS/frame/referrer policy cannot silently regress. See issue #303. + */ +export const securityHeadersConfig: HelmetOptions = { + contentSecurityPolicy: { + directives: { + defaultSrc: ["'self'"], + scriptSrc: ["'self'", "'unsafe-eval'", "'unsafe-inline'"], + styleSrc: ["'self'", "'unsafe-inline'"], + imgSrc: ["'self'", 'data:', 'blob:', 'https:'], + fontSrc: ["'self'"], + connectSrc: ["'self'", 'https://soroban-testnet.stellar.org'], + frameAncestors: ["'none'"], + baseUri: ["'self'"], + formAction: ["'self'"], + }, + }, + hsts: { maxAge: 63072000, includeSubDomains: true, preload: true }, + referrerPolicy: { policy: 'strict-origin-when-cross-origin' }, +}; diff --git a/backend/src/user/entities/user.entity.ts b/backend/src/user/entities/user.entity.ts index fbaf280b..4ab4acd5 100644 --- a/backend/src/user/entities/user.entity.ts +++ b/backend/src/user/entities/user.entity.ts @@ -17,7 +17,7 @@ export class User { @Column({ unique: true }) email: string; - @Column({ nullable: true }) + @Column({ nullable: true, unique: true }) walletAddress?: string; @Column({ type: 'text', nullable: true }) diff --git a/backend/src/user/migrations/1730000000000-add-unique-wallet-address.ts b/backend/src/user/migrations/1730000000000-add-unique-wallet-address.ts new file mode 100644 index 00000000..311c9310 --- /dev/null +++ b/backend/src/user/migrations/1730000000000-add-unique-wallet-address.ts @@ -0,0 +1,22 @@ +import { MigrationInterface, QueryRunner, TableUnique } from 'typeorm'; + +export class AddUniqueWalletAddress1730000000000 implements MigrationInterface { + name = 'AddUniqueWalletAddress1730000000000'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.createUniqueConstraint( + 'users', + new TableUnique({ + name: 'UQ_USERS_WALLET_ADDRESS', + columnNames: ['wallet_address'], + }), + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.dropUniqueConstraint( + 'users', + 'UQ_USERS_WALLET_ADDRESS', + ); + } +} diff --git a/backend/src/user/user.controller.ts b/backend/src/user/user.controller.ts index 11949f9b..1e759bb3 100644 --- a/backend/src/user/user.controller.ts +++ b/backend/src/user/user.controller.ts @@ -3,8 +3,10 @@ import { Post, Patch, Get, + Delete, Body, Param, + Request, UseGuards, } from '@nestjs/common'; import { ValidationPipe } from '@nestjs/common'; @@ -49,9 +51,17 @@ export class UserController { @ApiOperation({ summary: 'Link or update wallet address' }) linkWallet( @Body(new ValidationPipe({ whitelist: true })) dto: LinkWalletDto, - @Param('id') id: string, + @Request() req, ) { - return this.userService.linkWallet(id, dto); + return this.userService.linkWallet(req.user.id, dto); + } + + @UseGuards(AuthGuard('jwt')) + @Delete('link-wallet') + @ApiBearerAuth() + @ApiOperation({ summary: 'Unlink wallet address' }) + unlinkWallet(@Request() req) { + return this.userService.unlinkWallet(req.user.id); } @Get(':id') diff --git a/backend/src/user/user.service.spec.ts b/backend/src/user/user.service.spec.ts new file mode 100644 index 00000000..632ef2a8 --- /dev/null +++ b/backend/src/user/user.service.spec.ts @@ -0,0 +1,110 @@ +import { Test, type TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { ConflictException, NotFoundException } from '@nestjs/common'; +import { UserService } from './user.service'; +import { User } from './entities/user.entity'; +import { LinkWalletDto } from './dto/link-wallet.dto'; + +describe('UserService', () => { + let service: UserService; + let usersRepo: { + create: jest.Mock; + save: jest.Mock; + findOne: jest.Mock; + }; + + const WALLET = '0xabc1234567890abc1234567890abc1234567890'; + + beforeEach(async () => { + usersRepo = { + create: jest.fn((data) => ({ id: 'user-1', ...data })), + save: jest.fn(async (data) => data), + findOne: jest.fn(), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + UserService, + { + provide: getRepositoryToken(User), + useValue: usersRepo, + }, + ], + }).compile(); + + service = module.get(UserService); + }); + + it('should be defined', () => { + expect(service).toBeDefined(); + }); + + describe('linkWallet', () => { + it('links the wallet when it is not linked to any other account', async () => { + const user = { id: 'user-1', walletAddress: null } as User; + usersRepo.findOne.mockResolvedValueOnce(user); // getUserById + usersRepo.findOne.mockResolvedValueOnce(undefined); // no existing wallet + + const result = await service.linkWallet('user-1', { + walletAddress: WALLET, + } as LinkWalletDto); + + expect(result.walletAddress).toBe(WALLET); + expect(usersRepo.save).toHaveBeenCalled(); + }); + + it('throws ConflictException when the wallet is linked to another account', async () => { + const user = { id: 'user-1', walletAddress: null } as User; + const other = { id: 'user-2', walletAddress: WALLET } as User; + usersRepo.findOne.mockResolvedValueOnce(user); // getUserById + usersRepo.findOne.mockResolvedValueOnce(other); // wallet on other account + + await expect( + service.linkWallet('user-1', { walletAddress: WALLET } as LinkWalletDto), + ).rejects.toThrow(ConflictException); + expect(usersRepo.save).not.toHaveBeenCalled(); + }); + + it('allows re-linking the same wallet to the same account', async () => { + const user = { id: 'user-1', walletAddress: WALLET } as User; + usersRepo.findOne.mockResolvedValueOnce(user); // getUserById + usersRepo.findOne.mockResolvedValueOnce(user); // existing wallet is the same account + + const result = await service.linkWallet('user-1', { + walletAddress: WALLET, + } as LinkWalletDto); + + expect(result.walletAddress).toBe(WALLET); + }); + + it('throws NotFoundException when the user does not exist', async () => { + usersRepo.findOne.mockResolvedValueOnce(undefined); + + await expect( + service.linkWallet('missing', { + walletAddress: WALLET, + } as LinkWalletDto), + ).rejects.toThrow(NotFoundException); + }); + }); + + describe('unlinkWallet', () => { + it('clears the wallet address', async () => { + const user = { id: 'user-1', walletAddress: WALLET } as User; + usersRepo.findOne.mockResolvedValueOnce(user); // getUserById + + const result = await service.unlinkWallet('user-1'); + + expect(result.walletAddress).toBeNull(); + expect(usersRepo.save).toHaveBeenCalled(); + }); + + it('throws NotFoundException when the user does not exist', async () => { + usersRepo.findOne.mockResolvedValueOnce(undefined); + + await expect(service.unlinkWallet('missing')).rejects.toThrow( + NotFoundException, + ); + }); + }); +}); diff --git a/backend/src/user/user.service.ts b/backend/src/user/user.service.ts index b2a27bcf..f2fc8dfa 100644 --- a/backend/src/user/user.service.ts +++ b/backend/src/user/user.service.ts @@ -33,10 +33,22 @@ export class UserService { async linkWallet(id: string, dto: LinkWalletDto): Promise { const user = await this.getUserById(id); + const existing = await this.usersRepo.findOne({ + where: { walletAddress: dto.walletAddress }, + }); + if (existing && existing.id !== id) { + throw new ConflictException('Wallet already linked to another account'); + } user.walletAddress = dto.walletAddress; return this.usersRepo.save(user); } + async unlinkWallet(id: string): Promise { + const user = await this.getUserById(id); + user.walletAddress = null; + return this.usersRepo.save(user); + } + async getUserById(id: string): Promise { const user = await this.usersRepo.findOne({ where: { id } }); if (!user) throw new NotFoundException('User not found'); diff --git a/backend/test/security-headers.e2e-spec.ts b/backend/test/security-headers.e2e-spec.ts new file mode 100644 index 00000000..bc119501 --- /dev/null +++ b/backend/test/security-headers.e2e-spec.ts @@ -0,0 +1,78 @@ +import { Controller, Get, INestApplication, Module } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import request from 'supertest'; +import helmet from 'helmet'; +import { securityHeadersConfig } from '../src/security-headers'; + +// Minimal controller so the spec can assert the middleware headers without +// needing a database or the full app. +@Controller('probe') +class ProbeController { + @Get() + root() { + return { ok: true }; + } +} + +@Module({ controllers: [ProbeController] }) +class ProbeModule {} + +// Regression tests for the production Helmet configuration (issue #303). +// The config is sourced from the same `securityHeadersConfig` object that +// `backend/src/main.ts` applies, so these assertions lock the production +// header contract. If the non-secure PRNG/DNS settings change (which is +// not possible while we use default Helmet), enforce defaults explicitly. +describe('Security headers (production helmet config)', () => { + let app: INestApplication; + + beforeAll(async () => { + const moduleFixture = await Test.createTestingModule({ + imports: [ProbeModule], + }).compile(); + + app = moduleFixture.createNestApplication(); + app.use(helmet(securityHeadersConfig)); + await app.init(); + }); + + afterAll(async () => { + await app.close(); + }); + + it('sets a Content-Security-Policy restricting frame ancestors', async () => { + const res = await request(app.getHttpServer()).get('/probe').expect(200); + const csp = res.headers['content-security-policy'] as string; + expect(csp).toBeDefined(); + // frame-ancestors 'none' -> no other page may frame the API. + expect(csp).toContain("frame-ancestors 'none'"); + // default-src restricts to self + the Soroban testnet connect target. + expect(csp).toContain("default-src 'self'"); + }); + + it('enables HSTS with a long max age and preload', async () => { + const res = await request(app.getHttpServer()).get('/probe').expect(200); + const hsts = res.headers['strict-transport-security'] as string; + expect(hsts).toBeDefined(); + expect(hsts).toContain('max-age=63072000'); + expect(hsts).toContain('includeSubDomains'); + expect(hsts).toContain('preload'); + }); + + it('sets frame protection (X-Frame-Options)', async () => { + const res = await request(app.getHttpServer()).get('/probe').expect(200); + // Helmet's frame-ancestors directive also emits the legacy header. + expect(res.headers['x-frame-options']).toBeDefined(); + }); + + it('disables MIME type sniffing (X-Content-Type-Options)', async () => { + const res = await request(app.getHttpServer()).get('/probe').expect(200); + expect(res.headers['x-content-type-options']).toBe('nosniff'); + }); + + it('sets a strict referrer policy', async () => { + const res = await request(app.getHttpServer()).get('/probe').expect(200); + expect(res.headers['referrer-policy']).toBe( + 'strict-origin-when-cross-origin', + ); + }); +}); diff --git a/frontend/store/useGameStore.js b/frontend/store/useGameStore.js index c51c6fe9..0ac053d0 100644 --- a/frontend/store/useGameStore.js +++ b/frontend/store/useGameStore.js @@ -205,7 +205,26 @@ const useGameStore = create( const pointsPerCompletion = difficultyConfig?.pointsPerCompletion ?? POINTS_PER_COMPLETION; const newScore = score + pointsPerCompletion; - // Update the backend + // Optimistic update: apply the new progress immediately so the UI + // feels instant, then persist to the backend. If the request fails + // we roll back to the exact prior snapshot so a failed mutation + // never leaves the game state half-advanced (issue #299). + const previous = { + completedPuzzles, + completedDifficulties, + currentDifficulty, + currentPuzzleIndex, + score, + }; + const next = { + completedPuzzles: newCompletedPuzzles, + completedDifficulties: newCompletedDifficulties, + currentDifficulty: nextDifficulty, + currentPuzzleIndex: nextPuzzleIndex, + score: newScore, + }; + set(next); + try { await axios.post( apiUrl("/game/update"), @@ -219,15 +238,9 @@ const useGameStore = create( }, { withCredentials: true }, ); - - set({ - completedPuzzles: newCompletedPuzzles, - completedDifficulties: newCompletedDifficulties, - currentDifficulty: nextDifficulty, - currentPuzzleIndex: nextPuzzleIndex, - score: newScore, - }); } catch (error) { + // Restore prior state on failure. + set(previous); const entry = { action: "completePuzzle", message: error.message, time: Date.now() }; set((state) => ({ errors: [...state.errors, entry] })); } @@ -237,6 +250,13 @@ const useGameStore = create( const { user, nfts } = get(); if (!user) return; + // Optimistic update: add the NFT locally first, then persist. On + // failure we roll the inventory back to its prior contents (issue + // #299) so a failed request never leaves a phantom NFT behind. + const previousNfts = nfts; + const nextNfts = [...nfts, nft]; + set({ nfts: nextNfts }); + try { await axios.post( apiUrl("/nft/add"), @@ -246,9 +266,8 @@ const useGameStore = create( }, { withCredentials: true }, ); - - set({ nfts: [...nfts, nft] }); } catch (error) { + set({ nfts: previousNfts }); const entry = { action: "addNFT", message: error.message, time: Date.now() }; set((state) => ({ errors: [...state.errors, entry] })); } diff --git a/frontend/tests/useGameStore-optimistic.test.js b/frontend/tests/useGameStore-optimistic.test.js new file mode 100644 index 00000000..9c398aa0 --- /dev/null +++ b/frontend/tests/useGameStore-optimistic.test.js @@ -0,0 +1,101 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +vi.mock('axios', () => ({ + default: { + get: vi.fn(), + post: vi.fn(), + }, +})); + +import useGameStore from '@/store/useGameStore'; +import axios from 'axios'; + +// Reset the persisted singleton store state between tests. Rehydrating a +// fresh store leaves an empty base state, which we then re-seed so each +// test starts from the same snapshot (issue #299 optimistic rollback). +const resetStore = () => { + useGameStore.setState({ + user: { id: 'user-1' }, + currentDifficulty: 'easy', + currentPuzzleIndex: 0, + completedPuzzles: [], + completedDifficulties: [], + score: 0, + nfts: [], + errors: [], + difficultyConfig: null, + }); +}; + +describe('useGameStore optimistic-update rollback (issue #299)', () => { + beforeEach(() => { + vi.clearAllMocks(); + resetStore(); + }); + + it('applies completePuzzle optimistically, then persists on success', async () => { + useGameStore.setState({ + user: { id: 'user-1' }, + completedPuzzles: ['easy-0'], + score: 100, + }); + axios.post.mockResolvedValue({ data: { ok: true } }); + + // Start the action; state is applied synchronously before the await. + const promise = useGameStore.getState().completePuzzle('easy-1'); + + // Optimistic state is visible immediately (before the request settles). + expect(useGameStore.getState().completedPuzzles).toContain('easy-1'); + expect(useGameStore.getState().score).toBe(200); + + await promise; + + // Still applied after success (no rollback). + expect(useGameStore.getState().completedPuzzles).toContain('easy-1'); + expect(useGameStore.getState().score).toBe(200); + }); + + it('rolls back completePuzzle to the prior state when the request fails', async () => { + useGameStore.setState({ + user: { id: 'user-1' }, + completedPuzzles: ['easy-0'], + score: 100, + }); + axios.post.mockRejectedValue(new Error('network down')); + + const promise = useGameStore.getState().completePuzzle('easy-1'); + + // Optimistic while in flight. + expect(useGameStore.getState().completedPuzzles).toContain('easy-1'); + expect(useGameStore.getState().score).toBe(200); + + await promise; + + // Rolled back to the exact prior state on failure. + const state = useGameStore.getState(); + expect(state.completedPuzzles).toEqual(['easy-0']); + expect(state.score).toBe(100); + expect(state.errors.length).toBe(1); + expect(state.errors[0].action).toBe('completePuzzle'); + }); + + it('rolls back addNFT when the request fails, leaving inventory unchanged', async () => { + useGameStore.setState({ + user: { id: 'user-1' }, + nfts: [{ id: 'nft-1' }], + }); + axios.post.mockRejectedValue(new Error('network down')); + + const promise = useGameStore.getState().addNFT({ id: 'nft-2' }); + + // Optimistically added. + expect(useGameStore.getState().nfts).toHaveLength(2); + + await promise; + + // Rolled back — no phantom NFT remains. + const state = useGameStore.getState(); + expect(state.nfts).toEqual([{ id: 'nft-1' }]); + expect(state.errors[0].action).toBe('addNFT'); + }); +}); diff --git a/frontend/tsconfig.json b/frontend/tsconfig.json index d97952a1..1ec81d19 100644 --- a/frontend/tsconfig.json +++ b/frontend/tsconfig.json @@ -23,9 +23,6 @@ "resolveJsonModule": true, "isolatedModules": true, "jsx": "preserve", - "paths": { - "@/*": ["./*"] - }, "plugins": [ { "name": "next" 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/contracts/stellar_hunts_nft/src/lib.rs b/onchain/contracts/stellar_hunts_nft/src/lib.rs index 5b1e2e13..5327da0c 100644 --- a/onchain/contracts/stellar_hunts_nft/src/lib.rs +++ b/onchain/contracts/stellar_hunts_nft/src/lib.rs @@ -52,6 +52,7 @@ pub enum Error { AlreadyInitialized = 3, InvalidBaseUri = 4, MetadataTooLarge = 5, + NotInitialized = 6, } const MAX_BASE_URI_LEN: usize = 200; @@ -127,6 +128,18 @@ impl StellarHuntsNft { panic_with_error!(&env, Error::NotAuthorized); } + // Resolve the admin up front so a misconfigured (uninitialized) + // NFT contract surfaces a structured `NotInitialized` error before + // any badge state is written. Reading it here also ensures the + // badge/badge_data writes below can never be left as a partial + // state change if the contract was never configured. + let admin: Address = env + .storage() + .instance() + .get(&NftDataKey::Admin) + .ok_or(Error::NotInitialized) + .unwrap(); + let badge_key = NftDataKey::Badge(recipient.clone(), level.clone()); if env.storage().persistent().has(&badge_key) { panic_with_error!(&env, Error::AlreadyHasBadge); @@ -139,11 +152,6 @@ impl StellarHuntsNft { }; let badge_data_key = NftDataKey::BadgeData(recipient.clone(), level.clone()); env.storage().persistent().set(&badge_data_key, &badge_data); - let admin: Address = env - .storage() - .instance() - .get(&NftDataKey::Admin) - .expect("admin not set"); env.events().publish( (Symbol::new(&env, "level_badge_minted"),), diff --git a/onchain/contracts/stellar_hunts_nft/src/test.rs b/onchain/contracts/stellar_hunts_nft/src/test.rs index dfb3db41..0952b0da 100644 --- a/onchain/contracts/stellar_hunts_nft/src/test.rs +++ b/onchain/contracts/stellar_hunts_nft/src/test.rs @@ -84,15 +84,36 @@ 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()); +} + +#[test] +fn test_mint_uninitialized_is_structured_and_leaves_no_partial_state() { + let env = Env::default(); + env.mock_all_auths(); + + let game_id = env.register_contract(None, FakeGameContract); + let nft_id = env.register_contract(None, StellarHuntsNft); + + // NOTE: `init` is intentionally NOT called — the NFT contract is + // uninitialized. The game contract calls `mint_level_badge` directly. + let game = FakeGameContractClient::new(&env, &game_id); + let r = recipient(&env); + + // The call must fail with a *structured* error rather than an opaque + // `expect("admin not set")` panic. + let should_panic = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + game.mint(&nft_id, &r, &crate::Levels::Easy); })); assert!(should_panic.is_err()); + + // The admin read happens *before* any storage write, so the failed + // mint must not leave any partial badge state behind. + let nft = StellarHuntsNftClient::new(&env, &nft_id); + assert!(!nft.has_level_badge(&r, &crate::Levels::Easy)); + assert!(nft.get_badge_data(&r, &crate::Levels::Easy).is_none()); } #[test] diff --git a/onchain/contracts/stellar_hunts_receiver/src/lib.rs b/onchain/contracts/stellar_hunts_receiver/src/lib.rs index 481573b5..984b0907 100644 --- a/onchain/contracts/stellar_hunts_receiver/src/lib.rs +++ b/onchain/contracts/stellar_hunts_receiver/src/lib.rs @@ -5,7 +5,7 @@ // work. The presence of this contract keeps the test surface compatible // with the historical game test suite. -use soroban_sdk::{contract, contractimpl, Address, Env, String, Symbol}; +use soroban_sdk::{contract, contractimpl, Env, Symbol}; #[cfg(test)] use stellar_hunts_nft::{StellarHuntsNft, StellarHuntsNftClient};