✨ Add CLI authentication with OAuth device flow#56
Conversation
Implements comprehensive authentication system for the CLI: **Authentication Flow:** - OAuth 2.0 device authorization grant flow - Browser-based login with auto-filled code - User/organization context display - Token refresh with 30-day expiry - Secure token storage in ~/.vizzly/config.json (0600 perms) **Commands:** - `vizzly login` - Authenticate via browser OAuth flow - `vizzly logout` - Clear stored tokens - `vizzly whoami` - Display user info and token status - `vizzly project:select` - Configure project for directory - `vizzly project:list` - Show all configured projects - `vizzly project:token` - Display project token - `vizzly project:remove` - Remove project configuration **Token Priority (highest to lowest):** 1. CLI flag (--token) 2. Environment variable (VIZZLY_TOKEN) 3. Project mapping (~/.vizzly/config.json) 4. User access token (~/.vizzly/config.json) **Key Features:** - Project-specific tokens mapped to directories - Automatic token refresh on 401 responses - Graceful fallback through token priority chain - Directory traversal for project mapping lookup - JSON output support for scripting **Test Coverage:** - 114 new tests (698 total, all passing) - 100% coverage of auth features: - Global config utilities (28 tests) - Token resolution priority (16 tests) - AuthService (21 tests) - Login command (11 tests) - Logout command (8 tests) - Whoami command (14 tests) - Project commands (16 tests)
Code Review: CLI Authentication with OAuth Device FlowI've completed a comprehensive review of PR #56. This is an excellent implementation of OAuth device flow authentication! The code is well-structured, thoroughly tested, and follows security best practices. ✅ Strengths1. Excellent Architecture & Design
2. Outstanding Test Coverage
3. Security Best Practices
4. Great Developer Experience
🔍 Issues & RecommendationsHigh Priority1. Command Injection Vulnerability in browser.js 🚨Location: src/utils/browser.js:20-26 Issue: URLs are not escaped before being passed to shell commands. A malicious URL could execute arbitrary commands. Fix: Use execFile instead of exec with proper argument arrays to prevent shell injection. 2. Missing Input Validation in Project CommandsLocation: src/commands/project.js:88-93 The token extraction logic handles backward compatibility but doesn't validate token format. The fallback String(projectMapping.token) could produce invalid values. Recommendation: Add validation for token format (should match vzt_... pattern). Medium Priority3. Race Condition in Login FlowLocation: src/commands/login.js:77-87 The login command waits for Enter key press, then polls once. Users might press Enter before authorizing in the browser. Better approach: Implement polling with timeout (e.g., 60 attempts with 5-second intervals). 4. No Token Format ValidationProject tokens should follow format vzt_... but there's no validation. Consider adding a token validator utility. 5. Silent Errors in LogoutServer revocation failures only log to console. In --json mode, users won't see this. Consider logging to stderr. 6. Directory Permissions Not VerifiedThe directory is created with 0o700, but permissions aren't verified if it already exists. 🔒 Security AssessmentGood:
Concerns:
🎯 Overall AssessmentGrade: A- (Excellent work!) This PR demonstrates:
Recommendation: Approve with required fix for command injection vulnerability The command injection issue in browser.js is the only blocking concern. Everything else is polish and can be addressed in follow-up PRs if needed. 🎉 Great Job!This is a well-thought-out implementation. The OAuth device flow is perfect for CLI tools, the test coverage is exemplary, and the UX is polished. Fix the command injection issue and this is ready to ship! Files reviewed: 18 changed files, ~4000 lines |
- Replace exec() with execFile() to prevent shell injection attacks - Pass URLs as discrete arguments instead of string interpolation - Add comprehensive test coverage (16 tests) for all platforms - Test security scenarios with malicious URL patterns 📚 Add authentication documentation - Update README with authentication commands and token priority - Create comprehensive authentication guide (docs/authentication.md) - Update getting-started.md with auth workflow options - Add authentication commands to API reference - Document OAuth device flow, token refresh, and security
Summary
Implements a comprehensive authentication system for the Vizzly CLI using OAuth 2.0 device authorization grant flow.
Features
Authentication Commands
vizzly login- Browser-based OAuth authentication with auto-filled device codevizzly logout- Clear stored authentication tokensvizzly whoami- Display current user, organizations, and token statusvizzly project:select- Configure project-specific token for current directoryvizzly project:list- Show all configured projectsvizzly project:token- Display project token for current directoryvizzly project:remove- Remove project configurationToken Management
Token Priority (highest to lowest):
--token)VIZZLY_TOKEN)~/.vizzly/config.json)~/.vizzly/config.json)Features:
~/.vizzly/config.jsonwith 0600 permissionsDeveloper Experience
--json)--verbose)Technical Implementation
New Files
src/commands/login.js- OAuth device flow implementationsrc/commands/logout.js- Token cleanupsrc/commands/whoami.js- User info displaysrc/commands/project.js- Project selection/managementsrc/services/auth-service.js- Authentication API clientsrc/utils/global-config.js- Token storage utilitiessrc/utils/browser.js- Cross-platform browser openingModified Files
src/cli.js- Added new commandssrc/services/api-service.js- Token refresh on 401, accepts bothapiKeyandtokensrc/utils/config-loader.js- Token resolution priority logicsrc/commands/run.js- Minor updates for auth integrationTest Coverage
114 new tests added (698 total, all passing)
Testing approach:
Breaking Changes
None - this is purely additive functionality.
Migration Guide
Users can now authenticate in two ways:
Option 1: User authentication (recommended for local development)
Option 2: Environment variable (recommended for CI)
The CLI will automatically use the appropriate token based on priority.
Related Issues
Resolves VIZ-21