Use popup for OAuth on preview/sandbox/checkpoint domains

[roymiloh]

Summary

• On preview/sandbox/checkpoint domains (preview--*, preview-sandbox--*, checkpoint--* on base44.app) the app runs inside an iframe
• OAuth providers block their login pages from loading in iframes via X-Frame-Options: DENY, so the existing full-page redirect breaks
• This PR opens the OAuth flow in a centered popup instead, polls until the popup lands back on our origin, then mirrors the callback URL (including ?access_token=...) in the iframe so it processes the token exactly as a normal redirect would

Test plan

• Trigger loginWithProvider('google') on a preview--*.base44.app domain — should open a popup, complete OAuth, close popup, and authenticate the iframe
• Verify normal (non-preview) domains still use the full-page redirect
• Verify popup-blocked scenario silently no-ops (no redirect fallback, since redirecting the iframe would also fail)

🤖 Generated with Claude Code

[claude]

Claude finished @roymiloh's task —— View job

---

PR Review: Use popup for OAuth on preview/sandbox/checkpoint domains

I've reviewed this PR that adds popup-based OAuth authentication for iframe environments. Here's my comprehensive feedback:

✅ Code Quality and Best Practices

Strengths:

• Clean, well-documented implementation with clear JSDoc comments
• Good separation of concerns with dedicated functions (isPopupAuthDomain, loginViaPopup)
• Follows existing code patterns and conventions in the auth module
• Regex pattern is well-defined and specific to the required domains

Suggestions:

• Consider extracting the popup dimensions (500x600) to constants for easier configuration
• The "base44_auth" window name could also be a named constant

🔍 Potential Bugs or Issues

Found Issues:

1. Memory leak risk: The setInterval continues running if the popup is manually closed by the user. While you do check popup.closed, consider also implementing a max timeout to prevent infinite polling
2. No error handling for popup blockers: When window.open returns null (line 38-40), the function silently returns without any user feedback
3. Race condition: Between checking popup.location.href and closing the popup (lines 54-56), the popup could navigate away

Recommendations:

// Add max timeout protection
const maxPollTime = 5 * 60 * 1000; // 5 minutes
const startTime = Date.now();

const pollTimer = setInterval(() => {
  if (Date.now() - startTime > maxPollTime) {
    clearInterval(pollTimer);
    popup?.close();
    // Consider notifying user of timeout
    return;
  }
  // ... rest of polling logic
}, 300);

⚡ Performance Considerations

Good:

• Efficient polling interval of 300ms balances responsiveness with CPU usage
• Early exit when popup is closed reduces unnecessary processing

Could be improved:

• Consider using document.hidden API to pause polling when the parent window is not visible
• The regex test on every loginWithProvider call could be cached after first evaluation

🔒 Security Concerns

Positive security aspects:

• Proper origin validation before processing the callback URL (line 54)
• Cross-origin errors are properly caught and handled
• No sensitive data is logged or exposed

Security considerations:

1. Window reference: Ensure the popup window reference cannot be hijacked. The current implementation looks secure
2. Callback URL validation: Consider additional validation of the callback URL structure before redirecting to prevent potential open redirect vulnerabilities

📋 Test Coverage

Missing test coverage for the new functionality:

• No tests for isPopupAuthDomain() function
• No tests for loginViaPopup() behavior
• No tests for the popup flow in loginWithProvider()

Recommended tests to add:

describe('isPopupAuthDomain()', () => {
  test('should return true for preview domains', () => {
    global.window = { location: { hostname: 'preview--feature.base44.app' } };
    expect(isPopupAuthDomain()).toBe(true);
  });
  
  test('should return false for production domains', () => {
    global.window = { location: { hostname: 'app.base44.com' } };
    expect(isPopupAuthDomain()).toBe(false);
  });
});

describe('loginWithProvider() popup flow', () => {
  test('should use popup on preview domains', () => {
    const mockOpen = vi.fn().mockReturnValue({ /* mock popup */ });
    global.window.open = mockOpen;
    global.window.location = { hostname: 'preview--test.base44.app' };
    
    base44.auth.loginWithProvider('google');
    expect(mockOpen).toHaveBeenCalled();
  });
});

📝 Summary

This is a well-implemented solution to the iframe OAuth problem. The code is clean and follows good practices. The main areas for improvement are:

1. Add error handling for popup-blocked scenarios
2. Implement timeout protection for the polling mechanism
3. Add comprehensive test coverage for the new popup authentication flow
4. Consider minor performance optimizations like caching the domain check

The security implementation looks solid with proper origin checking. Once the test coverage is added and the edge cases are handled, this will be a robust solution.

[github-actions]

🚀 Package Preview Available!

---

Install this PR's preview build with npm:

npm i @base44-preview/sdk@0.8.19-pr.134.76a0e8e

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.19-pr.134.76a0e8e"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/sdk@0.8.19-pr.134.76a0e8e"
  }
}

• 📦 Preview Package: @base44-preview/sdk@0.8.19-pr.134.76a0e8e
• 🔗 View this commit on GitHub

---

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

[claude]

Claude finished @roymiloh's task —— View job

---

PR Review: Use popup for OAuth on preview/sandbox/checkpoint domains

I've completed my review of this PR that adds popup-based OAuth authentication for iframe environments. Here's my comprehensive feedback:

✅ Code Quality and Best Practices

Strengths:

• Clean, well-documented implementation with clear JSDoc comments
• Good separation of concerns with dedicated functions (isPopupAuthDomain, loginViaPopup)
• Follows existing code patterns and conventions in the auth module
• Appropriate use of TypeScript types

Suggestions:

• Consider extracting magic numbers to named constants:

  const POPUP_WIDTH = 500;
  const POPUP_HEIGHT = 600;
  const POPUP_POLL_INTERVAL = 300;
  const POPUP_WINDOW_NAME = "base44_auth";

🐛 Potential Bugs or Issues

1. Memory leak risk: If the user manually closes the popup, the setInterval continues running until they close it. While you check popup.closed, consider adding a maximum timeout to prevent infinite polling:

   const MAX_POLL_DURATION = 5 * 60 * 1000; // 5 minutes
   const startTime = Date.now();
   
   const pollTimer = setInterval(() => {
     if (Date.now() - startTime > MAX_POLL_DURATION) {
       clearInterval(pollTimer);
       popup?.close();
       // Consider showing an error to the user
       return;
     }
     // ... rest of polling logic
   }, POPUP_POLL_INTERVAL);

2. Silent failure on popup blocker: When window.open returns null (lines 38-40), the function silently returns without any user feedback. Users won't know why nothing happened.

3. Regex change in latest commit: The regex was changed from /^(preview-sandbox--|preview--|checkpoint--)[^.]+\.base44\.app$/ to /^(preview-sandbox--|preview--|checkpoint--)[^.]+\./. This now matches ANY domain starting with those prefixes, not just base44.app domains. Is this intentional? It could potentially match unintended domains.

⚡ Performance Considerations

Good practices:

• Efficient 300ms polling interval balances responsiveness with CPU usage
• Early exit when popup is closed reduces unnecessary processing
• Using try-catch for cross-origin checks is appropriate

Suggestions:

• Consider pausing polling when the parent window is not visible using the Page Visibility API:

  const pollTimer = setInterval(() => {
    if (document.hidden) return; // Skip polling when tab is hidden
    // ... rest of logic
  }, 300);

🔒 Security Concerns

Positive aspects:

• Proper origin validation before processing the callback URL (line 54)
• Cross-origin errors are properly caught and handled
• No sensitive data is exposed in logs

Concerns:

1. Domain matching is now too broad: The recent regex change allows any domain with the preview/sandbox/checkpoint prefixes. This could potentially be exploited if an attacker registers a domain like preview--malicious.evil.com.
2. Open redirect potential: While you check the origin matches, consider validating the callback URL structure more thoroughly to ensure it's a valid OAuth callback.

📋 Test Coverage

Critical gap: There are no tests for the new popup authentication functionality. The existing auth tests in /tests/unit/auth.test.js don't cover:

• isPopupAuthDomain() function
• loginViaPopup() function
• The popup flow branch in loginWithProvider()

Recommended tests:

describe('Popup Authentication', () => {
  describe('isPopupAuthDomain()', () => {
    test('should return true for preview domains', () => {
      global.window = { location: { hostname: 'preview--feature.base44.app' } };
      expect(isPopupAuthDomain()).toBe(true);
    });
    
    test('should return false for production domains', () => {
      global.window = { location: { hostname: 'app.base44.com' } };
      expect(isPopupAuthDomain()).toBe(false);
    });
    
    test('should return false when window is undefined', () => {
      const originalWindow = global.window;
      delete global.window;
      expect(isPopupAuthDomain()).toBe(false);
      global.window = originalWindow;
    });
  });

  describe('loginWithProvider() popup flow', () => {
    test('should use popup on preview domains', () => {
      const mockOpen = vi.fn().mockReturnValue({ /* mock popup */ });
      global.window.open = mockOpen;
      global.window.location = { hostname: 'preview--test.base44.app' };
      
      base44.auth.loginWithProvider('google');
      expect(mockOpen).toHaveBeenCalled();
    });
    
    test('should handle popup blocker gracefully', () => {
      global.window.open = vi.fn().mockReturnValue(null);
      global.window.location = { hostname: 'preview--test.base44.app' };
      
      // Should not throw
      expect(() => base44.auth.loginWithProvider('google')).not.toThrow();
    });
  });
});

📝 Summary

This is a well-implemented solution to the iframe OAuth problem. The code is clean and follows good practices. However, there are several areas that need attention:

1. High Priority: The regex change in the latest commit makes domain matching too permissive. Consider reverting to the stricter .base44\.app$ pattern or documenting why any domain suffix is acceptable.

2. Medium Priority: Add error handling for popup-blocked scenarios and implement a maximum timeout for the polling mechanism.

3. Medium Priority: Add comprehensive test coverage for all new functions.

4. Low Priority: Extract magic numbers to named constants for better maintainability.

Once these issues are addressed (especially the domain matching concern), this will be a robust solution for OAuth in iframe environments.

Todo List:

• Read and analyze the auth.ts file changes
• Review code quality and best practices
• Check for potential bugs or issues
• Analyze performance considerations
• Evaluate security concerns
• Assess test coverage
• Provide comprehensive feedback

[claude]

Claude finished @roymiloh's task —— View job

---

PR Review: Use popup for OAuth on preview/sandbox/checkpoint domains

I've completed my review of this PR that adds popup-based OAuth authentication for iframe environments. Here's my comprehensive feedback:

✅ Code Quality and Best Practices

Strengths:

• Clean, well-documented implementation with clear JSDoc comments
• Good separation of concerns with dedicated functions (isPopupAuthDomain, loginViaPopup)
• Follows existing code patterns and conventions in the auth module
• Appropriate use of TypeScript types

Suggestions:

• Consider extracting magic numbers to named constants:

  const POPUP_WIDTH = 500;
  const POPUP_HEIGHT = 600;
  const POPUP_POLL_INTERVAL = 300;
  const POPUP_WINDOW_NAME = "base44_auth";

🐛 Potential Bugs or Issues

1. Memory leak risk: If the user manually closes the popup, the setInterval continues running indefinitely. While you check popup.closed, consider adding a maximum timeout to prevent infinite polling:

   const MAX_POLL_DURATION = 5 * 60 * 1000; // 5 minutes
   const startTime = Date.now();
   
   const pollTimer = setInterval(() => {
     if (Date.now() - startTime > MAX_POLL_DURATION) {
       clearInterval(pollTimer);
       popup?.close();
       // Consider showing an error to the user
       return;
     }
     // ... rest of polling logic
   }, POPUP_POLL_INTERVAL);

2. Silent failure on popup blocker: When window.open returns null (lines 38-40), the function silently returns without any user feedback. Users won't know why nothing happened.

3. Overly permissive domain matching: The regex /^(preview-sandbox--|preview--|checkpoint--)[^.]+\./ now matches ANY domain starting with those prefixes, not just base44.app domains. This could potentially match unintended domains like preview--malicious.evil.com.

⚡ Performance Considerations

Good practices:

• Efficient 300ms polling interval balances responsiveness with CPU usage
• Early exit when popup is closed reduces unnecessary processing
• Using try-catch for cross-origin checks is appropriate

Suggestions:

• Consider pausing polling when the parent window is not visible using the Page Visibility API:

  const pollTimer = setInterval(() => {
    if (document.hidden) return; // Skip polling when tab is hidden
    // ... rest of logic
  }, 300);

🔒 Security Concerns

Positive aspects:

• Proper origin validation before processing the callback URL (line 54)
• Cross-origin errors are properly caught and handled
• No sensitive data is exposed in logs

Security issues:

1. Domain matching is too broad: The current regex allows any domain with the preview/sandbox/checkpoint prefixes. This could be exploited if an attacker registers a domain like preview--attack.malicious.com. Consider restricting to base44.app domains:

   const POPUP_AUTH_DOMAIN_REGEX = 
     /^(preview-sandbox--|preview--|checkpoint--)[^.]+\.base44\.app$/;

2. Potential open redirect: While you check the origin matches, consider additional validation of the callback URL structure to ensure it's a valid OAuth callback.

📋 Test Coverage

Critical gap: There are no tests for the new popup authentication functionality. The existing auth tests don't cover:

• isPopupAuthDomain() function
• loginViaPopup() function
• The popup flow branch in loginWithProvider()

Recommended tests:

describe('Popup Authentication', () => {
  describe('isPopupAuthDomain()', () => {
    test('should return true for preview domains', () => {
      global.window = { location: { hostname: 'preview--feature.base44.app' } };
      expect(isPopupAuthDomain()).toBe(true);
    });
    
    test('should return false for production domains', () => {
      global.window = { location: { hostname: 'app.base44.com' } };
      expect(isPopupAuthDomain()).toBe(false);
    });
    
    test('should return false when window is undefined', () => {
      const originalWindow = global.window;
      delete global.window;
      expect(isPopupAuthDomain()).toBe(false);
      global.window = originalWindow;
    });
  });

  describe('loginWithProvider() popup flow', () => {
    test('should use popup on preview domains', () => {
      const mockOpen = vi.fn().mockReturnValue({ /* mock popup */ });
      global.window.open = mockOpen;
      global.window.location = { hostname: 'preview--test.base44.app' };
      
      base44.auth.loginWithProvider('google');
      expect(mockOpen).toHaveBeenCalled();
    });
    
    test('should handle popup blocker gracefully', () => {
      global.window.open = vi.fn().mockReturnValue(null);
      global.window.location = { hostname: 'preview--test.base44.app' };
      
      // Should not throw
      expect(() => base44.auth.loginWithProvider('google')).not.toThrow();
    });
  });
});

📝 Summary

This is a well-implemented solution to the iframe OAuth problem. The code is clean and follows good practices. However, there are several issues that need attention:

1. High Priority: The regex is too permissive and could match unintended domains. Consider restricting to .base44\.app$ domains for security.

2. Medium Priority: Add error handling for popup-blocked scenarios and implement a maximum timeout for the polling mechanism to prevent memory leaks.

3. Medium Priority: Add comprehensive test coverage for all new functions.

4. Low Priority: Extract magic numbers to named constants for better maintainability.

Once these issues are addressed (especially the domain matching security concern), this will be a robust solution for OAuth in iframe environments.

Todo List:

• Read and analyze the auth.ts file changes
• Review code quality and best practices
• Check for potential bugs or issues
• Analyze performance considerations
• Evaluate security concerns
• Assess test coverage
• Provide comprehensive feedback

[claude]

Claude finished @roymiloh's task —— View job

---

PR Review: Use popup for OAuth on preview/sandbox/checkpoint domains

I've completed my review of this PR that adds popup-based OAuth authentication for iframe environments. Here's my comprehensive feedback:

✅ Code Quality and Best Practices

Strengths:

• Clean, well-documented implementation with clear JSDoc comments
• Good separation of concerns with dedicated functions (isPopupAuthDomain, loginViaPopup)
• Follows existing code patterns and conventions in the auth module
• Proper use of TypeScript types
• The switch from location polling to postMessage is a significant improvement in terms of security and reliability

Suggestions:

• Consider extracting magic numbers to named constants:

  const POPUP_WIDTH = 500;
  const POPUP_HEIGHT = 600;
  const POPUP_POLL_INTERVAL = 500;
  const POPUP_WINDOW_NAME = "base44_auth";

🐛 Potential Bugs or Issues

1. Overly permissive domain matching: The regex /^(preview-sandbox--|preview--|checkpoint--)[^.]+\./ matches ANY domain starting with those prefixes, not just base44.app domains. This could potentially match unintended domains like preview--malicious.evil.com. Consider restricting to base44.app domains:

   const POPUP_AUTH_DOMAIN_REGEX = 
     /^(preview-sandbox--|preview--|checkpoint--)[^.]+\.base44\.app$/;

2. No error handling for popup blockers: When window.open returns null (lines 44-46), the function silently returns without any user feedback. Users won't know why authentication didn't start.

3. Potential memory leak: The setInterval for checking popup.closed (lines 76-78) will continue running until the popup is closed. Consider adding a maximum timeout:

   const MAX_POLL_DURATION = 5 * 60 * 1000; // 5 minutes
   const startTime = Date.now();
   
   const pollTimer = setInterval(() => {
     if (Date.now() - startTime > MAX_POLL_DURATION) {
       cleanup();
       // Consider notifying user of timeout
       return;
     }
     if (popup.closed) cleanup();
   }, 500);

⚡ Performance Considerations

Good practices:

• Efficient 500ms polling interval for popup closure detection
• Proper cleanup of event listeners and intervals
• Using postMessage instead of location polling eliminates cross-origin access attempts

Suggestions:

• Consider using the Page Visibility API to pause polling when the tab is not visible:

  const pollTimer = setInterval(() => {
    if (document.hidden) return; // Skip when tab is hidden
    if (popup.closed) cleanup();
  }, 500);

🔒 Security Concerns

Positive aspects:

• Proper origin validation in the message handler (line 55)
• Verification that the message comes from the expected popup window (line 56)
• No sensitive data is exposed in logs
• Using postMessage is more secure than the previous location polling approach
• Validating the presence of access_token before processing (line 57)

Security considerations:

1. Domain validation is too broad: As mentioned above, the regex should be restricted to base44.app domains only
2. Message structure validation: Consider more thorough validation of the message data structure to ensure it contains only expected fields
3. Token validation: While you check for access_token presence, consider validating its format (e.g., ensuring it's a string)

📋 Test Coverage

Critical gap: There are no tests for the new popup authentication functionality. The existing test file doesn't cover:

• isPopupAuthDomain() function
• loginViaPopup() function
• The popup flow branch in loginWithProvider()
• The postMessage handling logic

Recommended tests:

describe('Popup Authentication', () => {
  describe('isPopupAuthDomain()', () => {
    test('should return true for preview domains', () => {
      global.window = { location: { hostname: 'preview--feature.base44.app' } };
      expect(isPopupAuthDomain()).toBe(true);
    });
    
    test('should return false for production domains', () => {
      global.window = { location: { hostname: 'app.base44.com' } };
      expect(isPopupAuthDomain()).toBe(false);
    });
    
    test('should not match non-base44 domains', () => {
      global.window = { location: { hostname: 'preview--evil.malicious.com' } };
      expect(isPopupAuthDomain()).toBe(false);
    });
  });

  describe('loginWithProvider() popup flow', () => {
    test('should use popup on preview domains', () => {
      const mockOpen = vi.fn().mockReturnValue({ /* mock popup */ });
      global.window.open = mockOpen;
      global.window.location = { hostname: 'preview--test.base44.app' };
      
      base44.auth.loginWithProvider('google');
      expect(mockOpen).toHaveBeenCalledWith(
        expect.stringContaining('popup_origin='),
        'base44_auth',
        expect.any(String)
      );
    });
    
    test('should handle popup blocker gracefully', () => {
      global.window.open = vi.fn().mockReturnValue(null);
      // Should not throw
      expect(() => base44.auth.loginWithProvider('google')).not.toThrow();
    });
    
    test('should validate message origin and source', () => {
      // Test postMessage validation logic
    });
  });
});

📝 Summary

This is a well-thought-out solution to the iframe OAuth problem. The switch from location polling to postMessage is a significant improvement. The code is clean and follows good practices. However, there are several important issues to address:

1. High Priority: Tighten the domain regex to only match .base44\.app$ domains for security
2. Medium Priority: Add error handling for popup-blocked scenarios
3. Medium Priority: Add comprehensive test coverage for all new functions
4. Low Priority: Implement timeout protection for the popup closure polling
5. Low Priority: Extract magic numbers to named constants

The postMessage approach is definitely the right direction, and once these issues are addressed, this will be a robust solution for OAuth in iframe environments.

Todo List:

• Read and analyze the auth.ts file changes
• Review code quality and best practices
• Check for potential bugs or issues
• Analyze performance considerations
• Evaluate security concerns
• Assess test coverage
• Provide comprehensive feedback

[claude]

Claude finished @roymiloh's task —— View job

---

PR Review: Use popup for OAuth on preview/sandbox/checkpoint domains

I've completed my review of this PR that adds popup-based OAuth authentication for iframe environments. Here's my comprehensive feedback:

✅ Code Quality and Best Practices

Strengths:

• Clean, well-documented implementation with clear JSDoc comments
• Good separation of concerns with dedicated functions (isPopupAuthDomain, loginViaPopup)
• Follows existing code patterns and conventions in the auth module
• Proper use of TypeScript types
• The switch from location polling to postMessage is a significant improvement in terms of security and reliability

Suggestions:

• Consider extracting magic numbers to named constants:

  const POPUP_WIDTH = 500;
  const POPUP_HEIGHT = 600;
  const POPUP_POLL_INTERVAL = 500;
  const POPUP_WINDOW_NAME = "base44_auth";

🐛 Potential Bugs or Issues

1. Overly permissive domain matching: The regex /^(preview-sandbox--|preview--|checkpoint--)[^.]+\./ matches ANY domain starting with those prefixes, not just base44.app domains. This could potentially match unintended domains like preview--malicious.evil.com. Consider restricting to base44.app domains:

   const POPUP_AUTH_DOMAIN_REGEX = 
     /^(preview-sandbox--|preview--|checkpoint--)[^.]+\.base44\.app$/;

2. No error handling for popup blockers: When window.open returns null (lines 44-46), the function silently returns without any user feedback. Users won't know why authentication didn't start.

3. Potential memory leak: The setInterval for checking popup.closed (lines 76-78) will continue running until the popup is closed. Consider adding a maximum timeout:

   const MAX_POLL_DURATION = 5 * 60 * 1000; // 5 minutes
   const startTime = Date.now();
   
   const pollTimer = setInterval(() => {
     if (Date.now() - startTime > MAX_POLL_DURATION) {
       cleanup();
       // Consider notifying user of timeout
       return;
     }
     if (popup.closed) cleanup();
   }, 500);

⚡ Performance Considerations

Good practices:

• Efficient 500ms polling interval for popup closure detection
• Proper cleanup of event listeners and intervals
• Using postMessage instead of location polling eliminates cross-origin access attempts

Suggestions:

• Consider using the Page Visibility API to pause polling when the tab is not visible:

  const pollTimer = setInterval(() => {
    if (document.hidden) return; // Skip when tab is hidden
    if (popup.closed) cleanup();
  }, 500);

🔒 Security Concerns

Positive aspects:

• Proper origin validation in the message handler (line 55)
• Verification that the message comes from the expected popup window (line 56)
• No sensitive data is exposed in logs
• Using postMessage is more secure than the previous location polling approach
• Validating the presence of access_token before processing (line 57)

Security considerations:

1. Domain validation is too broad: As mentioned above, the regex should be restricted to base44.app domains only
2. Message structure validation: Consider more thorough validation of the message data structure to ensure it contains only expected fields
3. Token validation: While you check for access_token presence, consider validating its format (e.g., ensuring it's a string)

📋 Test Coverage

Critical gap: There are no tests for the new popup authentication functionality. The existing test file doesn't cover:

• isPopupAuthDomain() function
• loginViaPopup() function
• The popup flow branch in loginWithProvider()
• The postMessage handling logic

Recommended tests:

describe('Popup Authentication', () => {
  describe('isPopupAuthDomain()', () => {
    test('should return true for preview domains', () => {
      global.window = { location: { hostname: 'preview--feature.base44.app' } };
      expect(isPopupAuthDomain()).toBe(true);
    });
    
    test('should return false for production domains', () => {
      global.window = { location: { hostname: 'app.base44.com' } };
      expect(isPopupAuthDomain()).toBe(false);
    });
    
    test('should not match non-base44 domains', () => {
      global.window = { location: { hostname: 'preview--evil.malicious.com' } };
      expect(isPopupAuthDomain()).toBe(false);
    });
  });

  describe('loginWithProvider() popup flow', () => {
    test('should use popup on preview domains', () => {
      const mockOpen = vi.fn().mockReturnValue({ /* mock popup */ });
      global.window.open = mockOpen;
      global.window.location = { hostname: 'preview--test.base44.app' };
      
      base44.auth.loginWithProvider('google');
      expect(mockOpen).toHaveBeenCalledWith(
        expect.stringContaining('popup_origin='),
        'base44_auth',
        expect.any(String)
      );
    });
    
    test('should handle popup blocker gracefully', () => {
      global.window.open = vi.fn().mockReturnValue(null);
      // Should not throw
      expect(() => base44.auth.loginWithProvider('google')).not.toThrow();
    });
    
    test('should validate message origin and source', () => {
      // Test postMessage validation logic
    });
  });
});

📝 Summary

This is a well-thought-out solution to the iframe OAuth problem. The switch from location polling to postMessage is a significant improvement. The code is clean and follows good practices. However, there are several important issues to address:

1. High Priority: Tighten the domain regex to only match .base44\.app$ domains for security
2. Medium Priority: Add error handling for popup-blocked scenarios
3. Medium Priority: Add comprehensive test coverage for all new functions
4. Low Priority: Implement timeout protection for the popup closure polling
5. Low Priority: Extract magic numbers to named constants

The postMessage approach is definitely the right direction, and once these issues are addressed, this will be a robust solution for OAuth in iframe environments.

Todo List:

• Check for CLAUDE.md files for repo-specific guidelines
• Read and analyze the auth.ts file changes
• Review code quality and best practices
• Check for potential bugs or issues
• Analyze performance considerations
• Evaluate security concerns
• Assess test coverage
• Provide comprehensive feedback