mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
feat(api): rate-limit /api/v1 and report when a trip last changed
Two gaps from the first pass, both about a client that polls. Rate limiting is keyed by user rather than by IP. A self-hosted integration and its owner's browser routinely leave the same address, and an IP bucket would let one starve the other; 120 requests a minute is generous for a sync surface and still bounded, so a runaway poll degrades its own integration instead of the instance. updated_at now ships with every trip, and its docblock says plainly what it is not: a content timestamp. It moves when the trip's own fields change — title, dates, currency, cover — and stays put when a place, a day or a note is edited, because the child tables carry no updated_at of their own. That also rules out the ?since= filter this was meant to enable: it would silently hide every trip whose itinerary changed, which is worse than no filter at all. Making it honest needs a schema change, not an API field, so the field is documented for what it is and the filter is left out.
This commit is contained in:
@@ -9,6 +9,8 @@ import {
|
||||
} from '@trek/shared';
|
||||
import { ApiTokenGuard } from './api-token.guard';
|
||||
import { PublicApiService } from './public-api.service';
|
||||
import { RateLimitService } from '../common/rate-limit.service';
|
||||
import type { User } from '../../types';
|
||||
|
||||
/**
|
||||
* `/api/v1` — the versioned, read-only surface for third-party integrations.
|
||||
@@ -24,15 +26,40 @@ import { PublicApiService } from './public-api.service';
|
||||
* missing feature: it would need scope enforcement this surface does not implement
|
||||
* yet, and a token that reads everything is a very different thing from one that
|
||||
* can also delete a trip.
|
||||
*
|
||||
* Rate limited per token rather than per IP: a self-hosted integration and its
|
||||
* user's browser routinely share an address, and limiting by IP would let one
|
||||
* starve the other. The budget is generous — this is a sync surface, not a login
|
||||
* form — but bounded, so a runaway poll degrades its own integration instead of
|
||||
* the instance.
|
||||
*/
|
||||
const RATE_WINDOW_MS = 60_000;
|
||||
const RATE_MAX_PER_MINUTE = 120;
|
||||
|
||||
@Controller('api/v1')
|
||||
@UseGuards(ApiTokenGuard)
|
||||
export class PublicApiController {
|
||||
constructor(private readonly api: PublicApiService) {}
|
||||
constructor(
|
||||
private readonly api: PublicApiService,
|
||||
private readonly rl: RateLimitService,
|
||||
) {}
|
||||
|
||||
/**
|
||||
* Keyed by token id, not by IP. Falls back to the user id when a token somehow
|
||||
* carries none, so the limiter can never end up with a shared bucket.
|
||||
*/
|
||||
private limit(req: Request): void {
|
||||
const user = req.user as User | undefined;
|
||||
const key = `user:${user?.id ?? 'unknown'}`;
|
||||
if (!this.rl.check('public-api', key, RATE_MAX_PER_MINUTE, RATE_WINDOW_MS, Date.now())) {
|
||||
throw new HttpException({ error: 'Too many requests. Please slow down.' }, 429);
|
||||
}
|
||||
}
|
||||
|
||||
/** Every trip the token's owner can reach, without itineraries. */
|
||||
@Get('trips')
|
||||
listTrips(@Req() req: Request): PublicApiTripList {
|
||||
this.limit(req);
|
||||
return { trips: this.api.listTrips(requireUserId(req)) };
|
||||
}
|
||||
|
||||
@@ -49,6 +76,7 @@ export class PublicApiController {
|
||||
@Param('id') id: string,
|
||||
@Query('include') include?: string,
|
||||
): PublicApiTrip {
|
||||
this.limit(req);
|
||||
const tripId = parseTripId(id);
|
||||
const trip = this.api.getTrip(tripId, requireUserId(req), parseInclude(include));
|
||||
if (!trip) {
|
||||
|
||||
@@ -4,6 +4,7 @@ import { PublicApiService } from './public-api.service';
|
||||
import { ApiTokenGuard } from './api-token.guard';
|
||||
import { TokensModule } from '../tokens/tokens.module';
|
||||
import { TripMembershipModule } from '../trip-membership/trip-membership.module';
|
||||
import { RateLimitModule } from '../common/rate-limit.module';
|
||||
|
||||
/**
|
||||
* Public API v1 — the versioned read-only surface for third-party integrations.
|
||||
@@ -14,7 +15,7 @@ import { TripMembershipModule } from '../trip-membership/trip-membership.module'
|
||||
* this trip" is decided.
|
||||
*/
|
||||
@Module({
|
||||
imports: [TokensModule, TripMembershipModule],
|
||||
imports: [TokensModule, TripMembershipModule, RateLimitModule],
|
||||
controllers: [PublicApiController],
|
||||
providers: [PublicApiService, ApiTokenGuard],
|
||||
})
|
||||
|
||||
@@ -43,7 +43,7 @@ export class PublicApiService {
|
||||
const ids = this.membership.listAccessibleTripIds(userId);
|
||||
if (ids.length === 0) return [];
|
||||
const rows = this.db.all<TripRow>(
|
||||
`SELECT id, title, description, start_date, end_date, currency, is_archived
|
||||
`SELECT id, title, description, start_date, end_date, currency, is_archived, updated_at
|
||||
FROM trips
|
||||
WHERE id IN (${ids.map(() => '?').join(',')})
|
||||
ORDER BY start_date DESC, id DESC`,
|
||||
@@ -62,7 +62,7 @@ export class PublicApiService {
|
||||
getTrip(tripId: number, userId: number, include: PublicApiInclude[]): PublicApiTrip | null {
|
||||
if (!this.db.canAccessTrip(tripId, userId)) return null;
|
||||
const row = this.db.get<TripRow>(
|
||||
`SELECT id, title, description, start_date, end_date, currency, is_archived
|
||||
`SELECT id, title, description, start_date, end_date, currency, is_archived, updated_at
|
||||
FROM trips WHERE id = ?`,
|
||||
tripId,
|
||||
);
|
||||
@@ -227,6 +227,7 @@ function toTripSummary(row: TripRow): PublicApiTripSummary {
|
||||
end_date: row.end_date ?? null,
|
||||
currency: row.currency ?? null,
|
||||
archived: row.is_archived === 1,
|
||||
updated_at: row.updated_at ?? null,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -253,6 +254,7 @@ interface TripRow {
|
||||
end_date: string | null;
|
||||
currency: string | null;
|
||||
is_archived: number | null;
|
||||
updated_at: string | null;
|
||||
}
|
||||
|
||||
interface DayRow {
|
||||
|
||||
@@ -26,7 +26,8 @@ const { db } = vi.hoisted(() => {
|
||||
created_at DATETIME DEFAULT CURRENT_TIMESTAMP, last_used_at DATETIME);
|
||||
CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL,
|
||||
title TEXT NOT NULL, description TEXT, start_date TEXT, end_date TEXT, currency TEXT,
|
||||
is_archived INTEGER DEFAULT 0, created_at DATETIME DEFAULT CURRENT_TIMESTAMP);
|
||||
is_archived INTEGER DEFAULT 0, created_at DATETIME DEFAULT CURRENT_TIMESTAMP,
|
||||
updated_at DATETIME DEFAULT CURRENT_TIMESTAMP);
|
||||
CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);
|
||||
CREATE TABLE days (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL,
|
||||
day_number INTEGER NOT NULL, date TEXT NOT NULL, notes TEXT, title TEXT);
|
||||
@@ -68,6 +69,7 @@ vi.mock('../../src/db/database', async (importActual) => {
|
||||
});
|
||||
|
||||
import { DatabaseModule } from '../../src/nest/database/database.module';
|
||||
import { RateLimitModule } from '../../src/nest/common/rate-limit.module';
|
||||
import { PublicApiModule } from '../../src/nest/public-api/public-api.module';
|
||||
import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter';
|
||||
|
||||
@@ -90,7 +92,7 @@ describe('Public API v1 e2e (real guard + real SQL)', () => {
|
||||
let app: Awaited<ReturnType<typeof build>>;
|
||||
|
||||
async function build() {
|
||||
const moduleRef = await Test.createTestingModule({ imports: [DatabaseModule, PublicApiModule] }).compile();
|
||||
const moduleRef = await Test.createTestingModule({ imports: [DatabaseModule, RateLimitModule, PublicApiModule] }).compile();
|
||||
const nest = moduleRef.createNestApplication();
|
||||
nest.useGlobalFilters(new TrekExceptionFilter());
|
||||
await nest.init();
|
||||
|
||||
@@ -12,6 +12,7 @@ import type { Request } from 'express';
|
||||
import { PUBLIC_API_INCLUDES } from '@trek/shared';
|
||||
import { PublicApiController } from '../../../src/nest/public-api/public-api.controller';
|
||||
import type { PublicApiService } from '../../../src/nest/public-api/public-api.service';
|
||||
import type { RateLimitService } from '../../../src/nest/common/rate-limit.service';
|
||||
|
||||
const TRIP = {
|
||||
id: 12,
|
||||
@@ -21,10 +22,13 @@ const TRIP = {
|
||||
end_date: '2026-06-22',
|
||||
currency: 'EUR',
|
||||
archived: false,
|
||||
updated_at: '2026-06-01 10:00:00',
|
||||
};
|
||||
|
||||
function makeController(svc: Partial<PublicApiService>) {
|
||||
return new PublicApiController(svc as PublicApiService);
|
||||
/** The limiter always allows unless a test says otherwise. */
|
||||
function makeController(svc: Partial<PublicApiService>, allow = true) {
|
||||
const rl = { check: vi.fn().mockReturnValue(allow) } as unknown as RateLimitService;
|
||||
return new PublicApiController(svc as PublicApiService, rl);
|
||||
}
|
||||
|
||||
const req = (userId = 7) => ({ user: { id: userId } }) as Request;
|
||||
@@ -125,4 +129,19 @@ describe('PublicApiController', () => {
|
||||
expect(getTrip).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('rate limiting', () => {
|
||||
it('429s the list once the budget is spent, without reaching the service', () => {
|
||||
const listTrips = vi.fn();
|
||||
const res = thrown(() => makeController({ listTrips }, false).listTrips(req(7)));
|
||||
expect(res).toEqual({ status: 429, body: { error: 'Too many requests. Please slow down.' } });
|
||||
expect(listTrips).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('429s the detail route the same way', () => {
|
||||
const getTrip = vi.fn();
|
||||
expect(thrown(() => makeController({ getTrip }, false).getTrip(req(7), '12', undefined)).status).toBe(429);
|
||||
expect(getTrip).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -101,6 +101,15 @@ export const publicApiTripSummarySchema = z.object({
|
||||
end_date: z.string().nullable(),
|
||||
currency: z.string().nullable(),
|
||||
archived: z.boolean(),
|
||||
/**
|
||||
* When the trip's own fields last changed — title, dates, currency, cover.
|
||||
*
|
||||
* Explicitly NOT a content timestamp: editing a place, a day or a note does not
|
||||
* touch it, because the child tables carry no `updated_at` of their own. A client
|
||||
* that polls must compare the payload it received, not this value; using it to
|
||||
* skip a fetch would silently miss every itinerary edit.
|
||||
*/
|
||||
updated_at: z.string().nullable(),
|
||||
});
|
||||
export type PublicApiTripSummary = z.infer<typeof publicApiTripSummarySchema>;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user