feat(login): implement login flow with http #10
Conversation
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/cli@0.0.1-pr.10.1d6afbbPrefer not to change any import paths? Install using npm alias so your code still imports npm i "base44@npm:@base44-preview/cli@0.0.1-pr.10.1d6afbb"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/cli@0.0.1-pr.10.1d6afbb"
}
}
Preview published to npm registry — try new features instantly! |
Pull Request Review: Implement Login Flow with HTTPSummaryThis PR successfully transitions from mock authentication to a real OAuth Device Code flow implementation. The changes are well-structured and follow OAuth 2.0 standards properly. ✅ Strengths1. Solid OAuth Implementation
2. Schema Validation
3. Token Refresh Logic
🐛 Potential Issues1. Critical: Infinite Retry Loop Risk (src/core/utils/httpClient.ts:25-27)// Retry the request with new token
request.headers.set("Authorization", `Bearer ${newAccessToken}`);
return ky(request);Issue: If the server returns 401 even after refresh (e.g., token was revoked), this could cause an infinite loop because the new request will trigger the same Fix: Add a flag to prevent retrying more than once: async function handleUnauthorized(
request: KyRequest,
_options: NormalizedOptions,
response: KyResponse
): Promise<Response | void> {
if (response.status !== 401) {
return;
}
// Prevent infinite retry loops
if (request.headers.get("X-Token-Refreshed") === "true") {
return;
}
const newAccessToken = await refreshAndSaveTokens();
if (!newAccessToken) {
return;
}
request.headers.set("Authorization", `Bearer ${newAccessToken}`);
request.headers.set("X-Token-Refreshed", "true");
return ky(request);
}2. Race Condition in Token Refresh (src/core/auth/config.ts:75-92)Multiple concurrent requests could trigger parallel token refreshes, causing one refresh to overwrite another's tokens. Recommendation: Add a simple locking mechanism: let refreshPromise: Promise<string | null> | null = null;
export async function refreshAndSaveTokens(): Promise<string | null> {
if (refreshPromise) {
return refreshPromise;
}
refreshPromise = (async () => {
try {
const auth = await readAuth();
const tokenResponse = await renewAccessToken(auth.refreshToken);
await writeAuth({
...auth,
accessToken: tokenResponse.accessToken,
refreshToken: tokenResponse.refreshToken,
});
return tokenResponse.accessToken;
} catch {
await deleteAuth();
return null;
} finally {
refreshPromise = null;
}
})();
return refreshPromise;
}3. Security: Hardcoded Placeholder Data (src/cli/commands/auth/login.ts:76-84)await writeAuth({
accessToken: response.accessToken,
refreshToken: response.refreshToken,
email: "user@base44.com", // ⚠️ Placeholder
name: "Base44 User", // ⚠️ Placeholder
});Issue: All users will have the same email/name, which could cause issues if other commands depend on this data. Recommendations:
4. Missing Error Context (src/core/auth/api.ts)The throw new AuthApiError(
`Failed to generate device code: ${response.status} ${response.statusText}`,
new Error(await response.text()) // Include response body
);5. OAuth Error Schema Incomplete (src/core/auth/schema.ts:62-71)The Fix: Either add all OAuth errors or use a more permissive schema: export const OAuthErrorSchema = z.object({
error: z.string(),
error_description: z.string().optional(),
error_uri: z.string().optional(),
});⚡ Performance Considerations
interval: deviceCodeResponse.interval * 1000,
🔒 Security Considerations
📝 Test CoverageMissing Tests: There are no tests for the authentication flow. Consider adding:
Example test structure: // tests/core/auth/api.test.ts
describe('generateDeviceCode', () => {
it('should successfully generate device code', async () => {
// Mock ky response
// Assert correct transformation
});
it('should throw AuthApiError on HTTP error', async () => {
// Mock 500 response
// Assert error thrown
});
});🎨 Code Quality
📋 Recommendations SummaryMust Fix (High Priority)
Should Fix (Medium Priority)
Nice to Have (Low Priority)
ConclusionThis is a solid implementation of OAuth Device Code flow that replaces the mock authentication correctly. The main concerns are the potential infinite retry loop and race condition in token refresh, both of which should be addressed before merging. Once those are fixed, this will be production-ready. Great work on following OAuth 2.0 standards and maintaining good code structure! 🎉 Recommendation: Request changes for issues #1 and #2, then approve after fixes. |
Pull Request Review: Login Flow Implementation with HTTPThank you for implementing the actual OAuth 2.0 Device Authorization Grant flow! This is a solid implementation overall. Here's my detailed feedback: ✅ Strengths
🐛 Critical Issues1. Race Condition in Token Refresh (httpClient.ts:56-62)The // Proactively refresh if token is expired or about to expire
if (isTokenExpired(auth)) {
const newAccessToken = await refreshAndSaveTokens();
if (newAccessToken) {
request.headers.set("Authorization", `Bearer ${newAccessToken}`);
return;
}
}
// This line could use the old expired token if refresh returns null
request.headers.set("Authorization", `Bearer ${auth.accessToken}`);Issue: If Fix: Should throw an error or skip setting the header entirely when refresh fails: if (isTokenExpired(auth)) {
const newAccessToken = await refreshAndSaveTokens();
if (!newAccessToken) {
throw new Error('Authentication expired. Please login again.');
}
request.headers.set("Authorization", `Bearer ${newAccessToken}`);
} else {
request.headers.set("Authorization", `Bearer ${auth.accessToken}`);
}2. Memory Leak in
|
Pull Request Review: Login Flow ImplementationSummaryThis PR implements an OAuth 2.0 device code flow for authentication with HTTP calls, replacing mock implementations. The implementation includes token refresh mechanisms and proper error handling. Overall, the code is well-structured and follows OAuth standards. Code Quality & Best Practices✅ Strengths
|
Pull Request Review: Login Flow with HTTP ImplementationSummaryThis PR implements a real OAuth 2.0 device code flow to replace the mock authentication. Overall, the implementation is solid and follows OAuth 2.0 best practices. Below are my findings across code quality, security, bugs, performance, and testing. ✅ StrengthsGood Architectural Decisions
OAuth 2.0 Compliance
🔴 Critical Issues1. Race Condition in httpClient Token Refresh (
|
Pull Request Review: Login Flow ImplementationSummaryThis PR implements a real OAuth2 device code flow for authentication, replacing the mock implementation. The changes introduce proper HTTP calls, token management with refresh capabilities, and a dedicated HTTP client with automatic token refresh. 🟢 StrengthsCode Quality
Architecture
🔴 Critical Issues1. Race Condition in getUserInfo() - CRITICAL BUG 🐛Location: const token = await waitForAuthentication(...);
const userInfo = await getUserInfo(); // ← Uses httpClient which needs auth!
await saveAuthData(token, userInfo);Problem: Fix: Either:
Example fix: // Option 1: Save minimal auth first
await writeAuth({
accessToken: token.accessToken,
refreshToken: token.refreshToken,
expiresAt: Date.now() + token.expiresIn * 1000,
email: '',
name: '',
});
const userInfo = await getUserInfo();
await writeAuth({
accessToken: token.accessToken,
refreshToken: token.refreshToken,
expiresAt: Date.now() + token.expiresIn * 1000,
email: userInfo.email,
name: userInfo.name,
});2. Unused ImportLocation: import { UserInfo } from "node:os"; // ← Never usedThis should be removed. 3. Security: Missing Token ValidationLocation: } catch {
// Refresh failed - delete auth, user needs to login again
await deleteAuth();
return null;
}Issue: Silent failure could mask security issues. If token refresh fails, you delete the auth file without logging why. Recommendation: } catch (error) {
// Log the error for debugging but don't expose it to user
console.error('Token refresh failed:', error);
await deleteAuth();
return null;
}4. Error Handling: httpClient Silently Swallows Auth ErrorsLocation: } catch {
// No auth available, continue without header
}Issue: If Better approach: } catch (error) {
// Only swallow "not logged in" errors
if (error instanceof Error && error.message.includes('not found')) {
return;
}
throw error; // Re-throw other errors like corrupted auth file
}5. Comment is OutdatedLocation: // For now, we store placeholder values until a /userinfo endpoint is availableThis comment is misleading since you're now calling 🟡 Performance & Best Practices6. Missing Response Type Validation in httpClientLocation: The 7. Potential Memory Leak in WeakSetLocation: const retriedRequests = new WeakSet<KyRequest>();Note: WeakSet is appropriate here, but be aware that if requests are held in closures/callbacks, they won't be GC'd. This is likely fine for a CLI tool but worth monitoring. 8. OAuth Scope HardcodedLocation: scope: "apps:read apps:write",Consider making this configurable or at least document why these specific scopes are needed. 🔵 Testing & Documentation9. No Tests for Core Auth Logic ❌This PR adds significant authentication logic but no tests. Critical flows that need testing:
Recommendation: Add integration tests that mock the OAuth server responses. 10. Missing Error Message ConsistencyDifferent error types use different formats:
Consider standardizing error messages across all API calls. 🟣 Security Review11. Token Storage Security
|
Pull Request Review: feat(login): implement login flow with httpSummaryThis PR implements a complete OAuth device code flow for authentication, replacing the mock implementation with real HTTP calls. The changes introduce proper token management with refresh capabilities and a well-structured HTTP client architecture. ✅ StrengthsCode Quality
Security
🐛 Issues & ConcernsCritical Issues1. Race Condition in httpClient Token Refresh (httpClient.ts:50-71)The // Scenario: Token expires during a request burst
// Request A: beforeRequest triggers refresh
// Request B: beforeRequest sees refresh in progress, waits
// Request A: Uses old token (set before refresh completed)
// Request A: Gets 401, afterResponse triggers another refreshRecommendation: Set the token on the request in 2. Timing Issue in Token Setting (httpClient.ts:51-64)try {
const auth = await readAuth();
if (isTokenExpired(auth)) {
const newAccessToken = await refreshAndSaveTokens();
if (newAccessToken) {
request.headers.set("Authorization", `Bearer ${newAccessToken}`);
return; // ✅ Good - uses new token
}
}
request.headers.set("Authorization", `Bearer ${auth.accessToken}`); // ⚠️ Uses stale auth.accessToken
} catch {
// No auth available, continue without header
}The Fix: try {
const auth = await readAuth();
if (isTokenExpired(auth)) {
const newAccessToken = await refreshAndSaveTokens();
request.headers.set("Authorization", `Bearer ${newAccessToken ?? auth.accessToken}`);
return;
}
request.headers.set("Authorization", `Bearer ${auth.accessToken}`);
} catch {
// No auth available, continue without header
}3. Missing Error Status Details (api.ts:138-139)if (!response.ok) {
throw new AuthApiError(`Failed to fetch user info: ${response.status}`);
}Should include Medium Priority Issues4. Incomplete Comment (login.ts:87)// For now, we store placeholder values until a /userinfo endpoint is availableThis comment is outdated - the code actually calls 5. Silent Token Refresh Failure (config.ts:107-109)} catch {
// Refresh failed - delete auth, user needs to login again
await deleteAuth();
return null;
}Silently swallowing all errors makes debugging difficult. Consider:
Better approach: } catch (error) {
if (error instanceof AuthApiError && error.message.includes('invalid_grant')) {
// Invalid refresh token - delete auth
await deleteAuth();
}
// For other errors (network, etc.), log but don't delete auth
console.error('Token refresh failed:', error);
return null;
}6. Missing Polling Error Handling (api.ts:76-77)The code handles Recommendation: if (error === "authorization_pending") {
return null;
} else if (error === "slow_down") {
// Should signal to increase polling interval
throw new Error("SLOW_DOWN"); // Let caller handle interval increase
}7. Token Type Not Validated (schema.ts:42)The token_type: z.string().refine(val => val.toLowerCase() === 'bearer', {
message: "Only Bearer tokens are supported"
}),Minor Issues8. Inconsistent Error Messages (api.ts:68-69, 116-117)Some errors use 9. Outdated Dependency PatternUsing default export for 10. Missing JSDoc for Public APIsFunctions like 🧪 Test CoverageMajor Gap: No tests are included for the new authentication functionality. Recommended test coverage:
🎯 Performance ConsiderationsPositive
Potential Improvements
📋 Additional Recommendations
🔐 Security Best Practices - Additional Notes
🎬 VerdictApproval: Conditional - The implementation is solid overall with good architecture, but has critical issues with token refresh timing that should be fixed before merge. Priority fixes before merge:
Post-merge improvements:
Great work overall! The OAuth implementation is architecturally sound and follows security best practices. Once the token refresh timing issues are resolved, this will be a solid foundation for the CLI authentication system. 🚀 |
Pull Request Review: feat(login): implement login flow with httpSummaryThis PR implements the actual OAuth device code flow with HTTP calls, replacing the mock implementation. Overall, the implementation is solid and follows OAuth 2.0 device flow standards. However, there are several areas that need attention. 🔴 Critical Issues1. Race Condition in Token Refresh (httpClient.ts:56-62)Location: The if (isTokenExpired(auth)) {
const newAccessToken = await refreshAndSaveTokens();
if (newAccessToken) {
request.headers.set("Authorization", `Bearer ${newAccessToken}`);
return;
}
}Problem: If two requests detect an expired token simultaneously, both will call Impact: Potential 401 errors on concurrent requests. Recommendation: After proactive refresh completes, re-read auth to ensure consistency: if (isTokenExpired(auth)) {
await refreshAndSaveTokens();
const refreshedAuth = await readAuth();
request.headers.set("Authorization", `Bearer ${refreshedAuth.accessToken}`);
return;
}2. Silent Error Swallowing (httpClient.ts:65-67)Location: } catch {
// No auth available, continue without header
}Problem: This swallows ALL errors, not just "auth file not found". If there's a parsing error, disk I/O error, or validation error, they'll be silently ignored. Impact: Debugging issues will be extremely difficult. Users won't know why requests are failing. Recommendation: } catch (error) {
if (error instanceof Error && error.message.includes("Authentication file not found")) {
// No auth available, continue without header
return;
}
// Log or rethrow other errors
throw error;
}3. Error Handling in auth/api.ts (multiple locations)Location: The const response = await authClient.get("oauth/userinfo", {
headers: { Authorization: `Bearer ${accessToken}` },
});Problem: If ky throws by default, this will throw a ky HTTPError instead of the AuthApiError you're checking for elsewhere. Inconsistent with the pattern used in other functions. Recommendation: Add 🟡 Code Quality Issues4. Unused Import (auth/api.ts:15)Location: import httpClient from "../utils/httpClient.js";Issue: Recommendation: Remove the unused import. 5. Outdated Comment (auth/login.ts:87)Location: // For now, we store placeholder values until a /userinfo endpoint is availableIssue: The comment is outdated - the Recommendation: Remove the outdated comment or update it. 6. Typo in Success Message (auth/login.ts:111)Location: log.success(`Successfully logged as ${chalk.bold(userInfo.email)}`);Issue: Grammar error - should be "logged in as" not "logged as" Recommendation: Fix to: 7. Error Handling Changed Without Update (cli/utils/runCommand.ts:20-24)Location: The error handling was simplified but now prints full stack traces: if (e instanceof Error) {
log.error(e.stack ?? e.message);
}Issue: Stack traces can be overwhelming for users. The previous version had specialized handling for Recommendation: Keep specialized error handling for known error types, only show stack traces for unexpected errors: if (e instanceof AuthValidationError || e instanceof AuthApiError) {
log.error(e.message);
} else if (e instanceof Error) {
log.error(e.stack ?? e.message);
}🟢 Security Considerations8. Token Storage ✅Tokens are stored in a local file. Ensure the auth file has appropriate permissions (600) to prevent other users from reading it. Recommendation: Verify in 9. No Token in Logs ✅Good job not logging tokens or sensitive data. 10. HTTPS Enforcement
|
Pull Request Review: feat(login): implement login flow with httpOverviewThis PR implements a real OAuth device code flow replacing the mock implementation. The changes add proper HTTP API integration using 🔴 Critical Issues1. Race Condition in Token Refresh (src/core/auth/config.ts:88-112)The if (refreshPromise) {
return refreshPromise;
}
refreshPromise = (async () => {
// ... refresh logic
})();Problem: There's a small window between checking Solution: Consider using a proper mutex/lock pattern or ensure the assignment happens before any await points: if (refreshPromise) {
return refreshPromise;
}
const promise = (async () => {
// ... logic
})();
refreshPromise = promise;
return promise;2. Unsafe Request Mutation (src/core/utils/httpClient.ts:39-41)retriedRequests.add(request);
request.headers.set("Authorization", `Bearer ${newAccessToken}`);
return ky(request);Problem: Mutating the original Solution: Clone the request or use ky's options: const options = { headers: { Authorization: `Bearer ${newAccessToken}` } };
return ky(request, options);3. Missing Error Context in Login Flow (src/cli/commands/auth/login.ts:37-79)The } catch (error) {
log.error("Authentication timeout or error");
throw error;
}Problem: Users won't know why authentication failed (network error, server error, timeout, etc.). The generic error message provides no actionable information. Solution: Provide specific error messages based on error type and include relevant details.
|
Pull Request Review: Login Flow ImplementationOverviewThis PR implements the actual OAuth 2.0 device flow for authentication, replacing the previous mock implementation. Overall, the implementation is well-structured and follows OAuth 2.0 standards. Below are my findings organized by category. ✅ Code Quality & Best PracticesStrengths
Issues & Suggestions1. Circular dependency risk
2. Missing error context in const response = await authClient.get("oauth/userinfo", {
headers: { Authorization: `Bearer ${accessToken}` },
throwHttpErrors: false, // Add this
});3. Silent error swallowing in httpClient 🔧 4. Unused variable 🧹 5. Error handling simplification removed 📝 🐛 Potential Bugs1. WeakSet memory leak potential 2. Token expiration check edge case 🔧 3. Double refresh possibility
If a token expires between the 4. Auth file deletion on refresh failure
⚡ Performance Considerations1. No request timeout configuration 📝 2. Polling interval optimization ✅ 3. Unnecessary promise overhead 🔧 const deviceCodeResponse = await runTask(
"Generating device code...",
generateDeviceCode, // Direct reference
{
successMessage: "Device code generated",
errorMessage: "Failed to generate device code",
}
);🔒 Security Concerns1. Client ID in source code
2. Token storage ✅
3. No token type validation 📝 token_type: z.literal("Bearer").or(z.literal("bearer")),4. Environment variable for API URL 🧪 Test CoverageCritical Issue: No tests for authentication flow ❌ The PR adds significant authentication logic but includes no tests. I found only Recommended test coverage:
📚 Additional Recommendations1. Add JSDoc comments 📝 2. Consider rate limiting 💡 3. Add logging 📝
4. Package version retrieval ✅ 5. Consider refresh token rotation 📝 6. Export missing types 🔧 export { default as httpClient } from "./httpClient.js";Summary
Priority Actions:
The implementation demonstrates good understanding of OAuth 2.0 device flow and includes thoughtful features like token refresh with race condition prevention. With tests and the above fixes, this will be solid production code. Great work overall! The architecture is clean and extensible. 🎉 |
* fix issue with error throwing, add refresh mechanism * added refresh logic fixes * fix package version issue and add a userInfo method * actual call the getUserInfo service * added userInfo call and log * small ui changes * get user info with access token * tiny changes * fix issue with correct way to use ky
* fix issue with error throwing, add refresh mechanism * added refresh logic fixes * fix package version issue and add a userInfo method * actual call the getUserInfo service * added userInfo call and log * small ui changes * get user info with access token * tiny changes * fix issue with correct way to use ky
* feat(login): implement login flow with http (#10) * fix issue with error throwing, add refresh mechanism * added refresh logic fixes * fix package version issue and add a userInfo method * actual call the getUserInfo service * added userInfo call and log * small ui changes * get user info with access token * tiny changes * fix issue with correct way to use ky * move the client init into the function * dont lazy load the appclient * performance fix for entity api
Summary
Implement actual login flow with http calls --
This is waiting for https://github.com/base44-dev/apper/pull/2449