Fix/sjip 1605 harden member directory export - #124
Merged
Merged
Conversation
Spreadsheet software evaluates a cell whose first character is =, +, - or @, so a member could set =HYPERLINK(...) as their profile text and have it run when an admin opens the export. Prefix such values with an apostrophe, applied through a single fast-csv transform so future columns are covered too.
cleanedUserAttributes is the column allow-list for reading someone else's profile. It withholds email but listed newsletter_email, which usually holds the same address, so GET /user/search handed every public member's address to any authenticated caller. The other four project configs already omit both.
console.log on a Sequelize error expands its enumerable properties, printing the failed query and its bind values three times over. Log the stack instead: same class, message and trace, none of the payload.
…process createQueriesAndUpdateBody fired SavedFilterModel.create() without awaiting it, and that create runs a beforeCreate uniqueness hook which throws on a duplicate title. The rejection escaped the request and terminated the process after the client had already received its 201. Awaiting the copies turns it into a normal 422 and also removes a race where the parent filter could reference query rows that were not yet committed.
The subscribe route pasted an unvalidated body field into the Mailchimp URL path, so "../../../lists/OTHER/members/victim@example.org" normalised onto a different list and was requested with the account API key. The model's isEmail validator ran only on the later database write, after the call had gone out. Validate at the route and percent-encode the path segment, so the value is both rejected early and unable to change the URL's shape if it ever slips past.
The api identifies a contact by the md5 of its lowercased email address, not by the address itself, so passing the raw email put an identifier Mailchimp does not recognise in the path. Hashing also supersedes the percent-encoding added in the previous commit: a hex digest cannot carry a "/" or a "..", so a crafted value can no longer retarget the call at another list.
.dockerignore never excluded .env, and the build stage starts with COPY . ., so a local `docker build .` copied the developer's real .env into the build-image layer. Published images were unaffected -- the prod stage copies only dist, package*, migrations and migrateUpWithWrapper.mjs -- and CI never had the file, since .env is git-ignored. The exposure was the intermediate layer in a local builder cache, which is exported by --target build-image or any --cache-to. The same COPY also fed that .env to `npm run test` in the build stage, because src/config/env.ts calls dotenv.config() at import. Build-time tests therefore read whatever the builder had on disk instead of a clean environment. Excluding the file fixes both. .claude/ is git-ignored scratch work with no place in the image.
migrateUpWithWrapper.mjs inferred success from "did anything appear on stderr" instead of reading the child's exit code, which broke both ways: a migration that failed while writing only to stdout exited 0, so the && in start:prd let the API boot on an unmigrated schema, and a single npm warning on a successful migration exited 2 and stopped the container from starting. npm writes nothing to stderr when a run-script fails, so the signal it depended on was never tied to failure at all. Its redaction never ran where it mattered. It keys off PGUSER/PGPASSWORD, which neither the deployed config nor .env define -- both supply DATABASE_URL and DATABASE_*. The only environment that set PG* was docker-compose.yml, where the password is "password". The leak it was written for in 2022 (317f9b4) was real: pg-connection-string 2.5.0 re-threw URL parse failures with err.input holding the full connection string, password included. Upstream fixed that in 2.9.1 ("Remove the input from the error message to avoid leaking sensitive information"); we are on 2.14.0. Running node-pg-migrate directly propagates exit codes correctly, which is the part the wrapper got wrong. The wrapper was the only consumer of process.env.PGUSER/PGPASSWORD, so the shai-hulud allowlist entries and its escape_file call go with it.
src/config/env.ts reads DATABASE_HOST/PORT/NAME/USER/PASSWORD, but .env.example advertised the libpq PG* names instead and docker-compose set only those. Under compose the app therefore booted with an undefined database, user and password and a NaN port, and failed on every DB-backed request -- silently, because neither new Sequelize() nor new Keycloak() throws on undefined config. Compose now supplies the DATABASE_* set the app reads plus DATABASE_URL for node-pg-migrate, matching the deployed config, and .env.example documents the same shape along with PORT, PROFILE_IMAGE_BUCKET and PERSONA_URL, which were missing entirely. depends_on moves to the long form so the migrations wait on the healthcheck the db service already declared. The list form only waits for the container to start, and now that a failed migration correctly exits nonzero it would block app startup rather than being swallowed. The obsolete top-level version key is dropped; Compose v2 ignores it and warns, and the condition syntax requires v2 anyway. The README's env-var list is updated to match; it still named the PG* set as what node-pg-migrate reads, which was only ever the fallback.
celinepelletier
approved these changes
Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security fixes on the member directory and its CSV export:
addressed by MD5 subscriber hash rather than the raw email
Also: .env out of the docker build context, the migration wrapper replaced by
node-pg-migrate directly so a failed migration actually fails, and
docker-compose/.env.example/README matched to the DATABASE_* set the app reads.