[WIP] Fix preview logout issue

[faygade]

No description provided.

[claude]

Claude finished @faygade's task —— View job

---

PR Review: Fix preview logout issue

Summary

This PR addresses a redirect loop issue in the authentication module by adding logic to detect when the nextUrl parameter is already a login URL and extracting the original destination URL from it.

Code Quality and Best Practices ✅

• Good error handling: The try-catch block gracefully handles URL parsing failures
• Clear comments: The added comments explain the purpose of the redirect loop prevention
• Follows existing patterns: The code maintains consistency with the existing codebase style
• Minimal changes: The fix is focused and doesn't introduce unnecessary complexity

Potential Bugs or Issues ⚠️

1. Incomplete URL validation: The check parsedUrl.pathname.endsWith("/login") might miss login URLs with trailing slashes or query parameters in the path. Consider using a more robust check:

   if (parsedUrl.pathname.endsWith("/login") || parsedUrl.pathname.endsWith("/login/"))

2. Missing validation for extracted URL: The originalFromUrl extracted from the query parameter is used directly without validation. This could potentially lead to open redirect vulnerabilities if an attacker crafts a malicious from_url parameter.

Performance Considerations ✅

• The URL parsing is wrapped in a try-catch block and only executes when needed
• No performance concerns - the added logic is minimal and executes synchronously

Security Concerns 🔒

1. Open Redirect Vulnerability: The current implementation trusts the from_url parameter without validation. An attacker could craft a login URL with a malicious from_url pointing to an external site:

   https://app.com/login?from_url=https://evil.com
   

   Recommendation: Add URL validation to ensure the redirect URL is either:

   • A relative URL
   • An absolute URL with the same origin
   • An explicitly allowed external domain

   Example fix:

   // Validate the extracted URL
   const isValidRedirect = (url: string): boolean => {
     try {
       const parsed = new URL(url, window.location.origin);
       // Allow same-origin URLs or configured trusted domains
       return parsed.origin === window.location.origin || 
              trustedDomains.includes(parsed.origin);
     } catch {
       // Relative URLs are allowed
       return url.startsWith('/');
     }
   };
   
   if (originalFromUrl && isValidRedirect(originalFromUrl)) {
     redirectUrl = originalFromUrl;
   }

Test Coverage ✅

• Good test coverage: The PR includes a comprehensive test case that verifies the redirect loop prevention logic
• Well-structured test: Uses proper setup/teardown with beforeEach and afterEach
• Clear test description: The test name clearly describes what is being tested

Additional Recommendations

1. Add more test cases for edge cases:

   • Login URLs with trailing slashes
   • Invalid from_url parameters
   • Relative URLs in from_url
   • Security validation tests

2. Consider logging: When a redirect loop is prevented, it might be helpful to log this event for debugging purposes

3. TypeScript improvements: Since this is a .ts file, consider adding more specific types for better type safety

Conclusion

The PR successfully addresses the redirect loop issue, but introduces a potential security vulnerability that should be addressed before merging. The open redirect issue is critical and needs URL validation to ensure secure redirects.

[github-actions]

🚀 Package Preview Available!

---

Install this PR's preview build with npm:

npm i @base44-preview/sdk@0.8.18-pr.116.605725a

Prefer not to change any import paths? Install using npm alias so your code still imports @base44/sdk:

npm i "@base44/sdk@npm:@base44-preview/sdk@0.8.18-pr.116.605725a"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/sdk@0.8.18-pr.116.605725a"
  }
}

• 📦 Preview Package: @base44-preview/sdk@0.8.18-pr.116.605725a
• 🔗 View this commit on GitHub

---

_{Preview published to npm registry — try new features instantly!}

[claude]

Claude finished @faygade's task —— View job

---

PR Review: Fix preview logout issue

Summary

This PR has been updated to address the security concerns from the previous review. The implementation now includes proper URL validation to prevent open redirect attacks while maintaining the redirect loop prevention functionality.

Code Quality and Best Practices ✅

• Excellent documentation: Added comprehensive JSDoc comments for the new isValidRedirectUrl function
• Clean, modular code: The validation logic is extracted into a dedicated function, following single responsibility principle
• Follows TypeScript best practices: Proper type annotations and error handling
• Maintains consistency: The code style aligns with the existing codebase

Security Review ✅

Previous vulnerability has been addressed!

The implementation now includes robust URL validation in the isValidRedirectUrl function that:

• ✅ Allows relative URLs starting with / (but not // to prevent protocol-relative attacks)
• ✅ Validates absolute URLs to ensure same-origin only
• ✅ Properly handles URL parsing failures by rejecting invalid URLs
• ✅ Prevents open redirect attacks by validating the extracted from_url parameter

The security implementation is well-thought-out and covers edge cases properly.

Potential Bugs or Issues ✅

The previous issue with incomplete URL validation has been resolved:

• The code now checks for both /login and /login/ endpoints
• URL validation is comprehensive and handles edge cases gracefully

Performance Considerations ✅

• URL parsing is minimal and only happens when needed
• The validation function is lightweight and won't impact performance
• No concerns about performance impact

Test Coverage ⭐ Excellent

The test suite is comprehensive and includes:

• ✅ Basic redirect loop prevention scenarios
• ✅ Tests for login URLs with trailing slashes
• ✅ Security tests for cross-origin redirect attempts
• ✅ Tests for protocol-relative URLs (//evil.com)
• ✅ Tests for relative URLs (allowed case)
• ✅ Proper test setup/teardown with mocked window objects

The security-focused tests demonstrate a thorough understanding of potential attack vectors.

Additional Positive Observations

1. Server-side compatibility: The isValidRedirectUrl function checks for typeof window !== "undefined" before accessing window.location.origin, ensuring it won't break in SSR contexts
2. Conservative security stance: The validation rejects URLs by default if parsing fails, which is the correct approach
3. Clear separation of concerns: The validation logic is isolated and testable

Minor Suggestions for Future Improvements

1. Consider adding debug logging when a redirect is rejected for security reasons (could help with troubleshooting)
2. The validation could potentially be extended to support a whitelist of trusted external domains if needed in the future

Conclusion ✅

This is a well-executed fix that successfully addresses both the original redirect loop issue and the security vulnerability identified in the previous review. The code is clean, secure, and thoroughly tested. Great work addressing the feedback!

Ready to merge once CI checks pass.