Add email verification for user registration#49
Conversation
Implements Issue #46 - optional email verification during join flow: Server changes: - Add configurable email provider (SMTP or Resend API) - Add verification code generation and storage - Add VerifyEmail and ResendVerificationEmail gRPC handlers - Require valid verification before creating principal when enabled CLI changes: - Add --verification-code flag to join command - Interactive flow: prompts for code when verification required Web app changes: - Update join page to handle verification step - Show verification code input when server requires it E2E testing: - Add MailHog support via docker-compose.test.yaml - Add email_verification.rs tests for CLI flow - Add join-verification.spec.ts Playwright tests for web - Update CI workflows to run MailHog for tests Configuration via environment variables: - ZOPP_EMAIL_VERIFICATION_REQUIRED (true/false) - ZOPP_EMAIL_PROVIDER (smtp/resend) - SMTP_HOST, SMTP_PORT, SMTP_USERNAME, SMTP_PASSWORD - RESEND_API_KEY - ZOPP_EMAIL_FROM, ZOPP_EMAIL_FROM_NAME
There was a problem hiding this comment.
11 issues found across 67 files
Confidence score: 3/5
- Email verification can be effectively bypassed if provider init fails in
apps/zopp-server/src/main.rs, since required verification is silently disabled; that’s concrete user-impacting risk. - Invite consumption isn’t atomic in
crates/zopp-store-postgres/src/lib.rs, so concurrent joins could reuse a token, and bootstrap invites inapps/zopp-server/src/handlers/auth.rscan be consumed by existing users when verification is required. - Migration in
crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sqlbackfills all users as unverified, which could lock out existing accounts if verification is intended only for new joins. - Pay close attention to
apps/zopp-server/src/main.rs,apps/zopp-server/src/handlers/auth.rs,crates/zopp-store-postgres/src/lib.rs,crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql- verification/consume-flow correctness and migration impact.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/zopp-server/src/handlers/verification.rs">
<violation number="1" location="apps/zopp-server/src/handlers/verification.rs:110">
P2: After incrementing failed attempts, the handler doesn’t immediately lock out when the counter reaches/exceeds MAX_ATTEMPTS. Under concurrent attempts this can return negative remaining and allow extra tries until a later request. Delete the verification and return the lockout response once `attempts >= MAX_ATTEMPTS`, and clamp remaining to non-negative.</violation>
</file>
<file name="apps/e2e-tests/tests/common/harness.rs">
<violation number="1" location="apps/e2e-tests/tests/common/harness.rs:630">
P2: Escape the email before interpolating it into the SQL command to avoid malformed queries or injection when test data contains single quotes.</violation>
<violation number="2" location="apps/e2e-tests/tests/common/harness.rs:659">
P2: Escape the email before interpolating it into the SQLite query to avoid malformed SQL or injection when test data contains single quotes.</violation>
</file>
<file name=".github/workflows/e2e.yaml">
<violation number="1" location=".github/workflows/e2e.yaml:35">
P2: Pin the MailHog image to a specific version (or digest) to keep CI runs reproducible; `latest` can change unexpectedly and break E2E tests.</violation>
</file>
<file name=".github/workflows/web-e2e.yaml">
<violation number="1" location=".github/workflows/web-e2e.yaml:67">
P2: Fail the workflow if MailHog does not become ready after the retry loop to avoid running tests without the email service.</violation>
</file>
<file name="apps/zopp-cli/src/commands/join.rs">
<violation number="1" location="apps/zopp-cli/src/commands/join.rs:348">
P2: The max-attempts guard never actually limits retries because it resets the counter, allowing unlimited verification attempts without a resend. Consider stopping the flow (or requiring a resend) once the limit is reached.</violation>
</file>
<file name="apps/zopp-web/tests/fixtures/verification-setup.ts">
<violation number="1" location="apps/zopp-web/tests/fixtures/verification-setup.ts:194">
P2: Unsafely interpolating `email` into a shell command/SQL query allows quote-breaking (and potential shell injection) in tests. Escape the email or use an argument-based execFileSync call to avoid the shell.</violation>
</file>
<file name="apps/zopp-server/src/handlers/auth.rs">
<violation number="1" location="apps/zopp-server/src/handlers/auth.rs:68">
P2: Bootstrap invites should still reject existing users when email verification is required. Add the same guard used in the non-verification flow so an AlreadyExists user can’t consume a bootstrap invite.</violation>
</file>
<file name="crates/zopp-store-postgres/src/lib.rs">
<violation number="1" location="crates/zopp-store-postgres/src/lib.rs:431">
P2: Consume invite should be atomic. Updating by token without `consumed = FALSE` lets concurrent requests both mark the invite used and proceed, so the invite can be consumed multiple times. Add a guard in the UPDATE to fail if already consumed and use rows_affected() to detect it.</violation>
</file>
<file name="apps/zopp-server/src/main.rs">
<violation number="1" location="apps/zopp-server/src/main.rs:319">
P1: If email verification is configured as required, failing to initialize the provider should abort startup instead of silently disabling verification. Otherwise a misconfigured server can accept joins without verification.</violation>
</file>
<file name="crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql">
<violation number="1" location="crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql:6">
P2: This backfills all existing users as unverified. If verification is only intended for new join flows, existing accounts will suddenly be marked unverified. Consider updating existing rows to true during the migration to preserve current user state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fixes issues identified by Cubic AI reviewer: P1 (Critical): - Server now aborts startup if email verification is required but provider initialization fails, preventing silent verification bypass P2 (Security/Correctness): - Verification handler now locks out and deletes record immediately when MAX_ATTEMPTS is reached, preventing negative remaining attempts - SQL injection prevention: escape single quotes in email before SQL interpolation in test harness and TypeScript fixtures - Pin MailHog image to v1.0.1 for reproducible CI builds - Fail web-e2e workflow if MailHog doesn't become ready - CLI join flow now requires resend when server returns 0 attempts remaining, preventing unlimited local retries - Bootstrap invites now reject verified existing users when email verification is required, matching non-verification flow behavior - consume_invite is now atomic in both PostgreSQL and SQLite stores, checking consumed=FALSE in UPDATE to prevent concurrent consumption - Migration now sets existing users as verified (DEFAULT TRUE) then changes default to FALSE for new users, preserving existing accounts Also updates sqlx metadata for both stores.
The test was creating a user WITH a principal, but according to our implementation: - Users with principals are verified immediately (no email verification) - Users without principals are unverified (email verification required) Changed test to create user without principal to properly test the mark_user_verified flow used after email verification.
Removed mock_smtp module and mailin-embedded dependency since we use MailHog for email testing. mailin-embedded was pulling in aws-lc-rs via rustls which conflicted with kube's ring-based rustls configuration, causing K8s tests to fail.
- Add getVerificationCodeFromMailHog helper function - Update setupTestData to handle email verification when MAILHOG_API_URL is set - Use --verification-code flag for CLI join command - Increase maxBuffer to prevent ENOBUFS errors - Also increase operator sync timeout for debugging CI
Use getByRole('heading') instead of getByText(/verification code/i)
which was matching multiple elements. Also use exact placeholder match.
|
@cubic-dev-ai All issues from your review have been addressed in commit 0cd5a81. Please re-review. |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 75 files
Confidence score: 3/5
crates/zopp-store-postgres/src/lib.rshas a concrete bug risk:consume_invitecan consume revoked tokens since the update doesn’t filterrevoked = FALSE, which can let revoked invites be treated as valid.- UI behavior risk in
apps/zopp-web/src/pages/register.rs: if verification succeeds butcomplete_registration_implfails, the UI remains stuck in a loading state becauseverifyingisn’t reset and errors route to the join form. - Given the medium-severity functional issues, there’s some user-impacting regression risk, so this feels slightly risky to merge without fixes.
- Pay close attention to
crates/zopp-store-postgres/src/lib.rs,apps/zopp-web/src/pages/register.rs,apps/zopp-web/tests/fixtures/test-setup.ts,crates/zopp-store-sqlite/src/lib.rs- functional correctness and test reliability.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="crates/zopp-store-postgres/src/lib.rs">
<violation number="1" location="crates/zopp-store-postgres/src/lib.rs:441">
P2: consume_invite can mark revoked invites as consumed because the update doesn’t exclude revoked tokens. Revoked invites are treated as not found elsewhere, so this should also filter revoked = FALSE (and return NotFound for revoked tokens).</violation>
</file>
<file name="apps/zopp-web/tests/fixtures/test-setup.ts">
<violation number="1" location="apps/zopp-web/tests/fixtures/test-setup.ts:34">
P2: Add a fetch timeout/abort so the retry loop can honor `timeoutMs` instead of hanging on a stalled request.</violation>
</file>
<file name="apps/zopp-web/src/pages/register.rs">
<violation number="1" location="apps/zopp-web/src/pages/register.rs:79">
P2: If `complete_registration_impl` fails after verification succeeds, the verification UI stays in a loading state with no error because `verifying` isn’t reset and errors go to the join form. Add an `else` branch to clear `verifying` and show a verification error.</violation>
</file>
<file name="crates/zopp-store-sqlite/src/lib.rs">
<violation number="1" location="crates/zopp-store-sqlite/src/lib.rs:4331">
P3: The new tests use `matches!` without asserting the result, so they don't actually verify the error type and will pass even if the wrong error is returned. Use `assert!(matches!(...))` (or `assert!(err == ...)`) to make the tests effective.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fixes 4 issues identified by Cubic AI reviewer: P2 (Security/Correctness): - consume_invite now filters revoked=FALSE to prevent consuming revoked invites in both PostgreSQL and SQLite stores - Add fetch timeout/abort to test-setup.ts getVerificationCodeFromMailHog to honor overall timeout instead of hanging on stalled requests - Fix verification UI to use correct signals when complete_registration_impl fails after successful verification, preventing stuck loading state P3 (Test quality): - Add assert!() around matches!() calls in SQLite tests to actually verify error types instead of silently passing Also updates sqlx metadata for both stores.
|
@cubic-dev-ai All 4 issues from your re-review have been addressed in commit d7832a4. Please re-review. |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 75 files
Confidence score: 4/5
- This PR looks safe to merge overall; the noted issues are low severity and limited to test/comment accuracy rather than core runtime behavior.
- Potential flakiness in
apps/e2e-tests/tests/email_verification.rsif the generated code can match "000000", which could intermittently fail the test suite. - Pay close attention to
apps/e2e-tests/tests/email_verification.rsandapps/e2e-tests/tests/common/harness.rs- test flakiness risk and misleading fallback comment.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/e2e-tests/tests/common/harness.rs">
<violation number="1" location="apps/e2e-tests/tests/common/harness.rs:235">
P3: The comment claims a fallback to a mock SMTP server, but the implementation errors when MailHog isn’t available. Update the comment to match the behavior (or implement the fallback) to avoid misleading test setup guidance.</violation>
</file>
<file name="apps/e2e-tests/tests/email_verification.rs">
<violation number="1" location="apps/e2e-tests/tests/email_verification.rs:77">
P3: Using "000000" as the invalid verification code can occasionally match a real generated code (range includes 000000), making this test flaky. Use a non-numeric placeholder or derive an invalid code from the real one to guarantee failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
P3 fixes: - Update harness.rs comment to accurately state MailHog is required (no fallback to mock SMTP exists) - Use non-numeric "XXXXXX" as invalid verification code in test to guarantee it never matches a real generated code
|
@cubic-dev-ai Fixed the 2 P3 issues in commit 2f4251f. Please re-review. |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 75 files
Confidence score: 3/5
- Resend flow in
apps/zopp-server/src/handlers/verification.rscan invalidate a valid code before confirming email delivery, which risks blocking real users if sending fails. - Several test-harness issues can lead to flakiness or false positives (readiness check, MailHog cleanup, and fixed invalid code), so there is some risk of unreliable CI signal.
- These are moderate-severity and user-impacting in one case, so there is some merge risk but likely manageable with quick follow-ups.
- Pay close attention to
apps/zopp-server/src/handlers/verification.rs,apps/zopp-web/tests/fixtures/verification-setup.ts,apps/e2e-tests/tests/common/harness.rs,apps/e2e-tests/tests/email_verification.rs- verification resend behavior and test flakiness/cleanup correctness.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/zopp-web/tests/fixtures/verification-setup.ts">
<violation number="1" location="apps/zopp-web/tests/fixtures/verification-setup.ts:43">
P2: waitForServer should only return true when the readiness endpoint returns success; otherwise tests can proceed while the server is still starting.</violation>
</file>
<file name="apps/zopp-server/src/handlers/verification.rs">
<violation number="1" location="apps/zopp-server/src/handlers/verification.rs:324">
P2: Resend invalidates the existing verification code before confirming the email was sent. If the send fails, the previous code is lost and the user cannot verify with the code they already received.</violation>
</file>
<file name="apps/e2e-tests/tests/email_verification.rs">
<violation number="1" location="apps/e2e-tests/tests/email_verification.rs:139">
P3: Using "000000" as the invalid verification code can intermittently pass because the generator can legitimately produce 000000. This makes the E2E test flaky.</violation>
</file>
<file name="apps/e2e-tests/tests/common/harness.rs">
<violation number="1" location="apps/e2e-tests/tests/common/harness.rs:269">
P2: Don’t ignore MailHog clear failures; stale messages can cause tests to pick up an old verification code and pass/fail nondeterministically. Propagate the error so the test fails fast when cleanup fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
P2 fixes: - verification-setup.ts: Check response.ok before returning true from waitForServer to ensure server is actually ready - verification.rs: Send email BEFORE updating DB in resend flow so old code is preserved if email sending fails - harness.rs: Propagate MailHog clear errors instead of ignoring them to fail fast on cleanup issues that could cause flaky tests P3 fixes: - email_verification.rs: Fix remaining "000000" to use "XXXXXX"
|
@cubic-dev-ai Please do a full re-review of the PR. |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 75 files
Confidence score: 3/5
- Potential user-impacting bug in
apps/zopp-server/src/handlers/auth.rs: creating the verification record before sending email can prevent resend attempts if the email fails, so some users may never receive verification. - There’s a security hardening gap in
crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sqlwhere verification codes are stored in plain text; hashing would reduce risk in a DB compromise scenario. - Pay close attention to
apps/zopp-server/src/handlers/auth.rs,crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql,apps/zopp-web/tests/fixtures/verification-setup.ts- verification email flow ordering, plaintext codes, and test gRPC URL misconfiguration.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/zopp-web/tests/fixtures/verification-setup.ts">
<violation number="1" location="apps/zopp-web/tests/fixtures/verification-setup.ts:151">
P2: grpcWebUrl points to an unused Envoy port even though Envoy is not started, which will break any test using verificationContext.grpcWebUrl. Use the server port when testing gRPC directly or start Envoy.</violation>
</file>
<file name="apps/zopp-server/src/handlers/auth.rs">
<violation number="1" location="apps/zopp-server/src/handlers/auth.rs:118">
P2: Verification record is stored before email is sent. If email fails, retries won't resend the email because a valid verification record exists. Send email first (like `resend_verification` does) to ensure user receives code before record is committed.</violation>
</file>
<file name="crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql">
<violation number="1" location="crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql:19">
P2: Consider hashing verification codes rather than storing them in plain text. Even for short-lived codes with attempt limiting, hashing provides defense-in-depth against database compromise scenarios (leaked backups, SQL injection, insider threats). Use a fast hash like SHA-256 for the code since it's already low-entropy and rate-limited—or Argon2id for stronger protection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
P2 fixes from Cubic full re-review: - auth.rs: Send verification email BEFORE storing record, matching the resend flow fix. Prevents storing record when email fails. - verification-setup.ts: Use server port for grpcWebUrl since Envoy isn't started in this test setup. Also update CLAUDE.md with clearer Cubic workflow instructions.
- Add generic argon2_hash and argon2_hash_raw functions to zopp-crypto - Refactor CLI to use zopp-crypto for key derivation and verification hash - Change storage schema from 'code' to 'code_hash' in both SQLite and Postgres - Update server handlers to hash codes before storage and comparison - Fix MailHog client to use v1 API for delete (v2 doesn't support DELETE) - Add create_unverified_test_user helper for testing unverified principal flows - Update E2E tests to use has_verification_record since plaintext codes are no longer stored
|
@cubic-dev-ai Please do a full re-review of the PR. |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 79 files
Confidence score: 3/5
- Storing verification codes as plain SHA-256 in
crates/zopp-storage/src/lib.rsenables offline brute-force if the table leaks; a slow hash or HMAC is needed to avoid a concrete security risk. apps/zopp-server/src/handlers/auth.rsappears to swallow storage lookup errors; non-NotFound failures could still send emails and mask internal errors, which is user-impacting.- Score reflects a notable security concern plus error-handling uncertainty; overall change seems manageable once those are addressed.
- Pay close attention to
crates/zopp-storage/src/lib.rs,apps/zopp-server/src/handlers/auth.rs,crates/zopp-store-sqlite/migrations/20260125000001_add_email_verification.sql- hashing approach and error handling consistency.
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="crates/zopp-storage/src/lib.rs">
<violation number="1" location="crates/zopp-storage/src/lib.rs:424">
P1: Storing verification codes as plain SHA-256 hashes makes offline brute-force trivial if the verification table is leaked. Use a slow password hash (e.g., Argon2id with per-record salt) or a server-secret HMAC so short codes aren’t crackable offline.</violation>
</file>
<file name="crates/zopp-store-sqlite/migrations/20260125000001_add_email_verification.sql">
<violation number="1" location="crates/zopp-store-sqlite/migrations/20260125000001_add_email_verification.sql:18">
P2: Comment says 'SHA-256 hash' but PR description states 'Argon2id hashes'. If the implementation uses Argon2id (as it should for brute-force resistance), update the comment to accurately reflect the hashing algorithm.</violation>
</file>
<file name="apps/zopp-server/src/handlers/auth.rs">
<violation number="1" location="apps/zopp-server/src/handlers/auth.rs:101">
P2: Don’t swallow storage errors when checking for an existing verification. If the lookup fails for reasons other than NotFound, the join should return an internal error instead of sending an email and potentially leaving no verification record.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Update comments from 'SHA-256 hash' to 'Argon2id hash' in storage structs - Update migration comments to reflect Argon2id usage - Fix auth.rs to properly handle storage errors vs NotFound when checking for existing verification records
|
@cubic-dev-ai Please do a full re-review of the PR. |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 79 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Remove all SaaS/cloud features that were added in PRs #49-#63 to refocus the project as an OSS self-hostable secrets manager. Preserves useful self-hosted infrastructure (email verification, Prometheus metrics, Helm charts, Amber Terminal web UI). Removed: - Organization and billing gRPC RPCs, messages, and proto definitions - Organization/billing server handlers and backend delegation - CLI `org` subcommand and organization command module - Store trait organization methods + SQLite/PostgreSQL/NoopStore implementations - Organization types (OrganizationId, OrganizationRole, Plan, SubscriptionStatus, etc.) - zopp-billing crate (Stripe integration) - Terraform AWS infrastructure (infra/terraform/) - Drop migrations added for both backends to clean up existing databases - Stale .sqlx query metadata for removed org/billing queries regenerated
…73) * refactor: strip organizations, billing, and Terraform cloud features Remove all SaaS/cloud features that were added in PRs #49-#63 to refocus the project as an OSS self-hostable secrets manager. Preserves useful self-hosted infrastructure (email verification, Prometheus metrics, Helm charts, Amber Terminal web UI). Removed: - Organization and billing gRPC RPCs, messages, and proto definitions - Organization/billing server handlers and backend delegation - CLI `org` subcommand and organization command module - Store trait organization methods + SQLite/PostgreSQL/NoopStore implementations - Organization types (OrganizationId, OrganizationRole, Plan, SubscriptionStatus, etc.) - zopp-billing crate (Stripe integration) - Terraform AWS infrastructure (infra/terraform/) - Drop migrations added for both backends to clean up existing databases - Stale .sqlx query metadata for removed org/billing queries regenerated * fix(deps): update bytes 1.10.1 → 1.11.1 (RUSTSEC-2026-0007) Fixes integer overflow vulnerability in BytesMut::reserve that could cause out-of-bounds memory access in release builds.
Summary
--verification-codeflag and interactive promptsTest plan
cargo test --workspace --all-features- all tests passdocker compose -f docker/docker-compose.test.yaml up -d && cargo test --package e2e-testscd apps/zopp-web && npm run test:e2eZOPP_EMAIL_VERIFICATION_REQUIRED=trueand verify join flow requires codeCloses #46
Summary by cubic
Adds optional email verification to the join flow across server, CLI, and web. When enabled, users must enter a 6-digit code sent by email. Implements #46.
New Features
Migration
Written for commit d9e309a. Summary will update on new commits.