diff --git a/backend/src/app.ts b/backend/src/app.ts index 5c3ccd9..87dea74 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -9,6 +9,7 @@ import adminSettingsRouter from './routes/adminSettings'; import adminEmailTemplatesRouter from './routes/adminEmailTemplates'; import adminCategoriesRouter from './routes/adminCategories'; import adminTagsRouter from './routes/adminTags'; +import adminUploadLinksRouter from './routes/adminUploadLinks'; import adminVersionRouter from './routes/adminVersion'; import filtersRouter from './routes/filters'; import customersRouter from './routes/customers'; @@ -20,6 +21,7 @@ import { attachCustomer } from './middleware/customerAuth'; import { requireAdminGate } from './middleware/adminGate'; import { asyncRoute } from './asyncRoute'; import { uploadsRouter } from './uploads'; +import { trimTrailingSlashes } from './utils'; const app = express(); // Express advertises itself in X-Powered-By by default, which hands an @@ -36,13 +38,6 @@ app.use(cookieParser()); app.use(asyncRoute(attachCustomer)); app.use('/uploads', uploadsRouter(process.env.UPLOADS_DIR || '/app/uploads')); -// Trimmed with a loop rather than a `/+$/` regex, which backtracks. -function trimTrailingSlashes(value: string): string { - let trimmed = value; - while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1); - return trimmed; -} - app.get('/api/config', (_req, res) => { const clientId = process.env.PAYPAL_CLIENT_ID; const isPlaceholder = !clientId || clientId.length < 10 || clientId === 'REPLACE_WITH_PAYPAL_CLIENT_ID'; @@ -79,6 +74,7 @@ app.use('/api/admin/settings', requireAdminGate, adminSettingsRouter); app.use('/api/admin/email-templates', requireAdminGate, adminEmailTemplatesRouter); app.use('/api/admin/categories', requireAdminGate, adminCategoriesRouter); app.use('/api/admin/tags', requireAdminGate, adminTagsRouter); +app.use('/api/admin/upload-links', requireAdminGate, adminUploadLinksRouter); app.use('/api/admin/version', requireAdminGate, adminVersionRouter); app.use('/api/admin', requireAdminGate, adminRouter); app.use('/api/customers/me/addresses', shippingAddressesRouter); diff --git a/backend/src/routes/adminUploadLinks.ts b/backend/src/routes/adminUploadLinks.ts new file mode 100644 index 0000000..b7a4f57 --- /dev/null +++ b/backend/src/routes/adminUploadLinks.ts @@ -0,0 +1,116 @@ +import { Router, Request, Response } from 'express'; +import { pool, requireRow } from '../db'; +import { asyncRoute } from '../asyncRoute'; +import { generateToken, hashToken } from '../uploadLinks'; +import { trimTrailingSlashes } from '../utils'; + +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, revoked_at, submission_count, max_submissions, last_used_at, created_at + FROM upload_links +`; + +/** + * 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; + revoked_at: string | null; + submission_count: number; + max_submissions: number | null; + last_used_at: string | null; + created_at: string; +} + +router.get('/', asyncRoute(async (_req: Request, res: Response) => { + const { rows } = await pool.query(`${LINK_SELECT} ORDER BY created_at DESC`); + 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' }); + } + + // 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( + `INSERT INTO upload_links (label, token_hash, max_submissions) + VALUES ($1, $2, $3) + RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`, + [label, hashToken(token), maxSubmissions] + ); + 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 ?? ''); + res.status(201).json({ ...link, token, url: `${base}/submit/${token}` }); +})); + +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( + `UPDATE upload_links SET revoked_at = COALESCE(revoked_at, now()) + WHERE id = $1 + RETURNING id, label, 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; diff --git a/backend/src/utils.ts b/backend/src/utils.ts index 382a78d..c1425f9 100755 --- a/backend/src/utils.ts +++ b/backend/src/utils.ts @@ -73,3 +73,21 @@ export function tagColorFor(name: string): string { export const MARKETING_CONSENT_TEXT = 'I want to receive occasional emails about new one-of-a-kind items from Redefined Designs. I can unsubscribe at any time.'; + +/** + * Strips trailing slashes so a base URL can be joined with a stored path. + * + * A loop rather than `/\/+$/`, which backtracks: sonarjs flags that pattern as + * super-linear, and the input here is an environment variable rather than + * anything hostile, but the cheap version is no harder to read. + * + * Shared because two callers now need it — `/api/config` sends + * `uploadsBaseUrl` this way, and the upload-link routes build a submission URL + * from PUBLIC_URL. Stored paths always begin with a slash, so trimming the + * base is what stops the join producing a double. + */ +export function trimTrailingSlashes(value: string): string { + let trimmed = value; + while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1); + return trimmed; +} diff --git a/backend/tests/integration/uploadLinks.integration.test.ts b/backend/tests/integration/uploadLinks.integration.test.ts new file mode 100644 index 0000000..6b7c31b --- /dev/null +++ b/backend/tests/integration/uploadLinks.integration.test.ts @@ -0,0 +1,110 @@ +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +describe('issuing an upload link', () => { + it('returns the token exactly once, at creation', async () => { + const created = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah' }); + + expect(created.status).toBe(201); + expect(created.body.label).toBe('Sarah'); + expect(created.body.token).toMatch(/^[A-Za-z0-9_-]{43}$/); + expect(created.body.url).toContain(`/submit/${created.body.token}`); + + const listed = await request(app).get('/api/admin/upload-links'); + expect(listed.status).toBe(200); + expect(listed.body).toHaveLength(1); + // The whole point of storing a digest: the listing cannot hand it back. + expect(listed.body[0].token).toBeUndefined(); + expect(listed.body[0].token_hash).toBeUndefined(); + }); + + it('stores the digest rather than the token', async () => { + const created = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Estate sale box 3' }); + + const { rows } = await pool.query<{ token_hash: string }>( + `SELECT token_hash FROM upload_links` + ); + expect(rows[0]?.token_hash).not.toBe(created.body.token); + expect(rows[0]?.token_hash).toMatch(/^[a-f0-9]{64}$/); + }); + + it('refuses a link with no label', async () => { + const res = await request(app).post('/api/admin/upload-links').send({ label: ' ' }); + expect(res.status).toBe(400); + }); + + it('refuses a non-positive submission cap', async () => { + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Bad cap', maxSubmissions: 0 }); + expect(res.status).toBe(400); + }); + + // Omitting the field is the common case, so it is the case that has to be + // safe. An unbounded link should be something asked for, not something that + // happens when nobody thought about it. + it('bounds a link that was created without a cap', async () => { + const res = await request(app).post('/api/admin/upload-links').send({ label: 'Sarah' }); + + expect(res.status).toBe(201); + expect(res.body.max_submissions).toBe(25); + }); + + it('allows unlimited when it is asked for explicitly', async () => { + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Always on', maxSubmissions: null }); + + expect(res.status).toBe(201); + expect(res.body.max_submissions).toBeNull(); + }); +}); + +describe('revoking an upload link', () => { + it('stamps revoked_at and reports it in the listing', async () => { + const created = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Temporary' }); + + const revoked = await request(app) + .post(`/api/admin/upload-links/${created.body.id}/revoke`); + + expect(revoked.status).toBe(200); + expect(revoked.body.revoked_at).not.toBeNull(); + }); + + // The useful fact is when access ended, so a second click must not rewrite + // it — and it must not be an error either, because a button that fails on a + // double-click teaches people to distrust it. + it('is idempotent, keeping the original timestamp', async () => { + const created = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Temporary' }); + + const first = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`); + const second = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`); + + expect(second.status).toBe(200); + expect(second.body.revoked_at).toBe(first.body.revoked_at); + }); + + it('404s for a link that does not exist', async () => { + const res = await request(app).post('/api/admin/upload-links/9999/revoke'); + expect(res.status).toBe(404); + }); +});