fix: resolve login issues for users with CLI authentication blocked by browsers or firewalls#1492
Conversation
…y browsers or firewalls
🦋 Changeset detectedLatest commit: 67b4e48 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughReplaces the CLI's local OAuth redirect flow with a backend-managed PKCE flow using RSA-based end-to-end encryption for token delivery; adds CLI crypto utilities, updates auth error codes, and removes the deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Backend as Backend API
participant Browser as Browser
rect rgb(200,225,255)
note over CLI: 1) Key generation & flow start
CLI->>CLI: generateKeyPair() → {publicKey, privateKey}
CLI->>Backend: POST /api/auth/cli/start { public_key }
Backend-->>CLI: { flow_id, verification_url }
end
rect rgb(255,245,200)
note over CLI,Browser: 2) User authorization
CLI->>Browser: open(verification_url)
Browser->>Backend: User authenticates & authorizes
Backend-->>Backend: complete PKCE exchange, store encrypted tokens
end
rect rgb(200,255,220)
note over CLI,Backend: 3) Polling for completion
loop until complete / timeout
CLI->>Backend: GET /api/auth/cli/status?flow_id
Backend-->>CLI: { status, encrypted_payload? }
end
end
rect rgb(230,230,255)
note over CLI: 4) Decrypt & finalize
CLI->>CLI: decryptTokens(encrypted_payload, privateKey) → tokens
CLI->>CLI: construct AuthCredentials / handle MFA / emit callbacks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/tm-core/src/modules/auth/utils/cli-crypto.ts (1)
68-98: Consider wrapping crypto operations and JSON parsing in try-catch for clearer error handling.The
JSON.parseon line 97 and crypto operations can throw various errors (malformed data, incorrect keys, tampered ciphertext). Currently, these would bubble up as generic errors. Wrapping them would allow returning domain-specificAuthenticationErrorwith appropriate codes.export function decryptTokens( payload: EncryptedTokenPayload, privateKeyPem: string ): DecryptedTokens { + try { // Decode base64 values const encryptedKey = Buffer.from(payload.encrypted_key, 'base64'); const encryptedData = Buffer.from(payload.encrypted_data, 'base64'); const iv = Buffer.from(payload.iv, 'base64'); const authTag = Buffer.from(payload.auth_tag, 'base64'); // Decrypt AES key using RSA private key const aesKey = crypto.privateDecrypt( { key: privateKeyPem, padding: crypto.constants.RSA_PKCS1_OAEP_PADDING, oaepHash: 'sha256' }, encryptedKey ); // Decrypt tokens using AES-256-GCM const decipher = crypto.createDecipheriv('aes-256-gcm', aesKey, iv); decipher.setAuthTag(authTag); const decrypted = Buffer.concat([ decipher.update(encryptedData), decipher.final() ]); return JSON.parse(decrypted.toString('utf8')) as DecryptedTokens; + } catch (error) { + throw new Error( + `Token decryption failed: ${error instanceof Error ? error.message : 'Unknown error'}` + ); + } }packages/tm-core/src/modules/auth/services/oauth-service.ts (1)
147-182: Minor: Step numbering in comments is inconsistent.Lines 151 and 171 both say "Step 2". Consider renumbering to reflect the actual sequence.
- // Step 2: Start the flow on the backend with our public key + // Step 2: Start the flow on the backend with our public key const startResponse = await this.startBackendFlow(); ... - // Step 2: Open browser with verification URL + // Step 3: Open browser with verification URL if (openBrowser && verification_url) { ... - // Step 3: Poll for completion + // Step 4: Poll for completion const credentials = await this.pollForCompletion(
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/six-eels-send.md(1 hunks)packages/tm-core/src/modules/auth/index.ts(0 hunks)packages/tm-core/src/modules/auth/services/oauth-service.ts(5 hunks)packages/tm-core/src/modules/auth/types.ts(1 hunks)packages/tm-core/src/modules/auth/utils/cli-crypto.ts(1 hunks)packages/tm-core/src/modules/auth/utils/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/tm-core/src/modules/auth/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
TypeScript test files must achieve minimum code coverage thresholds: 80% lines/functions and 70% branches globally, 90% for utilities, and 85% for middleware; new features must meet or exceed these thresholds
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/types.tspackages/tm-core/src/modules/auth/utils/index.tspackages/tm-core/src/modules/auth/services/oauth-service.ts
**/{utils,utilities,helpers}/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/{utils,utilities,helpers}/**/*.{js,ts}: Document all parameters and return values in JSDoc format, include descriptions for complex logic, and add examples for non-obvious usage
Support multiple log levels (debug, info, warn, error) with appropriate icons for different log levels and respect the configured log level
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/utils/index.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/*.{js,ts}: Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Do not add direct console.log calls outside the logging utility - use the central log function instead
Ensure silent mode is disabled in a finally block to prevent it from staying enabled
Do not access the global silentMode variable directly - use the exported silent mode control functions instead
Do not duplicate task ID formatting logic across modules - centralize formatting utilities
Use ContextGatherer class from utils/contextGatherer.js for AI-powered commands that need project context, supporting tasks, files, custom text, and project tree context
Use FuzzyTaskSearch class from utils/fuzzyTaskSearch.js for automatic task relevance detection with configurable search parameters
Use fuzzy search to supplement user-provided task IDs and display discovered task IDs to users for transparency
Do not replace explicit user task selections with fuzzy results - fuzzy search should supplement, not replace user selections
Use readJSON and writeJSON utilities for all JSON file operations instead of raw fs.readFileSync or fs.writeFileSync
Include error handling for JSON file operations and validate JSON structure after reading
Use path.join() for cross-platform path construction and path.resolve() for absolute paths, validating paths before file operations
Support both .env files and MCP session environment for environment variable resolution with fallbacks for missing values
Prefer updating the core function to accept an outputFormat parameter and check outputFormat === 'json' before displaying UI elements
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/types.tspackages/tm-core/src/modules/auth/utils/index.tspackages/tm-core/src/modules/auth/services/oauth-service.ts
**/{utils,utilities}/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
Implement the silent mode control functions (enableSilentMode, disableSilentMode, isSilentMode) in utility modules and always use isSilentMode() to check current state
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/utils/index.ts
**/{utils,utilities}/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/{utils,utilities}/**/*.{js,ts}: Use try/catch blocks for all file operations and return null or a default value on failure rather than allowing exceptions to propagate unhandled
Log detailed error information using the log utility in catch blocks for file operations
Create utilities for consistent task ID handling that support different ID formats (numeric, string, dot notation)
Implement reusable task finding utilities that support both task and subtask lookups and add context to subtask results
Implement cycle detection using graph traversal by tracking visited nodes and recursion stack, returning specific information about cycles
Detect circular dependencies using DFS and validate task references before operations
Export all utility functions explicitly in logical groups and include configuration constants from utility modules
Do not use default exports in utility modules - use named exports only
Group related exports together in utility modules and avoid creating circular dependencies
Make the log function respect silent mode by skipping logging when silent mode is enabled
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/utils/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Import modules with
.jsextension even in TypeScript source files for ESM compatibility
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/types.tspackages/tm-core/src/modules/auth/utils/index.tspackages/tm-core/src/modules/auth/services/oauth-service.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-11-24T18:00:23.032Z
Learning: Pull Request descriptions must include: Task Overview, Subtasks Completed (checklist), Implementation Details, Testing approach, Breaking Changes (if any), and Related Tasks.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1444
File: apps/cli/src/utils/auto-update/changelog.ts:103-111
Timestamp: 2025-11-25T18:32:29.828Z
Learning: The claude-task-master project uses a custom changelog format with PR numbers and author acknowledgements in the pattern `- [#PR](...) Thanks [author]! - Description`, which is parsed by the regex in apps/cli/src/utils/auto-update/changelog.ts.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-11-24T17:58:19.853Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/changeset.mdc:0-0
Timestamp: 2025-11-24T17:58:19.853Z
Learning: Provide a concise, single-line changeset summary in imperative mood (e.g., 'Add feature X', 'Fix bug Y') that describes what changed from a user/consumer perspective, not implementation details
Applied to files:
.changeset/six-eels-send.md
📚 Learning: 2025-07-17T21:33:57.585Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1002
File: .changeset/puny-friends-give.md:2-3
Timestamp: 2025-07-17T21:33:57.585Z
Learning: In the eyaltoledano/claude-task-master repository, the MCP server code in mcp-server/src/ is part of the main "task-master-ai" package, not a separate "mcp-server" package. When creating changesets for MCP server changes, use "task-master-ai" as the package name.
Applied to files:
.changeset/six-eels-send.md
📚 Learning: 2025-11-24T17:56:52.249Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/GEMINI.md:0-0
Timestamp: 2025-11-24T17:56:52.249Z
Learning: Applies to assets/.gemini/settings.json : Configure Task Master MCP server in ~/.gemini/settings.json with the command 'npx' and args ['-y', 'task-master-ai']
Applied to files:
.changeset/six-eels-send.md
📚 Learning: 2025-10-31T20:59:04.022Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1360
File: apps/cli/src/commands/models/fetchers.ts:12-64
Timestamp: 2025-10-31T20:59:04.022Z
Learning: For eyaltoledano/claude-task-master, when fetching the OpenRouter models list in apps/cli/src/commands/models/fetchers.ts, do not require an API key. Prefer an optional Authorization header if OPENROUTER_API_KEY is set, and surface a clear 401 error message if encountered.
Applied to files:
.changeset/six-eels-send.md
📚 Learning: 2025-11-24T22:09:45.455Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T22:09:45.455Z
Learning: Applies to apps/cli/src/**/*.{ts,tsx} : CLI (tm/cli) should be a thin presentation layer that calls tm-core methods and displays results; handle only CLI-specific concerns like argument parsing, output formatting, and user prompts
Applied to files:
packages/tm-core/src/modules/auth/utils/cli-crypto.ts
📚 Learning: 2025-10-08T19:57:00.982Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1282
File: packages/tm-core/src/utils/index.ts:16-34
Timestamp: 2025-10-08T19:57:00.982Z
Learning: For the tm-core package in the eyaltoledano/claude-task-master repository, the team prefers a minimal, need-based export strategy in index files rather than exposing all internal utilities. Exports should only be added when functions are actually consumed by other packages in the monorepo.
Applied to files:
packages/tm-core/src/modules/auth/utils/index.ts
📚 Learning: 2025-09-26T19:05:47.555Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:11-13
Timestamp: 2025-09-26T19:05:47.555Z
Learning: In the eyaltoledano/claude-task-master repository, internal tm/ packages use a specific export pattern where the "exports" field points to TypeScript source files (./src/index.ts) while "main" points to compiled output (./dist/index.js) and "types" points to source files (./src/index.ts). This pattern is used consistently across internal packages like tm/core and tm/ai-sdk-provider-grok-cli because they are consumed directly during build-time bundling with tsdown rather than being published as separate packages.
Applied to files:
packages/tm-core/src/modules/auth/utils/index.ts
📚 Learning: 2025-11-24T18:04:43.972Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.972Z
Learning: Applies to **/{utils,utilities}/**/*.{js,ts} : Export all utility functions explicitly in logical groups and include configuration constants from utility modules
Applied to files:
packages/tm-core/src/modules/auth/utils/index.ts
📚 Learning: 2025-11-24T18:04:43.972Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.972Z
Learning: Applies to **/{utils,utilities}/**/*.{js,ts} : Group related exports together in utility modules and avoid creating circular dependencies
Applied to files:
packages/tm-core/src/modules/auth/utils/index.ts
📚 Learning: 2025-11-24T18:04:43.972Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.972Z
Learning: Applies to **/{utils,utilities}/**/*.{js,ts} : Do not use default exports in utility modules - use named exports only
Applied to files:
packages/tm-core/src/modules/auth/utils/index.ts
📚 Learning: 2025-10-15T14:43:40.410Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.410Z
Learning: In the claude-task-master codebase, `getCredentials()` in `credential-store.ts` defaults to `allowExpired: true` to enable Supabase refresh flows. The Supabase client automatically handles token refresh when credentials are expired but have a valid refresh token. The `SupabaseSessionStorage` updates the credentials after Supabase performs the refresh. This is an intentional design pattern where the credential store is passive storage and Supabase manages the active token lifecycle.
Applied to files:
packages/tm-core/src/modules/auth/services/oauth-service.ts
🧬 Code graph analysis (1)
packages/tm-core/src/modules/auth/services/oauth-service.ts (5)
packages/tm-core/src/modules/auth/utils/cli-crypto.ts (4)
EncryptedTokenPayload(15-20)AuthKeyPair(36-39)generateKeyPair(45-59)decryptTokens(68-98)packages/tm-core/src/modules/auth/utils/index.ts (4)
EncryptedTokenPayload(13-13)AuthKeyPair(15-15)generateKeyPair(16-16)decryptTokens(17-17)packages/tm-core/src/modules/auth/index.ts (4)
OAuthService(8-8)OAuthFlowOptions(17-17)AuthCredentials(16-16)AuthenticationError(23-23)packages/tm-core/src/modules/auth/types.ts (3)
OAuthFlowOptions(32-45)AuthCredentials(5-14)AuthenticationError(120-137)scripts/modules/commands.js (1)
pollInterval(539-539)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (8)
.changeset/six-eels-send.md (1)
1-5: LGTM!The changeset follows the expected format with a concise, user-facing description in imperative mood.
packages/tm-core/src/modules/auth/types.ts (1)
107-115: LGTM!The new error codes appropriately reflect the backend-driven PKCE flow architecture. The grouping comments help clarify the error categories.
packages/tm-core/src/modules/auth/utils/index.ts (1)
11-18: LGTM!The exports are properly grouped with named exports and
.jsextension for ESM compatibility. This aligns with the team's minimal, need-based export strategy. Based on learnings and coding guidelines.packages/tm-core/src/modules/auth/utils/cli-crypto.ts (1)
45-59: LGTM!The RSA 2048-bit key generation with SPKI/PKCS8 PEM formats is appropriate for secure key exchange.
packages/tm-core/src/modules/auth/services/oauth-service.ts (4)
32-56: LGTM!The response interfaces are well-structured with appropriate optional fields for the backend PKCE flow.
248-263: Verify that sending username and hostname is acceptable for your privacy policy.The request body includes
os.hostname()andos.userInfo().username. While useful for device identification, this is PII that users may not expect to be transmitted. Ensure this aligns with your privacy policy and consider whether this data is strictly necessary.
309-409: The polling loop is well-implemented with proper timeout and error handling.Good resilience pattern: network errors are logged but don't abort the polling loop, allowing transient failures to recover. The status handling covers all expected states appropriately.
345-372: LGTM on the token decryption flow.The complete status handler correctly validates
encrypted_tokenspresence before decryption and constructsAuthCredentialswith proper field mapping.
| const session: Session = { | ||
| access_token: credentials.token, | ||
| refresh_token: credentials.refreshToken || '', | ||
| expires_in: credentials.expiresAt | ||
| ? Math.floor( | ||
| (new Date(credentials.expiresAt).getTime() - Date.now()) / 1000 | ||
| ) | ||
| : 3600, | ||
| token_type: 'bearer', | ||
| user: { | ||
| id: credentials.userId, | ||
| email: credentials.email, | ||
| app_metadata: {}, | ||
| user_metadata: {}, | ||
| aud: 'authenticated', | ||
| created_at: '' | ||
| } | ||
| }; | ||
|
|
||
| // Signal that the server is ready and URL is available | ||
| if (this.resolveAuthorizationReady) { | ||
| this.resolveAuthorizationReady(); | ||
| this.resolveAuthorizationReady = null; | ||
| } | ||
| }); | ||
| await this.supabaseClient.setSession(session); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for setSession calls and definitions in the codebase
rg -n "setSession" --type ts -B 2 -A 5Repository: eyaltoledano/claude-task-master
Length of output: 2774
🏁 Script executed:
# Look for Supabase type definitions and imports
rg -n "Session\|refresh_token" --type ts -B 1 -A 1 | head -100Repository: eyaltoledano/claude-task-master
Length of output: 57
🏁 Script executed:
# Check the oauth-service.ts file structure and any error handling
fd "oauth-service.ts" --exec cat -n {} \;Repository: eyaltoledano/claude-task-master
Length of output: 15400
🏁 Script executed:
# Look for Supabase type definitions or documentation references
rg -n "@supabase|refresh_token" --type ts -B 1 -A 1 | grep -A 5 -B 5 "setSession\|refresh_token" | head -80Repository: eyaltoledano/claude-task-master
Length of output: 6721
🏁 Script executed:
# Check test files for setSession and refresh_token patterns
find . -name "*.spec.ts" -o -name "*.test.ts" | xargs rg -l "setSession\|refresh_token" 2>/dev/nullRepository: eyaltoledano/claude-task-master
Length of output: 59
🏁 Script executed:
# Look at the supabase-client implementation more carefully
sed -n '340,360p' packages/tm-core/src/modules/integration/clients/supabase-client.tsRepository: eyaltoledano/claude-task-master
Length of output: 593
🌐 Web query:
Supabase JavaScript client setSession refresh_token empty string behavior
💡 Result:
Short answer: setSession expects a valid (non-empty) refresh_token. If you pass an empty string for refresh_token the client won't be able to obtain/rotate a session — you will not get a refreshed session and getSession/getUser will remain null (or an error about invalid refresh token will be returned). The docs require a refresh_token and say an invalid refresh token will throw; community threads confirm relying on empty tokens is brittle and only an empty access_token (not refresh_token) was historically used as a hack to force refresh in older client versions. [1][2][3]
Sources
- Supabase JS setSession docs (requires refresh_token; invalid refresh token throws). [1]
- Supabase docs for refresh/refreshSession behavior. [2]
- Community/GitHub discussion describing hacks (empty access_token) and setSession/refresh nuances. [3]
Empty refresh_token string will break Supabase session refresh capability.
When credentials.refreshToken is undefined, passing an empty string '' prevents Supabase's automatic token refresh mechanism from functioning. Supabase requires a valid refresh_token to rotate sessions; an empty string causes getSession() or getUser() to return null or throw an invalid refresh token error. This is inconsistent with the design pattern documented in the learnings where Supabase automatically manages token lifecycle for valid tokens. Instead of defaulting to an empty string, either ensure refreshToken is always populated or pass undefined/null to avoid breaking the refresh flow.
🤖 Prompt for AI Agents
In packages/tm-core/src/modules/auth/services/oauth-service.ts around lines 197
to 216, the session object sets refresh_token to an empty string when
credentials.refreshToken is missing which breaks Supabase's refresh flow; change
the assignment to pass undefined/null instead of '' (e.g., set refresh_token to
credentials.refreshToken ?? undefined or omit the refresh_token property when
missing) so Supabase can handle token rotation correctly, and keep the rest of
the session fields and the await this.supabaseClient.setSession(session) call
as-is.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/tm-core/src/modules/auth/services/oauth-service.ts (1)
205-224: Emptyrefresh_tokenstring still breaks Supabase session refresh.The warning at lines 199-203 acknowledges the issue, but the code still passes an empty string to
setSession. As noted in the previous review, Supabase requires a validrefresh_tokento manage token lifecycle—an empty string prevents automatic refresh. Consider passingundefinedinstead, or skip callingsetSessionentirely when no refresh token is available.const session: Session = { access_token: credentials.token, - refresh_token: credentials.refreshToken ?? '', + refresh_token: credentials.refreshToken ?? undefined as unknown as string,Or conditionally skip session setup when refresh token is missing.
🧹 Nitpick comments (4)
packages/tm-core/src/modules/auth/services/oauth-service.ts (3)
64-64: Consider clearing the keypair after successful authentication.The private key remains in memory after authentication completes. For defense in depth, consider clearing
this.keyPairat the end ofauthenticateWithBackendPKCEto minimize the window where the private key is exposed in memory.+ // Clear keypair from memory after use + this.keyPair = null; + return credentials; }
406-413: Consider limiting consecutive network failures during polling.Network errors are logged but swallowed, allowing polling to continue. If the network is persistently down, this will silently poll until timeout. Consider tracking consecutive failures and throwing early if a threshold is exceeded, to provide faster feedback to users.
+ private static readonly MAX_CONSECUTIVE_POLL_FAILURES = 5; + private async pollForCompletion( flowId: string, pollInterval: number, timeout: number ): Promise<AuthCredentials> { const statusUrl = `${this.baseUrl}/api/auth/cli/status?flow_id=${flowId}`; const startTime = Date.now(); + let consecutiveFailures = 0; // ... existing code ... } catch (error) { if (error instanceof AuthenticationError) { throw error; } - // Log network errors but continue polling - this.logger.debug('Poll request failed, will retry:', error); + consecutiveFailures++; + this.logger.debug( + `Poll request failed (attempt ${consecutiveFailures}), will retry:`, + error + ); + if (consecutiveFailures >= OAuthService.MAX_CONSECUTIVE_POLL_FAILURES) { + throw new AuthenticationError( + 'Too many consecutive network failures during polling', + 'BACKEND_UNREACHABLE', + error + ); + } } + + // Reset on successful poll (even if status is pending) + consecutiveFailures = 0;
263-270:os.userInfo()may throw on some systems.On certain platforms or containerized environments,
os.userInfo()can throw if user information is unavailable. Consider adding a fallback.body: JSON.stringify({ name: 'Task Master CLI', version: this.getCliVersion(), device: os.hostname(), - user: os.userInfo().username, + user: this.getSafeUsername(), platform: os.platform(), public_key: this.keyPair.publicKey })private getSafeUsername(): string { try { return os.userInfo().username; } catch { return 'unknown'; } }packages/tm-core/src/modules/auth/utils/cli-crypto.ts (1)
99-99: Consider validating the decrypted token structure.The JSON.parse result is cast directly to
DecryptedTokenswithout runtime validation. If the backend returns malformed data, this could lead to undefined behavior downstream. A simple validation could ensure required fields are present.- return JSON.parse(decrypted.toString('utf8')) as DecryptedTokens; + const tokens = JSON.parse(decrypted.toString('utf8')) as DecryptedTokens; + + // Validate required fields + if (!tokens.access_token || !tokens.user_id) { + throw new Error('Missing required fields in decrypted tokens'); + } + + return tokens;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
context7.json(1 hunks)packages/tm-core/src/modules/auth/services/oauth-service.ts(5 hunks)packages/tm-core/src/modules/auth/types.ts(1 hunks)packages/tm-core/src/modules/auth/utils/cli-crypto.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
TypeScript test files must achieve minimum code coverage thresholds: 80% lines/functions and 70% branches globally, 90% for utilities, and 85% for middleware; new features must meet or exceed these thresholds
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/types.tspackages/tm-core/src/modules/auth/services/oauth-service.ts
**/{utils,utilities,helpers}/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/{utils,utilities,helpers}/**/*.{js,ts}: Document all parameters and return values in JSDoc format, include descriptions for complex logic, and add examples for non-obvious usage
Support multiple log levels (debug, info, warn, error) with appropriate icons for different log levels and respect the configured log level
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/*.{js,ts}: Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Do not add direct console.log calls outside the logging utility - use the central log function instead
Ensure silent mode is disabled in a finally block to prevent it from staying enabled
Do not access the global silentMode variable directly - use the exported silent mode control functions instead
Do not duplicate task ID formatting logic across modules - centralize formatting utilities
Use ContextGatherer class from utils/contextGatherer.js for AI-powered commands that need project context, supporting tasks, files, custom text, and project tree context
Use FuzzyTaskSearch class from utils/fuzzyTaskSearch.js for automatic task relevance detection with configurable search parameters
Use fuzzy search to supplement user-provided task IDs and display discovered task IDs to users for transparency
Do not replace explicit user task selections with fuzzy results - fuzzy search should supplement, not replace user selections
Use readJSON and writeJSON utilities for all JSON file operations instead of raw fs.readFileSync or fs.writeFileSync
Include error handling for JSON file operations and validate JSON structure after reading
Use path.join() for cross-platform path construction and path.resolve() for absolute paths, validating paths before file operations
Support both .env files and MCP session environment for environment variable resolution with fallbacks for missing values
Prefer updating the core function to accept an outputFormat parameter and check outputFormat === 'json' before displaying UI elements
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/types.tspackages/tm-core/src/modules/auth/services/oauth-service.ts
**/{utils,utilities}/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
Implement the silent mode control functions (enableSilentMode, disableSilentMode, isSilentMode) in utility modules and always use isSilentMode() to check current state
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.ts
**/{utils,utilities}/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/{utils,utilities}/**/*.{js,ts}: Use try/catch blocks for all file operations and return null or a default value on failure rather than allowing exceptions to propagate unhandled
Log detailed error information using the log utility in catch blocks for file operations
Create utilities for consistent task ID handling that support different ID formats (numeric, string, dot notation)
Implement reusable task finding utilities that support both task and subtask lookups and add context to subtask results
Implement cycle detection using graph traversal by tracking visited nodes and recursion stack, returning specific information about cycles
Detect circular dependencies using DFS and validate task references before operations
Export all utility functions explicitly in logical groups and include configuration constants from utility modules
Do not use default exports in utility modules - use named exports only
Group related exports together in utility modules and avoid creating circular dependencies
Make the log function respect silent mode by skipping logging when silent mode is enabled
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Import modules with
.jsextension even in TypeScript source files for ESM compatibility
Files:
packages/tm-core/src/modules/auth/utils/cli-crypto.tspackages/tm-core/src/modules/auth/types.tspackages/tm-core/src/modules/auth/services/oauth-service.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-11-24T18:00:23.032Z
Learning: Pull Request descriptions must include: Task Overview, Subtasks Completed (checklist), Implementation Details, Testing approach, Breaking Changes (if any), and Related Tasks.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1444
File: apps/cli/src/utils/auto-update/changelog.ts:103-111
Timestamp: 2025-11-25T18:32:29.828Z
Learning: The claude-task-master project uses a custom changelog format with PR numbers and author acknowledgements in the pattern `- [#PR](...) Thanks [author]! - Description`, which is parsed by the regex in apps/cli/src/utils/auto-update/changelog.ts.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-07-31T21:48:00.389Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 997
File: apps/extension/src/services/task-repository.ts:25-57
Timestamp: 2025-07-31T21:48:00.389Z
Learning: In the eyaltoledano/claude-task-master repository, every task is always part of a tag - there is no concept of untagged tasks. The tag system is mandatory and comprehensive, meaning all tasks exist within a tag context (with 'master' as the default tag if none specified).
Applied to files:
context7.json
📚 Learning: 2025-11-24T18:05:23.901Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/AGENTS.md:0-0
Timestamp: 2025-11-24T18:05:23.901Z
Learning: Applies to assets/.claude/settings.json : Configure Claude Code tool allowlist in `.claude/settings.json` to allow Task Master bash commands and MCP tools
Applied to files:
context7.json
📚 Learning: 2025-10-31T20:59:04.022Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1360
File: apps/cli/src/commands/models/fetchers.ts:12-64
Timestamp: 2025-10-31T20:59:04.022Z
Learning: For eyaltoledano/claude-task-master, when fetching the OpenRouter models list in apps/cli/src/commands/models/fetchers.ts, do not require an API key. Prefer an optional Authorization header if OPENROUTER_API_KEY is set, and surface a clear 401 error message if encountered.
Applied to files:
context7.json
📚 Learning: 2025-09-26T19:03:33.225Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: package.json:130-132
Timestamp: 2025-09-26T19:03:33.225Z
Learning: In the eyaltoledano/claude-task-master repository, all packages with tm prefix are internal packages that are part of the monorepo structure and are not published externally.
Applied to files:
context7.json
📚 Learning: 2025-10-15T14:43:40.410Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.410Z
Learning: In the claude-task-master codebase, `getCredentials()` in `credential-store.ts` defaults to `allowExpired: true` to enable Supabase refresh flows. The Supabase client automatically handles token refresh when credentials are expired but have a valid refresh token. The `SupabaseSessionStorage` updates the credentials after Supabase performs the refresh. This is an intentional design pattern where the credential store is passive storage and Supabase manages the active token lifecycle.
Applied to files:
packages/tm-core/src/modules/auth/services/oauth-service.ts
📚 Learning: 2025-11-24T18:04:43.972Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.972Z
Learning: Applies to **/*.{js,ts} : Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Applied to files:
packages/tm-core/src/modules/auth/services/oauth-service.ts
🧬 Code graph analysis (2)
packages/tm-core/src/modules/auth/utils/cli-crypto.ts (1)
packages/tm-core/src/modules/auth/types.ts (1)
AuthenticationError(121-138)
packages/tm-core/src/modules/auth/services/oauth-service.ts (2)
packages/tm-core/src/modules/auth/utils/cli-crypto.ts (4)
EncryptedTokenPayload(16-21)AuthKeyPair(37-40)generateKeyPair(46-60)decryptTokens(69-107)packages/tm-core/src/modules/auth/types.ts (3)
OAuthFlowOptions(32-45)AuthCredentials(5-14)AuthenticationError(121-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (6)
context7.json (1)
1-4: Clarify the purpose and naming of this configuration file.The file name
context7.jsonis not self-descriptive. Additionally, thepublic_keyvalue appears to be an identifier/API key prefix rather than an actual RSA public key used for E2E encryption. According to the oauth-service.ts changes, the CLI generates its own RSA keypair and sends the public key to the backend—this file'spublic_keyseems unrelated to that flow.Consider:
- Renaming this file to something more descriptive (e.g.,
backend-config.jsonorapi-config.json)- Clarifying what
public_keyrepresents via a comment or documentationpackages/tm-core/src/modules/auth/types.ts (1)
107-116: LGTM!The new error codes are well-organized with clear categorization (PKCE flow errors vs E2E encryption errors) and align with the backend-managed PKCE flow implementation.
packages/tm-core/src/modules/auth/services/oauth-service.ts (1)
85-121: LGTM!Clean delegation to the backend PKCE flow with proper error wrapping and appropriate handling of MFA requirements (not treated as errors).
packages/tm-core/src/modules/auth/utils/cli-crypto.ts (3)
42-60: LGTM!The 2048-bit RSA keypair is appropriate for short-lived CLI authentication tokens. The key encoding formats (SPKI/PKCS8) are standard and compatible with the hybrid encryption scheme.
100-106: LGTM!Proper error handling that wraps all exceptions in
AuthenticationErrorwith the original error preserved as the cause for debugging.
13-40: LGTM!Clean interface definitions with appropriate documentation. The field types align correctly with the hybrid encryption scheme and expected server responses.
…y browsers or firewalls (#1492)
…y browsers or firewalls (#1492)
…y browsers or firewalls (#1492)
1 participant
What type of PR is this?
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
Bug Fixes
Security
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.