Conversation
…n of app in email
📝 WalkthroughWalkthroughAdds multi-user email verification: DB schema/migrations and rate-limits; verification code lifecycle with constant-time checks; SMTP-backed EmailService with test-mode; GraphQL mutations and runtime clients; step-driven Connect UI (email → password → code); Tauri SMTP settings with encrypted password storage; CI job and E2E tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Connect UI
participant Client as Ad4mClient
participant Server as GraphQL Server
participant DB as Database
participant Email as Email Service
Note over UI,Client: Step-driven flow (email → password → code)
User->>UI: submit email (signup/login)
UI->>Client: requestLoginVerification(email, appInfo?)
Client->>Server: runtimeRequestLoginVerification
Server->>DB: check user exists / create_verification_code
Server->>Email: send_verification_email(email, code, appInfo)
Email-->>Server: sent or captured (test-mode)
Server-->>Client: {success, requiresPassword}
User->>UI: submit code
UI->>Client: verifyEmailCode(email,code,verificationType)
Client->>Server: runtimeVerifyEmailCode
Server->>DB: verify_code(email,code)
DB-->>Server: valid -> cleanup
Server->>Server: issue JWT (with appInfo capabilities)
Server-->>Client: JWT
Client-->>UI: auth success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 4
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| this._multiUserVerificationCode = ""; | ||
| this._multiUserStep = "email"; | ||
| this._isOpen = false; | ||
| this.changeUIState("connected"); |
There was a problem hiding this comment.
Invalid UI state "connected" not in type definition
The new handleMultiUserCodeSubmit method calls this.changeUIState("connected"), but "connected" is not a valid value in the uiState union type (which only includes "connection_overview", "remote_connection", "settings", "start", "qr", "requestcap", "verifycode", "disconnected", "hosting", "agentlocked", and "multiuser_auth"). This could cause TypeScript compilation issues or undefined rendering behavior if the modal is reopened while in this invalid state.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Fix all issues with AI Agents 🤖
In @rust-executor/Cargo.toml:
- Line 117: Update the lettre dependency in Cargo.toml from the loose "0.11"
constraint to the specific patched release "0.11.19" (change the lettre entry so
it reads lettre = "0.11.19"), then run cargo update and audit (e.g., cargo audit
or check deps.rs/advisories) to confirm no remaining vulnerable transitive
dependencies and adjust other dependency versions if needed.
In @rust-executor/src/config.rs:
- Around line 12-16: set_smtp_config currently ignores failures from
SMTP_CONFIG.lock(), causing silent drops of updates when the mutex is poisoned
or cannot be acquired; change set_smtp_config to handle the lock error
explicitly by matching the result of SMTP_CONFIG.lock() and either propagating a
Result or logging/returning the error, e.g., return a Result from
set_smtp_config or call expect/unwrap_with_message only if appropriate, and
reference the SMTP_CONFIG.lock() call inside set_smtp_config to locate where to
add the error handling and proper propagation or logging.
In @rust-executor/src/email_service.rs:
- Around line 393-400: The code currently configures port 25 with Tls::None
(SmtpTransport::builder_dangerous(...).tls(Tls::None)), which sends credentials
in plaintext; update the handling in the email service to avoid silent insecure
behavior by either switching to Tls::Opportunistic for port 25 (so STARTTLS is
attempted when available) or, if Tls::None must be kept for internal relays,
emit a clear warning/log when self.smtp_config.port == 25 (use the same logger
used elsewhere in this module) to surface the security risk; ensure changes are
applied where SmtpTransport::builder_dangerous and Tls::None are referenced so
the insecure default is no longer silent.
In @rust-executor/src/graphql/mod.rs:
- Around line 49-50: The call to
crate::config::set_smtp_config(config.smtp_config.clone()) currently ignores
Mutex lock failures; update this call to handle and surface errors: either
change set_smtp_config to return Result and propagate/handle the error here, or
wrap the current call with explicit error logging by checking the lock result
and emitting a warn/error (e.g., log the failure to acquire the lock or to set
config) so SMTP initialization failures are not silent; reference the
set_smtp_config symbol and follow the existing pattern used in
mutation_resolvers.rs (the .lock().ok() error-handling style) when implementing
the fix.
In @rust-executor/src/graphql/mutation_resolvers.rs:
- Around line 933-943: In runtime_verify_email_code, the fallback AuthInfo block
(the branch where context.auth_token.is_empty()) currently grants ALL_CAPABILITY
which gives unrestricted admin access; change this to assign a limited,
user-scoped capability instead (e.g., replace ALL_CAPABILITY with
RUNTIME_USER_MANAGEMENT_LOGIN_CAPABILITY.clone() or another appropriate
user-scoped capability), ensure capabilities is set to
Some(vec![RUNTIME_USER_MANAGEMENT_LOGIN_CAPABILITY.clone()]) and keep any
user-specific fields (like user_email) so the JWT is scoped to that user rather
than using the wildcard ALL_CAPABILITY.
In @tests/js/email-verification-test-with-setup.sh:
- Around line 37-59: The setup script lacks error handling and continues after
failed steps; update the top of tests/js/email-verification-test-with-setup.sh
to enable immediate exit on failures (e.g., add set -e and optionally set -o
pipefail) and/or add explicit exit checks after critical commands like
./scripts/build-test-language.sh, ./scripts/prepareTestDirectory.sh, deno run
--allow-all scripts/get-builtin-test-langs.js, pnpm run
inject-language-language, pnpm run publish-test-languages, and pnpm run
inject-publishing-agent so the script logs the failing command and exits
non-zero when any of these commands fail.
♻️ Duplicate comments (3)
connect/src/web.ts (2)
717-723: Partial success scenario not handled - user may wait for email that was never sent.When user creation succeeds but the verification email fails to send, the backend returns
success: truewith anerrorfield containing the failure message. The frontend only checksresult.successand immediately moves to the code step without displaying or checkingresult.error. The user will be waiting for an email that was never sent.This was flagged in a previous review. Consider displaying
result.erroreven whensuccessis true:🔎 Proposed fix
if (result.success) { + // Check for partial success - user created but email failed + if (result.error) { + this._multiUserError = result.error; + // Stay on password step so user can see the error and retry + return; + } // User created, verification email sent this._multiUserStep = "code"; this._multiUserVerificationType = "signup"; } else {
758-758: Invalid UI state "connected" not in type definition.The
changeUIState("connected")call uses a value not defined in theuiStateunion type (lines 530-541). While the modal closes immediately after viahandleAuthChange("authenticated"), this leaves an invalid state that could cause issues if the modal is reopened.This was flagged in a previous review. Consider using a valid state or removing this call since the modal closes anyway:
🔎 Proposed fix
this._isOpen = false; - this.changeUIState("connected"); + // Modal is closing, reset to initial state for next open + this.changeUIState("connection_overview"); this.handleAuthChange("authenticated");rust-executor/src/db.rs (1)
2447-2498: Critical: Non-constant-time hash comparison enables timing attacks.The past review comment is correct. Line 2485 uses standard string equality (
stored_hash == code_hash) which is NOT constant-time. String equality operators short-circuit on the first mismatched character, leaking timing information about how many leading characters match. An attacker could potentially brute-force verification codes by measuring response time differences, even though the codes are hashed.Use a constant-time comparison library like
subtle::ConstantTimeEqfor secure hash comparison.🔎 Proposed fix using constant-time comparison
Add the
subtlecrate dependency toCargo.toml:subtle = "2.5"Then update the comparison logic:
+use subtle::ConstantTimeEq; + // ... - // Constant-time comparison - if stored_hash == code_hash { + // Constant-time comparison to prevent timing attacks + if stored_hash.as_bytes().ct_eq(code_hash.as_bytes()).into() { // Mark as verified and delete self.conn.execute( "DELETE FROM email_verifications WHERE email = ?1 AND verification_type = ?2",
🧹 Nitpick comments (11)
tests/js/tests/email-verification.test.ts (1)
157-173: Code expiration test doesn't verify actual expiration.As noted in the comment, this test doesn't actually verify that codes expire after 15 minutes - it only confirms that fresh codes work. Consider adding a separate integration test that manipulates the database timestamp or system clock to verify expiration enforcement.
Alternative approach for testing expiration
You could add a test utility that directly manipulates the database to set an expired timestamp:
it("should reject expired codes", async () => { const email = "expired@example.com"; await adminAd4mClient!.agent.createUser(email, "password123"); const code = await adminAd4mClient!.runtime.emailTestGetCode(email); // Add a test utility to expire the code in the database await adminAd4mClient!.runtime.emailTestExpireCode(email); // Now verify it's rejected try { await adminAd4mClient!.agent.verifyEmailCode(email, code!, "signup"); expect.fail("Should have rejected expired code"); } catch (e: any) { expect(e.message).to.include("expired"); } });rust-executor/src/lib.rs (1)
121-131: Discrepancy: Cleanup interval is 5 minutes, not hourly.The PR summary states that expired verification codes are cleaned up "hourly," but the code runs cleanup every 300 seconds (5 minutes). This is a 12x difference in frequency.
Verify the intended cleanup interval:
- If 5 minutes is correct, update the PR description
- If hourly is correct, change
from_secs(300)tofrom_secs(3600)Optional: Consider panic protection for long-running task
If
cleanup_expired_codes()panics, the background task will terminate silently. Consider wrapping the cleanup call:tokio::spawn(async { loop { tokio::time::sleep(tokio::time::Duration::from_secs(300)).await; - if let Err(e) = Ad4mDb::with_global_instance(|db| db.cleanup_expired_codes()) { - error!("Failed to cleanup expired verification codes: {}", e); - } else { - info!("Cleaned up expired verification codes"); + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + Ad4mDb::with_global_instance(|db| db.cleanup_expired_codes()) + })) { + Ok(Ok(())) => info!("Cleaned up expired verification codes"), + Ok(Err(e)) => error!("Failed to cleanup expired verification codes: {}", e), + Err(e) => error!("Panic in cleanup task: {:?}", e), } } });rust-executor/src/config.rs (1)
25-33: Consider limiting exposure of sensitive SMTP credentials.The
SmtpConfigstruct exposes all fields (includingpassword) as public. While this may be intentional for serialization, consider whether accessor methods with validation would provide better encapsulation, especially for credentials.If stricter encapsulation is desired, consider:
- Making fields private
- Providing getter methods
- Adding validation in constructors (e.g., non-empty host, valid port ranges)
- Implementing
Debugthat redacts the password fieldconnect/src/core.ts (2)
190-197: Good deprecation notice, but consider migration guidance.The deprecation warning is helpful, but users of this method would benefit from more specific guidance on how to migrate to the new UI-driven flow.
Consider enhancing the warning:
- * @deprecated This method is deprecated in favor of the UI-driven email verification flow. - * The new flow uses requestLoginVerification() -> verifyEmailCode() which is handled - * automatically by the ad4m-connect web component. This method is kept for backwards - * compatibility but will only work if email verification is not enabled on the server. + * @deprecated This method is deprecated in favor of the UI-driven email verification flow. + * The new flow uses requestLoginVerification() -> verifyEmailCode() which is handled + * automatically by the ad4m-connect web component. + * + * Migration: Use the ad4m-connect web component which handles the verification flow automatically, + * or call client.agent.requestLoginVerification() and client.agent.verifyEmailCode() directly. + * + * This method is kept for backwards compatibility but will only work if email verification + * is not enabled on the server.
235-235: Error message could be more actionable.The error message references implementation details ("email verification") that may not be meaningful to users still relying on the deprecated flow. Consider providing clearer migration guidance.
- throw new Error(`Failed to create/login user: ${createError.message}. If email verification is enabled, please use the UI-driven flow instead.`); + throw new Error(`Failed to create/login user: ${createError.message}. The server requires email verification. Please migrate to the ad4m-connect web component or use requestLoginVerification() and verifyEmailCode() directly.`);tests/js/email-verification-test-with-setup.sh (1)
14-15: Sleep-based synchronization is fragile and increases test time.Using fixed
sleepdurations (2-3 seconds) for process cleanup is unreliable and adds unnecessary wait time. Processes might not be fully terminated after the sleep, or the sleep might be longer than needed.Consider implementing polling-based wait with timeout:
wait_for_ports_free() { local timeout=10 local elapsed=0 local ports=(15920 15921 15922) while [ $elapsed -lt $timeout ]; do local all_free=true for port in "${ports[@]}"; do if lsof -ti:$port >/dev/null 2>&1; then all_free=false break fi done if $all_free; then return 0 fi sleep 0.5 elapsed=$((elapsed + 1)) done echo "⚠️ Warning: Ports still in use after timeout" return 1 }Then call
wait_for_ports_freeinstead ofsleep.Also applies to: 65-65
ui/src-tauri/src/commands/app.rs (1)
292-356: Port-based TLS logic is sound, but consider adding a timeout.The SMTP test function correctly handles port-based TLS:
- Port 465: Implicit TLS (wrapper mode)
- Port 25: No TLS
- Other ports (587, 2525): STARTTLS
However, the SMTP connection has no explicit timeout configured. If the server is unresponsive, this could block the async runtime's blocking thread pool.
🔎 Consider adding a connection timeout
let mailer = if config.port == 465 { // Port 465 uses implicit TLS (wrapper mode) use lettre::transport::smtp::client::{Tls, TlsParameters}; let tls_params = TlsParameters::builder(config.host.clone()) .dangerous_accept_invalid_certs(false) .build() .map_err(|e| format!("Failed to create TLS parameters: {}", e))?; SmtpTransport::builder_dangerous(&config.host) .port(config.port) .credentials(creds) .tls(Tls::Wrapper(tls_params)) + .timeout(Some(std::time::Duration::from_secs(30))) .build() } else if config.port == 25 { // ...similar for other branches }connect/src/components/MultiUserAuth.ts (2)
98-111: Consider extracting duplicated error message styles.The
.error-messageCSS block is duplicated identically in all three step views. Consider moving it to the parent component's styles or extracting to a shared style constant.🔎 Proposed refactor
Extract the shared styles at the top of the file:
const errorMessageStyles = css` .error-message { background-color: rgba(255, 0, 0, 0.1); border: 1px solid rgba(255, 0, 0, 0.3); border-radius: 8px; padding: 15px; color: #ff6b6b; text-align: center; } .error-message p { margin: 0; } `;Then include it in each template or move to a parent stylesheet.
Also applies to: 178-191, 270-295
230-239: Auto-submit may fire after user navigates away or during loading.The
setTimeoutcallback will execute even if the user navigates away (viaonBackToEmail) or ifisLoadingbecomes true before the timeout fires. Consider adding a guard or using the loading state check inside the timeout.🔎 Proposed fix
@input=${(e: Event) => { const input = e.target as HTMLInputElement; // Only allow digits const cleaned = input.value.replace(/\D/g, ""); changeVerificationCode(cleaned); // Auto-submit when 6 digits entered if (cleaned.length === 6) { - setTimeout(() => onCodeSubmit(), 100); + setTimeout(() => { + // Guard against state changes during timeout + if (!isLoading) { + onCodeSubmit(); + } + }, 100); } }}rust-executor/src/email_service.rs (1)
8-16: Arc wrapper is redundant with lazy_static.The
Arcwrapper aroundMutexis unnecessary here sincelazy_staticalready provides a static reference with'staticlifetime. TheMutexalone is sufficient for thread-safe interior mutability.🔎 Proposed simplification
lazy_static! { /// Test mode: captures sent emails instead of actually sending them /// Map of email address -> verification code - pub static ref TEST_MODE_EMAILS: Arc<Mutex<std::collections::HashMap<String, String>>> = - Arc::new(Mutex::new(std::collections::HashMap::new())); + pub static ref TEST_MODE_EMAILS: Mutex<std::collections::HashMap<String, String>> = + Mutex::new(std::collections::HashMap::new()); /// Flag to enable test mode (set via environment variable or config) - pub static ref EMAIL_TEST_MODE: Arc<Mutex<bool>> = Arc::new(Mutex::new(false)); + pub static ref EMAIL_TEST_MODE: Mutex<bool> = Mutex::new(false); }rust-executor/src/db.rs (1)
2410-2444: Consider using UPSERT instead of DELETE-then-INSERT.The verification code creation logic is sound (secure random generation, SHA-256 hashing, appropriate expiry). However, the DELETE-then-INSERT pattern (lines 2431-2441) could be replaced with a single UPSERT statement for atomicity and efficiency.
🔎 Proposed refactor using UPSERT
- // Delete any existing verification for this email+type combination - self.conn.execute( - "DELETE FROM email_verifications WHERE email = ?1 AND verification_type = ?2", - params![email, verification_type], - )?; - - // Insert new verification self.conn.execute( "INSERT INTO email_verifications (email, code_hash, created_at, expires_at, verified, verification_type) - VALUES (?1, ?2, ?3, ?4, 0, ?5)", + VALUES (?1, ?2, ?3, ?4, 0, ?5) + ON CONFLICT(email, verification_type) DO UPDATE SET + code_hash = excluded.code_hash, + created_at = excluded.created_at, + expires_at = excluded.expires_at, + verified = 0", params![email, code_hash, now, expires_at, verification_type], )?;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.circleci/config.ymlcli/src/ad4m_executor.rscli/src/dev.rsconnect/src/components/MultiUserAuth.tsconnect/src/core.tsconnect/src/web.tscore/src/agent/AgentClient.tscore/src/runtime/RuntimeClient.tscore/src/runtime/RuntimeResolver.tsrust-executor/Cargo.tomlrust-executor/src/agent/capabilities/defs.rsrust-executor/src/agent/capabilities/mod.rsrust-executor/src/config.rsrust-executor/src/db.rsrust-executor/src/email_service.rsrust-executor/src/graphql/graphql_types.rsrust-executor/src/graphql/mod.rsrust-executor/src/graphql/mutation_resolvers.rsrust-executor/src/lib.rstests/js/email-verification-test-with-setup.shtests/js/package.jsontests/js/tests/email-verification.test.tsui/src-tauri/Cargo.tomlui/src-tauri/src/app_state.rsui/src-tauri/src/commands/app.rsui/src-tauri/src/lib.rsui/src/components/Settings.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
rust-executor/src/graphql/mod.rs (2)
ui/src-tauri/src/commands/app.rs (1)
set_smtp_config(365-401)rust-executor/src/config.rs (1)
set_smtp_config(12-16)
rust-executor/src/lib.rs (1)
rust-executor/src/db.rs (1)
with_global_instance(1526-1535)
tests/js/tests/email-verification.test.ts (2)
core/src/Ad4mClient.ts (1)
Ad4mClient(19-72)tests/js/utils/utils.ts (3)
runHcLocalServices(36-64)startExecutor(66-120)apolloClient(122-160)
rust-executor/src/config.rs (1)
rust-executor/src/email_service.rs (1)
new(54-56)
ui/src-tauri/src/commands/app.rs (1)
ui/src-tauri/src/app_state.rs (1)
load(66-96)
core/src/runtime/RuntimeClient.ts (1)
core/src/runtime/RuntimeResolver.ts (4)
runtimeEmailTestModeEnable(527-530)runtimeEmailTestModeDisable(533-536)runtimeEmailTestGetCode(539-542)runtimeEmailTestClearCodes(545-548)
connect/src/core.ts (2)
core/src/Ad4mClient.ts (1)
Ad4mClient(19-72)connect/src/utils.ts (1)
connectWebSocket(15-53)
core/src/agent/AgentClient.ts (1)
core/src/runtime/RuntimeResolver.ts (2)
runtimeRequestLoginVerification(479-489)runtimeVerifyEmailCode(492-499)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (57)
rust-executor/src/agent/capabilities/mod.rs (1)
270-278: LGTM! Email verification capability correctly scoped to multi-user mode.The addition of
RUNTIME_USER_MANAGEMENT_VERIFY_CAPABILITYto unauthenticated capabilities is appropriate for the email verification flow. The capability is correctly gated behind themulti_user_enabledcheck, ensuring it's only available when multi-user mode is active. This aligns with the authentication pattern where unauthenticated users need to verify their email during signup or passwordless login.rust-executor/src/agent/capabilities/defs.rs (2)
12-12: LGTM! New VERIFY capability constant added.The new
VERIFYconstant follows the established naming convention and is appropriately defined for email verification operations.
473-479: LGTM! VERIFY capability definition is consistent with existing patterns.The
RUNTIME_USER_MANAGEMENT_VERIFY_CAPABILITYis well-structured and follows the same pattern as other user management capabilities (LOGIN, CREATE). The wildcard pointers are appropriate here, as email verification applies to all user management operations rather than specific resources.tests/js/tests/email-verification.test.ts (6)
42-95: Well-structured test setup and teardown.The test setup properly:
- Creates necessary directories and starts required services
- Enables multi-user and test modes
- Includes comprehensive cleanup in the
after()hook- Ensures test isolation via
beforeEach()code clearingThis provides a solid foundation for the test suite.
98-134: Excellent coverage of the full signup flow.This test comprehensively validates each step of the email verification signup process, from requesting verification through final JWT issuance. The assertions at each step provide clear failure points for debugging.
176-223: Excellent coverage of login flows including backward compatibility.These tests validate both the new passwordless email verification flow and ensure that traditional password-based login remains functional. The distinction between new users (
requiresPassword: true) and existing users (requiresPassword: false) is correctly tested.
225-247: Rate limiting test covers the essential behavior.The test correctly validates that rapid successive verification requests are blocked. The regex pattern for error matching (
/rate limit|too many|wait/i) is appropriately flexible to handle different error message formats while ensuring rate limiting is enforced.
249-298: Mock service functionality well-tested.These tests thoroughly validate the mock email service's capabilities:
- Handling multiple simultaneous users
- Code clearing functionality
- Test mode activation
This ensures the test infrastructure itself is reliable.
300-336: Critical security features properly validated.These tests verify two essential security properties:
- Each verification request generates a unique code
- Codes are single-use and cannot be reused
Both are crucial for preventing replay attacks and ensuring verification security.
cli/src/ad4m_executor.rs (1)
219-219: LGTM! Appropriate default for CLI SMTP configuration.Setting
smtp_config: Noneis correct for the CLI context, as SMTP configuration is managed by the launcher UI rather than command-line arguments. This follows the pattern of other optional configuration fields in theAd4mConfigstruct.ui/src-tauri/Cargo.toml (1)
40-40: The version constraint"0.11"is appropriate and follows semantic versioning best practices. It allows Cargo to use the latest patch version (currently 0.11.19), which ensures security updates within the 0.11 series while preventing unexpected major version upgrades. No known vulnerabilities affect the 0.11.x series; the historical CVEs (CVE-2020-28247, CVE-2021-38189) only impacted versions prior to 0.9.6.tests/js/package.json (1)
17-17: LGTM! Test script follows consistent patterns.The new
test-email-verificationscript follows the same structure and timeout settings as other test scripts in the package. The configuration is consistent with existing test infrastructure.rust-executor/src/lib.rs (1)
5-5: LGTM! New email service module added.The addition of the
email_servicepublic module is clean and follows the existing module structure.cli/src/dev.rs (1)
59-59: LGTM! SMTP config field properly initialized in dev scenarios.Both instances correctly initialize
smtp_config: Nonein theAd4mConfigfor development and testing scenarios, which is appropriate since SMTP is not needed for these local test flows.Also applies to: 212-212
rust-executor/src/graphql/graphql_types.rs (2)
670-676: LGTM! Clean GraphQL type for verification flow.The
VerificationRequestResultstruct is well-designed with appropriate derives and serde configuration. Therequires_passwordfield provides valuable context for the client to determine next steps in the verification flow.
682-682: LGTM! Multi-user signal routing enhancement.The optional
recipientfield enables per-recipient signal filtering, which is correctly integrated with theGetFiltertrait implementation at lines 898-902 to create composite filters..circleci/config.yml (2)
158-167: LGTM! Consistent CI job configuration.The new email verification test job follows the established pattern for integration tests, using the shared setup command and appropriate timeout settings.
197-199: LGTM! Proper workflow dependency configuration.The job is correctly added to the workflow with a dependency on
build-and-test, ensuring tests run after successful builds.connect/src/core.ts (1)
224-228: API signature is correct and all callers are properly updated.The
createUsermethod returnsUserCreationResultwith{success, error, did}fields as expected. All call sites in the codebase (connect/src/core.ts, connect/src/web.ts, and all test files) correctly handle the return type with proper success checks and error handling.ui/src/components/Settings.tsx (4)
115-127: LGTM - SMTP state initialization.The state variables for SMTP configuration are well-structured with appropriate types and default values.
160-177: LGTM - SMTP config loading.The effect correctly loads SMTP configuration on mount with sensible defaults (port 587 for STARTTLS).
298-330: LGTM - SMTP handlers implementation.The save and test handlers are well-implemented with proper loading states and error handling.
650-853: LGTM - SMTP configuration UI.The UI section is well-structured with:
- Clear field labels and helpful hints
- Password visibility toggle
- Test email functionality with status feedback
- Provider-specific guidance for Gmail and Mailgun
- Appropriate conditional rendering based on
multiUserEnabledui/src-tauri/src/lib.rs (4)
43-45: LGTM - Import additions for SMTP commands.The new SMTP-related commands are properly imported for the Tauri invoke handler.
251-255: LGTM - SMTP command registration.The new commands (
get_smtp_config,set_smtp_config,test_smtp_config) are correctly registered in the invoke handler.
278-323: LGTM - Config extraction with migration support.The logic correctly:
- Prefers
multi_user_configwhen available- Falls back to deprecated
tls_configfor backwards compatibility- Properly extracts and converts both TLS and SMTP configurations for the executor
339-340: LGTM - Executor config includes SMTP and multi-user settings.The
smtp_configandenable_multi_userfields are correctly passed to the executor configuration.core/src/runtime/RuntimeClient.ts (1)
422-457: LGTM - Email test mode client methods.The four new methods for email test mode are correctly implemented:
- Follow the established pattern of other RuntimeClient methods
- Use proper GraphQL mutations with correct unwrapping
- Return types align with the resolver definitions in
RuntimeResolver.tsThese methods enable integration tests to capture verification codes without actual email delivery.
ui/src-tauri/src/app_state.rs (2)
34-39: LGTM - MultiUserConfig structure.The struct correctly groups SMTP and TLS configurations under the multi-user feature, with an
enabledflag for toggling the functionality.
46-48: LGTM - LauncherState extension with deprecation notice.The
tls_configdeprecation is properly documented, and the newmulti_user_configfield is correctly added. Theskip_serializing_ifattribute ensures clean serialization when the deprecated field isNone.core/src/agent/AgentClient.ts (3)
16-16: LGTM - Import addition.The
VerificationRequestResulttype is correctly imported for use in the new authentication methods.
535-549: LGTM - createUser updated with appInfo parameter.The method correctly accepts an optional
appInfoparameter and passes it to the GraphQL mutation, enabling branded verification emails with app name/icon.
563-589: LGTM - New email verification methods.Both methods are well-implemented:
requestLoginVerificationreturns a structured result indicating success and whether password is requiredverifyEmailCodeaccepts the verification type to distinguish between registration and login flowsThese align with the resolver definitions in
RuntimeResolver.ts.core/src/runtime/RuntimeResolver.ts (3)
7-7: LGTM - Agent type imports.The imports for
UserCreationResultandAuthInfoInputare correctly added to support the new user creation and verification mutations.
211-221: LGTM - VerificationRequestResult type.The new GraphQL ObjectType is well-defined with:
success: indicates operation outcomemessage: provides user-facing feedbackrequiresPassword: guides the client flow for password-protected accounts
478-548: LGTM - New mutations for email verification and test mode.The resolver additions correctly define the GraphQL schema for:
runtimeRequestLoginVerification- initiates email verificationruntimeVerifyEmailCode- validates verification codesruntimeTestEmail- sends test emailsruntimeCreateUser/runtimeLoginUser- user management- Email test mode helpers (
runtimeEmailTestModeEnable,runtimeEmailTestModeDisable,runtimeEmailTestGetCode,runtimeEmailTestClearCodes)These return appropriate mock values for client-side testing, which aligns with the resolver's purpose as documented in the comment on lines 223-228.
ui/src-tauri/src/commands/app.rs (5)
2-2: LGTM - Updated imports.The imports are correctly updated to include the new configuration types.
183-192: LGTM - TLS config getter with migration support.The getter correctly prefers
multi_user_config.tls_configwhile falling back to the deprecatedtls_configfor backwards compatibility.
213-230: LGTM - TLS config setter with migration.The setter correctly:
- Creates a new
MultiUserConfigif none exists- Preserves existing SMTP config during migration (line 221)
- Updates both new and deprecated locations for compatibility
240-290: LGTM - Multi-user config management.The
get_multi_user_configandset_multi_user_configfunctions provide direct access with appropriate validation for SMTP and TLS settings.
358-401: LGTM - SMTP config management with migration support.The
get_smtp_configandset_smtp_configfunctions correctly:
- Store SMTP config within
multi_user_config- Preserve existing TLS config when migrating (line 387)
- Validate required fields before saving
connect/src/components/MultiUserAuth.ts (1)
38-113: LGTM!The email step implementation is clean with proper input handling, keyboard support (Enter key), and disabled states during loading.
connect/src/web.ts (5)
499-512: LGTM!The new state properties for the multi-user verification flow are properly typed and initialized with sensible defaults.
661-696: LGTM!The email submission handler correctly differentiates between existing users (proceeds to code verification) and new users (proceeds to password step for signup). The appInfo is properly constructed with app metadata.
744-750: Client rebuild issue resolved.The code now properly calls
buildClient()andcheckAuth()after setting the token, which ensures the client uses the new JWT for API calls. This addresses a previous review concern.
768-773: LGTM!The back-to-email handler properly resets the flow state and clears sensitive data (password, verification code).
1160-1178: LGTM!The MultiUserAuth component is properly wired with all required props, state values, and handlers matching the component's interface.
rust-executor/src/email_service.rs (3)
74-85: LGTM!The test mode capture logic correctly stores the verification code without sending an email, and the lock is released before any async operations. The logging of the code in test mode is intentional for test debugging.
155-174: Good URL validation for icon paths.The check for
http://,https://, ordata:URI schemes correctly prevents local file path exposure in emails. However, consider thatapp_nameandicon_urlare directly interpolated into HTML without escaping - if they contain HTML special characters, this could break email layout.If app metadata could come from untrusted sources, HTML escaping should be added. For now, since this comes from app config, it should be safe.
416-422: LGTM!The use of
spawn_blockingis correct for running the synchronous lettre SMTP transport within an async context, preventing blocking of the async runtime.rust-executor/src/graphql/mutation_resolvers.rs (4)
568-651: LGTM!The verification email sending logic properly handles:
- Test mode with dummy SMTP config
- Production mode with real SMTP
- Partial success when email fails (user created but email not sent)
- Appropriate rate limit exemption for signup emails
783-794: Rate limit check is implemented correctly.The rate limit is checked before generating a verification code, preventing abuse. The error message is returned in the response rather than thrown, providing a good UX.
1003-1048: LGTM!Test mode mutations are properly restricted to admin-only access via
ALL_CAPABILITYcheck, preventing unauthorized access to captured verification codes.
36-54: LGTM!The helper function properly encapsulates perspective access control, reusing the existing
can_access_perspectivecheck and providing a clear access denied error.rust-executor/src/db.rs (3)
261-291: LGTM! Well-designed email verification schema.The database schema for email verification is properly designed with appropriate indexes and constraints:
- UNIQUE constraint on
(email, verification_type)prevents duplicate pending verifications- Index on
expires_atenables efficient cleanup of expired codes- Index on
- Rate limits table with email as PRIMARY KEY is simple and efficient
2500-2513: LGTM! Efficient expired code cleanup.The cleanup implementation correctly uses the indexed
expires_atcolumn for efficient deletion of expired verification codes. The logic is straightforward and correct.
2515-2557: Rate limiting implementation is correct.The rate limiting logic correctly enforces a 60-second cooldown between verification code requests. The error message helpfully includes the remaining wait time.
Note: There's a small TOCTOU (time-of-check-time-of-use) gap between
check_rate_limitandupdate_rate_limitcalls, but this is acceptable for rate limiting—worst case is slightly more lenient enforcement, which doesn't compromise security.
| regex = "1.5.4" | ||
| json5 = "0.4" | ||
| rayon = "1.7.0" # For parallel processing in filtering optimization | ||
| lettre = "0.11" # For sending verification emails via SMTP |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
lettre Rust crate version 0.11 security vulnerabilities latest version
💡 Result:
Latest 0.11.x: 0.11.19 (latest published 2 months ago as of Jan 5, 2026). [1]
Security status: 0.11 series has dependency-related security warnings reported by scanners (deps.rs and Socket show known/possible vulnerabilities in dependencies such as tracing-subscriber and others). Scanners flag the crate as containing known security vulnerabilities; you should review the dependency advisories and upgrade to the latest patch (0.11.19) or follow upstream fixes. [2][3]
Sources:
- lettre docs (latest 0.11.19). [1]
- deps.rs security summary for lettre (vulnerabilities flagged). [2]
- Socket/package security overview for lettre (latest + alerts). [3]
Upgrade lettre to 0.11.19 and review security advisories for dependency vulnerabilities.
The 0.11 series contains known security vulnerabilities in its dependencies. Upgrade to 0.11.19 (latest stable patch) and review dependency advisories on deps.rs and security scanners. Use a specific version constraint (e.g., lettre = "0.11.19") rather than loose versioning for stability and security compliance.
🤖 Prompt for AI Agents
In @rust-executor/Cargo.toml around line 117, Update the lettre dependency in
Cargo.toml from the loose "0.11" constraint to the specific patched release
"0.11.19" (change the lettre entry so it reads lettre = "0.11.19"), then run
cargo update and audit (e.g., cargo audit or check deps.rs/advisories) to
confirm no remaining vulnerable transitive dependencies and adjust other
dependency versions if needed.
There was a problem hiding this comment.
doesn't "0.11" mean that it will take the latest 0.11.x release?
There was a problem hiding this comment.
@lucksus Yes, you're absolutely correct! In Cargo, "0.11" is shorthand for "^0.11", which means it will accept any version >= 0.11.0 and < 0.12.0. This includes the latest 0.11.x patch releases like 0.11.19.
The bot's suggestion to pin to a specific version like "0.11.19" is about a different concern: reproducible builds and explicit security management. Here are the trade-offs:
Loose constraint ("0.11"):
- ✅ Automatically gets security patches in 0.11.x
- ✅ Less maintenance (no manual version bumps)
- ❌ Builds may differ across environments/times
- ❌ Patch updates could introduce subtle breakage
Pinned version ("0.11.19"):
- ✅ Reproducible builds everywhere
- ✅ Explicit control over which version is used
- ✅ Security audits are clearer
- ❌ Requires manual updates for security patches
- ❌ Won't get automatic patch fixes
For this PR, since you're adding email verification (a security-sensitive feature), either approach is reasonable:
- Keep
"0.11"if you want automatic patch updates viacargo update - Pin to
"0.11.19"if you prefer explicit version management
The key is to run cargo update regularly (or in CI) to ensure you're getting the latest compatible versions, especially for security-related crates.
| cleanup_processes() { | ||
| echo "🧹 Killing any existing AD4M processes..." | ||
| # Kill any processes using the AD4M ports | ||
| pkill -f "rust-executor" 2>/dev/null || true | ||
| pkill -f "ad4m" 2>/dev/null || true | ||
|
|
||
| # Wait a moment for processes to die | ||
| sleep 2 | ||
|
|
||
| # Force kill if still running | ||
| lsof -ti:15920 | xargs kill -9 2>/dev/null || true | ||
| lsof -ti:15921 | xargs kill -9 2>/dev/null || true | ||
| lsof -ti:15922 | xargs kill -9 2>/dev/null || true | ||
| } |
There was a problem hiding this comment.
Broad process kill patterns could affect unrelated processes.
The pkill -f commands with patterns "rust-executor" and "ad4m" will terminate any process whose command line contains these strings, potentially killing unrelated processes. This is especially risky in CI environments where multiple jobs might run concurrently.
🔎 Proposed fix using more specific process identification
Consider using a PID file approach or more specific patterns:
cleanup_processes() {
echo "🧹 Killing any existing AD4M processes..."
- # Kill any processes using the AD4M ports
- pkill -f "rust-executor" 2>/dev/null || true
- pkill -f "ad4m" 2>/dev/null || true
+ # Kill processes by port instead of name pattern
+ lsof -ti:15920 | xargs kill -15 2>/dev/null || true
+ lsof -ti:15921 | xargs kill -15 2>/dev/null || true
+ lsof -ti:15922 | xargs kill -15 2>/dev/null || true
# Wait a moment for processes to die
- sleep 2
+ sleep 3
# Force kill if still running
lsof -ti:15920 | xargs kill -9 2>/dev/null || trueThis approach targets processes by the ports they're actually using rather than by name patterns.
📝 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.
| cleanup_processes() { | |
| echo "🧹 Killing any existing AD4M processes..." | |
| # Kill any processes using the AD4M ports | |
| pkill -f "rust-executor" 2>/dev/null || true | |
| pkill -f "ad4m" 2>/dev/null || true | |
| # Wait a moment for processes to die | |
| sleep 2 | |
| # Force kill if still running | |
| lsof -ti:15920 | xargs kill -9 2>/dev/null || true | |
| lsof -ti:15921 | xargs kill -9 2>/dev/null || true | |
| lsof -ti:15922 | xargs kill -9 2>/dev/null || true | |
| } | |
| cleanup_processes() { | |
| echo "🧹 Killing any existing AD4M processes..." | |
| # Kill processes by port instead of name pattern | |
| lsof -ti:15920 | xargs kill -15 2>/dev/null || true | |
| lsof -ti:15921 | xargs kill -15 2>/dev/null || true | |
| lsof -ti:15922 | xargs kill -15 2>/dev/null || true | |
| # Wait a moment for processes to die | |
| sleep 3 | |
| # Force kill if still running | |
| lsof -ti:15920 | xargs kill -9 2>/dev/null || true | |
| lsof -ti:15921 | xargs kill -9 2>/dev/null || true | |
| lsof -ti:15922 | xargs kill -9 2>/dev/null || true | |
| } |
| echo "🧹 Cleaning testing data..." | ||
| node scripts/cleanTestingData.js | ||
|
|
||
| # Step 2: Prepare test environment | ||
| echo "🔧 Preparing test environment..." | ||
| echo " - Building test languages..." | ||
| ./scripts/build-test-language.sh | ||
|
|
||
| echo " - Preparing test directory..." | ||
| ./scripts/prepareTestDirectory.sh | ||
|
|
||
| echo " - Getting builtin test languages..." | ||
| deno run --allow-all scripts/get-builtin-test-langs.js | ||
|
|
||
| echo " - Injecting language language..." | ||
| pnpm run inject-language-language | ||
|
|
||
| echo " - Publishing test languages..." | ||
| pnpm run publish-test-languages | ||
|
|
||
| echo " - Injecting publishing agent..." | ||
| pnpm run inject-publishing-agent | ||
|
|
There was a problem hiding this comment.
Missing error handling could hide test setup failures.
The script continues executing even if critical setup steps fail (e.g., if build-test-language.sh fails). This could lead to misleading test results where tests fail due to improper setup rather than actual bugs.
🔎 Proposed fix to add error handling
Add set -e at the top of the script to exit on any error, or add explicit error checking:
#!/bin/bash
+set -e # Exit on any error
+set -u # Exit on undefined variable
+set -o pipefail # Exit on pipe failures
# Script to run multi-user test with proper setupAlternatively, add error checking after critical steps:
echo " - Building test languages..."
./scripts/build-test-language.sh
+if [ $? -ne 0 ]; then
+ echo "❌ Failed to build test languages"
+ exit 1
+fiCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @tests/js/email-verification-test-with-setup.sh around lines 37-59, The setup
script lacks error handling and continues after failed steps; update the top of
tests/js/email-verification-test-with-setup.sh to enable immediate exit on
failures (e.g., add set -e and optionally set -o pipefail) and/or add explicit
exit checks after critical commands like ./scripts/build-test-language.sh,
./scripts/prepareTestDirectory.sh, deno run --allow-all
scripts/get-builtin-test-langs.js, pnpm run inject-language-language, pnpm run
publish-test-languages, and pnpm run inject-publishing-agent so the script logs
the failing command and exits non-zero when any of these commands fail.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rust-executor/src/db.rs (1)
2580-2621: Consider stricter rate limiting for production.The current 60-second cooldown allows up to 1,440 verification requests per email per day. While adequate for initial implementation and combined with other protections (5 max attempts, 15-minute expiry), consider implementing:
- Longer cooldown period (e.g., 5-10 minutes) for production
- Daily request cap per email address
- Progressive backoff after repeated requests
The current implementation is functional and the user experience is good with the helpful error message showing remaining wait time.
tests/js/tests/email-verification.test.ts (1)
318-327: Minor: Timing-based delay may cause test flakiness.The
sleep(100)delay to "ensure new code generation" is a timing assumption. If the backend or network is slow, 100ms may not be sufficient; if the backend is fast, this is unnecessary overhead.Consider using an assertion or retry mechanism instead of a fixed delay to verify the new code has been generated.
rust-executor/src/email_service.rs (2)
47-61: Consider logging on lock acquisition failure.The
enable_test_mode()anddisable_test_mode()functions silently ignore mutex lock failures. While mutex poisoning is rare, silent failure could make debugging difficult if it occurs.🔎 Optional improvement to log lock failures
pub fn enable_test_mode() { - if let Ok(mut test_mode) = EMAIL_TEST_MODE.lock() { - *test_mode = true; - log::info!("Email service test mode ENABLED - emails will be captured, not sent"); + match EMAIL_TEST_MODE.lock() { + Ok(mut test_mode) => { + *test_mode = true; + log::info!("Email service test mode ENABLED - emails will be captured, not sent"); + } + Err(e) => { + log::error!("Failed to acquire EMAIL_TEST_MODE lock: {}", e); + } } }
360-365: Verification code is not HTML-escaped in template.The
codevariable is interpolated directly into the HTML template without escaping. While verification codes are internally generated as 6-digit numeric strings (low risk), applyingescape_htmlwould provide defense-in-depth.🔎 Minor hardening suggestion
<div class="code-container"> <div class="code-label">Your Verification Code</div> - <div class="code">{code}</div> + <div class="code">{escaped_code}</div> </div>And add at the beginning of
render_verification_email:let escaped_code = escape_html(code);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rust-executor/src/db.rsrust-executor/src/email_service.rsrust-executor/src/graphql/mutation_resolvers.rstests/js/tests/email-verification.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- rust-executor/src/graphql/mutation_resolvers.rs
🧰 Additional context used
🧬 Code graph analysis (3)
rust-executor/src/email_service.rs (2)
rust-executor/src/db.rs (2)
new(68-320)with_global_instance(1534-1543)ui/src-tauri/src/app_state.rs (1)
new(50-64)
tests/js/tests/email-verification.test.ts (2)
core/src/Ad4mClient.ts (1)
Ad4mClient(19-72)tests/js/utils/utils.ts (3)
runHcLocalServices(36-64)startExecutor(66-120)apolloClient(122-160)
rust-executor/src/db.rs (1)
rust-executor/src/utils.rs (1)
constant_time_eq(25-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
rust-executor/src/db.rs (5)
10-10: Imports look good.The new imports support the email verification implementation appropriately:
constant_time_eqfor secure comparisons,rand::Rngfor code generation,sha2for hashing, and time utilities for expiry management.Also applies to: 17-17, 21-21, 23-23
262-299: Well-designed database schema.The email verification tables are properly structured:
UNIQUE(email, verification_type)constraint prevents multiple active verifications- Indices on
expires_atsupport efficient queries and cleanup- Migration-safe
ALTER TABLE(ignoring errors) handles existing deployments gracefullyfailed_attemptscolumn withDEFAULT 0supports brute-force protection
2418-2452: Secure verification code generation.The implementation correctly:
- Generates cryptographically random 6-digit codes
- Hashes codes with SHA-256 before storage
- Deletes any existing verification before creating a new one
- Sets appropriate 15-minute expiry
- Initializes
failed_attemptsto 0 for brute-force tracking
2456-2541: Excellent security implementation—timing attack vulnerability resolved.The verification logic is now secure and properly addresses previous review feedback:
Constant-time hash comparison (line 2510) correctly prevents timing attacks, resolving the previously flagged issue where
==was leaking timing information.Additional security measures:
- Brute-force protection with 5 max failed attempts before invalidation
COALESCE(failed_attempts, 0)(line 2479) handles backward compatibility for pre-migration records- Expiry check precedes comparison, reducing unnecessary work
- Single-use enforcement via deletion on success
- Clear error message when code is invalidated
2624-2644: Test helper is appropriate.The
set_verification_code_expiryfunction provides necessary testing support for expiry scenarios with appropriate error handling.tests/js/tests/email-verification.test.ts (5)
1-18: LGTM!Standard test setup with appropriate imports and ESM compatibility handling.
42-95: LGTM!Test lifecycle management is well-structured with proper setup of local Holochain services, executor initialization, test mode enablement, and cleanup. The
beforeEachclearing of codes ensures test isolation.
97-187: LGTM!The signup flow tests comprehensively cover the happy path, invalid code rejection, and code expiration. The expiration test now properly simulates expired codes using
emailTestSetExpirywith a past timestamp.
238-257: LGTM!The rate limiting test correctly verifies that
requestLoginVerificationreturnssuccess: falsewith an appropriate message on the second immediate request. This properly tests the rate limiting behavior.
492-519: Test validates concurrent rate limiting - ensure backend atomicity.This test makes 5 concurrent failed verification attempts and expects all to fail with the code being invalidated. This is a good security test, but it relies on the backend atomically tracking and enforcing the 5-attempt limit under concurrent load.
If this test becomes flaky, consider verifying that the backend uses atomic operations (e.g., database transactions or locks) for the failed attempt counter.
rust-executor/src/email_service.rs (5)
9-35: LGTM!Correct implementation of HTML escaping for preventing injection attacks. Both functions properly escape the necessary special characters (
<,>,&,",').
63-86: LGTM!The
get_test_codefunction properly validates against the database before returning cached codes, ensuring consistency between the cache and the authoritative source. Stale cache entries are correctly evicted when the DB no longer has the code.
95-145: LGTM!The
EmailServiceimplementation correctly branches between test mode (capturing codes) and normal mode (sending emails). The default tofalseon lock failure is a safe choice that prevents test code from being accidentally treated as production behavior.
429-476: LGTM!The port-based TLS handling is well-implemented:
- Port 465 correctly uses implicit TLS (Wrapper mode)
- Port 25 uses opportunistic TLS with an appropriate security warning
- Other ports (587, 2525) require STARTTLS
Certificate validation is properly enabled (
dangerous_accept_invalid_certs(false)) in all branches.
478-484: LGTM!Using
spawn_blockingfor the synchronous SMTP send operation is the correct approach to avoid blocking the async runtime. Error handling properly maps transport errors toAnyError.
| } catch (e) { | ||
| this._multiUserError = e.message || "Failed to log in. Please check your credentials."; | ||
| this._multiUserError = "Invalid or expired code. Please try again."; | ||
| this._multiUserVerificationCode = ""; |
There was a problem hiding this comment.
Misleading error hides connection failures after successful verification
In handleMultiUserCodeSubmit, the catch block shows "Invalid or expired code" for ALL errors, including connection failures that occur after successful verification. When verifyEmailCode succeeds (consuming the single-use code on the server) but buildClient() or checkAuth() subsequently fails, the user sees a misleading error suggesting their code was invalid. The user cannot retry with the same code since it was already consumed, yet the token was successfully stored. This leads to user confusion and unnecessary code re-requests.
…s not configured.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rust-executor/src/email_service.rs (2)
47-61: Silent failure on mutex poisoning may mask panics.Using
.lock().ok()silently ignores mutex poisoning. If a thread panics while holding the lock, subsequent calls will silently fail without logging any indication of the problem.🔎 Consider logging poisoning errors
pub fn enable_test_mode() { - if let Ok(mut test_mode) = EMAIL_TEST_MODE.lock() { + match EMAIL_TEST_MODE.lock() { + Ok(mut test_mode) => { *test_mode = true; log::info!("Email service test mode ENABLED - emails will be captured, not sent"); + } + Err(e) => { + log::error!("Failed to acquire EMAIL_TEST_MODE lock (poisoned): {}", e); + } } }
63-85: Potential race condition between DB check and cache access.The function checks the database first, then accesses the cache. Between these two operations, another thread could invalidate the code. While unlikely in practice for test scenarios, this could lead to returning a stale code that's already been used.
For test mode usage this is acceptable, but consider documenting this behavior or using a single lock scope if stricter consistency is needed in the future.
rust-executor/src/graphql/mutation_resolvers.rs (1)
1139-1163: Consider using i64 for expires_at parameter.The
expires_atparameter is typed asi32and cast toi64. While this works for testing purposes, Unix timestamps after 2038-01-19 will overflow an i32. Consider usingi64directly if GraphQL schema supports it.This is a minor concern since this is a test-only helper, but worth noting for future-proofing.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust-executor/src/db.rsrust-executor/src/email_service.rsrust-executor/src/graphql/mutation_resolvers.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T15:16:29.623Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 619
File: rust-executor/src/perspectives/perspective_instance.rs:639-641
Timestamp: 2025-09-03T15:16:29.623Z
Learning: In the AD4M codebase, database operations like `add_many_links` during critical synchronization processes (such as `diff_from_link_language`) should use `.expect()` rather than error logging, as failures indicate severe system issues where fail-fast behavior is preferred over continuing with potentially corrupted state.
Applied to files:
rust-executor/src/graphql/mutation_resolvers.rs
🧬 Code graph analysis (3)
rust-executor/src/email_service.rs (2)
rust-executor/src/db.rs (2)
new(68-320)with_global_instance(1534-1543)ui/src-tauri/src/app_state.rs (1)
new(50-64)
rust-executor/src/graphql/mutation_resolvers.rs (2)
rust-executor/src/email_service.rs (3)
enable_test_mode(48-53)disable_test_mode(56-61)get_test_code(65-85)rust-executor/src/agent/capabilities/defs.rs (1)
get_user_default_capabilities(485-545)
rust-executor/src/db.rs (1)
rust-executor/src/utils.rs (1)
constant_time_eq(25-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (23)
rust-executor/src/email_service.rs (5)
9-35: LGTM! HTML escaping functions are correctly implemented.Both
escape_htmlandescape_html_attributeproperly escape the necessary characters to prevent HTML/attribute injection attacks. This addresses the previous review comment about escaping user-provided values in email templates.
103-144: LGTM! Test mode email capture is well-implemented.The test mode correctly captures verification codes in memory instead of sending emails, with appropriate logging. The fallback to normal sending when test mode is disabled works as expected.
199-223: LGTM! App context rendering with proper HTML escaping.User-provided
app_nameandapp_iconvalues are correctly escaped usingescape_htmlandescape_html_attributerespectively, preventing HTML injection in verification emails. The URL validation (http/https/data:) before rendering the image is a good security practice.
442-475: LGTM! Port-based TLS handling with appropriate security warning.The implementation correctly uses:
Tls::Wrapperfor port 465 (implicit TLS)Tls::Opportunisticfor port 25 with a clear security warningTls::Requiredfor other ports (STARTTLS)This addresses the previous review comment about port 25 security.
477-483: LGTM! Async email sending with proper error handling.Using
spawn_blockingfor the synchronous SMTP transport is the correct pattern to avoid blocking the async runtime. Error handling properly chains both the spawn error and the send error.rust-executor/src/graphql/mutation_resolvers.rs (11)
467-467: LGTM! Added app_info parameter for email context.The optional
app_infoparameter allows verification emails to display the requesting application's name and icon, improving user experience and transparency.
568-604: LGTM! Rate limiting applied before verification code generation.Rate limiting is now checked and updated atomically before generating the verification code. This prevents abuse of the signup endpoint and addresses the previous rate limit bypass concern. The rate limit is also properly skipped in test mode.
659-671: LGTM! Cleanup of verification code on email delivery failure.The verification code is properly cleaned up when email sending fails (but preserved in test mode), preventing stale tokens from accumulating in the database.
686-714: LGTM! SMTP not configured case now returns informative message.When SMTP is not configured, the function now returns
success: truewith an error message explaining the situation, and the verification code is cleaned up. This addresses the previous review comment about silent success when SMTP is unconfigured.
768-777: LGTM! User-scoped capabilities instead of admin access.The fallback
AuthInfonow usesget_user_default_capabilities()instead ofALL_CAPABILITY, ensuring email-verified users receive appropriately scoped permissions rather than unrestricted admin access. This addresses the previous security concern.
838-863: LGTM! Rate limiting before user existence check prevents enumeration.Rate limit is now checked and updated before revealing whether the user exists. This prevents attackers from enumerating registered email addresses by bypassing rate limits. The error handling for rate limit update failures is also proper, preventing bypass via DB errors.
872-879: Verify user enumeration mitigation is complete.While rate limiting is now applied before the user existence check, the response still differs: existing users get
requires_password: false(email sent), while new users getrequires_password: true. An attacker who stays under the rate limit can still distinguish between existing and non-existing accounts.Consider whether this differentiation is intentional for UX reasons (guiding new users to sign up) or if a more uniform response would be preferable for security.
This may be an acceptable trade-off for UX, but please confirm this is the intended behavior.
992-1011: LGTM! Verification code validation with brute force protection.The
verify_codefunction now returns an error when the code is invalidated due to too many failed attempts, and the mutation properly propagates this error. This addresses the previous concern about brute force attacks on verification codes.
1021-1032: LGTM! User-scoped capabilities for email-verified users.The JWT generated after successful email verification now uses
get_user_default_capabilities()instead ofALL_CAPABILITY, ensuring proper permission scoping in multi-user mode. This addresses the previous critical security concern.
1062-1090: LGTM! Test email mutation with proper admin-only access.The
runtime_test_emailmutation correctly requiresALL_CAPABILITY(admin-only) and properly handles the case when SMTP is not configured.
1092-1137: LGTM! Test mode mutations are properly restricted to admins.All test mode helper mutations (
runtime_email_test_mode_enable,runtime_email_test_mode_disable,runtime_email_test_get_code,runtime_email_test_clear_codes) correctly requireALL_CAPABILITY, preventing non-admin users from manipulating test mode state.rust-executor/src/db.rs (7)
10-10: LGTM!The new imports are appropriate for the email verification functionality. Notably,
constant_time_eqfromcrate::utilsaddresses the previously flagged timing attack concern.Also applies to: 17-17, 21-21, 23-23
262-299: LGTM!The email verification schema is well-designed:
- Unique constraint on
(email, verification_type)prevents duplicate active codes- Indices on
expires_atoptimize lookups and cleanup queriesfailed_attemptscolumn withDEFAULT 0handles brute-force protectionThe
let _ =pattern for the ALTER TABLE migration is acceptable for idempotent column additions.
2418-2452: LGTM!The verification code generation is well-implemented:
- 6-digit codes (100000-999999) provide sufficient entropy for rate-limited, time-bounded verification
- SHA-256 hashing prevents plaintext code storage
- 15-minute expiry is reasonable
- Atomic delete-before-insert ensures single active code per email+type
2543-2556: LGTM!Simple and effective cleanup function for expired verification codes.
2558-2571: LGTM!Idempotent cleanup function for verification codes when email sending fails.
2573-2636: LGTM!Rate limiting implementation is correct:
- 60-second cooldown between verification emails is reasonable for anti-abuse
- Proper upsert pattern with
ON CONFLICT DO UPDATEhas_verification_codecorrectly filters byverified=0andexpires_at > now
2638-2659: LGTM!Test helper function for testing expiration scenarios. Properly validates that a verification code exists before modifying.
| // Auto-submit when 6 digits entered | ||
| if (cleaned.length === 6) { | ||
| setTimeout(() => onCodeSubmit(), 100); | ||
| } |
There was a problem hiding this comment.
Auto-submit may fire multiple times without guard
The verification code auto-submit uses setTimeout without cancellation when 6 digits are entered. If a user quickly types, deletes, and re-types digits, multiple setTimeout callbacks accumulate. When they fire, handleMultiUserCodeSubmit lacks a guard checking if _multiUserLoading is already true, potentially causing duplicate verification requests. The second request would fail (code already consumed), resulting in an error message appearing even after successful authentication. Adding a cancellation mechanism or an early return guard when _multiUserLoading is true would prevent this race condition.
Additional Locations (1)
| /// Test mode: captures sent emails instead of actually sending them | ||
| /// Map of email address -> verification code | ||
| pub static ref TEST_MODE_EMAILS: Arc<Mutex<std::collections::HashMap<String, String>>> = | ||
| Arc::new(Mutex::new(std::collections::HashMap::new())); |
There was a problem hiding this comment.
Test mode cache ignores verification type causing incorrect code
The TEST_MODE_EMAILS cache maps email to code without considering verification type, but the database stores codes by (email, verification_type) pairs. When send_verification_email is called for different types (signup vs login) on the same email, the cache overwrites the previous code. Meanwhile, get_test_code uses has_verification_code which checks for any code regardless of type. This mismatch means if a test creates both signup and login codes for the same email, get_test_code returns the wrong code, causing verification to fail unexpectedly. The cache key needs to include verification_type to match the database model.
Additional Locations (1)
| format!("Failed to send verification email: {}", e), | ||
| graphql_value!(null), | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Login verification code not cleaned up on email failure
In runtime_request_login_verification, when email sending fails or SMTP is not configured, the function returns an error or early-returns without cleaning up the verification code that was already created in the database at lines 882-893. This contrasts with runtime_create_user which explicitly calls delete_verification_code in both failure scenarios. The orphaned codes will accumulate in the database until the background cleanup task runs, and could potentially be exploited to fill the email_verifications table with orphaned records since rate limiting is bypassed by the early return paths.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
connect/src/components/MultiUserAuth.ts (1)
98-111: Consider extracting duplicated error message styles.The
.error-messagestyles are identical across all three step branches (email, password, and code). Moving this CSS to the parent component or a shared location would improve maintainability.🔎 Suggested refactor
Move the error-message styles to the parent component (web.ts) or extract to a shared style constant that can be imported and reused across components.
Also applies to: 178-191, 268-280
connect/src/web.ts (2)
736-784: Complex nested signup flow handling multiple edge cases.The signup branch handles three distinct scenarios (SMTP not configured, email sent successfully, email failed) with deeply nested conditions. The error handling conflates buildClient/checkAuth failures with signup failures, making debugging difficult.
Consider refactoring to separate concerns:
- Extract each scenario (no SMTP, email sent, email failed) into separate methods
- Catch buildClient/checkAuth errors separately to provide clearer error messages
- Simplify the conditional branches
This would improve maintainability and make the error states clearer for users.
873-882: Consider slightly longer debounce for auto-submit.The 100ms debounce for auto-submit when 6 digits are entered is very short. Users typing quickly might not perceive any delay, which could feel abrupt. Consider 300-500ms to give users a moment to review before submission.
The timeout cancellation logic correctly prevents race conditions, and the dual
_multiUserLoadingchecks provide good defense in depth.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
connect/src/components/MultiUserAuth.tsconnect/src/web.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
connect/src/web.ts (4)
794-797: Good defensive guard against duplicate submissions.The early return when
_multiUserLoadingis true prevents race conditions from duplicate code submission attempts.
841-851: Clean state reset implementation.The method properly cancels pending timeouts and resets all relevant state when navigating back to email step.
1255-1273: MultiUserAuth integration is complete and correct.All required props are properly passed, including the new step-based properties and callbacks. The integration correctly wires the component to the state management handlers.
499-515: State initialization is correct.The new state variables are properly initialized with appropriate types, including the timeout reference with the correct TypeScript return type.
| format!("Failed to send verification email: {}", e), | ||
| graphql_value!(null), | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Missing verification code cleanup on email failure
In runtime_request_login_verification, a verification code is created in the database (lines 881-893), but if SMTP is not configured (early return at lines 925-937) or if email sending fails (error at lines 950-955), the code is not cleaned up. This leaves orphaned codes in the database. The user's rate limit is consumed but they never receive the code, forcing them to wait 60 seconds before requesting another. This contrasts with runtime_create_user which properly calls delete_verification_code in both failure scenarios.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @connect/src/web.ts:
- Around line 792-838: The code calls changeUIState("connected") from
handleMultiUserCodeSubmit and loginToHosting but "connected" is not in the
uiState union and the catch block masks non-verification failures; update the
uiState type to include "connected" or change those calls to an existing state
(e.g., use connectionState/requestcap), and in handleMultiUserCodeSubmit
distinguish verification errors from post-verification failures by checking
where the error came from (wrap tempClient.agent.verifyEmailCode separately and
only set _multiUserError = "Invalid or expired code" on verification failure),
otherwise preserve/log the real error (e.g., store/log the caught error object)
and show a different user-facing message for connection/authentication failures
while still resetting _multiUserVerificationCode as appropriate; refer to
handleMultiUserCodeSubmit, changeUIState, loginToHosting,
tempClient.agent.verifyEmailCode, _client.buildClient, _client.checkAuth, and
_multiUserError when making changes.
🧹 Nitpick comments (4)
rust-executor/src/graphql/graphql_types.rs (1)
679-684: NeighbourhoodSignalFilter recipient support and composite filter look soundAdding the optional
recipientfield plus theuuid|recipient_didcomposite inget_filter()preserves backwards compatibility (falls back to justuuid) while enabling per-recipient routing. Just be aware this string format is now part of the implicit subscription contract; if you ever need to evolve it, consider a small helper (e.g.NeighbourhoodFilterKey::to_string/from_string) instead of ad‑hocformat!to avoid divergence across callers.Also applies to: 895-903
connect/src/components/MultiUserAuth.ts (1)
200-299: Code input sanitization and delegation of auto-submit is correctRestricting the code field to digits (
replace(/\D/g, ""), numeric inputMode, maxlength=6) and pushing the auto-submit logic intochangeVerificationCodeavoids the earliersetTimeoutrace in the component itself and keeps timing concerns in the parent. This is a solid separation of concerns; only small nit is the repeated.error-messagestyles across steps, which could be centralized later if desired.Also applies to: 223-247
rust-executor/src/graphql/mutation_resolvers.rs (1)
723-807: Login and login-verification flows now align with user-scoped capabilities and safer rate limitingA few points look especially good here:
runtime_login_usernow usesget_user_default_capabilities()for the no-token case and always setsuser_email, avoiding the earlier ALL_CAPABILITY overreach while keeping app context when a token is present.runtime_request_login_verification:
- Enforces
RUNTIME_USER_MANAGEMENT_LOGIN_CAPABILITY.- Applies
check_rate_limit+update_rate_limitbefore revealing whether the user exists, and treatsupdate_rate_limitfailures as a hard error, which closes the prior rate-limit bypass and user-enumeration window.- Cleanly separates new-user (
requires_password: true, is_existing_user: false) from existing-user flows, and returns a structuredVerificationRequestResultrather than relying on message parsing.- Uses test mode + dummy SMTP config to allow tests to run without real SMTP, while falling back to password login (and a clear message) when SMTP is not configured.
The only trade-off is that login verification codes created when SMTP is unavailable are left for the background cleanup task rather than being deleted immediately, but with rate limiting enforced up front this is mainly a storage concern and seems acceptable.
Also applies to: 809-968
core/src/runtime/RuntimeResolver.ts (1)
481-562: LGTM! Test mutations follow established patterns.All new mutation handlers appropriately return test fixtures consistent with this file's purpose as a test resolver (as documented in lines 226-231). The implementations provide realistic mock data for email verification flows.
Optional: Consider using an enum for verificationType
The
verificationTypeparameter (lines 499, 557) is currently a string. For improved type safety across the codebase, you might consider defining a GraphQL enum:@registerEnumType enum VerificationType { LOGIN = "login", SIGNUP = "signup", // ... other types }However, since this is test/mock code, a string is perfectly acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
connect/src/components/MultiUserAuth.tsconnect/src/web.tscore/src/agent/AgentClient.tscore/src/runtime/RuntimeResolver.tsrust-executor/src/graphql/graphql_types.rsrust-executor/src/graphql/mutation_resolvers.rstests/js/tests/email-verification.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T15:16:29.623Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 619
File: rust-executor/src/perspectives/perspective_instance.rs:639-641
Timestamp: 2025-09-03T15:16:29.623Z
Learning: In the AD4M codebase, database operations like `add_many_links` during critical synchronization processes (such as `diff_from_link_language`) should use `.expect()` rather than error logging, as failures indicate severe system issues where fail-fast behavior is preferred over continuing with potentially corrupted state.
Applied to files:
rust-executor/src/graphql/mutation_resolvers.rs
🧬 Code graph analysis (3)
core/src/agent/AgentClient.ts (1)
core/src/runtime/RuntimeResolver.ts (2)
runtimeRequestLoginVerification(482-493)runtimeVerifyEmailCode(496-503)
connect/src/components/MultiUserAuth.ts (2)
connect/src/core.ts (1)
Event(34-37)connect/src/web.ts (2)
changeEmail(1007-1009)changePassword(1011-1013)
tests/js/tests/email-verification.test.ts (2)
core/src/Ad4mClient.ts (1)
Ad4mClient(19-72)tests/js/utils/utils.ts (3)
runHcLocalServices(36-64)startExecutor(66-120)apolloClient(122-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (9)
rust-executor/src/graphql/graphql_types.rs (1)
670-677: VerificationRequestResult shape matches runtime surfaceThe GraphQL object cleanly mirrors the TS
VerificationRequestResult(success, message, requiresPassword, isExistingUser) and camelCase serde is consistent with the rest of the schema. No issues here.connect/src/components/MultiUserAuth.ts (1)
3-19: Step-based props and callbacks are coherent and future-proofThe expanded
MultiUserAuthProps(verificationCode, step, verificationType, and the per-step callbacks) make the rendering logic explicit and keep the component nicely dumb. The three branches for"email" | "password" | "code"map cleanly to those props, and disabling actions based onisLoading/empty inputs avoids accidental double-submits. Looks good.Also applies to: 21-37, 39-113
connect/src/web.ts (1)
499-516: Multi-user email/password flow and auto-submit wiring are robustThe new multi-user fields and handlers hang together well:
handleMultiUserEmailSubmitcorrectly branches onrequiresPassword/isExistingUserinstead of brittle message inspection and sets the step/verificationType accordingly.handleMultiUserPasswordSubmitcleanly separates login vs signup, rebuilds the main client and callscheckAuth()after setting the token, and now surfaces partial-success SMTP failures viaresult.errorinstead of silently continuing.changeMultiUserVerificationCode+handleMultiUserCodeSubmituse a short debounce, cancel pending timeouts, and guard on_multiUserLoading, which should prevent duplicate verification requests from both auto-submit and button clicks.Overall this fixes the previous race conditions and incomplete multi-user wiring from earlier revisions.
Also applies to: 664-704, 702-790, 852-882, 1255-1271
rust-executor/src/graphql/mutation_resolvers.rs (2)
461-721: runtime_create_user: rate limiting, test mode, and SMTP edge cases are handled wellThe extended
runtime_create_userflow is solid:
- Multi-user enablement and duplicate-user checks remain early exits.
- Wallet/DID creation is preserved and saved when a passphrase is available.
- Rate limiting is applied before verification code creation (and skipped in test mode), with
update_rate_limitfailures treated as hard failures for the email step but not for user creation itself.- Test mode + dummy SMTP config are handled explicitly, and on non-test paths you now clean up verification codes on both SMTP failures and missing config, returning a clear
UserCreationResult.errorwhile keepingsuccess: trueso the frontend can allow password-only login.This is a good balance between security (rate limiting, cleanup) and UX (user always created, but informed if email couldn’t be sent).
970-1066: verify_email_code and email test helpers are consistent and well-scoped
runtime_verify_email_code:
- Checks
RUNTIME_USER_MANAGEMENT_VERIFY_CAPABILITYand multi-user mode first.- Delegates code validity and failed-attempt handling to
db.verify_code, returning specific errors when codes are invalidated vs. simply expired/incorrect.- Ensures the user key exists before issuing a JWT, and, like
runtime_login_user, uses user-scoped default capabilities when no existing token is present while preserving app context when a token exists.The
runtime_test_emailandruntime_email_test_*helpers:
- Are all gated behind
ALL_CAPABILITY, appropriate for admin-only test-mode operations.- Provide the right hooks for the JS tests (enable/disable test mode, get/clear codes, set expiry), with DB operations using the same
Ad4mDblocking pattern as the rest of the file.This wiring matches the test suite expectations and keeps powerful test knobs behind an admin capability.
Also applies to: 1068-1169
tests/js/tests/email-verification.test.ts (2)
1-310: Email verification E2E tests are comprehensive and now assert the right behaviorsThe suite does a good job:
- Global setup/teardown and per-test
emailTestClearCodes()keep state isolated.- Signup/login, invalid/expired codes, rate limiting, and single-use semantics are all exercised, including the new
emailTestSetExpiryhelper so expiration is actually validated.- The rate-limit test now checks
request2.success === falseand inspects the message rather than relying on a thrown error, fixing the earlier “passes for the wrong reason” issue.This gives strong coverage for the backend changes.
312-523: Security-focused tests around failed attempts and concurrency are well designedThe “Security Features” block validates:
- Unique code generation per request.
- Single-use behavior.
- Failed-attempt counting, including invalidation after 5 attempts, requirement for a new code, counter reset after a success, and concurrent failed attempts via
Promise.all.These scenarios map closely to the intended defense-in-depth behavior of the verification system and should catch regressions in the DB logic.
core/src/agent/AgentClient.ts (1)
16-16: AgentClient multi-user helpers correctly mirror the runtime GraphQL APIThe additions here look clean:
createUser(email, password, appInfo?)extends the existing mutation without breaking callers and forwardsappInfoas expected.requestLoginVerificationandverifyEmailCodewrap the new runtime mutations with the right argument names and field selections (success,message,requiresPassword,isExistingUserfor the former; token string for the latter).- Importing
VerificationRequestResultfromRuntimeResolverkeeps the TS types in sync with the GraphQL surface.No issues from the client-side wiring perspective.
Also applies to: 535-590
core/src/runtime/RuntimeResolver.ts (1)
211-224: LGTM! Well-structured verification result type.The
VerificationRequestResulttype is well-designed with clear fields that provide all necessary information for the verification flow (success status, message, password requirement, and user existence check).
| private async handleMultiUserCodeSubmit() { | ||
| // Guard against duplicate calls - if already loading, ignore this request | ||
| if (this._multiUserLoading) { | ||
| return; | ||
| } | ||
|
|
||
| // Cancel any pending auto-submit timeout since we're submitting now | ||
| if (this._codeSubmitTimeout !== null) { | ||
| clearTimeout(this._codeSubmitTimeout); | ||
| this._codeSubmitTimeout = null; | ||
| } | ||
|
|
||
| try { | ||
| this._multiUserLoading = true; | ||
| this._multiUserError = null; | ||
|
|
||
| // Use the existing connectMultiUser method with credentials | ||
| this._client.options.userEmail = this._multiUserEmail; | ||
| this._client.options.userPassword = this._multiUserPassword; | ||
| this._client.options.backendUrl = this.backendUrl; | ||
| // Build temporary client to call verifyEmailCode | ||
| const tempClient = this._client.buildTempClient(this.backendUrl); | ||
| const token = await tempClient.agent.verifyEmailCode( | ||
| this._multiUserEmail, | ||
| this._multiUserVerificationCode, | ||
| this._multiUserVerificationType | ||
| ); | ||
|
|
||
| // Success! Store token and authenticate | ||
| this._client.setToken(token); | ||
| this._client.setUrl(this.backendUrl); | ||
|
|
||
| const client = await this._client.connectMultiUser(); | ||
| // Rebuild client with new token to establish authenticated connection | ||
| await this._client.buildClient(); | ||
| await this._client.checkAuth(); | ||
|
|
||
| // Success! Clear form and close modal | ||
| // Clear form and close modal | ||
| this._multiUserEmail = ""; | ||
| this._multiUserPassword = ""; | ||
| this._multiUserVerificationCode = ""; | ||
| this._multiUserStep = "email"; | ||
| this._isOpen = false; | ||
| this.changeUIState("connected"); | ||
| this.handleAuthChange("authenticated"); | ||
| } catch (e) { | ||
| this._multiUserError = e.message || "Failed to log in. Please check your credentials."; | ||
| this._multiUserError = "Invalid or expired code. Please try again."; | ||
| this._multiUserVerificationCode = ""; | ||
| } finally { | ||
| this._multiUserLoading = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
UI state "connected" is still not part of the uiState union and error messaging is overly generic
changeUIState("connected") is used after successful multi-user login/code verification and in loginToHosting, but "connected" is not included in the uiState union type. This works at runtime but weakens type safety and makes it harder to reason about the UI state machine. Also, the catch block in handleMultiUserCodeSubmit still reports "Invalid or expired code" for all errors, including failures in buildClient() / checkAuth() after a successful verification, which can mislead users when a good code is consumed but connection fails.
Consider:
- Either adding
"connected"to theuiStateunion or replacing those calls with an existing, handled state (e.g. relying solely onconnectionStateand"requestcap"). - Preserving the original error (or at least checking for a clear verification failure vs. a post-verification connection failure) so you can show a more accurate message, while logging the raw error for debugging.
Also applies to: 840-850, 533-545
🤖 Prompt for AI Agents
In @connect/src/web.ts around lines 792 - 838, The code calls
changeUIState("connected") from handleMultiUserCodeSubmit and loginToHosting but
"connected" is not in the uiState union and the catch block masks
non-verification failures; update the uiState type to include "connected" or
change those calls to an existing state (e.g., use connectionState/requestcap),
and in handleMultiUserCodeSubmit distinguish verification errors from
post-verification failures by checking where the error came from (wrap
tempClient.agent.verifyEmailCode separately and only set _multiUserError =
"Invalid or expired code" on verification failure), otherwise preserve/log the
real error (e.g., store/log the caught error object) and show a different
user-facing message for connection/authentication failures while still resetting
_multiUserVerificationCode as appropriate; refer to handleMultiUserCodeSubmit,
changeUIState, loginToHosting, tempClient.agent.verifyEmailCode,
_client.buildClient, _client.checkAuth, and _multiUserError when making changes.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust-executor/src/graphql/mutation_resolvers.rs (1)
462-485: Add email validation before user creation.The email parameter is not validated for format correctness before being used to create a user key (line 489) or added to the database (line 535). Invalid email addresses could cause issues with key generation, database constraints, or email sending.
🔎 Recommended validation
Add email format validation at the beginning of the function:
async fn runtime_create_user( &self, context: &RequestContext, email: String, password: String, app_info: Option<AuthInfoInput>, ) -> FieldResult<UserCreationResult> { + // Validate email format + let email = email.trim().to_lowercase(); + if !email.contains('@') || !email.contains('.') { + return Ok(UserCreationResult { + did: String::new(), + success: false, + error: Some("Invalid email format".to_string()), + }); + } + // Check capability (empty tokens get user management caps in multi-user mode) check_capability(
🤖 Fix all issues with AI Agents
In @rust-executor/src/graphql/mutation_resolvers.rs:
- Around line 843-866: The current code performs db_ref.check_rate_limit(&email)
and then db_ref.update_rate_limit(&email) separately, creating a TOCTOU race
where concurrent requests can bypass the limit; fix by performing the
check-and-update atomically (either add and call a new db method like
check_and_update_rate_limit(&email) or run both calls inside a database
transaction while holding the db.lock() for the duration), replace the separate
check_rate_limit/update_rate_limit sequence with that atomic operation and
propagate the returned error into the existing VerificationRequestResult flow.
- Around line 809-834: Email addresses are not normalized causing case-sensitive
mismatches; update runtime_request_login_verification to normalize the incoming
email (e.g., trim and to_lowercase) immediately after receiving it and use that
normalized variable for all subsequent checks and DB lookups, and apply the same
normalization at the start of runtime_create_user and runtime_verify_email_code
so user creation, verification, and lookups consistently use the
lowercased/trimmed email.
- Around line 1000-1026: The runtime_verify_email_code function currently
accepts verification_type as an unchecked String; validate that
verification_type is either "login" or "signup" before proceeding and return a
FieldError for any other value. Locate runtime_verify_email_code and add a
simple match or if-check against the two allowed literals ("login", "signup");
if invalid, return Err(FieldError::new("Invalid verification_type, expected
'login' or 'signup'", graphql_value!(null))). Ensure this validation occurs
before calling verify_code or any DB queries that depend on verification_type.
🧹 Nitpick comments (1)
rust-executor/src/graphql/mutation_resolvers.rs (1)
1129-1148: Consider environment checks for test mode functions.The test mode functions are properly protected with ALL_CAPABILITY checks, but there's no verification that the system is running in a development or test environment. If test mode is accidentally enabled in production, it bypasses rate limiting (line 577 in
runtime_create_user) and alters email behavior, which could be a security risk.💡 Suggested environment guard
Consider adding an environment check to prevent accidental test mode activation in production:
async fn runtime_email_test_mode_enable(&self, context: &RequestContext) -> FieldResult<bool> { use crate::agent::capabilities::ALL_CAPABILITY; // Check capability - admin only check_capability(&context.capabilities, &ALL_CAPABILITY)?; + // Prevent enabling test mode in production + if std::env::var("AD4M_ENVIRONMENT").unwrap_or_default() == "production" { + return Err(FieldError::new( + "Test mode cannot be enabled in production environment", + graphql_value!(null), + )); + } + crate::email_service::enable_test_mode(); Ok(true) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust-executor/src/graphql/mutation_resolvers.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T15:16:29.623Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 619
File: rust-executor/src/perspectives/perspective_instance.rs:639-641
Timestamp: 2025-09-03T15:16:29.623Z
Learning: In the AD4M codebase, database operations like `add_many_links` during critical synchronization processes (such as `diff_from_link_language`) should use `.expect()` rather than error logging, as failures indicate severe system issues where fail-fast behavior is preferred over continuing with potentially corrupted state.
Applied to files:
rust-executor/src/graphql/mutation_resolvers.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
rust-executor/src/graphql/mutation_resolvers.rs (1)
768-792: LGTM! User-scoped capabilities properly implemented.The fallback auth info now correctly uses
get_user_default_capabilities()instead ofALL_CAPABILITY, ensuring that users logging in without a pre-existing auth token receive appropriate user-level permissions rather than unrestricted admin access. This properly scopes the JWT to user capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust-executor/src/graphql/mutation_resolvers.rs (1)
715-799: Normalize email in password login to match signup/verification behavior
runtime_create_user,runtime_request_login_verification, andruntime_verify_email_codeall normalize email (trim().to_lowercase()), butruntime_login_userstill uses the rawTo keep behavior consistent and avoid subtle login failures, normalize here as well:
Proposed change to normalize email in
runtime_login_userasync fn runtime_login_user( &self, context: &RequestContext, email: String, password: String, ) -> FieldResult<String> { + // Normalize email to match signup and verification flows + let email = email.trim().to_lowercase(); + // Check capability (empty tokens get login capability in multi-user mode) check_capability( &context.capabilities,
🧹 Nitpick comments (2)
rust-executor/src/agent/mod.rs (1)
390-391: LGTM! Appropriate log level adjustment.Changing from
debug!totrace!for these detailed diagnostic messages is correct. These logs track fine-grained execution flow (user lookups and key discovery) and fire frequently, makingtrace!the appropriate level to reduce noise in debug logs while preserving troubleshooting capability.Also applies to: 407-407
rust-executor/src/graphql/mutation_resolvers.rs (1)
1085-1185: Admin-only email test helpers are correct; consider normalizing email for test utilitiesThe new test/admin mutations:
runtime_test_emailruntime_email_test_mode_enable/disableruntime_email_test_get_coderuntime_email_test_clear_codesruntime_email_test_set_expiryare all correctly gated behind
ALL_CAPABILITY, and they use the same DB/email_service helpers as the main flows, which is good for end-to-end testing fidelity.Two minor consistency tweaks you might consider:
- Normalize the
runtime_email_test_get_codeandruntime_email_test_set_expirythe same way as in the main mutations, so test utilities behave identically regardless of email casing.- Optionally, constrain
verification_typeinruntime_email_test_set_expiryto the expected values to mirror the main flows.These are non-blocking and mainly about avoiding surprises in tests.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust-executor/src/agent/mod.rsrust-executor/src/db.rsrust-executor/src/graphql/mutation_resolvers.rs
🧰 Additional context used
🧬 Code graph analysis (2)
rust-executor/src/graphql/mutation_resolvers.rs (4)
rust-executor/src/agent/mod.rs (3)
did(143-145)with_global_instance(277-288)global_instance(273-275)rust-executor/src/db.rs (2)
with_global_instance(1534-1543)global_instance(64-66)rust-executor/src/email_service.rs (4)
enable_test_mode(48-53)disable_test_mode(56-61)get_test_code(65-85)clear_test_codes(88-92)rust-executor/src/agent/capabilities/defs.rs (1)
get_user_default_capabilities(485-545)
rust-executor/src/db.rs (1)
rust-executor/src/utils.rs (1)
constant_time_eq(25-55)
🔇 Additional comments (4)
rust-executor/src/graphql/mutation_resolvers.rs (2)
461-707: Signup email verification and rate-limiting flow look soundThe updated
runtime_create_userflow is coherent:
- Email is normalized up front, matching the DB/user-key lookups.
- Multi‑user gating happens before any side effects.
- You generate per‑user keys and persist the wallet when a passphrase is available, with a clear warning when not.
- Rate limiting for signup verification uses the new atomic
check_and_update_rate_limitand is skipped in test mode, which avoids TOCTOU and test fragility.- Verification codes are created via the DB helper, and you correctly:
- send emails only when SMTP or test‑mode is available,
- clean up verification codes on SMTP absence or send failure, and
- return informative, non-fatal messages while keeping
success: truewhen the user is created but email isn't sent.No issues to block here; the behavior aligns with the multi-user + email-verification goals.
801-982: Login verification now correctly rate-limited and cleans up codes on failureThe new
runtime_request_login_verificationimplementation looks robust:
- Email normalization is applied consistently.
- Rate limiting is enforced via the atomic
check_and_update_rate_limitbefore any user existence checks, which both prevents enumeration at high volume and closes the prior TOCTOU window between check and update.- You distinguish new vs existing users after the rate limit, returning a clear
requires_passwordsignal for new users.- Verification codes are created via the DB helper, and you:
- use test mode + dummy SMTP config to capture codes without requiring real SMTP,
- clean up codes when SMTP is unconfigured, and
- clean up codes on email send failures (except in test mode) while surfacing a proper error.
This addresses the earlier concerns around enumeration, rate-limit bypass, and orphaned codes.
rust-executor/src/db.rs (2)
262-300: Email verification and rate-limit schema is well structuredThe new schema pieces look solid:
email_verificationscapturescode_hash,created_at,expires_at,verification_typeandfailed_attempts, with:
- a
UNIQUE(email, verification_type)constraint to ensure at most one active code per email/type, and- indexes on
expires_atto support lookups and periodic cleanup efficiently.email_rate_limitsuses- The
ALTER TABLE ... ADD COLUMN failed_attemptsguarded by ignoring the result cleanly handles upgrades: new databases get the column fromCREATE TABLE, and existing ones get it fromALTER TABLEwithout breaking startup if it already exists.No blocking issues here; the schema matches the intended verification + rate-limiting design.
2415-2710: Hashed verification codes, constant-time comparison, and atomic rate limiting are implemented correctlyThe new email verification and rate-limiting helpers form a coherent, secure core:
create_verification_code:
- Generates a 6‑digit code in
[100000, 999999].- Stores only a SHA‑256 hash (
code_hash) plus timestamps, never the plaintext code.- Enforces single active code per
(email, verification_type)by deleting any prior row before insert.- Sets a 15‑minute TTL.
verify_code:
- Recomputes
code_hashand compares it to the stored hash usingconstant_time_eq, addressing earlier timing‑attack concerns.- Rejects and deletes entries that are expired or have reached
MAX_FAILED_ATTEMPTS, and incrementsfailed_attemptson each mismatch.- Returns
Ok(false)for simple mismatches/expiry and anErrwhen a code has been invalidated due to too many attempts, which the GraphQL layer surfaces distinctly.
cleanup_expired_codesanddelete_verification_codeensure bounded table growth and explicit cleanup on email failures/SMTP absence.
has_verification_codeprovides a simple "any active code" check over non-expired, non-verified rows.
check_rate_limit,update_rate_limit, and especiallycheck_and_update_rate_limit:
- Implement a 60‑second cooldown per email.
- Use a transaction in
check_and_update_rate_limitto atomically check and update, which closes the earlier TOCTOU gap where concurrent requests could slip past the limit.
set_verification_code_expirygives a targeted test helper with a clear error when no matching code exists.Overall, the DB layer now cleanly supports the intended security properties (no plaintext codes, constant-time comparison, bounded retries, and robust rate limiting).
| is_existing_user: false, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Rate limit blocks signup verification email for new users
Medium Severity
The signup flow for new users has conflicting rate limiting. When a new user enters their email, runtime_request_login_verification calls check_and_update_rate_limit which updates the timestamp. When the user immediately submits their password (a few seconds later), runtime_create_user also calls check_and_update_rate_limit, which fails because 60 seconds haven't elapsed. This results in the user account being created successfully but no verification email being sent, with a confusing error about rate limiting. The user must then wait ~55 seconds before they can request a new verification email.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust-executor/src/graphql/mutation_resolvers.rs (1)
571-596: Consider checking rate limit before creating user in database.The rate limit is checked after the user is created in the database (line 538). If the rate limit is exceeded, the function returns success with an error message indicating the verification email wasn't sent. This means the user account exists but is unverified and the user must wait ~60 seconds before requesting another verification email.
While this behavior is functionally correct (the user can still login with password), it could be confusing for users who receive a "rate limit exceeded" error after account creation. Consider checking the rate limit earlier in the flow (before line 538) to fail fast and avoid creating unverified accounts that immediately hit rate limiting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust-executor/src/graphql/mutation_resolvers.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rust-executor/src/graphql/mutation_resolvers.rs (3)
rust-executor/src/email_service.rs (4)
enable_test_mode(48-53)disable_test_mode(56-61)get_test_code(65-85)clear_test_codes(88-92)rust-executor/src/db.rs (2)
with_global_instance(1534-1543)global_instance(64-66)rust-executor/src/agent/capabilities/defs.rs (1)
get_user_default_capabilities(485-545)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
rust-executor/src/graphql/mutation_resolvers.rs (3)
649-663: Verification code cleanup logic looks correct.The cleanup of verification codes when email sending fails (lines 649-663) or SMTP is not configured (lines 684-695) properly addresses previous review comments. The exception for test mode (line 653) is correct since test mode captures codes in memory for testing purposes.
The error messages clearly communicate the state to users, and users can still login with password even if verification emails aren't sent.
Also applies to: 684-695
1097-1198: Test helper functions properly secured.All test helper functions correctly require
ALL_CAPABILITY(admin only), which is appropriate given they can manipulate verification codes and test mode. The implementations are straightforward and delegate properly to the email_service module.Note:
runtime_test_email(lines 1097-1125) sends real emails regardless of test mode, which is correct behavior for an admin tool to verify SMTP configuration.
834-879: Enumeration protection is limited to per-email rate limiting; different email addresses can be enumerated.For new users (lines 841-863), the rate limit is checked but not updated. This prevents the conflict where both
runtime_request_login_verificationandruntime_create_userwould update the rate limit within seconds of each other.However, since rate limits are per-email only (no IP-based or global rate limiting exists), an attacker can enumerate whether different email addresses are registered by observing
is_existing_userin the response. The rate limit only prevents repeatedly checking the same email address.The comment at line 844 states "This prevents user enumeration," but the implementation only prevents repeated enumeration of a single email—not enumeration of multiple distinct emails. Please confirm this is the intended trade-off.
| check_capability(&context.capabilities, &ALL_CAPABILITY)?; | ||
|
|
||
| Ok(crate::email_service::get_test_code(&email)) | ||
| } |
There was a problem hiding this comment.
Test helpers missing email normalization causing lookup failures
Low Severity
The test helper functions runtime_email_test_get_code and runtime_email_test_set_expiry do not normalize the email parameter (trim and lowercase), while the main functions runtime_create_user and runtime_request_login_verification do. When a verification code is created, the email is stored normalized (e.g., "test@example.com"). If emailTestGetCode is called with mixed-case email (e.g., "Test@Example.com"), the lookup will fail because the key doesn't match. This inconsistency causes test code retrieval to return null when emails are provided with different casing than during creation.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @rust-executor/src/graphql/mutation_resolvers.rs:
- Around line 999-1028: The runtime_verify_email_code handler currently accepts
any string for verification_type; add validation to allow only "login" or
"signup" before proceeding (e.g., at start of runtime_verify_email_code after
normalizing email and checking capabilities). If verification_type is not one of
those two values, return an appropriate FieldError similar to other errors.
Ensure this validated value is then passed to downstream calls like
db.verify_code() so only accepted types reach database logic.
🧹 Nitpick comments (1)
rust-executor/src/graphql/mutation_resolvers.rs (1)
535-596: Consider checking rate limit before creating the user.The current flow creates the user account (line 538) before checking the rate limit (line 585). If the rate limit check fails, the user exists in the database but hasn't received a verification email and must wait 60 seconds before requesting another. This creates a confusing UX where the account exists but is in limbo.
Consider reordering to check the rate limit earlier (right after the existence check at line 532 and before user creation at line 535), so that rate-limited requests fail without creating the account. This way, users aren't left with orphaned accounts that can't be verified.
🔎 Proposed refactor
if existing_user.is_some() { return Ok(UserCreationResult { did: String::new(), success: false, error: Some("User already exists".to_string()), }); } + // Check if test mode is enabled + let test_mode = crate::email_service::EMAIL_TEST_MODE + .lock() + .ok() + .map(|mode| *mode) + .unwrap_or(false); + + // Apply rate limiting before creating user (but skip in test mode) + if !test_mode { + let db_lock = db.lock().expect("Couldn't get lock on Ad4mDb"); + let db_ref = db_lock.as_ref().expect("Ad4mDb not initialized"); + + if let Err(e) = db_ref.check_and_update_rate_limit(&email) { + log::warn!("Rate limit exceeded for signup: {}", e); + return Ok(UserCreationResult { + did: String::new(), + success: false, + error: Some(e.to_string()), + }); + } + } + // Add user to database with hashed password { let db_lock = db.lock().expect("Couldn't get lock on Ad4mDb"); let db_ref = db_lock.as_ref().expect("Ad4mDb not initialized"); db_ref.add_user(&email, &did, &password).map_err(|e| { FieldError::new(format!("Failed to add user: {}", e), graphql_value!(null)) })?; } // ... (rest of user creation flow) - // Check if test mode is enabled (we check this early to use in rate limiting) - let test_mode = crate::email_service::EMAIL_TEST_MODE - .lock() - .ok() - .map(|mode| *mode) - .unwrap_or(false); - - // Apply rate limiting before generating verification code (but skip in test mode) - if !test_mode { - let db_lock = db.lock().expect("Couldn't get lock on Ad4mDb"); - let db_ref = db_lock.as_ref().expect("Ad4mDb not initialized"); - - if let Err(e) = db_ref.check_and_update_rate_limit(&email) { - log::warn!("Rate limit exceeded for signup verification: {}", e); - return Ok(UserCreationResult { - did, - success: true, - error: Some(format!( - "User created successfully but verification email was not sent due to rate limiting: {}", - e - )), - }); - } - }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust-executor/src/graphql/mutation_resolvers.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rust-executor/src/graphql/mutation_resolvers.rs (3)
rust-executor/src/email_service.rs (5)
new(99-101)enable_test_mode(48-53)disable_test_mode(56-61)get_test_code(65-85)clear_test_codes(88-92)rust-executor/src/db.rs (1)
new(68-320)rust-executor/src/agent/capabilities/defs.rs (1)
get_user_default_capabilities(485-545)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
rust-executor/src/graphql/mutation_resolvers.rs (3)
715-802: LGTM! Password-based login implementation looks solid.The function properly normalizes email addresses, validates credentials, and generates JWT tokens with appropriate user-scoped capabilities rather than admin-level access.
804-997: Well-designed rate limiting handles signup flow correctly.The differentiated rate limiting approach properly addresses the signup flow issue flagged in earlier reviews:
- For new users (lines 845-858): Uses
check_rate_limitwithout updating the timestamp, preventing enumeration while avoiding rate limit conflicts when the user subsequently submits their password- For existing users (lines 869-882): Uses atomic
check_and_update_rate_limitsince an email will actually be sentCleanup paths for failed email delivery (lines 973-983) and unconfigured SMTP (lines 935-946) are handled correctly, with appropriate test-mode exceptions.
1100-1211: Test helper functions are well-implemented.All test utilities correctly:
- Enforce admin-only access via
ALL_CAPABILITYchecks- Apply email normalization consistently (lines 1166, 1198)
- Provide useful testing functionality for the verification system
The test mode design allows automated testing without actually sending emails, which is a good practice.
| async fn runtime_verify_email_code( | ||
| &self, | ||
| context: &RequestContext, | ||
| email: String, | ||
| code: String, | ||
| verification_type: String, | ||
| ) -> FieldResult<String> { | ||
| use crate::agent::capabilities::{ | ||
| get_user_default_capabilities, token, AuthInfo, DEFAULT_TOKEN_VALID_PERIOD, | ||
| }; | ||
|
|
||
| // Normalize email: trim whitespace and convert to lowercase | ||
| let email = email.trim().to_lowercase(); | ||
|
|
||
| // Check capability | ||
| check_capability( | ||
| &context.capabilities, | ||
| &RUNTIME_USER_MANAGEMENT_VERIFY_CAPABILITY, | ||
| )?; | ||
|
|
||
| // Check if multi-user mode is enabled | ||
| let multi_user_enabled = | ||
| Ad4mDb::with_global_instance(|db| db.get_multi_user_enabled().unwrap_or(false)); | ||
|
|
||
| if !multi_user_enabled { | ||
| return Err(FieldError::new( | ||
| "Multi-user mode is not enabled", | ||
| graphql_value!(null), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Add validation for verification_type parameter.
The verification_type parameter (line 1004) accepts any string without validation. Only "login" and "signup" are valid values, and passing invalid values could cause unexpected behavior in downstream code (e.g., db.verify_code() at line 1035).
🔎 Suggested validation
async fn runtime_verify_email_code(
&self,
context: &RequestContext,
email: String,
code: String,
verification_type: String,
) -> FieldResult<String> {
use crate::agent::capabilities::{
get_user_default_capabilities, token, AuthInfo, DEFAULT_TOKEN_VALID_PERIOD,
};
// Normalize email: trim whitespace and convert to lowercase
let email = email.trim().to_lowercase();
+ // Validate verification_type
+ if verification_type != "login" && verification_type != "signup" {
+ return Err(FieldError::new(
+ "Invalid verification_type. Must be 'login' or 'signup'",
+ graphql_value!(null),
+ ));
+ }
+
// Check capability
check_capability(
&context.capabilities,
&RUNTIME_USER_MANAGEMENT_VERIFY_CAPABILITY,
)?;📝 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.
| async fn runtime_verify_email_code( | |
| &self, | |
| context: &RequestContext, | |
| email: String, | |
| code: String, | |
| verification_type: String, | |
| ) -> FieldResult<String> { | |
| use crate::agent::capabilities::{ | |
| get_user_default_capabilities, token, AuthInfo, DEFAULT_TOKEN_VALID_PERIOD, | |
| }; | |
| // Normalize email: trim whitespace and convert to lowercase | |
| let email = email.trim().to_lowercase(); | |
| // Check capability | |
| check_capability( | |
| &context.capabilities, | |
| &RUNTIME_USER_MANAGEMENT_VERIFY_CAPABILITY, | |
| )?; | |
| // Check if multi-user mode is enabled | |
| let multi_user_enabled = | |
| Ad4mDb::with_global_instance(|db| db.get_multi_user_enabled().unwrap_or(false)); | |
| if !multi_user_enabled { | |
| return Err(FieldError::new( | |
| "Multi-user mode is not enabled", | |
| graphql_value!(null), | |
| )); | |
| } | |
| async fn runtime_verify_email_code( | |
| &self, | |
| context: &RequestContext, | |
| email: String, | |
| code: String, | |
| verification_type: String, | |
| ) -> FieldResult<String> { | |
| use crate::agent::capabilities::{ | |
| get_user_default_capabilities, token, AuthInfo, DEFAULT_TOKEN_VALID_PERIOD, | |
| }; | |
| // Normalize email: trim whitespace and convert to lowercase | |
| let email = email.trim().to_lowercase(); | |
| // Validate verification_type | |
| if verification_type != "login" && verification_type != "signup" { | |
| return Err(FieldError::new( | |
| "Invalid verification_type. Must be 'login' or 'signup'", | |
| graphql_value!(null), | |
| )); | |
| } | |
| // Check capability | |
| check_capability( | |
| &context.capabilities, | |
| &RUNTIME_USER_MANAGEMENT_VERIFY_CAPABILITY, | |
| )?; | |
| // Check if multi-user mode is enabled | |
| let multi_user_enabled = | |
| Ad4mDb::with_global_instance(|db| db.get_multi_user_enabled().unwrap_or(false)); | |
| if !multi_user_enabled { | |
| return Err(FieldError::new( | |
| "Multi-user mode is not enabled", | |
| graphql_value!(null), | |
| )); | |
| } |
🤖 Prompt for AI Agents
In @rust-executor/src/graphql/mutation_resolvers.rs around lines 999 - 1028, The
runtime_verify_email_code handler currently accepts any string for
verification_type; add validation to allow only "login" or "signup" before
proceeding (e.g., at start of runtime_verify_email_code after normalizing email
and checking capabilities). If verification_type is not one of those two values,
return an appropriate FieldError similar to other errors. Ensure this validated
value is then passed to downstream calls like db.verify_code() so only accepted
types reach database logic.
| graphql_value!(null), | ||
| ) | ||
| })? | ||
| }; |
There was a problem hiding this comment.
Inconsistent error handling after successful user creation
Medium Severity
In runtime_create_user, when create_verification_code fails (lines 599-610), the function throws a FieldError via the ? operator. However, at this point the user has already been successfully added to the database (line 538). This is inconsistent with how other post-creation failures are handled—rate limiting, email sending failure, and SMTP not configured all return UserCreationResult with success: true and an error message. This inconsistency causes user confusion: the client sees an error and thinks account creation failed, but the user actually exists. On retry, they get "User already exists".
2 participants
Summary
Implements a complete email verification system for AD4M's multi-user mode, enabling secure passwordless login and user registration via email verification codes. This PR adds SMTP configuration, email templates, verification code management, and integrates app branding into verification emails.
Overview
This PR introduces email-based authentication as the primary method for multi-user AD4M instances. Users can now sign up and log in using their email address, receiving beautifully designed verification emails with 6-digit codes. The system supports both signup and login flows with proper security measures including code expiration and single-use enforcement.
Key Features
📧 Email Verification System
🔐 Multi-User Authentication Flows
🎨 Branded Email Templates
⚙️ SMTP Configuration UI
🔌 AuthInfo Passthrough
Technical Implementation
Backend (Rust)
New Email Service (
rust-executor/src/email_service.rs):GraphQL Mutations (
rust-executor/src/graphql/mutation_resolvers.rs):runtimeCreateUser(email, password, appInfo)- User registration with email verificationruntimeRequestLoginVerification(email, appInfo)- Login verification requestruntimeVerifyEmailCode(email, code, verificationType)- Code verification and JWT issuanceConfiguration (
rust-executor/src/config.rs):SmtpConfigwith host, port, username, password, from_addressMultiUserConfigalongside TLS settingsSecurity Features:
Frontend (TypeScript)
Settings UI (
ui/src/components/Settings.tsx):Tauri Commands (
ui/src-tauri/src/commands/app.rs):get_smtp_config()- Retrieve current SMTP settingsset_smtp_config(config)- Save SMTP configuration (preserves TLS config!)test_smtp_config(config, testEmail)- Send test emailAd4m-Connect Integration (
connect/src/web.ts):handleMultiUserEmailSubmit()- Passes app info to login verificationhandleMultiUserPasswordSubmit()- Passes app info to user creationCore API (
core/src/agent/AgentClient.ts,core/src/runtime/RuntimeResolver.ts):appInfoparameterAuthInfoInputBug Fixes
Critical Fix: TLS Configuration Preservation
Issue: Saving SMTP settings would wipe out TLS configuration, breaking remote connections.
Root Cause:
set_smtp_configwas creating newMultiUserConfigwithtls_config: None.Solution:
get_tls_configandset_tls_configto usemulti_user_configas primary sourcetls_configfieldSMTP Connection Error Fix
Issue: TLS connection errors with all SMTP providers (
error:0A00010B:SSL routines:tls_validate_record_header:wrong version number).Root Cause: Default
SmtpTransport::relay()TLS behavior didn't match port requirements.Solution: Port-based TLS mode selection:
Tls::Wrapper(implicit TLS/SSL)Tls::Required(STARTTLS)Tls::None(plain text)Email Templates
Login Email Example
Subject: Your AD4M Login Verification Code
Signup Email Example
Subject: Complete Your AD4M Account Registration
Similar format, but with signup-specific messaging
Configuration Example
SMTP Settings (Settings UI)
SMTP Host: smtp.mailgun.org
Port: 587
Username: postmaster@mg.example.com
Password: ••••••••••
From Address: noreply@example.com
Manual Testing
Test Mode
Used in integration tests to mock email service.
// Enable test mode
email_service::enable_test_mode();
// Get captured code for testing
let code = email_service::get_test_code("user@example.com");
Security Considerations
Note
Introduces email-based auth for multi-user with SMTP-delivered 6‑digit codes and a step-driven UI flow.
lettre), DB tables for verification codes and rate limits, constant‑time compares, background cleanup, and GraphQL mutations:runtimeRequestLoginVerification,runtimeVerifyEmailCode,runtimeCreateUser(withappInfo); adds user‑scoped default capabilitiesSmtpConfigand global wiring; updates executor/launcher depsAgentClient/RuntimeClientmethods for request/verify and email test helpers; deprecates password-onlyconnectMultiUseremail → password → code(MultiUserAuth+web.tshandlers)integration-tests-email-verification)Written by Cursor Bugbot for commit 30c6f5f. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.