The quality gate was red on one condition only — new_security_hotspots_reviewed at 75 against a threshold of 100 — and the outstanding hotspot was admin.ts's `${ADMIN_ITEM_SELECT} WHERE i.id = $1`.
Worth being exact about what was wrong with it, because it was not what it looked like. The value was already parameterized: itemId was bound as $1, travelled through the driver's separate parameter channel, and never entered the query text. What was interpolated was a module constant containing no caller data. S2077 fires on the template literal rather than on the value, because the rule cannot tell a constant from a request field — and neither, at a glance, can a person reading it.
So the fix is not to parameterize something already parameterized. It is to stop interpolating at query call sites at all, which turns a property somebody has to verify into one they can see. Every query whose shape is fixed is now a named constant and every such call passes an identifier: ADMIN_ITEM_BY_ID for the two admin routes, PUBLIC_ITEM_BY_ID, LINK_LIST, and the two draft-queue shapes. Seven interpolating call sites become three.
The three that remain cannot become constants and now say so rather than looking like ones nobody got to. admin.ts and items.ts build their WHERE at run time from buildItemFilterSql, whose fragments are string literals whose only interpolations are placeholder indices; that reasoning was already written down and is unchanged. draftingWorker interpolates a table name, and this is the one query here that genuinely cannot be parameterized in any form — a bound parameter is a value, and Postgres will not accept an identifier as one, so the choice is interpolation or nothing. What makes it safe is the closed 'categories' | 'tags' union, and the comment now says that instead of merely asserting there is nothing to worry about.
Also clears the project's only open Sonar issue, S1854 on adminUploadLinks, which #260 introduced and which I had deferred as a tidiness point. It was more than that: outcome was initialised at its declaration and assigned the same value again in the catch, which made two different failures look like one. A template that will not render, or a stored template that cannot be loaded, is not an SMTP problem, and reporting it as "not configured" sent the admin looking in the wrong place. The SMTP-rejection conflation that was actually agreed stays, and is now the only thing that catch conflates.
Closes #294
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
206 lines
8.4 KiB
TypeScript
206 lines
8.4 KiB
TypeScript
import { Router, Request, Response } from 'express';
|
|
import { pool, requireRow } from '../db';
|
|
import { asyncRoute } from '../asyncRoute';
|
|
import { generateToken, hashToken } from '../uploadLinks';
|
|
import { trimTrailingSlashes, isValidEmail } from '../utils';
|
|
import { sendMail, MailOutcome } from '../mailer';
|
|
import { renderTemplate } from '../emailTemplates';
|
|
import { loadStoredTemplate } from './adminEmailTemplates';
|
|
|
|
const router = Router();
|
|
|
|
/**
|
|
* Issuing and retiring the links that open the public intake endpoint (#222).
|
|
*
|
|
* A link is named because provenance matters more than convenience here. When
|
|
* one is shared further than intended the question is *which* one, and the
|
|
* answer has to come from somewhere — so every submission records the link it
|
|
* arrived through, and revoking kills that link rather than the feature.
|
|
*
|
|
* The token is returned by exactly one response in this file and is
|
|
* unrecoverable afterwards. That is why the admin screen has to present it as
|
|
* a one-time reveal rather than a field to come back to, and why losing it
|
|
* means issuing a new link rather than looking the old one up.
|
|
*/
|
|
|
|
/**
|
|
* Shaped so a `SELECT *` can never leak the digest into a response.
|
|
*
|
|
* Spelling the columns out is the point: `SELECT *` here would put
|
|
* `token_hash` into every listing the moment somebody added a convenience.
|
|
*/
|
|
const LINK_SELECT = `
|
|
SELECT id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at
|
|
FROM upload_links
|
|
`;
|
|
|
|
/** Every link, newest first. A whole query, so the call interpolates nothing (#294). */
|
|
const LINK_LIST = `${LINK_SELECT} ORDER BY created_at DESC`;
|
|
|
|
/**
|
|
* The cap a link gets when nobody chose one.
|
|
*
|
|
* Not a tuned number — large enough that an ordinary contributor never meets
|
|
* it, small enough that a link shared further than intended cannot be used
|
|
* indefinitely before anyone notices. The point is that the default is finite
|
|
* at all.
|
|
*/
|
|
const DEFAULT_MAX_SUBMISSIONS = 25;
|
|
|
|
interface UploadLinkRow {
|
|
id: number;
|
|
label: string;
|
|
contact_email: string | null;
|
|
revoked_at: string | null;
|
|
submission_count: number;
|
|
max_submissions: number | null;
|
|
last_used_at: string | null;
|
|
created_at: string;
|
|
}
|
|
|
|
/**
|
|
* What the mail tells the recipient about how much they may send.
|
|
*
|
|
* Words rather than a bare number for an uncapped link, so the sentence reads
|
|
* as a sentence instead of showing an empty space where a figure should be.
|
|
* An uncapped link is a deliberate choice the admin already had to make, so it
|
|
* is emailable like any other.
|
|
*/
|
|
function submissionsAllowed(maxSubmissions: number | null): string {
|
|
if (maxSubmissions === null) return 'as many items as you like';
|
|
return maxSubmissions === 1 ? '1 item' : `${maxSubmissions} items`;
|
|
}
|
|
|
|
router.get('/', asyncRoute(async (_req: Request, res: Response) => {
|
|
const { rows } = await pool.query<UploadLinkRow>(LINK_LIST);
|
|
res.json(rows);
|
|
}));
|
|
|
|
router.post('/', asyncRoute(async (req: Request, res: Response) => {
|
|
const label = typeof req.body?.label === 'string' ? req.body.label.trim() : '';
|
|
if (label === '') {
|
|
return res.status(400).json({ error: 'a label is required' });
|
|
}
|
|
|
|
const email = typeof req.body?.email === 'string' ? req.body.email.trim() : '';
|
|
if (email === '' || !isValidEmail(email)) {
|
|
return res.status(400).json({ error: 'a valid email address is required' });
|
|
}
|
|
|
|
// Three cases, deliberately distinct. Absent means nobody decided, which
|
|
// gets the bounded default. An explicit null means unlimited — a decision
|
|
// someone made, visible in the request. A number is itself. Reading absent
|
|
// as unlimited is what would make every link unbounded by default.
|
|
const rawCap = req.body?.maxSubmissions;
|
|
let maxSubmissions: number | null = DEFAULT_MAX_SUBMISSIONS;
|
|
if (rawCap === null) {
|
|
maxSubmissions = null;
|
|
} else if (rawCap !== undefined && rawCap !== '') {
|
|
const parsed = Number(rawCap);
|
|
if (!Number.isInteger(parsed) || parsed < 1) {
|
|
return res.status(400).json({ error: 'maxSubmissions must be a positive whole number' });
|
|
}
|
|
maxSubmissions = parsed;
|
|
}
|
|
|
|
const token = generateToken();
|
|
const { rows } = await pool.query<UploadLinkRow>(
|
|
`INSERT INTO upload_links (label, token_hash, max_submissions, contact_email)
|
|
VALUES ($1, $2, $3, $4)
|
|
RETURNING id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
|
[label, hashToken(token), maxSubmissions, email]
|
|
);
|
|
const link = requireRow(rows, 'the upload_links INSERT');
|
|
|
|
// PUBLIC_URL is already required alongside SMTP and is what every other
|
|
// outbound link is built from. Absent in local development, which yields a
|
|
// relative URL the admin screen can still show and copy usefully.
|
|
const base = trimTrailingSlashes(process.env.PUBLIC_URL ?? '');
|
|
const url = `${base}/submit/${token}`;
|
|
|
|
// Awaited, and its outcome reported rather than swallowed. Every other sender
|
|
// in this codebase fires and forgets because nobody is waiting on the answer;
|
|
// here somebody is — the admin is looking at the screen, and whether they
|
|
// now have to send the link by hand is the thing they need to know.
|
|
//
|
|
// A failure does not roll the link back. The token is shown exactly once, so
|
|
// a rollback would leave the admin retrying and holding a different link,
|
|
// discarding work that succeeded for the sake of tidiness.
|
|
let outcome: MailOutcome;
|
|
try {
|
|
const template = renderTemplate('uploadLink', await loadStoredTemplate('uploadLink'), {
|
|
submitUrl: url,
|
|
label: link.label,
|
|
submissionsAllowed: submissionsAllowed(link.max_submissions)
|
|
});
|
|
outcome = await sendMail(email, template.subject, template.html);
|
|
} catch (err) {
|
|
// Reported, not thrown. The link exists and is usable; the admin needs to
|
|
// be told the mail did not go, not handed a 500 for a link that was made.
|
|
//
|
|
// Assigned only here, not also at the declaration. The duplicate initialiser
|
|
// was flagged as S1854 (#294), and it was worse than redundant: it made the
|
|
// two ways of reaching this line look like one. A template that will not
|
|
// render, or a stored template that cannot be loaded, is not an SMTP
|
|
// problem, and reporting it as "not configured" pointed the admin at the
|
|
// wrong thing entirely.
|
|
//
|
|
// An SMTP rejection landing here and being reported as unconfigured is the
|
|
// conflation that was actually agreed: a fourth outcome would be a real
|
|
// distinction, nothing consumes it, and the admin's next action is identical
|
|
// either way — copy the link and send it by hand.
|
|
console.error(`[upload-links] could not email ${email}:`, err);
|
|
outcome = 'skipped-unconfigured';
|
|
}
|
|
|
|
res.status(201).json({
|
|
...link,
|
|
token,
|
|
url,
|
|
mail: { sent: outcome === 'sent', outcome }
|
|
});
|
|
}));
|
|
|
|
/**
|
|
* Forgives the current ceiling window without deleting anything.
|
|
*
|
|
* The count is derived from item_drafts rows, which are real submissions with
|
|
* real items in the review queue — so a reset moves the window's start rather
|
|
* than removing anything. Recovery is automatic as the window rolls; this is
|
|
* for the case where the ceiling was hit legitimately and waiting is not
|
|
* acceptable.
|
|
*
|
|
* Declared above `/:id/revoke` deliberately: Express matches in order, and
|
|
* `reset-ceiling` would otherwise be read as an id.
|
|
*/
|
|
router.post('/reset-ceiling', asyncRoute(async (_req: Request, res: Response) => {
|
|
await pool.query(
|
|
`INSERT INTO admin_settings (key, value, updated_at)
|
|
VALUES ('intake_ceiling_reset_at', $1, now())
|
|
ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value, updated_at = now()`,
|
|
[new Date().toISOString()]
|
|
);
|
|
res.json({ reset: true });
|
|
}));
|
|
|
|
router.post('/:id/revoke', asyncRoute(async (req: Request, res: Response) => {
|
|
// COALESCE so revoking twice keeps the original timestamp. The useful fact
|
|
// is when access ended, and a second click should neither rewrite that nor
|
|
// fail — a button that errors on a double-click teaches people to distrust
|
|
// it, which is the last thing wanted on the control that contains a leak.
|
|
const { rows } = await pool.query<UploadLinkRow>(
|
|
`UPDATE upload_links SET revoked_at = COALESCE(revoked_at, now())
|
|
WHERE id = $1
|
|
RETURNING id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
|
[req.params.id]
|
|
);
|
|
|
|
const link = rows[0];
|
|
if (!link) {
|
|
return res.status(404).json({ error: 'not found' });
|
|
}
|
|
res.json(link);
|
|
}));
|
|
|
|
export default router;
|