Skip to content

Conversation

@Robdel12
Copy link
Contributor

@Robdel12 Robdel12 commented Dec 1, 2025

Summary

  • Remove fallback that used JWT access tokens as SDK API keys
  • Update error messages to guide users to link a project via vizzly project:select
  • Update tests to reflect new behavior

Problem

After running vizzly login, commands like vizzly doctor would fail with "Invalid API token format" because the JWT was being sent to SDK endpoints that expect vzt_* format tokens.

Root Cause

config-loader.js had a fallback (lines 106-111) that used the user's JWT access token as apiKey when no project token was configured. SDK endpoints reject JWTs since they expect project tokens.

Token Types

Token Format Purpose
User JWT eyJ... User auth: login, list orgs/projects
Project Token vzt_* SDK operations: upload screenshots

The Fix

Remove the JWT fallback. Users must either:

  1. Set VIZZLY_TOKEN environment variable
  2. Link a project via vizzly project:select (stores vzt_* token)

Fixes #95

User access tokens (JWTs from `vizzly login`) were being used as a
fallback for SDK API keys when no project token was configured. SDK
endpoints expect `vzt_*` format tokens, causing "Invalid API token
format" errors.

- Remove JWT fallback in config-loader (was lines 106-111)
- Update error messages to guide users to link a project
- Update tests to reflect new behavior

Fixes #95
@claude
Copy link

claude bot commented Dec 1, 2025

Code Review ✅

This is an excellent security fix that properly separates user authentication tokens (JWTs) from project API tokens. The implementation is clean and well-tested.

Strengths

1. Security & Correctness

  • Critical fix: Removes the dangerous fallback that used JWT access tokens for SDK endpoints (config-loader.js:106-111)
  • Proper token separation: User JWTs are now correctly isolated for user-level operations only (login, list orgs/projects)
  • Clear token hierarchy: The priority chain (CLI flag > Env var > Project mapping) is now secure and well-defined

2. Error Messages

  • Helpful guidance: Updated error messages guide users to vizzly project:select or setting VIZZLY_TOKEN rather than just vizzly login
  • Consistency: Both error paths in api-service.js:121 and api-service.js:127 now have identical, clear messaging

3. Test Coverage

  • Comprehensive updates: All test cases properly updated to reflect the new behavior
  • Well-documented: Test file header clearly explains the priority order and why JWTs aren't used as API keys
  • Security-focused naming: Test renamed from "should use user access token when no other sources" to "should NOT use user access token as API key (security fix)" - much clearer intent

4. Documentation

  • Clear commit message: Uses gitmoji (🐛) and explains both the problem and solution
  • Excellent PR description: Table showing token types and their purposes is particularly helpful
  • Inline comments: Added comments in tests explaining the security rationale

🔍 Observations

1. Unused Import (Minor)
The getAccessToken import is now unused in config-loader.js:5 since the fallback was removed. Consider cleaning this up:

-import { getAccessToken, getProjectMapping } from './global-config.js';
+import { getProjectMapping } from './global-config.js';

2. Backwards Compatibility
This is a breaking change for users who were inadvertently relying on the JWT fallback after running vizzly login. However, this breakage is necessary and correct since the old behavior was fundamentally broken. Users will need to:

  • Run vizzly project:select to link a project, OR
  • Set VIZZLY_TOKEN environment variable

The error messages properly guide users through this, so the migration path is clear.

3. Token Refresh Logic
The token refresh logic in api-service.js:79-117 correctly uses JWT refresh tokens for user authentication. This is appropriate since it's for user-level API calls, not SDK endpoints. Good separation of concerns here.

📊 Test Coverage

The test updates are thorough:

  • ✅ All priority levels tested
  • ✅ Edge cases covered (null mappings, missing token field)
  • ✅ Negative case explicitly tested (JWT should NOT be used)
  • ✅ Security rationale documented in comments

🎯 Overall Assessment

This PR correctly fixes a security/correctness issue where user authentication tokens were being incorrectly used for SDK operations. The fix is:

  • Secure: Proper token separation
  • Well-tested: Comprehensive test updates
  • Well-documented: Clear error messages and comments
  • Minimal: Only changes what's necessary

Recommendation: Approve and merge

Optional Cleanup

Consider removing the unused getAccessToken import in a follow-up commit (or amend this one if you'd like).


Fixes #95 as stated - the root cause has been properly addressed.

@Robdel12 Robdel12 merged commit 3b7703e into main Dec 1, 2025
18 checks passed
@Robdel12 Robdel12 deleted the fix/jwt-token-api-key-fallback branch December 1, 2025 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Authentication error: Invalid or expired API token

2 participants