Implement security enhancements in auth-client: validate base URL, cl…#11
Implement security enhancements in auth-client: validate base URL, cl…#11
Conversation
…ient ID, and token structure. Improve error handling and streamline token management by utilizing SecurityManager methods.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cursor-link-cli/src/utils/auth-client.ts (2)
42-47: Set secure permissions on config directory (0700)Prevents other users from listing token files.
private ensureConfigDir() { if (!fs.existsSync(CONFIG_DIR)) { - fs.mkdirSync(CONFIG_DIR, { recursive: true }); + fs.mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 }); + } else { + try { fs.chmodSync(CONFIG_DIR, 0o700); } catch {} } }
76-79: Persist tokens with 0600 perms and atomicallyAvoid world-readable 0644 via umask defaults; use atomic rename.
saveToken(tokenData: StoredToken) { this.ensureConfigDir(); - fs.writeFileSync(TOKEN_FILE, JSON.stringify(tokenData, null, 2)); + const tmp = path.join(CONFIG_DIR, 'token.json.tmp'); + const payload = JSON.stringify(tokenData, null, 2); + fs.writeFileSync(tmp, payload, { mode: 0o600 }); + fs.renameSync(tmp, TOKEN_FILE); }
🧹 Nitpick comments (13)
cursor-link-cli/src/utils/security.ts (4)
41-52: Make token clearing more resilientIf chmod/write fails (permissions/AV), fall back to rm -f. Also prefer rmSync over unlinkSync.
if (fs.existsSync(TOKEN_FILE)) { - // Overwrite with zeros before deletion (basic security) + // Overwrite with zeros before deletion (best-effort) const stats = fs.statSync(TOKEN_FILE); const zeros = Buffer.alloc(stats.size, 0); - fs.writeFileSync(TOKEN_FILE, zeros); - fs.unlinkSync(TOKEN_FILE); + try { fs.chmodSync(TOKEN_FILE, 0o600); } catch {} + try { fs.writeFileSync(TOKEN_FILE, zeros); } catch {} + fs.rmSync(TOKEN_FILE, { force: true }); } } catch (error) { - console.warn('Warning: Could not securely clear token file:', error); + console.warn('Warning: Could not securely clear token file:', error); + try { fs.rmSync(TOKEN_FILE, { force: true }); } catch {} }
57-61: Client ID regex: avoid leading/trailing hyphen; consider underscores/dots if server allowsCurrent pattern allows "-" at ends and disallows "" or ".". Tighten ends and optionally allow [.].
- const clientIdPattern = /^[a-zA-Z0-9-]{3,50}$/; + // Starts/ends with alnum; internal [-._] allowed. Adjust if server forbids _ or . + const clientIdPattern = /^[A-Za-z0-9](?:[A-Za-z0-9._-]{1,48})[A-Za-z0-9]$/;Please confirm allowed characters on the auth server before merging.
91-104: Nit: prefer error_description over terse error codeShow human-friendly descriptions first.
- const safeFields = ['error', 'error_description', 'message']; + const safeFields = ['error_description', 'message', 'error'];
5-7: Avoid duplicating token path constants across modulesCONFIG_DIR/TOKEN_FILE are defined here and in auth-client.ts. Extract to a shared const/module to prevent drift.
cursor-link-cli/src/utils/auth-client.ts (6)
70-72: Self-heal on corrupt token fileIf JSON.parse fails, clear the token to avoid repeated failures.
- const sanitizedError = SecurityManager.sanitizeError(error); - console.error(`Error reading stored token: ${sanitizedError}`); + const sanitizedError = SecurityManager.sanitizeError(error); + console.error(`Error reading stored token: ${sanitizedError}`); + this.clearToken(); return null;
121-129: Allow env override for client IDKeeps defaults but enables testing/alternate clients.
- const clientId = "cursor-link-cli"; + const clientId = process.env.CURSOR_LINK_CLIENT_ID || "cursor-link-cli";
151-157: Prefer verification_uri_complete when present; only prompt for code if neededImproves UX and matches OAuth device flow guidance.
- console.log(chalk.cyan('\n📱 Device Authorization in Progress')); - console.log(chalk.white(`Please visit: ${verification_uri}`)); - console.log(chalk.white(`Enter code: ${chalk.bold.yellow(user_code)}\n`)); + console.log(chalk.cyan('\n📱 Device Authorization in Progress')); + const displayUrl = verification_uri_complete || verification_uri; + console.log(chalk.white(`Please visit: ${displayUrl}`)); + if (!verification_uri_complete) { + console.log(chalk.white(`Enter code: ${chalk.bold.yellow(user_code)}\n`)); + } else { + console.log(); + }
156-164: Guard browser open against non-http(s) URLsBe defensive with untrusted inputs.
- const urlToOpen = verification_uri_complete || verification_uri; + const urlToOpen = verification_uri_complete || verification_uri; if (urlToOpen) { - console.log(chalk.gray('🌐 Opening browser...')); - try { - await open(urlToOpen); + console.log(chalk.gray('🌐 Opening browser...')); + try { + if (/^https?:\/\//i.test(urlToOpen)) { + await open(urlToOpen); + } } catch (error) { console.log(chalk.gray('Failed to open browser automatically. Please visit the URL manually.')); } }
267-271: Nit: avoid printing “undefined” when user fields are missingFallback gracefully to email or id.
- if (token.user) { - console.log(chalk.gray(` User: ${token.user.name} (${token.user.email})`)); - } + if (token.user) { + const userLabel = token.user.name || token.user.email || token.user.id; + console.log(chalk.gray(` User: ${userLabel}`)); + }
12-14: De-duplicate token path constantsCONFIG_DIR/TOKEN_FILE are also defined in SecurityManager. Extract to a shared module (e.g., utils/paths.ts) and import from both to prevent drift.
cursor-link-cli/SECURITY.md (3)
50-57: Reflect URL validation hardening in docsMention that embedded credentials are blocked and ::1 is allowed in dev.
-// ✅ http://localhost:3000 - allowed in dev +// ✅ http://localhost:3000 - allowed in dev +// ✅ http://[::1]:3000 - allowed in dev // ❌ http://malicious-site.com - blocked +// ❌ https://user:pass@cursor.link - blocked (credentials in URL)
102-107: Document secure file/dir permissionsClarify 0600/0700 to reduce leakage on multi-user systems.
-**Permissions**: File created with default user permissions +**Permissions**: Directory 0700; token file 0600
95-117: Nit: minor Markdown/grammar tweaks for readabilitySmall fixes: remove double spaces for line breaks, standardize bullet punctuation, and tweak phrasing.
-### 1. Transport Security -- **Production**: Requires HTTPS endpoints -- **Development**: Allows localhost with HTTP -- **Protection**: Prevents man-in-the-middle attacks +### 1. Transport Security +- **Production**: Require HTTPS endpoints. +- **Development**: Allow localhost with HTTP. +- **Protection**: Prevent man-in-the-middle attacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cursor-link-cli/SECURITY.md(1 hunks)cursor-link-cli/src/utils/auth-client.ts(7 hunks)cursor-link-cli/src/utils/security.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/better-auth-device-authorization.mdc)
When calling device.token, set grant_type to "urn:ietf:params:oauth:grant-type:device_code"
Files:
cursor-link-cli/src/utils/security.tscursor-link-cli/src/utils/auth-client.ts
🧠 Learnings (2)
📚 Learning: 2025-09-01T20:24:00.229Z
Learnt from: CR
PR: R44VC0RP/cursor.link#0
File: .cursor/rules/better-auth-device-authorization.mdc:0-0
Timestamp: 2025-09-01T20:24:00.229Z
Learning: Applies to cli-auth.ts : In polling logic, handle OAuth device flow errors: continue on authorization_pending, increase interval on slow_down, and terminate on access_denied or expired_token
Applied to files:
cursor-link-cli/src/utils/auth-client.ts
📚 Learning: 2025-09-01T20:24:00.229Z
Learnt from: CR
PR: R44VC0RP/cursor.link#0
File: .cursor/rules/better-auth-device-authorization.mdc:0-0
Timestamp: 2025-09-01T20:24:00.229Z
Learning: Applies to **/*.{ts,tsx} : When calling device.token, set grant_type to "urn:ietf:params:oauth:grant-type:device_code"
Applied to files:
cursor-link-cli/src/utils/auth-client.ts
🧬 Code graph analysis (1)
cursor-link-cli/src/utils/auth-client.ts (1)
cursor-link-cli/src/utils/security.ts (1)
SecurityManager(12-105)
🪛 LanguageTool
cursor-link-cli/SECURITY.md
[grammar] ~46-~46: There might be a mistake here.
Context: ...}); ``` ### 🔒 Security Enhancements Our implementation adds these securi...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...vice Authorization Request (Section 3.1) - ✅ Device Authorization Response (Section...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...ice Authorization Response (Section 3.2) - ✅ User Interaction (Section 3.3) - ✅ Dev...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...3.2) - ✅ User Interaction (Section 3.3) - ✅ Device Access Token Request (Section 3...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...evice Access Token Request (Section 3.4) - ✅ Device Access Token Response (Section ...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...vice Access Token Response (Section 3.5) - ✅ Error Response Handling (Section 3.5.2...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...est Practices ### 1. Transport Security - Production: Requires HTTPS endpoints -...
(QB_NEW_EN)
[grammar] ~102-102: There might be a mistake here./.cursor-l...
Context: ...-middle attacks ### 2. Token Management - Storage: Tokens stored in `
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...agement - Storage: Tokens stored in ~/.cursor-link/token.json - Permissions: File created with default...
(QB_NEW_EN)
[grammar] ~104-~104: There might be a mistake here.
Context: ...le created with default user permissions - Expiration: Tokens validated for expir...
(QB_NEW_EN)
[grammar] ~105-~105: There might be a mistake here.
Context: ...validated for expiration with 60s buffer - Cleanup: Secure deletion overwrites da...
(QB_NEW_EN)
[grammar] ~108-~108: There might be a mistake here.
Context: ...fore file removal ### 3. Error Handling - Sanitization: Error messages filtered ...
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ...tive data logged ### 4. Client Security - ID Validation: Client IDs must match s...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ...HTTPS-only 2. Base URL: Defaults to https://cursor.link 3. Client ID: Uses cursor-link-cli (mus...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...Man-in-the-Middle: HTTPS enforcement 2. Token Theft: Secure file permissions &...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ...eft**: Secure file permissions & cleanup 3. Phishing: URL validation prevents mali...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ... validation prevents malicious redirects 4. Info Disclosure: Error message sanitiz...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: ...Disclosure**: Error message sanitization 5. Client Impersonation: Client ID valida...
(QB_NEW_EN)
⏰ 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: Vade Review
🔇 Additional comments (3)
cursor-link-cli/src/utils/auth-client.ts (2)
18-23: Base URL validation at startup: LGTMGood early exit with clear guidance.
185-188: Grant type is correctly set per guidelinesgrant_type uses urn:ietf:params:oauth:grant-type:device_code. Compliant.
cursor-link-cli/SECURITY.md (1)
20-28: Docs: grant_type snippet matches our guidelineThe example correctly sets grant_type to urn:ietf:params:oauth:grant-type:device_code.
| const requiredFields = ['access_token', 'expires_at']; | ||
| return requiredFields.every(field => | ||
| token.hasOwnProperty(field) && token[field] != null | ||
| ); |
There was a problem hiding this comment.
Critical: malformed expires_at can bypass expiry checks
validateToken only checks presence, so a non-numeric expires_at (e.g., "oops") passes validation. isTokenExpired will then compute Date.now() > ("oops" - 60000) ⇒ false, treating an invalid token as valid. Enforce types.
Apply:
static validateToken(token: any): boolean {
- if (!token || typeof token !== 'object') return false;
-
- // Required fields for a valid token
- const requiredFields = ['access_token', 'expires_at'];
- return requiredFields.every(field =>
- token.hasOwnProperty(field) && token[field] != null
- );
+ if (!token || typeof token !== 'object') return false;
+ const has = (o: any, k: string) => Object.prototype.hasOwnProperty.call(o, k);
+ if (!has(token, 'access_token') || !has(token, 'expires_at')) return false;
+ if (typeof token.access_token !== 'string' || token.access_token.trim().length === 0) return false;
+ if (typeof token.expires_at !== 'number' || !Number.isFinite(token.expires_at)) return false;
+ return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const requiredFields = ['access_token', 'expires_at']; | |
| return requiredFields.every(field => | |
| token.hasOwnProperty(field) && token[field] != null | |
| ); | |
| static validateToken(token: any): boolean { | |
| if (!token || typeof token !== 'object') return false; | |
| const has = (o: any, k: string) => Object.prototype.hasOwnProperty.call(o, k); | |
| if (!has(token, 'access_token') || !has(token, 'expires_at')) return false; | |
| if (typeof token.access_token !== 'string' || token.access_token.trim().length === 0) return false; | |
| if (typeof token.expires_at !== 'number' || !Number.isFinite(token.expires_at)) return false; | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In cursor-link-cli/src/utils/security.ts around lines 20-23, validateToken
currently only checks for presence and allows non-numeric expires_at values;
update validateToken to require access_token be a non-empty string and
expires_at be a finite number (or a string that parses to an integer) — reject
if Number.isFinite(Number(token.expires_at)) is false or access_token is not a
string/empty. Also harden isTokenExpired to treat invalid or unparsable
expires_at as expired (return true) before doing the Date.now() comparison.
| static isTokenExpired(token: any): boolean { | ||
| if (!token.expires_at) return true; | ||
|
|
||
| // Add 60 second buffer to avoid edge cases | ||
| const bufferTime = 60 * 1000; | ||
| return Date.now() > (token.expires_at - bufferTime); | ||
| } |
There was a problem hiding this comment.
Critical: guard isTokenExpired against non-numeric expires_at
Add strict type/finite checks so invalid tokens are treated as expired.
static isTokenExpired(token: any): boolean {
- if (!token.expires_at) return true;
-
- // Add 60 second buffer to avoid edge cases
- const bufferTime = 60 * 1000;
- return Date.now() > (token.expires_at - bufferTime);
+ if (typeof token?.expires_at !== 'number' || !Number.isFinite(token.expires_at)) {
+ return true;
+ }
+ const bufferTime = 60 * 1000;
+ return Date.now() > (token.expires_at - bufferTime);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static isTokenExpired(token: any): boolean { | |
| if (!token.expires_at) return true; | |
| // Add 60 second buffer to avoid edge cases | |
| const bufferTime = 60 * 1000; | |
| return Date.now() > (token.expires_at - bufferTime); | |
| } | |
| static isTokenExpired(token: any): boolean { | |
| if (typeof token?.expires_at !== 'number' || !Number.isFinite(token.expires_at)) { | |
| return true; | |
| } | |
| const bufferTime = 60 * 1000; | |
| return Date.now() > (token.expires_at - bufferTime); | |
| } |
🤖 Prompt for AI Agents
In cursor-link-cli/src/utils/security.ts around lines 29 to 35, the
isTokenExpired function should treat tokens with invalid or non-numeric
expires_at as expired; add a guard that checks typeof token.expires_at ===
'number' and Number.isFinite(token.expires_at) (or, if you want to accept
numeric strings, attempt Number(token.expires_at) and validate with
Number.isFinite) and return true immediately when the check fails, otherwise
proceed with the existing buffer calculation and comparison against Date.now().
| static validateBaseUrl(baseUrl: string): boolean { | ||
| try { | ||
| const url = new URL(baseUrl); | ||
|
|
||
| // In production, require HTTPS | ||
| if (process.env.NODE_ENV === 'production') { | ||
| return url.protocol === 'https:'; | ||
| } | ||
|
|
||
| // In development, allow localhost with HTTP | ||
| if (url.protocol === 'http:' && | ||
| (url.hostname === 'localhost' || url.hostname === '127.0.0.1')) { | ||
| return true; | ||
| } | ||
|
|
||
| // Otherwise require HTTPS | ||
| return url.protocol === 'https:'; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden URL validation: forbid embedded credentials; allow IPv6 loopback
Prevent auth endpoints like https://user:pass@host, and support ::1 in development.
static validateBaseUrl(baseUrl: string): boolean {
try {
const url = new URL(baseUrl);
+ // Disallow embedded credentials
+ if (url.username || url.password) return false;
+
// In production, require HTTPS
if (process.env.NODE_ENV === 'production') {
return url.protocol === 'https:';
}
// In development, allow localhost with HTTP
- if (url.protocol === 'http:' &&
- (url.hostname === 'localhost' || url.hostname === '127.0.0.1')) {
+ if (
+ url.protocol === 'http:' &&
+ (url.hostname === 'localhost' || url.hostname === '127.0.0.1' || url.hostname === '::1')
+ ) {
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static validateBaseUrl(baseUrl: string): boolean { | |
| try { | |
| const url = new URL(baseUrl); | |
| // In production, require HTTPS | |
| if (process.env.NODE_ENV === 'production') { | |
| return url.protocol === 'https:'; | |
| } | |
| // In development, allow localhost with HTTP | |
| if (url.protocol === 'http:' && | |
| (url.hostname === 'localhost' || url.hostname === '127.0.0.1')) { | |
| return true; | |
| } | |
| // Otherwise require HTTPS | |
| return url.protocol === 'https:'; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| static validateBaseUrl(baseUrl: string): boolean { | |
| try { | |
| const url = new URL(baseUrl); | |
| // Disallow embedded credentials | |
| if (url.username || url.password) return false; | |
| // In production, require HTTPS | |
| if (process.env.NODE_ENV === 'production') { | |
| return url.protocol === 'https:'; | |
| } | |
| // In development, allow localhost with HTTP | |
| if ( | |
| url.protocol === 'http:' && | |
| (url.hostname === 'localhost' || url.hostname === '127.0.0.1' || url.hostname === '::1') | |
| ) { | |
| return true; | |
| } | |
| // Otherwise require HTTPS | |
| return url.protocol === 'https:'; | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In cursor-link-cli/src/utils/security.ts around lines 66-86, the URL validation
must reject URLs that embed credentials and should treat IPv6 loopback as a
valid dev host; update validateBaseUrl to return false when url.username or
url.password are non-empty (to forbid user:pass@host), and in the development
branch allow HTTP if url.hostname is 'localhost', '127.0.0.1', or '::1'; keep
the production branch requiring url.protocol === 'https:' and preserve the
try/catch returning false on parse errors.
1 participant
…ient ID, and token structure. Improve error handling and streamline token management by utilizing SecurityManager methods.
Summary by CodeRabbit
New Features
Security
Documentation