bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity

Upload links should require an email address and send the link to it on creation #260

New Issue
Closed
opened 2026-09-01 12:22:19 -05:00 by bermudalamb · 1 comment
bermudalamb commented 2026-09-01 12:22:19 -05:00
Owner
Copy Link
Copy Source

Requested for future consideration, captured now so it is not lost. Not started.

The ask. In Admin | Upload Links, creating a link should require an email address, and should automatically email that address with the number of submissions the link allows.

Where it lands. upload_links currently holds label, token_hash, revoked_at, submission_count, max_submissions, last_used_at, created_at — no recipient. The create route (routes/adminUploadLinks.ts:59) validates a label and a three-case cap, and nothing there sends mail. So this needs a migration, a required field in the form, validation on the route, and a new mail template. The submission count is already there as max_submissions, so "how many are available" is a read, not a new concept.

The decision worth making first, before any code. Today the token is shown in the admin exactly once, at creation, and only its SHA-256 digest is stored — the same reasoning as a password reset link. Emailing the link moves that secret into an inbox, where it is retained, forwardable, and searchable, and anyone holding it can upload to the shop without an account. That may well be an acceptable trade for the convenience, and it is roughly what a password reset already does; but it is a deliberate loosening of the current design rather than an obvious extension of it, and it should be chosen knowingly. Whether the mail carries the working link or only tells the recipient a link is coming by another route is the first question.

Other open questions:

  • max_submissions may be null, meaning unlimited. What does the email say then — "unlimited", or is an unlimited link simply not emailable?
  • Is the address required on existing links too? The column has to arrive nullable and be backfilled or grandfathered, since links already exist in QA.
  • Should the mail be re-sendable if the recipient loses it? That cannot resend the same token, because only the digest is stored — it would have to issue a new link and revoke the old one, which is a different feature.
  • Should revoking a link notify the address as well?
  • Does the address belong on the link, or is a link-to-contributor relationship the thing actually wanted? The second is more work and more useful if the same people submit repeatedly.

One operational note that will otherwise waste an afternoon. QA sets MAIL_ALLOWLIST=thomlamb@gmail.com, hardcoded in docker-compose.qa.yml and deliberately not a stack variable, because it is the entire safety property that stops a QA run emailing real people. Any address outside it is skipped with a [mail-blocked] warning naming the address — the flow still succeeds, no mail arrives. So testing this in QA against an arbitrary contributor address will look like a silent failure unless the address is allowlisted or a +suffix variant is used. Production is unrestricted and correct to be.

Happy to brainstorm this properly when it comes up the queue — the security question above is the one I would want settled before writing anything.

Requested for future consideration, captured now so it is not lost. Not started. **The ask.** In Admin | Upload Links, creating a link should require an email address, and should automatically email that address with the number of submissions the link allows. **Where it lands.** `upload_links` currently holds `label`, `token_hash`, `revoked_at`, `submission_count`, `max_submissions`, `last_used_at`, `created_at` — no recipient. The create route (`routes/adminUploadLinks.ts:59`) validates a label and a three-case cap, and nothing there sends mail. So this needs a migration, a required field in the form, validation on the route, and a new mail template. The submission count is already there as `max_submissions`, so "how many are available" is a read, not a new concept. **The decision worth making first, before any code.** Today the token is shown in the admin exactly once, at creation, and only its SHA-256 digest is stored — the same reasoning as a password reset link. Emailing the link moves that secret into an inbox, where it is retained, forwardable, and searchable, and anyone holding it can upload to the shop without an account. That may well be an acceptable trade for the convenience, and it is roughly what a password reset already does; but it is a deliberate loosening of the current design rather than an obvious extension of it, and it should be chosen knowingly. Whether the mail carries the working link or only tells the recipient a link is coming by another route is the first question. **Other open questions:** - `max_submissions` may be null, meaning unlimited. What does the email say then — "unlimited", or is an unlimited link simply not emailable? - Is the address required on *existing* links too? The column has to arrive nullable and be backfilled or grandfathered, since links already exist in QA. - Should the mail be re-sendable if the recipient loses it? That cannot resend the same token, because only the digest is stored — it would have to issue a new link and revoke the old one, which is a different feature. - Should revoking a link notify the address as well? - Does the address belong on the link, or is a link-to-contributor relationship the thing actually wanted? The second is more work and more useful if the same people submit repeatedly. **One operational note that will otherwise waste an afternoon.** QA sets `MAIL_ALLOWLIST=thomlamb@gmail.com`, hardcoded in `docker-compose.qa.yml` and deliberately not a stack variable, because it is the entire safety property that stops a QA run emailing real people. Any address outside it is skipped with a `[mail-blocked]` warning naming the address — the flow still succeeds, no mail arrives. So testing this in QA against an arbitrary contributor address will look like a silent failure unless the address is allowlisted or a `+suffix` variant is used. Production is unrestricted and correct to be. Happy to brainstorm this properly when it comes up the queue — the security question above is the one I would want settled before writing anything.
bermudalamb commented 2026-09-04 09:06:02 -05:00
Author
Owner
Copy Link
Copy Source

Done. Merged as PR #292, 13 commits.

The security question this issue asked to settle first was settled first: the mail carries the working link. The framing of it as a loosening comparable to a password reset overstated the case. A reset token takes over an account; an upload token grants one capability — submit photos into a queue where a person must approve them before anything is published. It reads nothing, it is revocable, max_submissions caps it, and #227 caps the whole intake surface regardless. The worst outcome of a leaked upload link is junk in the review queue, which is bounded and reversible.

The other open questions, answered: the address is required for new links while the column stays nullable, so links already in QA are grandfathered rather than backfilled with something untrue; it lives on the link rather than on a contributor entity; an uncapped link is emailable and its allowance renders as words rather than a missing number; re-sending a lost link and notifying on revoke are both out of scope, since only the digest is stored and "re-send" really means minting a new link and revoking the old one.

A failed send never loses the link. The token is shown exactly once, so rolling back would leave the admin retrying and holding a different one. The link is created, the send attempted, and the response reports which of sent / skipped-unconfigured / skipped-blocked happened — which matters precisely because of the operational note in this issue: QA's MAIL_ALLOWLIST silently skips any address outside it and returns as though it sent.

Three things the final whole-branch review caught that per-task reviews structurally could not, all fixed before merge:

  • Test edits in one task reintroduced live Gmail sends at the integration layer — the same hazard an earlier task had removed at the unit layer, in a different commit.
  • The awaited send had no timeout. Nodemailer defaults to two minutes to connect, so an unreachable SMTP host would commit the token and never render it. Now bounded at 5s; this was a real hole in the "never loses the link" invariant.
  • The SAMPLE_VALUES guard that should have caught the new placeholders was itself dead, iterating a hardcoded key list that had gone stale. It now derives from TEMPLATES.

Confirmed in CI: runs 809/810 on main at b8ea33f passed with the end-to-end step included.

One follow-up for the cleanup work rather than here: env.setup.ts still inherits SMTP_USER/SMTP_PASSWORD/MAIL_ALLOWLIST from the shell, so a future mail-touching integration test written without its own mock reopens the same live-SMTP hazard. Seven existing files already hit mail-sending routes unmocked.

Done. Merged as PR #292, 13 commits. The security question this issue asked to settle first was settled first: **the mail carries the working link.** The framing of it as a loosening comparable to a password reset overstated the case. A reset token takes over an account; an upload token grants one capability — submit photos into a queue where a person must approve them before anything is published. It reads nothing, it is revocable, `max_submissions` caps it, and #227 caps the whole intake surface regardless. The worst outcome of a leaked upload link is junk in the review queue, which is bounded and reversible. The other open questions, answered: the address is required for new links while the column stays nullable, so links already in QA are grandfathered rather than backfilled with something untrue; it lives on the link rather than on a contributor entity; an uncapped link is emailable and its allowance renders as words rather than a missing number; re-sending a lost link and notifying on revoke are both out of scope, since only the digest is stored and "re-send" really means minting a new link and revoking the old one. **A failed send never loses the link.** The token is shown exactly once, so rolling back would leave the admin retrying and holding a different one. The link is created, the send attempted, and the response reports which of `sent` / `skipped-unconfigured` / `skipped-blocked` happened — which matters precisely because of the operational note in this issue: QA's `MAIL_ALLOWLIST` silently skips any address outside it and returns as though it sent. Three things the final whole-branch review caught that per-task reviews structurally could not, all fixed before merge: - Test edits in one task reintroduced live Gmail sends at the integration layer — the same hazard an earlier task had removed at the unit layer, in a different commit. - The awaited send had no timeout. Nodemailer defaults to two minutes to connect, so an unreachable SMTP host would commit the token and never render it. Now bounded at 5s; this was a real hole in the "never loses the link" invariant. - The `SAMPLE_VALUES` guard that should have caught the new placeholders was itself dead, iterating a hardcoded key list that had gone stale. It now derives from `TEMPLATES`. Confirmed in CI: runs 809/810 on `main` at `b8ea33f` passed with the end-to-end step included. One follow-up for the cleanup work rather than here: `env.setup.ts` still inherits `SMTP_USER`/`SMTP_PASSWORD`/`MAIL_ALLOWLIST` from the shell, so a future mail-touching integration test written without its own mock reopens the same live-SMTP hazard. Seven existing files already hit mail-sending routes unmocked.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
calamb (Chelsea Lamb) bermudalamb (Thom Lamb)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#260