#AMM-1247- feat: VAPT Captcha implementation #35
Conversation
…lity added, env injection for captcha script and sitekey; login form validation
WalkthroughA CAPTCHA challenge has been integrated into the login flow. This includes a new CAPTCHA component and service, updates to the login component and service to handle the CAPTCHA token, environment variable additions for CAPTCHA configuration, and template and style changes to support and enforce CAPTCHA validation during authentication. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginComponent
participant CaptchaComponent
participant CaptchaService
participant LoginService
participant Backend
User->>LoginComponent: Enter username & password
LoginComponent->>CaptchaComponent: Render CAPTCHA
CaptchaComponent->>CaptchaService: Load CAPTCHA script
CaptchaService-->>CaptchaComponent: Script loaded
User->>CaptchaComponent: Solve CAPTCHA
CaptchaComponent->>LoginComponent: Emit tokenResolved(token)
LoginComponent->>LoginService: authenticateUser(username, password, doLogout, captchaToken)
LoginService->>Backend: POST /user/userAuthenticate {username, password, doLogout, captchaToken}
Backend-->>LoginService: Authentication response
LoginService-->>LoginComponent: Response
LoginComponent->>CaptchaComponent: reset() (on success or error)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Caution No docstrings were generated. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
src/app/login/login.component.ts (1)
207-223: 🛠️ Refactor suggestionAdd CAPTCHA token validation before authentication.
The login method should validate that a CAPTCHA token exists before attempting authentication for security compliance.
login(doLogOut) { + if (!this.captchaToken) { + this.alertMessage.alert('Please complete the CAPTCHA verification', 'error'); + return; + } this.encryptpassword = this.encrypt(this.Key_IV, this.password); this.loginservice .authenticateUser(this.userID, this.encryptpassword, doLogOut, this.captchaToken) .subscribe( (response: any) => { console.error("response",response); if ( response !== undefined && response !== null && response.previlegeObj !== undefined && response.previlegeObj !== null ) { this.successCallback(response); } }, (error: any) => this.errorCallback(error) ); - this.resetCaptcha(); }
🧹 Nitpick comments (8)
src/environments/environment.ci.ts.template (1)
47-48: Fix spacing consistency in environment object.There's a missing space after the colon in the
siteKeyproperty assignment, which is inconsistent with the formatting style used elsewhere in the file.Apply this diff to maintain consistent formatting:
- siteKey:siteKey, + siteKey: siteKey,src/environments/environment.local.ts (1)
52-53: Fix spacing consistency and consider local development implications.There's a spacing inconsistency in the property definitions. Also, consider that empty CAPTCHA configuration values may cause the CAPTCHA feature to not function in local development.
Apply this diff to maintain consistent formatting:
- siteKey:siteKey, - captchaChallengeURL:captchaChallengeURL + siteKey: siteKey, + captchaChallengeURL: captchaChallengeURLConsider documenting in comments or README how to configure CAPTCHA for local development testing.
scripts/ci-prebuild.js (1)
50-51:Details
❓ Verification inconclusive
Fix formatting and verify secure CAPTCHA configuration in deployment.
The CAPTCHA environment variables are correctly added to support the template rendering process. However, there's a spacing inconsistency, and it's important to ensure proper CAPTCHA configuration in production deployments.
Apply this diff for consistent formatting:
- SITE_KEY:'', - CAPTCHA_CHALLENGE_URL:'' + SITE_KEY: '', + CAPTCHA_CHALLENGE_URL: ''Please verify that:
- The CAPTCHA service (likely Cloudflare Turnstile) is properly configured in production
- The SITE_KEY and CAPTCHA_CHALLENGE_URL are securely managed and not exposed in logs
- Fallback behavior is implemented when CAPTCHA service is unavailable
🏁 Script executed:
#!/bin/bash # Description: Check for CAPTCHA-related security considerations in the codebase # Search for any hardcoded CAPTCHA keys or URLs that might be a security risk echo "Searching for potential hardcoded CAPTCHA credentials..." rg -i "turnstile|captcha" --type ts --type js -A 3 -B 3 # Check for proper error handling in CAPTCHA implementation echo "Checking for CAPTCHA error handling..." ast-grep --pattern 'catch ($err) { $$$ }' | head -10Length of output: 13940
Apply consistent formatting and confirm secure CAPTCHA deployment
Formatting of the new environment variables should be updated for consistency, and please verify that CAPTCHA keys are securely managed in your CI/CD and production environments.
• scripts/ci-prebuild.js
- SITE_KEY:'', - CAPTCHA_CHALLENGE_URL:'' + SITE_KEY: '', + CAPTCHA_CHALLENGE_URL: ''• Verify that:
- No CAPTCHA keys or URLs are hardcoded (CI prebuild defaults to empty strings).
- SITE_KEY and CAPTCHA_CHALLENGE_URL are injected from a secure secrets store in CI/CD and never logged.
- The application gracefully handles CAPTCHA service failures (fallback path or user messaging).
src/app/services/loginService/login.service.ts (1)
60-60: Add spacing around the captchaToken parameter.While the CAPTCHA token integration is correct, improve readability by adding proper spacing.
Apply this formatting improvement:
- public authenticateUser(uname, pwd, doLogout,captchaToken) { + public authenticateUser(uname, pwd, doLogout, captchaToken) {src/app/captcha/captcha.component.ts (1)
49-53: Add defensive check for turnstile.remove method.The cleanup logic assumes
turnstile.removeexists but checks for it. Consider more robust error handling.ngOnDestroy() { - if (this.widgetId && typeof turnstile !== 'undefined' && turnstile.remove) { - turnstile.remove(this.widgetId); + if (this.widgetId && typeof turnstile !== 'undefined') { + try { + if (typeof turnstile.remove === 'function') { + turnstile.remove(this.widgetId); + } + } catch (error) { + console.warn('Failed to remove CAPTCHA widget:', error); + } } + this.widgetId = null; }src/app/services/captcha-service/captcha.service.ts (1)
4-4: Consider using providedIn for better tree-shaking.The service should use
providedIn: 'root'for better Angular dependency injection and tree-shaking optimization.-@Injectable() +@Injectable({ + providedIn: 'root' +})src/app/login/login.component.ts (2)
57-57: Initialize captchaToken property.The property should be initialized to prevent undefined access and improve type safety.
-captchaToken: string; +captchaToken: string = '';
383-385: Add input validation for CAPTCHA token.The token handler should validate the token format for additional security.
onCaptchaResolved(token: string) { + if (!token || typeof token !== 'string' || token.trim().length === 0) { + console.warn('Invalid CAPTCHA token received'); + return; + } this.captchaToken = token; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
scripts/ci-prebuild.js(1 hunks)src/app/app.module.ts(3 hunks)src/app/captcha/captcha.component.html(1 hunks)src/app/captcha/captcha.component.ts(1 hunks)src/app/login/login.component.css(1 hunks)src/app/login/login.component.ts(9 hunks)src/app/login/login.html(2 hunks)src/app/services/captcha-service/captcha.service.ts(1 hunks)src/app/services/loginService/login.service.ts(1 hunks)src/environments/environment.ci.ts.template(2 hunks)src/environments/environment.local.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/app/captcha/captcha.component.ts (2)
src/app/login/login.component.ts (1)
Component(36-416)src/environments/environment.local.ts (1)
environment(41-54)
src/app/services/captcha-service/captcha.service.ts (1)
src/environments/environment.local.ts (1)
environment(41-54)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build
- GitHub Check: package
🔇 Additional comments (13)
src/environments/environment.ci.ts.template (1)
33-34: LGTM! Proper template variable setup.The CAPTCHA configuration constants are correctly implemented using EJS template placeholders, following the established pattern in this environment template file.
src/app/captcha/captcha.component.html (1)
1-1: LGTM! Clean CAPTCHA container implementation.The template correctly provides a container for the Cloudflare Turnstile CAPTCHA widget with appropriate ID and template reference. The implementation is minimal and focused, which is ideal for a CAPTCHA component.
src/environments/environment.local.ts (1)
38-39: LGTM! Appropriate local development configuration.The CAPTCHA configuration constants are properly initialized with empty strings for local development, which is a reasonable default for development environments where CAPTCHA might not be required.
src/app/login/login.html (4)
16-16: Good addition of form validation.The
requiredattribute enforces mandatory username input, improving form validation.
26-26: Good addition of form validation.The
requiredattribute enforces mandatory password input, completing the basic form validation requirements.
31-34: Excellent accessibility implementation for CAPTCHA.The CAPTCHA integration includes proper accessibility features:
- ARIA
role="group"andaria-labelledbyfor semantic grouping- Screen reader-only label with
sr-onlyclass- Centered styling for visual alignment
This implementation follows WCAG guidelines for accessibility compliance.
37-37: Secure implementation of conditional login button.The login button is properly disabled until both CAPTCHA token is present and form is valid, preventing submission without completing security verification. The
disabledLoginButtonclass provides visual feedback for the disabled state.src/app/app.module.ts (3)
285-286: Properly organized imports for CAPTCHA functionality.The CAPTCHA service and component imports are correctly placed and follow the existing import organization pattern in the module.
323-323: Correct component declaration.The
CaptchaComponentis properly added to the declarations array, enabling its use throughout the application.
494-494: Proper service registration.The
CaptchaServiceis correctly added to the providers array, making it available for dependency injection throughout the application.src/app/login/login.component.css (2)
95-106: Excellent accessibility implementation with sr-only class.The
.sr-onlyclass follows the standard pattern for screen reader-only content, using proper CSS properties to hide content visually while maintaining accessibility. This is essential for the CAPTCHA label accessibility.
109-114: Good visual feedback for disabled login button.The
.disabledLoginButtonclass provides clear visual indication of the disabled state with:
- Muted gray background and text colors
not-allowedcursor for user feedback- Removed box shadow for flat appearance
!importantdeclarations ensure style precedenceThis enhances user experience by clearly indicating when the button cannot be clicked.
src/app/services/loginService/login.service.ts (1)
67-67: Secure integration of CAPTCHA token in authentication payload.The CAPTCHA token is properly included in the authentication request payload, enabling server-side verification of the CAPTCHA challenge. This strengthens the security of the login process as required for VAPT compliance.
| import { CaptchaService } from '../services/captcha-service/captcha.service'; | ||
| import { environment } from 'environments/environment'; | ||
|
|
||
| declare const turnstile: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper type definitions for turnstile.
The global turnstile declaration lacks type safety. Consider creating proper TypeScript interfaces or installing official types if available.
-declare const turnstile: any;
+interface TurnstileWidget {
+ render(element: HTMLElement, options: {
+ sitekey: string;
+ theme?: string;
+ callback?: (token: string) => void;
+ }): string;
+ reset(widgetId: string): void;
+ remove?(widgetId: string): void;
+}
+
+declare const turnstile: TurnstileWidget;🤖 Prompt for AI Agents
In src/app/captcha/captcha.component.ts at line 5, replace the generic 'any'
type declaration for the global 'turnstile' variable with a proper TypeScript
interface or type definition. If official type definitions for 'turnstile'
exist, install and use them; otherwise, define an interface that accurately
describes the properties and methods used from 'turnstile' to ensure type safety
and improve code maintainability.
| loadScript(): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| if (this.scriptLoaded) return resolve(); | ||
|
|
||
| const script = document.createElement('script'); | ||
| script.src = environment.captchaChallengeURL; | ||
| script.async = true; | ||
| script.defer = true; | ||
| script.onload = () => { | ||
| this.scriptLoaded = true; | ||
| resolve(); | ||
| }; | ||
| script.onerror = (event) => { | ||
| this.scriptLoaded = false; | ||
| reject( | ||
| new Error( | ||
| `Failed to load CAPTCHA script from ${environment.captchaChallengeURL}: ${event}`, | ||
| ), | ||
| ); | ||
| }; | ||
| document.head.appendChild(script); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add race condition protection and environment validation.
The current implementation has potential race conditions and missing validation that could cause issues in production.
+private loadingPromise: Promise<void> | null = null;
loadScript(): Promise<void> {
+ // Return existing promise if script is already being loaded
+ if (this.loadingPromise) {
+ return this.loadingPromise;
+ }
+
+ if (this.scriptLoaded) return Promise.resolve();
+
+ if (!environment.captchaChallengeURL) {
+ return Promise.reject(new Error('CAPTCHA challenge URL is not configured'));
+ }
+
+ this.loadingPromise = new Promise((resolve, reject) => {
- return new Promise((resolve, reject) => {
- if (this.scriptLoaded) return resolve();
const script = document.createElement('script');
script.src = environment.captchaChallengeURL;
script.async = true;
script.defer = true;
script.onload = () => {
this.scriptLoaded = true;
+ this.loadingPromise = null;
resolve();
};
script.onerror = (event) => {
this.scriptLoaded = false;
+ this.loadingPromise = null;
reject(
new Error(
`Failed to load CAPTCHA script from ${environment.captchaChallengeURL}: ${event}`,
),
);
};
document.head.appendChild(script);
});
+
+ return this.loadingPromise;
}🤖 Prompt for AI Agents
In src/app/services/captcha-service/captcha.service.ts around lines 8 to 30, the
loadScript method lacks protection against race conditions and does not validate
the environment variable. To fix this, add a mechanism to queue or block
multiple simultaneous calls to loadScript until the script is fully loaded,
preventing concurrent script insertions. Also, validate that
environment.captchaChallengeURL is defined and a valid URL before attempting to
load the script, and handle the case where it is missing or invalid by rejecting
the promise early with a clear error.
| resetCaptcha() { | ||
| if (this.captchaCmp && typeof this.captchaCmp.reset === 'function') { | ||
| this.captchaCmp.reset(); | ||
| this.captchaToken = ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve CAPTCHA reset method with proper error handling.
The reset method should include better error handling and null checking.
resetCaptcha() {
- if (this.captchaCmp && typeof this.captchaCmp.reset === 'function') {
+ try {
+ if (this.captchaCmp && typeof this.captchaCmp.reset === 'function') {
this.captchaCmp.reset();
- this.captchaToken = '';
+ }
+ } catch (error) {
+ console.warn('Failed to reset CAPTCHA:', error);
+ } finally {
+ this.captchaToken = '';
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resetCaptcha() { | |
| if (this.captchaCmp && typeof this.captchaCmp.reset === 'function') { | |
| this.captchaCmp.reset(); | |
| this.captchaToken = ''; | |
| } | |
| } | |
| resetCaptcha() { | |
| try { | |
| if (this.captchaCmp && typeof this.captchaCmp.reset === 'function') { | |
| this.captchaCmp.reset(); | |
| } | |
| } catch (error) { | |
| console.warn('Failed to reset CAPTCHA:', error); | |
| } finally { | |
| this.captchaToken = ''; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/app/login/login.component.ts around lines 387 to 392, the resetCaptcha
method lacks robust error handling and null checks. Enhance the method by adding
explicit null or undefined checks for this.captchaCmp before calling reset, and
wrap the reset call in a try-catch block to handle any potential errors
gracefully. Also ensure that this.captchaToken is reset only if the reset call
succeeds without exceptions.
📋 Description
JIRA ID: AMM-1247
This feature is part of VAPT and includes captcha implementation, includes validators for login form.
✅ Type of Change
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores