Fix/1449 signup email verification

[dawoldo]

🤔 What

Implemented a secure email verification workflow for both account registration and email changes.

• Pending Email Storage: Added pendingMail attribute to the TCaver model to store unverified email addresses.
• Account Verification Enhancement: Updated the verify-email controller to support both initial account activation and email change confirmation.
• Secure Email Change: Revamped the change-email controller to initiate a verification flow instead of immediate updates.
• Conflict Management: Implemented logic to prevent email collisions across both primary and pending email fields.
• Cancellation Support: Users can now cancel a pending email change by re-submitting their current email address.

🤷‍♂️ Why

Previously, changing an email address was immediate and unverified, which posed several risks:

1. Security: An attacker with temporary access to an account could change the email address and take full control without verification.
2. Account Lockout: If a user made a typo when changing their email, they could lose access to their account since the primary login identifier was updated immediately.
3. Data Integrity: There was no verification that the user actually owned the new email address.

This implementation ensures that the user's login identifier remains unchanged until the new email is verified, and ownership is proven through a secure token.

🔍 How

• Model Layer: Modified api/models/TCaver.js to include a pendingMail field.

BEGIN;
-- 1. Add the pending_mail column to store unverified email addresses
ALTER TABLE public.t_caver ADD COLUMN pending_mail varchar(50);
-- 2. Add an index to optimize uniqueness checks across both mail and pending_mail
CREATE INDEX t_caver_pending_mail_idx ON public.t_caver (pending_mail);
COMMIT;

• Workflow:
  • When a user requests an email change, the new address is stored in pendingMail.
  • mailIsValid is set to false, and an activationCode is generated and sent via email.
  • The user continues to log in using their original mail.
  • Upon clicking the verification link, the verify-email controller promotes pendingMail to mail and clears the pending state.
• Conflict Detection: Updated queries to check both mail and pendingMail fields across all users, ensuring that an email address cannot be claimed even as a pending change by another user.
• Identity Retention: The system now explicitly checks if the new email is identical to the current one, and if so, interprets it as a request to cancel any existing pending changes.

🧪 Testing

Comprehensive integration tests have been added in test/integration/4_routes/Account/change-email.test.js covering:

• Success Paths: Email change initiation and subsequent verification.
• Conflict Handling: Verification that neither primary nor pending emails can be duplicated.
• Login Persistence: Ensuring login works with the old email while a change is pending.
• Cancellation: Verifying that submittting the current email clears the pending state.
• Edge Cases: Handling scenarios where a pending email is taken by another user before verification is completed.

[ClemRz]

Issues (Must Fix)

• [sql/9_03_2026_05_08_add_caver_pending_mail.sql:1] The migration file is missing the required \c grottoce; prefix (needed for Docker init compatibility) and lacks IF NOT EXISTS for idempotency. Additionally, the PR body describes an index (CREATE INDEX t_caver_pending_mail_idx ON public.t_caver (pending_mail)) that is absent from the actual migration file. Since the change-email controller queries pendingMail for conflict detection, this index is important for performance.

  \c grottoce;
  
  ALTER TABLE t_caver ADD COLUMN IF NOT EXISTS pending_mail VARCHAR(50) DEFAULT NULL;
  CREATE INDEX IF NOT EXISTS t_caver_pending_mail_idx ON t_caver (pending_mail);
  

• [test/customSQL.js] Per project conventions, any index added in sql/ must also be added to the INDEX_OPTIMIZATION_MIGRATION block in test/customSQL.js so the test database has the same indexes as production. The t_caver_pending_mail_idx index is missing here.

  CREATE INDEX IF NOT EXISTS t_caver_pending_mail_idx ON t_caver (pending_mail);
  

• [api/controllers/v1/auth/verify-email.js:22-25] The conflict check at verification time only looks at mail of other users but doesn't exclude the current user and doesn't check other users' pendingMail. This is inconsistent with the more thorough check in change-email.js. If another user has the same email as their pendingMail, the verification would succeed and create a state where two users have the same email (one as mail, one as pendingMail). Consider aligning with the pattern in change-email.js:

  const alreadyInUse = await TCaver.findOne({
    id: { '!=': caver.id },
    or: [{ mail: caver.pendingMail }, { pendingMail: caver.pendingMail }],
  });
  

Suggestions (Should Consider)

• [api/controllers/v1/auth/verify-email.js:44-48] There's a TOCTOU (time-of-check-time-of-use) race condition between the conflict check and the update. If two users verify at the exact same time with the same target email, both could pass the check and one update would violate the DB unique constraint on mail. Consider wrapping the check + update in error handling that catches the unique constraint violation from PostgreSQL (error code 23505) and returns res.conflict() gracefully, rather than letting it bubble up as a 500.

• [api/controllers/v1/account/change-email.js:31-34] Same TOCTOU concern here — between the findOne conflict check and the updateOne, another user could claim the same email. The DB unique constraint on mail protects the final commit in verify-email, but there's no unique constraint on pending_mail. Two users could end up with the same pendingMail value if requests arrive concurrently. Consider adding a unique constraint on pending_mail (with a partial index excluding NULLs) or accepting this as a low-risk edge case since only one can ultimately verify.

• [api/controllers/v1/account/change-email.js:3-5] The controller uses req.param('email') which reads from both query string and body. The Swagger spec documents it as in: query, but the tests send it in the request body. Consider documenting it as a requestBody in the Swagger spec to match actual usage, or at minimum noting that both are accepted.

• [test/integration/4_routes/Account/change-email.test.js:155] The test assumes the fixture password is 'testtest'. If this changes, the test will break silently. Consider importing the password from a shared constant or adding a comment referencing where this value is defined in fixtures.

Nitpicks (Optional)

• [api/controllers/v1/account/change-email.js:67] The final success path returns res.ok() (which yields 204 since no data is passed). Consider returning a message like res.ok({ message: 'Verification email sent.' }) for consistency with the cancellation path that returns a message body. This would also make the Swagger 200 response more accurate.

• [assets/swaggerV1.yaml:3148-3149] There are two consecutive blank lines after the '409' response. Minor formatting issue.

[ClemRz]

Issues (Must Fix)

• [sql/9_03_2026_05_08_add_caver_pending_mail.sql:1] The migration file is missing the required \c grottoce; prefix (needed for Docker init compatibility) and lacks IF NOT EXISTS for idempotency. The PR body also describes an index (CREATE INDEX t_caver_pending_mail_idx) that is absent from the actual migration file. Since the update.js controller queries pendingMail for conflict detection, this index matters for performance.

  \c grottoce;
  
  ALTER TABLE t_caver ADD COLUMN IF NOT EXISTS pending_mail VARCHAR(50) DEFAULT NULL;
  CREATE INDEX IF NOT EXISTS t_caver_pending_mail_idx ON t_caver (pending_mail);
  

• [test/customSQL.js] Per project conventions, any index added in sql/ must also be added to the INDEX_OPTIMIZATION_MIGRATION block in test/customSQL.js so the test database has the same indexes. The t_caver_pending_mail_idx index is missing here.

• [api/controllers/v1/auth/verify-email.js:23-26] The conflict check at verification time only looks at { mail: caver.pendingMail } without excluding the current user and without checking other users' pendingMail. This is inconsistent with the more thorough check in update.js. Two specific problems:

  1. If the user's own mail happens to equal their pendingMail (shouldn't happen normally, but defensive coding), the query would find the user themselves and incorrectly return 409.
  2. If another user has the same value as their pendingMail, the verification would succeed, creating a state where two users have the same email (one as mail, one as pendingMail).

  Consider aligning with the pattern in update.js:

  const alreadyInUse = await TCaver.findOne({
    id: { '!=': caver.id },
    or: [{ mail: caver.pendingMail }, { pendingMail: caver.pendingMail }],
  });
  

• [api/controllers/v1/account/update.js:87-88] The controller fetches caver a second time at line 87 (const caver = await TCaver.findOne({ id: req.token.id })) even though currentCaver was already fetched at line 43 for the email flow. When the email field is present, this results in two identical DB queries. Consider reusing currentCaver or restructuring so the fetch only happens once. More importantly, if the email field is NOT present in the request body, currentCaver is never fetched, but caver is — so the two fetches serve different code paths. This is fine functionally but the naming (currentCaver vs caver) is confusing since they hold the same data.

Suggestions (Should Consider)

• [api/controllers/v1/auth/verify-email.js:53-58] The notifyEmailChanged call is fire-and-forget but has no .catch() handler. If notifyEmailChanged rejects (despite the service's documentation saying it never throws), the unhandled rejection would crash the process in Node.js. Consider adding .catch() for safety:

  if (caver.pendingMail) {
    AccountNotificationService.notifyEmailChanged({
      oldEmail: caver.mail,
      nickname: caver.nickname,
      languageId: caver.language,
    }).catch((err) => {
      sails.log.error('Failed to send email change notification:', err);
    });
    return res.ok({ message: 'Email successfully changed.' });
  }
  

• [test/integration/4_routes/Account/update-notifications.test.js:33-39] The afterEach restores mail and password but doesn't restore pendingMail, activationCode, or mailIsValid. Since the tests now set these fields (via the PATCH + verify-email flow), stale state could leak between tests if a test fails mid-way. Consider:

  afterEach(async () => {
    notifyEmailChangedStub.restore();
    notifyPasswordChangedStub.restore();
    await TCaver.updateOne({ id: 3 }).set({
      mail: originalEmail,
      password: originalPasswordHash,
      pendingMail: null,
      activationCode: null,
      mailIsValid: true,
    });
  });
  

• [assets/swaggerV1.yaml] The PATCH /account endpoint now returns 200 with a message body when cancelling a pending email change, but the Swagger spec only documents 204 as the success response. Consider adding a 200 response to document this new behavior, or changing the cancellation response to 204 for consistency.

• [api/controllers/v1/auth/verify-email.js:44-48] There's a TOCTOU (time-of-check-time-of-use) race condition between the conflict check and the update. If two users verify at the exact same time targeting the same email, both could pass the check. The DB unique constraint on mail protects the final commit, but the resulting error would bubble up as a 500 rather than a clean 409. Consider wrapping the update in error handling that catches PostgreSQL unique constraint violations (error code 23505) and returns res.conflict() gracefully.

Nitpicks (Optional)

• [assets/swaggerV1.yaml:3244-3245] There are two consecutive blank lines after the '409' response definition. Minor formatting issue.

• [test/integration/4_routes/Account/change-email.test.js:155] The test hardcodes 'testtest' as the fixture password. Consider using AuthTokenService.TEST_PASSWORD (which is already imported and used in update-notifications.test.js) for consistency and resilience to fixture changes.