redirect to login on app base url

[netanelgilad]

No description provided.

[github-actions]

🚀 Package Preview Available!

---

Install this PR's preview build with npm:

npm i @base44-preview/sdk@0.7.10-pr.39.3c96213

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.7.10-pr.39.3c96213"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/sdk@0.7.10-pr.39.3c96213"
  }
}

• 📦 Preview Package: @base44-preview/sdk@0.7.10-pr.39.3c96213
• 🔗 View this commit on GitHub

---

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

[claude]

Claude finished @netanelgilad's task —— View job

---

Pull Request Review

I've completed my review of this PR that changes how login redirects work in the SDK.

Review Checklist

• Gather context and understand the changes
• Review code quality and best practices
• Identify potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage
• Provide comprehensive feedback

Summary

This PR modifies the login redirect behavior to use an appBaseUrl configuration option instead of the server URL. The changes remove authentication handling from the axios client and simplify the redirect logic.

Code Quality and Best Practices ✅

Positive aspects:

• Clean removal of unnecessary code from axios-client.ts
• Good documentation update in JSDoc comments (src/client.ts:21)
• Proper removal of unused parameters and callbacks
• Consistent code style maintained

Suggestions:

• Consider adding JSDoc documentation for the appBaseUrl parameter in the auth module

Potential Bugs or Issues 🐛

1. Breaking Change: The login redirect URL format has changed from ${serverUrl}/login?from_url=...&app_id=... to ${appBaseUrl}/login?from_url=.... This is a breaking change that could affect existing applications.

2. Missing app_id parameter: The new implementation removes the app_id query parameter from the login URL. If the login page relies on this parameter, it could cause issues.

3. Default behavior concern: When appBaseUrl is not provided, the redirect uses a relative URL /login?from_url=.... This assumes the application is hosted at the same domain, which might not always be the case.

Performance Considerations ⚡

• Good removal of the 100ms setTimeout in axios error handling
• No performance concerns identified
• The changes actually improve performance slightly by removing unnecessary authentication checks from the axios interceptor

Security Concerns 🔒

1. Open redirect vulnerability: The redirectToLogin function constructs URLs using user-provided input without validation. Consider adding URL validation to prevent open redirect attacks:

   const redirectUrl = nextUrl
     ? new URL(nextUrl, window.location.origin).toString()
     : window.location.href;

   This code could be exploited if nextUrl contains a malicious URL.

2. Recommendation: Add a whitelist of allowed redirect domains or validate that the redirect URL belongs to your application's domain.

Test Coverage ✅

Excellent test coverage:

• Tests properly updated to reflect the new behavior
• New test cases added for appBaseUrl configuration
• Edge cases covered (with and without appBaseUrl)
• Good test organization and clarity

Minor suggestion:

• Consider adding a test for the potential open redirect vulnerability scenario

Overall Assessment

The implementation is clean and the test coverage is comprehensive. However, there are important considerations:

1. This is a breaking change that needs to be documented in release notes
2. The removal of app_id from the login URL might affect existing login pages
3. Security validation for redirect URLs should be added

Recommendations

1. Add URL validation in redirectToLogin to prevent open redirects
2. Document the breaking change clearly
3. Consider making this change backwards compatible by supporting both old and new URL formats
4. Add migration guide for users upgrading to this version

---