PR 7 — Account settings + vote reminders + email compliance footer#7
PR 7 — Account settings + vote reminders + email compliance footer#7
Conversation
…footer (PR 7)
Adds three Account-screen features and the backend infrastructure for
governance vote reminder emails. All password / vault mutations are
atomic and round-trippable; account deletion is GDPR-complete; reminder
emails link to the frontend (not the backend API) and carry a
transparency footer on every send.
Change password (atomic, vault-aware)
- POST /auth/change-password accepts an optional {vault:{blob,etag}}
alongside the new authHash. Single DB transaction updates
users.stored_auth and vaults together — both land or neither does.
- Returns 409 vault_rewrap_required if the client omits the rewrapped
blob while a vault exists, so we can never leave a vault the user
can no longer decrypt.
Notification preferences
- users.notification_prefs JSON blob; GET + PUT /auth/prefs with
zod-validated shape. Default {voteReminders:{enabled:true}} — opt-out,
not opt-in.
Delete account (GDPR erasure)
- DELETE /auth/account requires session auth AND fresh password re-proof
(oldAuthHash). Runs atomically:
1. pendingRegistrations.purgeForEmail(email) — the one table keyed
by email rather than user_id; without this a stale verification
token could silently re-mint the account.
2. users.deleteById(id) — ON DELETE CASCADE handles sessions,
vaults, email_verifications, tracked_masternodes,
vote_reminder_log, vote_receipts.
- Clears sid + csrf cookies, returns 204.
Governance vote reminders
- Schema: vote_reminder_log.proposal_hash -> scope_key (stores
cycle:<voting_deadline_unix>). ON DELETE CASCADE audited on every
user-scoped table.
- New reminderLog module (idempotent has/insert).
- New reminderDispatcher.tick(): cycle derived from earliest active
proposal's end_epoch; walks verified opted-in users; for each bucket
(days_before, final_24h) skips BOTH if the user has any relayed
receipt this cycle (voteReceipts.hasAnyRelayedInCycle). Injectable
`now` for deterministic tests.
- Mailer sendVoteReminder: generic, bucket-aware subject + body; no
per-proposal hashes leaked.
- server.js wires the dispatcher behind an hourly setInterval.
Mailer URL fix
- Every templated link now derives from publicBaseUrl
(FRONTEND_URL -> CORS_ORIGIN -> http://localhost:3000). Reminder
emails land on sysnode.info/governance and /account, not the old
hardcoded syscoin.dev/* (the backend API host). Tests assert the
frontend host is used and syscoin.dev is absent.
Compliance footer on every outbound email
- New buildFooter({kind,to,accountUrl}) in three flavors:
vote_reminder — opt-in, manage link visible
account_security — transactional, cannot disable, review link
account_verification — no account yet, safe-ignore path
- Every template declares footerKind; buildMessage throws if missing,
so a new email template cannot ship without a compliance footer.
- Footer always surfaces the recipient address (HTML-escaped) so
forwarded/misrouted mail is recognizable. Attribution line marks
every message as automated, do-not-reply.
Made-with: Cursor
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e54f7016f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| proposal_hash TEXT NOT NULL, | ||
| scope_key TEXT NOT NULL, |
There was a problem hiding this comment.
Adding scope_key only by editing 001_init.sql breaks upgrades for any database that already applied the old 001_init.sql (which still has proposal_hash). lib/db.js records applied migrations by filename, so 001_init.sql will not be re-run, and those instances keep the old schema; then startup code that prepares queries on scope_key (via createReminderLog in server.js) will fail with no such column: scope_key, preventing the service from booting after deploy. This needs a new migration file that renames/rebuilds vote_reminder_log for existing databases.
Useful? React with 👍 / 👎.
…und 1 P1) Move the proposal_hash -> scope_key rename out of 001_init.sql and into a dedicated 002_vote_reminder_log_scope_key.sql. lib/db.js tracks applied migrations by filename, so an in-place edit to 001 only reaches fresh databases; any existing deployment would keep proposal_hash and startup would blow up on reminderLog's `scope_key` prepared statements. - 001_init.sql: reverted to the shipped schema (proposal_hash). No semantic change for fresh databases — 002 renames the column immediately afterward — but 001 on disk now matches 001 as applied on every existing production row, which is what the migration tracker assumes. - 002_vote_reminder_log_scope_key.sql: single ALTER TABLE RENAME COLUMN. SQLite >= 3.25 updates the UNIQUE(user_id, *, bucket) constraint in place so no further DDL is needed. - db.test.js: regression test exercises the upgrade path explicitly — simulate a pre-PR-7 DB (proposal_hash + no 002 row) and assert migrate() renames the column while preserving data and the UNIQUE constraint. Made-with: Cursor
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6a1f83bf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const existingVault = | ||
| vaults && typeof vaults.get === 'function' | ||
| ? vaults.get(req.user.id) | ||
| : null; | ||
| if (existingVault && !parsed.data.vault) { | ||
| return res.status(409).json({ error: 'vault_rewrap_required' }); |
There was a problem hiding this comment.
The /auth/change-password handler checks whether a vault row exists before runAtomic, then updates stored_auth inside the transaction. In a multi-worker/process deployment sharing the same DB, another request can create the user's first vault row after this precheck but before the password rotation commits, so a request without vault still succeeds and leaves the vault wrapped under the old key. That reintroduces the lockout state this 409 guard is intended to prevent; the existence check should be performed in the same transaction as the auth update (or guarded via a conditional write).
Useful? React with 👍 / 👎.
…schema (Codex round 2) Backend Codex PR 7 round-2 findings addressed: P2 routes/auth.js:556 — vault-presence check now runs inside the same runAtomic() transaction as the auth rotation. Previously the check ran as a standalone SELECT before runAtomic, which in a multi-worker deployment opens a TOCTOU window where a peer request can create the user's first vault row between our pre-check and our COMMIT. The resulting /auth/change-password would succeed with a new authHash that cannot open the peer-wrapped vault → permanent lockout. Moving the check into the transaction collapses that window to zero (better-sqlite3 serializes writes in-process; SQLite's write lock serializes across processes). Added a regression test that spies on vaults.get and asserts every call during /change-password happens with db.inTransaction === true. db/migrations revert — per pre-production lifecycle, edits to the v1 schema belong in 001_init.sql directly; there is no deployed database to migrate from. The scope_key column is now defined in-place and the previous 002_vote_reminder_log_scope_key.sql forward migration + its regression test have been removed. Added a reviewer-facing note in the 001_init.sql header (and a pointer at the vote_reminder_log table) explaining the rule: edit in place until v1 ships, new files thereafter. Made-with: Cursor
|
@codex review Round-2 P2 findings addressed in ec511e2:
Review guidance — sysnode-backend is pre-production. There is no deployed database that has applied |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec511e2f1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // invalid_blob / blob_too_large — all of which roll back | ||
| // the transaction here before we touch auth state. Caught | ||
| // below and translated into HTTP status codes. | ||
| const putOut = vaults.put(req.user.id, { |
There was a problem hiding this comment.
createAuthRouter documents vaults as optional for auth-only mounts, but this path unconditionally dereferences vaults.put when a client sends a vault payload. In configurations where vaults is intentionally omitted (or a harness only mounts auth), /auth/change-password will throw a TypeError and surface a 500 instead of a deterministic API error, which makes the endpoint brittle in the exact mode the router claims to support. Add an explicit guard/rejection for vault-bearing requests when no vault repo is configured.
Useful? React with 👍 / 👎.
…ent (Codex round 2 P3) createAuthRouter documents `vaults` as optional (auth-only harnesses may omit it), but /auth/change-password previously dereferenced `vaults.put` unconditionally when the client sent a vault payload, surfacing a TypeError → 500 instead of a deterministic API error in the exact mode the factory claims to support. Fail fast with 503 server_misconfigured (the same code used for other config-level faults, e.g. a missing KDF pepper) when the client sends a vault body but no repo is wired. The auth-only path (no vault in body) continues to work without a repo, matching the optional contract. Tests added in tests/auth.routes.test.js: - vault-bearing request with no vault.put → 503 server_misconfigured and auth is NOT rotated (old password still logs in). - auth-only rotation still succeeds when vault.put/.get are absent, proving the guard is scoped to the vault-bearing branch only. Made-with: Cursor
|
@codex review Round-2 P3 addressed in 97af6c5:
Schema reviewer guidance (restating from previous ping): sysnode-backend is pre-production, so forward migrations are not relevant — in-place edits to |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97af6c50ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (vaults && typeof vaults.put !== 'function') { | ||
| throw new Error('createAuthRouter: vaults.put must be a function'); |
There was a problem hiding this comment.
/auth/change-password relies on vaults.get to enforce vault_rewrap_required, but createAuthRouter only validates vaults.put during setup. In deployments/tests that pass a partial vault repo with put but no get, existingVault is forced to null and an auth-only password change can succeed even when a vault row exists, which can leave the account unable to decrypt its vault. This should fail fast as misconfiguration (or require vaults.get) whenever vault-backed password rotation is enabled.
Useful? React with 👍 / 👎.
…iring change-password (Codex round 2 P3 followup) /auth/change-password uses BOTH `vaults.put` (to rewrap the blob) and `vaults.get` (to enforce the vault_rewrap_required 409 inside the atomic rotation). The previous factory guard only validated `.put`. A partial repo with `put` but no `get` would make the in-transaction `existingVault` fall through to `null`, silently bypassing the 409 guard — a vault-bearing user could rotate their password without rewrapping and permanently lock themselves out of their vault. Strengthen the factory: when `vaults` is provided, both `.put` and `.get` must be functions. `vaults: undefined` (auth-only mount) is still accepted per the optional contract. The in-transaction call site is simplified accordingly — the defensive `typeof vaults.get === 'function'` check is redundant once the factory enforces the invariant at construction. Tests reworked in tests/auth.routes.test.js: - Dropped two monkey-patch tests that modeled post-construction `delete ctx.vaults.put/.get` — states the factory now prevents. - Added a focused `createAuthRouter factory contract` describe block with pure-input tests: partial-repo shapes throw at construction, complete repo accepted, `vaults: undefined` accepted, `runAtomic` still required regardless of vaults. Made-with: Cursor
|
@codex review Round-2 P3 followup addressed in 803e9f8:
Pre-production reviewer reminder (repeated for completeness): forward migrations are not relevant yet — in-place edits to |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 803e9f8d02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const scopeKey = `cycle:${earliestEndEpoch}`; | ||
| const proposalHashes = proposals.map((p) => p.hash); |
There was a problem hiding this comment.
tick() defines the reminder cycle from the earliest proposal deadline, but proposalHashes is built from all active proposals. When active proposals have mixed endEpoch values, a user who voted only on a later-deadline proposal is treated as already-voted and gets suppressed for the earlier-deadline cycle, so required reminders are skipped. This also conflicts with the cycle semantics documented in this file (new earlier deadline should form a new cycle). Filter hashes to the proposals in the chosen cycle before calling hasAnyRelayedInCycle.
Useful? React with 👍 / 👎.
…ent cycle (Codex round 2 P2) lib/reminderDispatcher.js — `tick()` computed the cycle from the earliest end_epoch but then built `proposalHashes` from ALL active proposals. When active proposals spanned multiple cycles (e.g. a proposal for superblock N+1 was submitted while superblock N voting is still open), a user who voted only on the later-cycle proposal was treated as already-voted for the earlier cycle and had their reminder silently suppressed — the exact opposite of the product intent, which is "if user voted in THIS cycle, skip; otherwise remind". Fix: compute `cycleProposals` as the set of proposals whose deadlineMs matches the earliest deadline (Syscoin governance rule: proposals voting for the same superblock share the same voting- deadline block, hence the same end_epoch). Use that filtered set for (a) the `hasAnyRelayedInCycle` gate, (b) the email body's `proposalCount`, and (c) the tick result's `proposalCount` metric. Tests (lib/reminderDispatcher.test.js): - Updated the `proposalsEndingIn` fixture so P1 and P2 share the same end_epoch, matching the semantic meaning of "same cycle" the fixture has always intended (the prior +1h offset was a test-only fiction). - Added a targeted regression test: with P1 ending in 48h (current cycle, days_before bucket) and P2 ending in 96h (future cycle, too_early on its own), a user who voted on P2 must still get the reminder for P1's cycle, with proposalCount=1 rather than 2. Made-with: Cursor
|
@codex review Round-2 P2 addressed in a7387da:
Pre-production reviewer reminder (again): forward migrations are not relevant — in-place edits to |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Adds three Account-screen features and the backend infrastructure for governance vote reminder emails. All password / vault mutations are atomic and round-trippable; account deletion is GDPR-complete; reminder emails link to the frontend host (not the backend API) and every outbound email now carries a transparency footer.
Frontend companion: syscoin/sysnode-info#account-settings-pr7
What's in it
Change password (atomic, vault-aware)
POST /auth/change-passwordaccepts an optionalvault:{blob,etag}alongside the newauthHash. One DB transaction updatesusers.stored_authandvaultstogether — both land or neither does.409 vault_rewrap_requiredif the client omits the rewrapped blob while a vault exists, so we can never leave a vault the user can no longer decrypt.Notification preferences
users.notification_prefsJSON blob.GET + PUT /auth/prefswith zod-validated shape. Default{voteReminders:{enabled:true}}— opt-out, not opt-in.Delete account (GDPR erasure)
DELETE /auth/accountrequires session auth and fresh password re-proof (oldAuthHash). Runs in a single transaction:pendingRegistrations.purgeForEmail(email)— the one table keyed by email rather thanuser_id; without this a stale verification token could silently re-mint the account.users.deleteById(id)—ON DELETE CASCADEhandlessessions,vaults,email_verifications,tracked_masternodes,vote_reminder_log,vote_receipts.sid+csrfcookies, returns204.Governance vote reminders
vote_reminder_log.proposal_hash→scope_key(storescycle:<voting_deadline_unix>).ON DELETE CASCADEaudited on every user-scoped table.reminderLogmodule (idempotenthas/insert).reminderDispatcher.tick(): cycle derived from the earliest active proposal'send_epoch; walks verified opted-in users; for each bucket (days_before,final_24h) skips both if the user has any relayed receipt this cycle (voteReceipts.hasAnyRelayedInCycle). Injectablenowfor deterministic tests.mailer.sendVoteReminder: generic, bucket-aware subject + body; no per-proposal hashes leaked.server.jswires the dispatcher behind an hourlysetInterval.Mailer URL fix
publicBaseUrl(FRONTEND_URL→CORS_ORIGIN→http://localhost:3000). Reminder emails land onsysnode.info/governanceand/account, not the old hardcodedsyscoin.dev/*(the backend API host). Tests assert the frontend host is used andsyscoin.devis absent.Compliance footer on every outbound email
buildFooter({kind,to,accountUrl})in three flavors:vote_reminder— opt-in, manage link visibleaccount_security— transactional, cannot disable, review linkaccount_verification— no account yet, safe-ignore pathfooterKind;buildMessagethrows if missing, so a new email template cannot ship without a compliance footer.Test plan
reminderLog,deleteById,hasAnyRelayedInCycle,listWithRemindersEnabled, mailer URL assertions,buildFooterby kind, unknown-kind throws, everysendXxxcarries its footer, every template declaresfooterKind.days_beforeemail arrives with links tosysnode.info/governanceand/account, notsyscoin.dev, and contains the compliance footer./auth/mereturns401, the vault is gone, and re-registering the same email works.Notes for review
pending_registrationsis intentionally not in the FK cascade chain (it's keyed by email, notuser_id), sodeleteById's contract explicitly delegates that row to the caller — documented in thedeleteByIdcomment inlib/users.jsso nobody wires a "just calldeleteById" path around it later and silently leaks re-registration tokens.List-Unsubscribe/List-Unsubscribe-Post(RFC 8058) one-click unsubscribe — Gmail/Yahoo enforce this for bulk senders. Needs a signed unsubscribe endpoint + route + spec review; belongs in a small follow-up.attributionTextas a one-liner once there is an official address.Made with Cursor