Fix/sjip 1605 harden member directory export

.dockerignore

@@ -9,4 +9,6 @@ LICENSE
 .eslintrc
 .prettierrc
 docker-compose.yml
-.vs-code
+.vs-code
+.env*
+.claude

.env.example

@@ -1,14 +1,25 @@
+PORT=
+
 KEYCLOAK_URL=
 KEYCLOAK_REALM=
 KEYCLOAK_CLIENT=
-PGHOST=
-PGPORT=
-PGDATABASE=
-PGUSER=
-PGPASSWORD=
+
+# The app reads the DATABASE_* set; node-pg-migrate reads DATABASE_URL.
+# Both point at the same database, so they have to be kept in sync.
+DATABASE_HOST=
+DATABASE_PORT=
+DATABASE_NAME=
+DATABASE_USER=
+DATABASE_PASSWORD=
+DATABASE_URL=
+
 ADMIN_ROLE_NAME=
+PROFILE_IMAGE_BUCKET=
+PERSONA_URL=
+
 SMARTSHEET_ID=
 SMARTSHEET_TOKEN=
+
 MAILCHIMP_API_KEY=
 MAILCHIMP_USERNAME=
 MAILCHIMP_KF_LIST_ID=

.github/workflows/shai-hulud-allowed-patterns.txt

@@ -17,5 +17,3 @@ process.env.MAILCHIMP_USERNAME
 process.env.MAILCHIMP_KF_LIST_ID
 process.env.MAILCHIMP_KF_DATASET_LIST_ID
 process.env.NODE_ENV
-process.env.PGUSER
-process.env.PGPASSWORD

.github/workflows/shai-hulud-check.sh

@@ -18,7 +18,6 @@ escape_file () {
 }
 
 # file to escape before scanning (the patterns listed in shai-hulud-allowed-patterns.txt will be removed)
-escape_file "$DIRECTORY/migrateUpWithWrapper.mjs"
 escape_file "$DIRECTORY/src/config/env.ts"
 escape_file "$DIRECTORY/src/db/config.ts"
 

Dockerfile

@@ -17,7 +17,6 @@ WORKDIR /app
 COPY --from=build-image ./app/dist ./dist
 COPY package* ./
 COPY migrations ./migrations
-COPY migrateUpWithWrapper.mjs ./migrateUpWithWrapper.mjs
 # --omit=optional drops chromedriver, an unused optional dependency of keycloak-connect that
 # pulls in a large subtree (axios, proxy-agent, basic-ftp) this API never loads.
 RUN npm ci --omit=dev --omit=optional

README.md

@@ -19,7 +19,7 @@ First, you need to have an `.env` file. With, minimally:
 - `DATABASE_NAME`
 - `DATABASE_USER`
 - `DATABASE_PASSWORD`
-- `PGHOST`, `PGPORT`, `PGDATABASE`, `PGUSER`, `PGPASSWORD` — the same values again, read by `node-pg-migrate` instead of by the app
+- `DATABASE_URL` — the same database as a connection string, read by `node-pg-migrate` instead of by the app
 
 :warning: Comments in `.env` must start with `#`. A line starting with `;` is skipped silently by `dotenv`, but makes `docker compose` refuse to parse the file at all.
 

docker-compose.yml

@@ -1,18 +1,21 @@
-version: '3.9'
-
 services:
-  app: 
+  app:
     build: .
-    depends_on: 
-      - db
+    depends_on:
+      # The list form only waits for the container to start, but start:prd runs the
+      # migrations first and they fail outright if postgres is not accepting yet.
+      db:
+        condition: service_healthy
     ports:
       - "1212:1212"
     environment:
-      PGUSER: postgres
-      PGPASSWORD: password 
-      PGPORT: 5432
-      PGHOST: db 
-      PGDATABASE: users
+      DATABASE_HOST: db
+      DATABASE_PORT: 5432
+      DATABASE_NAME: users
+      DATABASE_USER: postgres
+      DATABASE_PASSWORD: password
+      # The app reads the DATABASE_* set above; node-pg-migrate reads DATABASE_URL.
+      DATABASE_URL: postgres://postgres:password@db:5432/users
   db:
     # Pinned: migration 1688561786592 indexes on a function selecting from an unqualified
     # "pgmigrations", and PG 15+ runs CREATE INDEX with a secure search_path, so it no longer

package.json

@@ -17,8 +17,8 @@
         "migrate": "node-pg-migrate",
         "dev": "nodemon",
         "start": "ts-node ./src/index.ts",
-        "start:prd": "node migrateUpWithWrapper.mjs && node ./dist/src/index.js",
-        "localstack": "node migrateUpWithWrapper.mjs && node --inspect=0.0.0.0 ./dist/src/index.js",
+        "start:prd": "node-pg-migrate up && node ./dist/src/index.js",
+        "localstack": "node-pg-migrate up && node --inspect=0.0.0.0 ./dist/src/index.js",
         "test": "jest --silent",
         "test:watch": "jest --watch"
     },

src/config/project/include.ts

@@ -19,8 +19,6 @@ const cleanedUserAttributes = [
     'research_domains',
     'research_area_description',
     'locale',
-    'newsletter_email',
-    'newsletter_subscription_status',
 ];
 
 const roleOptions = [

src/db/dal/savedFilter.test.ts

@@ -1,7 +1,8 @@
 import { StatusCodes } from 'http-status-codes';
 
 import sequelizeConnection from '../config';
-import { getFiltersUsingQuery } from './savedFilter';
+import SavedFilterModel from '../models/SavedFilter';
+import { createQueriesAndUpdateBody, getFiltersUsingQuery } from './savedFilter';
 
 const QUERY_ID = '0a1292c2-0bab-4190-a8d1-6db6e125af8a';
 const KEYCLOAK_ID = '3999fd60-80d2-477d-819e-93f6873efdb2';
@@ -62,3 +63,47 @@ describe('getFiltersUsingQuery', () => {
         expect(await getFiltersUsingQuery(QUERY_ID, KEYCLOAK_ID)).toEqual([{ id: QUERY_ID, title: 'a filter' }]);
     });
 });
+
+describe('createQueriesAndUpdateBody', () => {
+    const OTHER_QUERY_ID = '11111111-1111-4111-8111-111111111111';
+    const otherUsersQuery = { id: OTHER_QUERY_ID, keycloak_id: 'someone-else', title: 'Cohort A', queries: [] };
+    const body = { content: [{ filterID: OTHER_QUERY_ID }] };
+
+    const copy = () => createQueriesAndUpdateBody(body, [otherUsersQuery], KEYCLOAK_ID);
+
+    let createMock: jest.SpyInstance;
+
+    beforeEach(() => {
+        createMock = jest.spyOn(SavedFilterModel, 'create');
+    });
+
+    afterEach(() => {
+        jest.restoreAllMocks();
+    });
+
+    // Unhandled here means process.exit(1), because the copy runs outside the route's try/catch.
+    it('surfaces a failed copy to the caller instead of letting the rejection escape', async () => {
+        // What the beforeCreate uniqueness hook throws when the title is already taken.
+        createMock.mockRejectedValue({ key: 'title.not_unique' });
+
+        await expect(copy()).rejects.toEqual({ key: 'title.not_unique' });
+    });
+
+    it('waits for the copies to finish before returning the rewritten body', async () => {
+        let written = false;
+        createMock.mockImplementation(
+            () =>
+                new Promise((resolve) =>
+                    setTimeout(() => {
+                        written = true;
+                        resolve(undefined);
+                    }, 0),
+                ),
+        );
+
+        const result = await copy();
+
+        expect(written).toBe(true);
+        expect(JSON.stringify(result)).not.toContain(OTHER_QUERY_ID);
+    });
+});

src/db/dal/savedFilter.ts

@@ -148,9 +148,8 @@ export const createQueriesAndUpdateBody = async (body, queries, keycloak_id) =>
             return query;
         });
     if (toCreate.length) {
-        toCreate.forEach((query) => {
-            create(keycloak_id, query);
-        });
+        // Awaited: the beforeCreate uniqueness hook throws, and an escaped rejection kills the process.
+        await Promise.all(toCreate.map((query) => create(keycloak_id, query)));
         let newContent = JSON.stringify(structuredClone(body));
         newIds.forEach(({ newID, oldID }) => {
             newContent = newContent.replace(oldID, newID);

src/external/mailchimp.test.ts

@@ -33,6 +33,35 @@ describe('Mailchimp service', () => {
         action: SubscriptionStatus.SUBSCRIBED,
     };
 
+    describe('Request URL', () => {
+        const TRAVERSAL = '../../../lists/OTHER-LIST/members/victim@example.org';
+
+        const ok = () =>
+            (global.fetch as unknown as jest.Mock).mockImplementation(() => ({ status: 200, text: () => '' }));
+
+        const urlSent = () => (global.fetch as unknown as jest.Mock).mock.calls[0][0];
+
+        // Mailchimp's own worked example, fed in mixed case so the lowercasing is covered too.
+        it("matches mailchimp's documented subscriber hash", async () => {
+            ok();
+
+            await handleNewsletterUpdate({ ...payload, email: 'Urist.McVankab@FreddiesJokes.com' });
+
+            expect(new URL(urlSent()).pathname).toBe('/3.0/lists/KF_ID/members/62eeb292278cc15f5817cb78f7790b08');
+        });
+
+        it('reduces a path traversal to a hash instead of retargeting the call', async () => {
+            ok();
+
+            await handleNewsletterUpdate({ ...payload, email: TRAVERSAL });
+
+            const { pathname } = new URL(urlSent());
+
+            expect(pathname).toMatch(/^\/3\.0\/lists\/KF_ID\/members\/[0-9a-f]{32}$/);
+            expect(pathname).not.toContain('/lists/OTHER-LIST/');
+        });
+    });
+
     describe('Update subscription state', () => {
         it('should do nothing and return FAILED if email is empty', async () => {
             const inputNull = { ...payload, email: null };

src/external/mailchimp.ts

@@ -1,3 +1,5 @@
+import { createHash } from 'crypto';
+
 import { mailchimpApiKey, mailchimpKidsfirstListId, mailchimpUsername } from '../config/env';
 import { NewsletterPayload, SubscriptionStatus } from '../utils/newsletter';
 
@@ -93,5 +95,9 @@ const sendGetSubscriptionRequest = async (email: string): Promise<SubscriptionSt
         : SubscriptionStatus.UNSUBSCRIBED;
 };
 
+// The api addresses a contact by the md5 of its lowercased email, never by the email itself. Hashing
+// is also what stops a "/" or ".." in the value from retargeting the call at another list.
+const subscriberHash = (email: string) => createHash('md5').update(email.toLowerCase()).digest('hex');
+
 const retrieveMailchimpUrl = (server: string, listId: string, email: string) =>
-    `https://${server}.api.mailchimp.com/3.0/lists/${listId}/members/${email}`;
+    `https://${server}.api.mailchimp.com/3.0/lists/${listId}/members/${subscriberHash(email)}`;

src/routes/newsletter.ts

@@ -1,5 +1,7 @@
 import { Router } from 'express';
+import createHttpError from 'http-errors';
 import { StatusCodes } from 'http-status-codes';
+import validator from 'validator';
 
 import { refreshNewsletterStatus, subscribeNewsletter, unsubscribeNewsletter } from '../db/dal/newsletter';
 
@@ -20,7 +22,14 @@ newsletterRouter.put('/refresh/:newsletter_type?', async (req, res, next) => {
 newsletterRouter.put('/subscribe/:newsletter_type?', async (req, res, next) => {
     try {
         const keycloak_id = req['kauth']?.grant?.access_token?.content?.sub;
-        const result = await subscribeNewsletter(keycloak_id, req.body.newsletter_email);
+        const newsletter_email = req.body?.newsletter_email;
+
+        // Checked here because it reaches the Mailchimp URL before the model validator ever runs.
+        if (typeof newsletter_email !== 'string' || !validator.isEmail(newsletter_email)) {
+            throw createHttpError(StatusCodes.BAD_REQUEST, 'A valid newsletter_email is required.');
+        }
+
+        const result = await subscribeNewsletter(keycloak_id, newsletter_email);
 
         res.status(StatusCodes.OK).send(result);
     } catch (e) {

src/tests/app.test.ts

@@ -4,11 +4,13 @@ import request from 'supertest';
 
 import { getToken, publicKey } from '../../test/authTestUtils';
 import buildApp from '../app';
+import { subscribeNewsletter } from '../db/dal/newsletter';
 import * as savedFilterDal from '../db/dal/savedFilter';
 import { createUser, getUserById, updateUser } from '../db/dal/user';
 import { create as createUserSet, getByIds } from '../db/dal/userSets';
 import { IUserInput } from '../db/models/User';
 
+jest.mock('../db/dal/newsletter');
 jest.mock('../db/dal/user');
 jest.mock('../db/dal/userSets');
 
@@ -350,6 +352,39 @@ describe('Express app', () => {
         });
     });
 
+    describe('PUT /newsletter/subscribe', () => {
+        const send = (body: object) =>
+            request(app)
+                .put('/newsletter/subscribe')
+                .set({ Authorization: `Bearer ${getToken()}` })
+                .send(body);
+
+        beforeEach(() => {
+            (subscribeNewsletter as jest.Mock).mockReset();
+        });
+
+        // The value reaches the Mailchimp URL path, so a non-email must not get that far.
+        it('should return 400 and not call the dal when the email is a path traversal', async () => {
+            await send({ newsletter_email: '../../../lists/OTHER-LIST/members/victim@example.org' }).expect(400);
+
+            expect(subscribeNewsletter).not.toHaveBeenCalled();
+        });
+
+        it('should return 400 when the email is missing', async () => {
+            await send({}).expect(400);
+
+            expect(subscribeNewsletter).not.toHaveBeenCalled();
+        });
+
+        it('should pass a valid email through', async () => {
+            (subscribeNewsletter as jest.Mock).mockResolvedValue({ newsletter_email: 'jane@example.org' });
+
+            await send({ newsletter_email: 'jane@example.org' }).expect(200);
+
+            expect(subscribeNewsletter).toHaveBeenCalledWith('12345-678-90abcdef', 'jane@example.org');
+        });
+    });
+
     describe('request body sanitisation', () => {
         const sqon = {
             op: 'and',