Conversation
There was a problem hiding this comment.
Pull request overview
Adds Cognito signup handling to reduce auth failures when users retry signup while still UNCONFIRMED, and switches Cognito email verification from code-based to link-based.
Changes:
- Configured Cognito User Pool to send link-based verification emails.
- Added a PreSignUp Lambda trigger that deletes existing UNCONFIRMED users matching the signup email.
- Provisioned IAM role + scoped Cognito permissions for the new PreSignUp Lambda.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cdk_backend/lib/cdk_backend-stack.ts |
Updates User Pool verification email settings; adds PreSignUp Lambda + IAM role/policy and attaches as PRE_SIGN_UP trigger. |
cdk_backend/lambda/preSignUp/index.py |
Implements PreSignUp handler to find and delete UNCONFIRMED users by email before allowing signup to proceed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,27 @@ | |||
| import boto3 | |||
| import os | |||
There was a problem hiding this comment.
os is imported but never used. Remove the unused import to avoid lint noise and keep the Lambda handler minimal.
| import os |
| # Check if an UNCONFIRMED user with this email already exists | ||
| response = client.list_users( | ||
| UserPoolId=user_pool_id, | ||
| Filter=f'email = "{email}"', | ||
| ) | ||
|
|
||
| for user in response.get('Users', []): | ||
| if user['UserStatus'] == 'UNCONFIRMED': | ||
| client.admin_delete_user( | ||
| UserPoolId=user_pool_id, | ||
| Username=user['Username'], | ||
| ) |
There was a problem hiding this comment.
The list_users / admin_delete_user calls are not wrapped in error handling. Any ClientError/throttling/network hiccup will raise and block the Cognito PreSignUp flow (signup fails). Catch exceptions, log context (userPoolId/email), and decide whether to proceed (return event) or fail with a clear error intentionally.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| response = client.list_users( | ||
| UserPoolId=user_pool_id, | ||
| Filter=f'email = "{email}"', | ||
| ) |
There was a problem hiding this comment.
Filter=f'email = "{email}"' interpolates unescaped user input into the Cognito filter expression. If the email contains a double-quote (valid in some email forms / can be provided by a malicious client), this will produce an invalid filter or change the query. Escape/quote the value before building the filter (e.g., safely JSON-quote the string or replace "), or validate the email to a safe subset first.
| response = client.list_users( | ||
| UserPoolId=user_pool_id, | ||
| Filter=f'email = "{email}"', | ||
| ) | ||
|
|
||
| for user in response.get('Users', []): | ||
| if user['UserStatus'] == 'UNCONFIRMED': | ||
| client.admin_delete_user( | ||
| UserPoolId=user_pool_id, | ||
| Username=user['Username'], | ||
| ) | ||
| print(f"Deleted UNCONFIRMED user: {user['Username']}") | ||
|
|
There was a problem hiding this comment.
list_users can paginate via PaginationToken/NextToken. This code only inspects the first page, so it could miss matches if multiple users share the same email (e.g., from historical data/imports) and the result spans pages. Consider looping until the pagination token is exhausted before deciding no UNCONFIRMED users exist.
| response = client.list_users( | |
| UserPoolId=user_pool_id, | |
| Filter=f'email = "{email}"', | |
| ) | |
| for user in response.get('Users', []): | |
| if user['UserStatus'] == 'UNCONFIRMED': | |
| client.admin_delete_user( | |
| UserPoolId=user_pool_id, | |
| Username=user['Username'], | |
| ) | |
| print(f"Deleted UNCONFIRMED user: {user['Username']}") | |
| pagination_token = None | |
| while True: | |
| list_users_kwargs = { | |
| 'UserPoolId': user_pool_id, | |
| 'Filter': f'email = "{email}"', | |
| } | |
| if pagination_token: | |
| list_users_kwargs['PaginationToken'] = pagination_token | |
| response = client.list_users(**list_users_kwargs) | |
| for user in response.get('Users', []): | |
| if user['UserStatus'] == 'UNCONFIRMED': | |
| client.admin_delete_user( | |
| UserPoolId=user_pool_id, | |
| Username=user['Username'], | |
| ) | |
| print(f"Deleted UNCONFIRMED user: {user['Username']}") | |
| pagination_token = response.get('PaginationToken') | |
| if not pagination_token: | |
| break |
| for user in response.get('Users', []): | ||
| if user['UserStatus'] == 'UNCONFIRMED': | ||
| client.admin_delete_user( | ||
| UserPoolId=user_pool_id, | ||
| Username=user['Username'], | ||
| ) | ||
| print(f"Deleted UNCONFIRMED user: {user['Username']}") |
There was a problem hiding this comment.
Deleting any existing UNCONFIRMED user on sign-up enables a low-effort denial-of-service: an attacker can repeatedly attempt sign-up with someone else’s email to keep deleting that person’s unconfirmed account before they can complete verification. Consider adding guardrails (e.g., only delete if the existing UNCONFIRMED account is older than a short threshold, or only delete if it was created via the same client/app) and make sure this behavior is acceptable from a security/product standpoint.
| except Exception as error: | ||
| print(f'Error in pre-signup trigger: {error}') | ||
|
|
There was a problem hiding this comment.
The broad except Exception logs and then continues as if the trigger succeeded. If deletion/listing fails (throttling/permissions), Cognito will proceed and the original sign-up may still fail due to the existing user, making the root cause harder to diagnose. Consider logging a full stack trace and failing the trigger (raise) when the cleanup is required for sign-up to succeed.
Added PreSignUp lambda to handle unconfirmed users who are trying to signup again.
Changed email verification to link based as opposed to confirmation code based.