clear http only cookies on logout #91

roymiloh · 2026-01-26T12:44:23Z

No description provided.

claude · 2026-01-26T12:44:38Z

Claude encountered an error —— View job

I'll analyze this and get back to you.

github-actions · 2026-01-26T12:44:53Z

🚀 Package Preview Available!

Install this PR's preview build with npm:

npm i @base44-preview/[email protected]

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/[email protected]"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/[email protected]"
  }
}

📦 Preview Package: @base44-preview/[email protected]
🔗 View this commit on GitHub

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

claude · 2026-01-26T13:26:22Z

Claude finished @roymiloh's task —— View job

PR Review: Clear HTTP-only cookies on logout

I've reviewed the changes to the logout functionality in src/modules/auth.ts. Here's my feedback:

🔍 Summary of Changes

The PR modifies the logout() method to redirect to a server-side logout endpoint instead of just reloading the page, which allows proper clearing of HTTP-only cookies.

✅ Code Quality

Good: Clean implementation that maintains existing functionality while adding HTTP-only cookie support
Good: Proper URL encoding using encodeURIComponent()
Good: Maintains backward compatibility by using current URL as default redirect

🐛 Potential Issues

Minor: The comment update from "Removes the token from localStorage..." to just "Logout the current user" loses some documentation detail about what the method does

⚡ Performance

No performance concerns - the change is a simple redirect

🔒 Security Assessment

Excellent: This change improves security by properly clearing HTTP-only cookies through server-side logout
Good: Maintains token cleanup from axios headers and localStorage
Good: Properly encodes the redirect URL to prevent injection attacks

🧪 Test Coverage

Issue: The existing tests are now outdated and will fail:
- Tests expect window.location.reload() but code now sets window.location.href
- Tests don't verify the new server-side logout URL construction
- Missing tests for the HTTP-only cookie clearing behavior

Required test updates:

Update test at line ~176 to expect window.location.href assignment instead of reload()
Add test to verify correct logout URL construction with serverUrl and appId
Update test descriptions to reflect new behavior

🎯 Recommendations

Update the JSDoc comment in auth.types.ts:217-240 to reflect the new behavior of redirecting to server-side logout endpoint
Fix the failing tests before merging - the logout tests need updating to match the new implementation
Consider adding a comment in the code explaining why we redirect to the server (to clear HTTP-only cookies)

📝 Overall Assessment

This is a good security improvement that addresses the limitation of client-side JavaScript not being able to clear HTTP-only cookies. The implementation is clean and maintains backward compatibility. However, the tests must be updated before this can be merged.

claude · 2026-01-26T13:57:35Z

Claude finished @roymiloh's task —— View job

PR Review: Clear HTTP-only cookies on logout

I've reviewed the updated changes to the logout functionality. Here's my feedback:

Todo List:

Read modified files to understand changes
Review code quality and implementation
Check for potential bugs or issues
Evaluate performance considerations
Analyze security implications
Verify test coverage and updates
Provide comprehensive feedback

🔍 Summary of Changes

The PR modifies the logout() method in src/modules/auth.ts to redirect to a server-side logout endpoint instead of reloading the page, enabling proper clearing of HTTP-only cookies. The tests have been updated to reflect this new behavior.

✅ Code Quality

Good: Clean implementation that maintains existing functionality while adding HTTP-only cookie support
Good: Proper URL encoding using encodeURIComponent()
Good: Maintains backward compatibility - uses current URL as default redirect when no redirectUrl is provided
Good: Proper error handling for localStorage operations remains intact

🐛 Potential Issues

None identified: The implementation appears solid and handles edge cases appropriately

⚡ Performance

No concerns: The change is a simple redirect with minimal performance impact
The removal of window.location.reload() and replacement with a direct redirect may actually be slightly more efficient

🔒 Security Assessment

Excellent: This change significantly improves security by properly clearing HTTP-only cookies through server-side logout
Good: Token cleanup from axios headers and localStorage is maintained
Good: Properly encodes the redirect URL to prevent injection attacks
Good: Handles non-browser environments gracefully by skipping browser-specific operations

🧪 Test Coverage

Excellent: Tests have been properly updated to reflect the new logout behavior
Coverage includes:
- Verifying redirect to server-side logout endpoint
- Testing with and without custom redirect URLs
- Ensuring proper URL encoding of the from_url parameter
- Maintaining existing tests for token removal from axios headers
- Maintaining existing tests for localStorage cleanup and error handling

Key test improvements:

Tests now verify window.location.href assignment instead of reload() (lines 325-362)
Added test for redirect with custom URL parameter
Added test for redirect using current page URL when no parameter provided
Proper mocking of window.location object

📝 Implementation Details

The logout flow now works as follows:

Removes token from axios headers (always happens)
If in browser environment:
- Removes tokens from localStorage (with error handling)
- Constructs logout URL with from_url parameter
- Redirects to server-side logout endpoint: ${serverUrl}/api/apps/${appId}/auth/logout?from_url=...

🎯 Overall Assessment

This is an excellent security improvement that properly addresses the limitation of client-side JavaScript not being able to clear HTTP-only cookies. The implementation is clean, secure, and well-tested. The test updates comprehensively cover the new behavior and maintain coverage of existing functionality.

The PR is ready to merge. Great work on both the implementation and the thorough test updates!

netanelgilad · 2026-01-27T13:17:26Z

src/modules/auth.ts

+        const fromUrl = redirectUrl || window.location.href;
+
+        // Redirect to server-side logout endpoint to clear HTTP-only cookies
+        const logoutUrl = `${options.serverUrl}/api/apps/${appId}/auth/logout?from_url=${encodeURIComponent(fromUrl)}`;


use appBaseUrl instead of serverUrl

netanelgilad · 2026-01-27T13:23:46Z

src/modules/auth.ts

@@ -66,4 +66,4 @@
      const providerPath = provider === "google" ? "" : `/${provider}`;
      const loginUrl = `${
        options.serverUrl


netanelgilad · 2026-01-27T13:24:20Z

src/modules/auth.ts

      const providerPath = provider === "google" ? "" : `/${provider}`;
      const loginUrl = `${
        options.serverUrl
      }/api/apps/auth${providerPath}/login?app_id=${appId}&from_url=${encodeURIComponent(


/login/social/<provider id>

claude · 2026-01-28T19:35:09Z

Claude finished @roymiloh's task —— View job

PR Review: Clear HTTP-only cookies on logout

I've reviewed the changes to the logout functionality. Here's my comprehensive feedback:

Todo List:

Check for CLAUDE.md file for repo-specific guidelines
Read modified files to understand changes
Review code quality and implementation
Check for potential bugs or issues
Evaluate performance considerations
Analyze security implications
Verify test coverage and updates
Provide comprehensive feedback

🔍 Summary of Changes

The PR successfully addresses the latest review comments by changing serverUrl to appBaseUrl in the logout redirect logic. This change ensures proper clearing of HTTP-only cookies through server-side logout.

✅ Code Quality

Excellent: Clean implementation that addresses the review feedback
Good: Proper URL construction using appBaseUrl as requested
Good: Maintains backward compatibility with existing logout behavior
Good: Clear code structure and proper error handling

🐛 Potential Issues

None identified: The implementation properly addresses all review comments and maintains robust error handling

⚡ Performance

No concerns: The change is a simple redirect with minimal performance impact
The direct redirect approach is efficient and straightforward

🔒 Security Assessment

Excellent: Properly clears HTTP-only cookies through server-side logout endpoint
Good: Maintains secure token cleanup from axios headers and localStorage
Good: URL encoding prevents injection attacks with encodeURIComponent()
Good: Graceful handling of non-browser environments

🧪 Test Coverage

Comprehensive: Tests have been properly updated to reflect the new behavior
Coverage includes:
- Server-side logout redirect with correct URL construction (auth.test.js:338)
- Redirect with custom URL parameter (auth.test.js:326-342)
- Default redirect using current page URL (auth.test.js:345-362)
- Token removal from axios headers and localStorage
- Error handling for localStorage operations

Key test improvements:

Tests verify the correct logout endpoint: /api/apps/${appId}/auth/logout
Proper URL parameter encoding verification
Complete coverage of both custom and default redirect scenarios

📝 Implementation Details

The logout implementation now:

Always removes token from axios headers
In browser environments:
- Removes tokens from localStorage (with error handling)
- Constructs logout URL: ${appBaseUrl}/api/apps/${appId}/auth/logout?from_url=...
- Redirects to server-side endpoint to clear HTTP-only cookies

💡 Minor Suggestion

Consider updating the JSDoc comment in auth.types.ts:218 to reflect the new behavior more accurately. The current comment mentions "reloads the page" but the implementation now redirects to a server-side logout endpoint. This is a minor documentation update that would improve clarity.

🎯 Overall Assessment

This PR successfully implements the requested changes and provides a secure, well-tested solution for clearing HTTP-only cookies on logout. The code quality is excellent, the implementation addresses all review feedback, and the test coverage is comprehensive. The change from serverUrl to appBaseUrl has been properly implemented as requested.

The PR is ready to merge. Great work on implementing the review feedback and maintaining high code quality!

claude · 2026-01-28T19:39:49Z

Claude finished @roymiloh's task —— View job

PR Review: Clear HTTP-only cookies on logout

I've reviewed the changes to the logout functionality. Here's my comprehensive feedback:

Todo List:

Check for CLAUDE.md file for repo-specific guidelines
Read modified files to understand changes
Review code quality and implementation
Check for potential bugs or issues
Evaluate performance considerations
Analyze security implications
Verify test coverage and updates
Provide comprehensive feedback

🔍 Summary of Changes

The PR successfully implements server-side logout to clear HTTP-only cookies by redirecting to a logout endpoint instead of just reloading the page. All previous review feedback regarding using appBaseUrl instead of serverUrl has been properly addressed.

✅ Code Quality

Excellent: Clean, well-structured implementation that maintains existing functionality
Good: Proper error handling for localStorage operations with try-catch blocks
Good: Clear separation of concerns between browser and non-browser environments
Good: Consistent use of appBaseUrl throughout as requested in reviews
Good: Proper URL encoding using encodeURIComponent() to prevent injection attacks

🐛 Potential Issues

None identified: The implementation properly handles all edge cases including:
- Non-browser environments (skips browser-specific operations)
- localStorage errors (caught and logged)
- Missing redirect URL (defaults to current page URL)
- Empty tokens (early return in setToken)

⚡ Performance

No concerns: The implementation is efficient with minimal overhead
The direct redirect approach is actually more performant than the previous window.location.reload() as it avoids an unnecessary page reload

🔒 Security

Excellent security improvements:
- Properly clears HTTP-only cookies through server-side logout endpoint
- Maintains secure token cleanup from both axios headers and localStorage
- URL encoding prevents potential XSS attacks in redirect URLs
- Graceful error handling prevents exposing sensitive information
- The logout endpoint pattern /api/apps/${appId}/auth/logout follows RESTful conventions

🧪 Test Coverage

Comprehensive test coverage with all scenarios covered:
- ✅ Server-side logout redirect with correct URL construction (auth.test.js:334-351)
- ✅ Redirect with custom URL parameter (auth.test.js:352-370)
- ✅ Default redirect using current page URL when no parameter provided
- ✅ Token removal from axios headers verification
- ✅ localStorage cleanup with error handling
- ✅ Non-browser environment handling

The tests properly verify:

Correct logout URL format: ${appBaseUrl}/api/apps/${appId}/auth/logout?from_url=...
Proper URL parameter encoding
All edge cases and error scenarios

📝 Implementation Details

The logout flow now works as follows:

Always: Removes token from axios default headers
In browser environments:
- Removes tokens from localStorage (with error handling)
- Constructs logout URL with encoded from_url parameter
- Redirects to server-side endpoint to clear HTTP-only cookies

💡 Minor Documentation Suggestion

The only minor improvement would be updating the JSDoc comment in the types file (if one exists) to reflect that the logout method now redirects to a server-side endpoint rather than just reloading the page. However, this is not critical for functionality.

🎯 Overall Assessment

This is a well-implemented security enhancement that properly addresses the limitation of client-side JavaScript not being able to clear HTTP-only cookies. The code is clean, secure, thoroughly tested, and addresses all previous review feedback. The implementation follows best practices for both security and error handling.

The PR is ready to merge. Excellent work on implementing a secure logout mechanism with comprehensive test coverage!

clear http only cookies on logout

b6d6367

test

89aeee3

netanelgilad reviewed Jan 27, 2026

View reviewed changes

netanelgilad approved these changes Jan 27, 2026

View reviewed changes

appbaseurl

50facc9

test

9b9c35e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clear http only cookies on logout #91

clear http only cookies on logout #91

Uh oh!

roymiloh commented Jan 26, 2026

Uh oh!

claude bot commented Jan 26, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Jan 26, 2026 •

edited

Loading

Uh oh!

claude bot commented Jan 26, 2026 •

edited

Loading

Uh oh!

claude bot commented Jan 26, 2026 •

edited

Loading

Uh oh!

netanelgilad Jan 27, 2026

Uh oh!

netanelgilad Jan 27, 2026

Uh oh!

netanelgilad Jan 27, 2026

Uh oh!

claude bot commented Jan 28, 2026 •

edited

Loading

Uh oh!

claude bot commented Jan 28, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clear http only cookies on logout #91

Are you sure you want to change the base?

clear http only cookies on logout #91

Uh oh!

Conversation

roymiloh commented Jan 26, 2026

Uh oh!

claude bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Package Preview Available!

Uh oh!

claude bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Clear HTTP-only cookies on logout

🔍 Summary of Changes

✅ Code Quality

🐛 Potential Issues

⚡ Performance

🔒 Security Assessment

🧪 Test Coverage

🎯 Recommendations

📝 Overall Assessment

Uh oh!

claude bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Clear HTTP-only cookies on logout

Todo List:

🔍 Summary of Changes

✅ Code Quality

🐛 Potential Issues

⚡ Performance

🔒 Security Assessment

🧪 Test Coverage

📝 Implementation Details

🎯 Overall Assessment

Uh oh!

netanelgilad Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

netanelgilad Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

netanelgilad Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Clear HTTP-only cookies on logout

Todo List:

🔍 Summary of Changes

✅ Code Quality

🐛 Potential Issues

⚡ Performance

🔒 Security Assessment

🧪 Test Coverage

📝 Implementation Details

💡 Minor Suggestion

🎯 Overall Assessment

Uh oh!

claude bot commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Clear HTTP-only cookies on logout

Todo List:

🔍 Summary of Changes

✅ Code Quality

🐛 Potential Issues

⚡ Performance

🔒 Security

🧪 Test Coverage

📝 Implementation Details

💡 Minor Documentation Suggestion

🎯 Overall Assessment

Uh oh!

Reviewers

Assignees

Labels

Projects

claude bot commented Jan 26, 2026 •

edited

Loading

github-actions bot commented Jan 26, 2026 •

edited

Loading

claude bot commented Jan 26, 2026 •

edited

Loading

claude bot commented Jan 26, 2026 •

edited

Loading

claude bot commented Jan 28, 2026 •

edited

Loading

claude bot commented Jan 28, 2026 •

edited

Loading