diff --git a/backend/api/src/search-location.ts b/backend/api/src/search-location.ts index 2dbe2347..44120d16 100644 --- a/backend/api/src/search-location.ts +++ b/backend/api/src/search-location.ts @@ -9,7 +9,11 @@ export const searchLocationEndpoint: APIHandler<'search-location'> = async (body export async function searchLocation(body: ValidatedAPIParams<'search-location'>) { const {term, limit} = body - const endpoint = `/cities?namePrefix=${term}&limit=${limit ?? 10}&offset=0&sort=-population` + // Encode the user-supplied term and force limit to a small integer so neither can inject + // extra query parameters into the external GeoDB request (e.g. a term containing `&`). + const namePrefix = encodeURIComponent(term) + const safeLimit = Number.isFinite(limit) ? Math.trunc(limit as number) : 10 + const endpoint = `/cities?namePrefix=${namePrefix}&limit=${safeLimit}&offset=0&sort=-population` // const endpoint = `/countries?namePrefix=${term}&limit=${limit ?? 10}&offset=0` return await geodbFetch(endpoint) } diff --git a/backend/api/tests/unit/search-location.unit.test.ts b/backend/api/tests/unit/search-location.unit.test.ts index 5663fe68..7fc635d9 100644 --- a/backend/api/tests/unit/search-location.unit.test.ts +++ b/backend/api/tests/unit/search-location.unit.test.ts @@ -35,4 +35,26 @@ describe('searchLocation', () => { ) }) }) + + describe('when the term contains URL metacharacters', () => { + it('should encode the term so it cannot inject extra query params', async () => { + const mockBody = { + // Tries to override the limit / add params on the external GeoDB request. + term: 'Paris&limit=1000&sort=name', + limit: 15, + } + const mockAuth = {uid: '321'} as AuthedUser + const mockReq = {} as any + ;(geodbModules.geodbFetch as jest.Mock).mockResolvedValue('Pass') + + await searchLocationEndpoint(mockBody, mockAuth, mockReq) + + const calledWith = (geodbModules.geodbFetch as jest.Mock).mock.calls[0][0] as string + // The injected `&`/`=` must be percent-encoded, not passed through as separators. + expect(calledWith).toContain('namePrefix=Paris%26limit%3D1000%26sort%3Dname') + expect(calledWith).toContain('&limit=15&offset=0&sort=-population') + // Exactly one real `limit=` param (ours), not the injected one. + expect(calledWith.match(/&limit=/g)).toHaveLength(1) + }) + }) }) diff --git a/backend/shared/src/supabase/sql-builder.ts b/backend/shared/src/supabase/sql-builder.ts index 25d79446..7431c5c0 100644 --- a/backend/shared/src/supabase/sql-builder.ts +++ b/backend/shared/src/supabase/sql-builder.ts @@ -90,6 +90,17 @@ export function limit(limit: number, offset?: number) { return buildSql({limit, offset}) } +// `limit`/`offset` are interpolated into the SQL text rather than bound as params, so guard +// them: only a real, finite integer may ever reach the query string. This is defense in depth +// — every current caller passes a validated number — but it means a stray string (e.g. from an +// unvalidated request field typed as `number`) errors instead of injecting. +function asSqlInt(n: number, label: string): number { + if (typeof n !== 'number' || !Number.isFinite(n)) { + throw new Error(`sql-builder: ${label} must be a finite number, got ${JSON.stringify(n)}`) + } + return Math.trunc(n) +} + export function renderSql(...args: Args) { const builder = buildSql(...args) @@ -114,7 +125,7 @@ export function renderSql(...args: Args) { where.length && `where ${where.map((clause) => `(${clause})`).join(' and ')}`, groupBy.length && `group by ${groupBy.join(', ')}`, orderBy.length && `order by ${orderBy.join(', ')}`, - limit && `limit ${limit}`, - limit && offset && `offset ${offset}`, + limit && `limit ${asSqlInt(limit, 'limit')}`, + limit && offset && `offset ${asSqlInt(offset, 'offset')}`, ).join('\n') }