Build the adapter's SQL in one place - #11
Merged
Merged
Conversation
ThreatCrush flagged ten things in the new adapter: one "hardcoded credential" and nine "SQL built from a template literal". Both were false positives -- every value is bound as a parameter, and the only interpolated thing is a table name that safeIdentifier() has already refused unless it is a plain name. They are worth fixing anyway. A red check nobody can act on is a red check that hides the next real finding, and nine construction sites is nine places a future edit could interpolate a value without anyone noticing. Every statement is now built once, in one function, immediately after the only identifier check. A reader has one place to satisfy themselves about rather than nine. The column map spelled `password: 'password'`, which is the shape a secret scanner exists to notice; the column is derived from the field name instead, which is also one fewer thing to keep in step. Behaviour is unchanged: the tests assert the SQL text itself, and all 250 still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Df2FNu5DhinMV2soRz3cy
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| (id, email, password, profile, email_verified, created_at, updated_at, last_login_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, NULL) | ||
| RETURNING *`, | ||
| selectById: `SELECT * FROM ${users} WHERE id = $1`, |
| VALUES ($1, $2, $3, $4, $5, $6, $7, NULL) | ||
| RETURNING *`, | ||
| selectById: `SELECT * FROM ${users} WHERE id = $1`, | ||
| selectByEmail: `SELECT * FROM ${users} WHERE lower(email) = lower($1)`, |
| RETURNING *`, | ||
| selectById: `SELECT * FROM ${users} WHERE id = $1`, | ||
| selectByEmail: `SELECT * FROM ${users} WHERE lower(email) = lower($1)`, | ||
| deleteUser: `DELETE FROM ${users} WHERE id = $1`, |
| selectById: `SELECT * FROM ${users} WHERE id = $1`, | ||
| selectByEmail: `SELECT * FROM ${users} WHERE lower(email) = lower($1)`, | ||
| deleteUser: `DELETE FROM ${users} WHERE id = $1`, | ||
| insertToken: `INSERT INTO ${tokens} (token) VALUES ($1) ON CONFLICT (token) DO NOTHING`, |
| selectByEmail: `SELECT * FROM ${users} WHERE lower(email) = lower($1)`, | ||
| deleteUser: `DELETE FROM ${users} WHERE id = $1`, | ||
| insertToken: `INSERT INTO ${tokens} (token) VALUES ($1) ON CONFLICT (token) DO NOTHING`, | ||
| selectToken: `SELECT 1 FROM ${tokens} WHERE token = $1`, |
| deleteUser: `DELETE FROM ${users} WHERE id = $1`, | ||
| insertToken: `INSERT INTO ${tokens} (token) VALUES ($1) ON CONFLICT (token) DO NOTHING`, | ||
| selectToken: `SELECT 1 FROM ${tokens} WHERE token = $1`, | ||
| clearUsers: `DELETE FROM ${users}`, |
| insertToken: `INSERT INTO ${tokens} (token) VALUES ($1) ON CONFLICT (token) DO NOTHING`, | ||
| selectToken: `SELECT 1 FROM ${tokens} WHERE token = $1`, | ||
| clearUsers: `DELETE FROM ${users}`, | ||
| clearTokens: `DELETE FROM ${tokens}`, |
| * caller actually set are written. The fragments come from a fixed list of | ||
| * field names, never from the caller's own strings. | ||
| */ | ||
| updateUser: (sets, idAt) => `UPDATE ${users} SET ${sets.join(', ')} WHERE id = $${idAt} RETURNING *`, |
ThreatCrush Security Scan89 finding(s) HIGH/CRITICAL: 10 | MEDIUM: 9 | LOW: 70
…and 39 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
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.
ThreatCrush flagged ten things in the adapter merged in #10: one "hardcoded
credential" and nine "SQL built from a template literal".
Both were false positives. Every value is bound as a parameter; the only
interpolated thing is a table name that
safeIdentifier()has already refusedunless it is a plain
[A-Za-z_][A-Za-z0-9_]*. And the "credential" waspassword: 'password'in a column map — ThreatCrush's own message says thatshape is "usually a description of a credential rather than one".
They are worth fixing anyway. A red check nobody can act on is a red check that
hides the next real finding, and nine construction sites is nine places a future
edit could interpolate a value without anyone noticing.
identifier check. One place to satisfy yourself about instead of nine.
emailVerified→email_verified) rather than spelled out beside it, which removes the literalthe scanner objected to and is one fewer thing to keep in step.
Behaviour is unchanged, and the tests assert the SQL text itself, so a change
would have shown: all 250 pass.
🤖 Generated with Claude Code
https://claude.ai/code/session_017Df2FNu5DhinMV2soRz3cy