Sanitize user input and strengthen limit validation for GeoDB API requests and SQL queries

- Encoded user-supplied terms to prevent query parameter injection in GeoDB API calls.
- Forced `limit` and `offset` to be finite integers in SQL builder for added defense against injection risks.
- Added unit tests to verify term encoding and limit validation behavior.

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)
 }

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)
+    })
+  })
 })

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')
 }