feature(reset password)#664
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesForgot Password Flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)app/src/screens/AuthScreen.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.58][ERROR]: unable to find a config; path 🔧 ESLint
app/src/screens/AuthScreen.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@NancyWei123 match the theme and design style for reset password |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/screens/AuthScreen.js (1)
304-343: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd focused tests for forgot-password branches.
Please add AuthScreen tests for: invalid email (local validation), successful reset request, and rejected reset request; assert loading/disabled UI state and message path behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/screens/AuthScreen.js` around lines 304 - 343, Add unit tests for the AuthScreen component focused on the handleForgotPassword function and showMessage helper. Create test cases for three scenarios: invalid email validation (verify setEmailError is called and showMessage displays the validation error), successful password reset request (verify loading state transitions, sendPasswordResetEmail is called with the trimmed email, and the success message is shown), and failed password reset request (verify the error is caught, getFirebaseErrorMessage is called to format the error, setEmailError updates the state with the error message, and showMessage displays the error). For each scenario, assert that the loading state (setLoading) is properly managed and that the correct message path (either success or error message) is triggered through showMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/screens/AuthScreen.js`:
- Around line 332-338: The catch block in the forgot password error handler is
exposing Firebase backend error details via getFirebaseErrorMessage(error),
which can leak information about account existence to unauthenticated users.
Remove the call to getFirebaseErrorMessage(error) and instead use a generic,
non-revealing message. Replace the setEmailError and showMessage calls with a
single generic message (such as "check your email for password reset
instructions") that does not disclose whether the email exists or whether the
operation succeeded, following Firebase's security best practice for password
reset flows.
---
Nitpick comments:
In `@app/src/screens/AuthScreen.js`:
- Around line 304-343: Add unit tests for the AuthScreen component focused on
the handleForgotPassword function and showMessage helper. Create test cases for
three scenarios: invalid email validation (verify setEmailError is called and
showMessage displays the validation error), successful password reset request
(verify loading state transitions, sendPasswordResetEmail is called with the
trimmed email, and the success message is shown), and failed password reset
request (verify the error is caught, getFirebaseErrorMessage is called to format
the error, setEmailError updates the state with the error message, and
showMessage displays the error). For each scenario, assert that the loading
state (setLoading) is properly managed and that the correct message path (either
success or error message) is triggered through showMessage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13cafc27-5bd0-4d67-b97c-8504cf63a202
📒 Files selected for processing (1)
app/src/screens/AuthScreen.js
| } catch (error) { | ||
| console.log('Forgot password error:', error); | ||
|
|
||
| const msg = getFirebaseErrorMessage(error); | ||
| setEmailError(msg); | ||
|
|
||
| showMessage('Cannot send reset email', msg); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Firebase Auth JavaScript SDK v11, what errors can sendPasswordResetEmail(auth, email) return, and how does Email Enumeration Protection affect whether user existence is disclosed?
💡 Result:
In Firebase Auth JavaScript SDK v11, sendPasswordResetEmail(auth, email) can throw these errors related to the provided email: 1) auth/invalid-email — the email address is not valid. [1] 2) auth/user-not-found — thrown if there is no user corresponding to the email address. [1] 3) auth/too-many-requests — thrown if requests are blocked from a device due to unusual activity (rate limiting). [2] How Email Enumeration Protection changes user existence disclosure By default (without enumeration protection), calling sendPasswordResetEmail for an unregistered email can result in auth/user-not-found, which discloses that the email address does not exist. [1] If you enable Email enumeration protection for your project, Firebase changes error reporting for password reset flows: it “removes error responses” for password reset calls (including the client SDK method sendPasswordResetEmail). In that mode, the request results in no specific error that indicates the email is unregistered (Firebase sends a password reset email only if the email exists, but there are “no specific error messages indicating when emails aren't sent”). [3] Practically, this means user existence is no longer disclosed via the thrown error (the SDK won’t reliably throw auth/user-not-found for non-existent emails); instead, your app should always show a generic “check your email” success message regardless of whether the email exists. [4][3]
Citations:
- 1: https://firebase.google.cn/docs/reference/js/v8/firebase.auth.Auth
- 2: https://firebase.google.com/docs/reference/js/v8/firebase.auth.Error
- 3: https://cloud.google.com/identity-platform/docs/admin/email-enumeration-protection
- 4: https://firebase.google.com/docs/auth/web/password-auth
🏁 Script executed:
#!/bin/bash
# Find getFirebaseErrorMessage definition to see if it returns user-facing error messages
rg "getFirebaseErrorMessage" --max-count 5 -B 2 -A 8Repository: roshankumar0036singh/Uni-Event
Length of output: 1982
🏁 Script executed:
#!/bin/bash
# Verify the exact code at the catch block in AuthScreen.js
rg "Forgot password error" -B 3 -A 6Repository: roshankumar0036singh/Uni-Event
Length of output: 734
Do not expose backend error details in password-reset error handling; use a generic message instead.
The catch block at lines 332–338 surfaces Firebase backend error details to unauthenticated users via getFirebaseErrorMessage(error). When Email Enumeration Protection is disabled on your Firebase project, errors like auth/user-not-found leak information about account existence. Even with enumeration protection enabled, Firebase's official best practice is to always show a generic "check your email" message regardless of whether the email exists or the request succeeded.
Replace the error handling with a generic success message that does not disclose whether the email exists or the operation actually succeeded:
Recommended fix
} catch (error) {
- console.log('Forgot password error:', error);
-
- const msg = getFirebaseErrorMessage(error);
- setEmailError(msg);
-
- showMessage('Cannot send reset email', msg);
+ console.error('Forgot password request failed:', error?.code || error);
+ showMessage(
+ 'Check your email',
+ 'If an account exists for this email, you will receive a password reset link shortly.',
+ );
} finally {
setLoading(false);
}🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 335-335: Avoid using the initial state variable in setState
Context: setEmailError(msg)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/screens/AuthScreen.js` around lines 332 - 338, The catch block in the
forgot password error handler is exposing Firebase backend error details via
getFirebaseErrorMessage(error), which can leak information about account
existence to unauthenticated users. Remove the call to
getFirebaseErrorMessage(error) and instead use a generic, non-revealing message.
Replace the setEmailError and showMessage calls with a single generic message
(such as "check your email for password reset instructions") that does not
disclose whether the email exists or whether the operation succeeded, following
Firebase's security best practice for password reset flows.
|
resolev the issue |
I have fixed the problem
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/screens/AuthScreen.js (1)
727-732: 📐 Maintainability & Code Quality | 🔵 TrivialConsider sourcing the success color from the theme for consistency.
color: 'green'is hardcoded, while the rest of the form drives colors fromtheme.colors.*. The theme palette exposescolors.success: '#00C853'for semantic consistency across light and dark modes, matching the pattern used elsewhere in the form.♻️ Suggested approach
- {successMessage ? ( - <Text style={styles.successText}>{successMessage}</Text> - ) : null} + {successMessage ? ( + <Text style={[styles.successText, { color: theme.colors.success }]}> + {successMessage} + </Text> + ) : null}successText: { - color: 'green', fontSize: 12, marginTop: -8, marginLeft: 4, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/screens/AuthScreen.js` around lines 727 - 732, The successText style object contains a hardcoded color value of 'green' which is inconsistent with the pattern used elsewhere in the form where colors are sourced from the theme object. Replace the hardcoded color: 'green' with a reference to the theme colors palette using theme.colors.success to ensure semantic consistency across light and dark modes and maintain the established theming pattern throughout the form.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/screens/AuthScreen.js`:
- Around line 727-732: The successText style object contains a hardcoded color
value of 'green' which is inconsistent with the pattern used elsewhere in the
form where colors are sourced from the theme object. Replace the hardcoded
color: 'green' with a reference to the theme colors palette using
theme.colors.success to ensure semantic consistency across light and dark modes
and maintain the established theming pattern throughout the form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c8cafc1b-7753-4f95-98a4-48b40c9898a8
📒 Files selected for processing (1)
app/src/screens/AuthScreen.js
|
2 participants
Description
Add forget password feature.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
Summary by CodeRabbit