Skip to content

Conversation

@kfirstri
Copy link
Contributor

Changes asked by security team to not give the user the userCode in the searchparams

@kfirstri kfirstri self-assigned this Jan 13, 2026
@github-project-automation github-project-automation bot moved this to Backlog in Dev Jan 13, 2026
@github-actions
Copy link
Contributor

🚀 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:

npm i "base44@npm:@base44-preview/[email protected]"

Or add it to your package.json dependencies:

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

Preview published to npm registry — try new features instantly!

@claude
Copy link

claude bot commented Jan 13, 2026

PR Review: Fix(login) use the verification url without the code

Summary

This PR implements a security improvement by switching from verification_uri_complete (which includes the user code in the URL) to the base verification_uri, requiring users to manually enter the code. This is a valid security enhancement that prevents potential code leakage through URL logs, browser history, or referrer headers.


Code Quality & Best Practices ✅

Positive:

  • Clean, focused changes that do exactly what's needed
  • Consistent naming conventions maintained
  • Type safety preserved throughout
  • Clear separation of concerns between schema and UI code

Schema Changes (src/core/auth/schema.ts:18-33):

  • Properly removed verification_uri_complete field from Zod schema
  • Transformation logic correctly updated to exclude the removed field
  • No breaking changes to other parts of the schema

UI Changes (src/cli/commands/auth/login.ts:30-32):

  • Improved message clarity: "Verification code:" is more explicit than "Your code is:"
  • Better user instruction: "Please confirm this code at:" clearly indicates the user needs to enter the code manually
  • Minor improvement to loading message (line 47) for consistency

Security Considerations ✅

Improvements Made:
This change enhances security by:

  1. Preventing URL-based code exposure - The user code is no longer embedded in URLs that could be:
    • Logged by proxy servers
    • Stored in browser history
    • Leaked via referrer headers
    • Exposed in screenshot sharing
  2. Following OAuth 2.0 Device Flow best practices - RFC 8628 recommends separating the verification URI from the user code for better security posture

No New Vulnerabilities Introduced:

  • User code is still displayed in the terminal (acceptable for CLI tools)
  • No changes to token handling or storage mechanisms
  • Authentication flow remains secure

Potential Issues & Considerations ⚠️

1. User Experience Trade-off

  • Users now need to manually copy and enter the code at the verification URL
  • This is slightly less convenient than clicking a pre-filled link
  • Recommendation: This is an acceptable trade-off for security, but consider:
    • Adding a note in the output that users can copy/paste the code
    • Possibly highlighting that the code expires (using the expiresIn value)

2. Missing Test Coverage ❗

  • No test files found for the authentication flow (**/*auth*.test.ts, **/*login*.test.ts)
  • Recommendation: Add integration tests to verify:
    • The correct URL format is displayed
    • The device code flow still works end-to-end
    • Error handling remains functional

3. Schema Validation Gap
The schema change removes verification_uri_complete but doesn't mark it as optional first. If the OAuth server still returns this field:

  • Zod will ignore the extra field (default behavior)
  • This is actually fine for this use case
  • Recommendation: Consider documenting this decision or add .strict() if you want to enforce exact schema matching

Example of potential improvement:

// If you want to be explicit about accepting but ignoring the field:
.passthrough() // or .strip() - current default behavior

Performance Considerations ✅

No performance impact:

  • Removed field reduces payload size slightly (negligible)
  • No additional API calls or processing
  • Authentication flow timing unchanged

Additional Recommendations

1. Documentation
Consider updating any user documentation or README files that might reference the old authentication flow with screenshots or instructions.

2. Error Messaging
The change is good, but consider enhancing the user flow with:

log.info(
  `Verification code: ${chalk.bold(deviceCodeResponse.userCode)}` +
    `\nPlease confirm this code at: ${deviceCodeResponse.verificationUri}` +
    `\n${chalk.dim(`Code expires in ${Math.floor(deviceCodeResponse.expiresIn / 60)} minutes`)}`
);

3. Testing Checklist
Before merging, manually verify:

  • The OAuth server still returns verification_uri (without _complete)
  • Users can successfully authenticate by manually entering the code
  • Error messages are still helpful if authentication fails
  • The experience works on different terminal emulators

Verdict: ✅ APPROVE WITH MINOR SUGGESTIONS

This PR successfully implements the requested security improvement. The code changes are clean, focused, and maintain consistency with the existing codebase. The main areas for improvement are:

  1. Critical: Verify end-to-end authentication flow still works
  2. Important: Add test coverage for the authentication flow
  3. Nice to have: Consider UX improvements mentioned above

The security team's request is well-founded, and this implementation correctly addresses the concern without introducing new issues.


Generated by Claude Code 🤖

@kfirstri kfirstri merged commit b0ef50c into main Jan 13, 2026
6 checks passed
@kfirstri kfirstri deleted the align-login-to-security-requirements branch January 13, 2026 11:40
@github-project-automation github-project-automation bot moved this from Backlog to Done in Dev Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants