feat: preview zmodel doc #2239
feat: preview zmodel doc #2239
Conversation
📝 WalkthroughWalkthroughAdds ZModel preview functionality: VS Code auth provider, documentation cache, language-server import resolver, preview command that aggregates imports and requests docs, release-notes UI, and activation wiring to start/stop the language client and register components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant VSC as VS Code
participant AUTH as ZenStackAuthenticationProvider
participant WEB as Browser
participant SSO as accounts.zenstack.dev
U->>VSC: Trigger sign-in (prompt)
VSC->>AUTH: createSession(scopes)
AUTH->>WEB: open AUTH_URL/sign-in
WEB->>SSO: Authenticate
SSO-->>WEB: Redirect to vscode://.../auth-callback#access_token=...
WEB->>VSC: URI callback
VSC->>AUTH: handleAuthCallback(uri)
AUTH->>AUTH: parse JWT, create & persist session
AUTH-->>VSC: onDidChangeSessions event
sequenceDiagram
autonumber
actor U as User
participant VSC as VS Code
participant EXT as ZModelPreview
participant LSP as LanguageClient/Server
participant CACHE as DocumentationCache
participant API as Doc Service
U->>VSC: zenstack.preview-zmodel
VSC->>EXT: previewZModelFile()
EXT->>EXT: requireAuth()
EXT->>LSP: zenstack/getAllImportedZModelURIs(docUri)
LSP-->>EXT: { importedURIs, hasSyntaxErrors }
alt Syntax errors
EXT-->>U: Show error and abort
else OK
EXT->>EXT: Read main + imported files -> models[]
EXT->>CACHE: getCachedResponse(models)
alt Cache hit
CACHE-->>EXT: markdown
else Cache miss
EXT->>API: POST /generate-docs { models }
API-->>EXT: markdown
EXT->>CACHE: setCachedResponse(models, markdown)
end
EXT->>VSC: Write temp .md and open preview
end
sequenceDiagram
autonumber
participant VSC as VS Code
participant EXT as Extension
participant LSP as LanguageClient
participant SRV as Language Server
VSC->>EXT: activate(context)
EXT->>EXT: register ZenStackAuthenticationProvider
EXT->>LSP: startLanguageClient(context)
LSP->>SRV: initialize/start
SRV-->>LSP: ready
EXT->>EXT: instantiate DocumentationCache, ZModelPreview, ReleaseNotesManager
VSC->>EXT: deactivate()
EXT->>LSP: stop()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. 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: 13
🧹 Nitpick comments (23)
packages/schema/build/post-build.js (1)
31-40: Make the copy step more robust and consistent with bundle.jsWrap the copy in try/catch and log source→dest; prefer
cpSyncwith{ force: true }for consistency with other copy ops.-// Copy release notes HTML file to dist -const releaseNotesSource = 'src/release-notes.html'; -const releaseNotesDest = 'dist/release-notes.html'; - -if (fs.existsSync(releaseNotesSource)) { - console.log('Copying release notes HTML file to dist'); - fs.copyFileSync(releaseNotesSource, releaseNotesDest); -} else { - console.warn('Release notes HTML file not found at:', releaseNotesSource); -} +// Copy release notes HTML file to dist +const releaseNotesSource = 'src/release-notes.html'; +const releaseNotesDest = 'dist/release-notes.html'; +if (fs.existsSync(releaseNotesSource)) { + try { + fs.cpSync(releaseNotesSource, releaseNotesDest, { force: true }); + console.log(`Copied release notes: ${releaseNotesSource} -> ${releaseNotesDest}`); + } catch (err) { + console.warn('Failed to copy release notes HTML file:', err); + } +} else { + console.warn('Release notes HTML file not found at:', releaseNotesSource); +}packages/schema/build/bundle.js (1)
20-24: Align copy behavior with post-build and add a fallback logUse
cpSyncwith{ force: true }and add anelsewarning to aid diagnosing missing assets.- // Copy release notes HTML file - if (fs.existsSync('src/release-notes.html')) { - fs.copyFileSync('src/release-notes.html', 'bundle/release-notes.html'); - console.log('Copied release notes HTML file to bundle'); - } + // Copy release notes HTML file + if (fs.existsSync('src/release-notes.html')) { + fs.cpSync('src/release-notes.html', 'bundle/release-notes.html', { force: true }); + console.log('Copied release notes HTML file to bundle'); + } else { + console.warn('Release notes HTML file not found at: src/release-notes.html'); + }packages/schema/src/release-notes.html (2)
66-68: Show platform-specific keybindingInclude Windows/Linux and macOS variants to avoid confusion.
- <code>Cmd+Shift+V</code> + <code>Cmd+Shift+V</code> (macOS) / <code>Ctrl+Shift+V</code> (Windows/Linux)
77-82: Consistency and style nitsCapitalize “ZModel” consistently and tighten wording.
- <li>Ensure your zmodel is error-free before generating</li> - <li>Use your main zmodel file, it will include all imported models for a complete documentation</li> + <li>Ensure your ZModel is error-free before generating</li> + <li>Use your main ZModel file; it will include all imported models for complete documentation</li> - Add clear, descriptive comments in your ZModel. The more context you provide, the better the - results. + Add clear, descriptive comments in your ZModel. More context yields better results.packages/schema/src/documentation-cache.ts (2)
22-27: Handle async cleanup in constructor
clearExpiredCache()returns a Promise; calling it unawaited can emit unhandled rejections. Fire-and-forget with explicit void and catch.- // clear expired cache entries on initialization - this.clearExpiredCache(); + // clear expired cache entries on initialization (fire-and-forget) + void this.clearExpiredCache().catch((err) => + console.warn('Failed to clear expired documentation cache:', err) + );
44-51: Be cautious with Settings Sync churnContinuously resetting
setKeysForSyncwith ephemeral, 5‑minute keys can cause unnecessary sync noise. Consider gating by a user setting or syncing only a stable prefix key that tracks membership.packages/schema/src/release-notes-manager.ts (3)
43-51: Tighten Webview security and resource access.Set localResourceRoots and add a CSP in the HTML to mitigate accidental remote loads when enableScripts is true.
const panel = vscode.window.createWebviewPanel( 'zenstackReleaseNotes', 'ZenStack - New Feature Announcement!', vscode.ViewColumn.One, { enableScripts: true, - retainContextWhenHidden: true, + retainContextWhenHidden: true, + localResourceRoots: [this.extensionContext.extensionUri], } );If release-notes.html lacks a CSP, consider injecting one before assignment (e.g., restrict script-src to panel.webview.cspSource).
20-21: Make fire-and-forget explicit to satisfy no-floating-promises linters.Prefix with void to signal intentional non-awaited call.
- this.showReleaseNotesIfFirstTime(); + void this.showReleaseNotesIfFirstTime();
31-34: Avoid clobbering other sync keys with setKeysForSync.setKeysForSync replaces the whole set; if other parts add keys, they’ll be lost. Track a union and set them together.
- this.extensionContext.globalState.setKeysForSync([this.releaseNoteVersionKey]); + const keys = new Set<string>([this.releaseNoteVersionKey]); + // TODO: add other keys here if any, before calling setKeysForSync once + this.extensionContext.globalState.setKeysForSync(Array.from(keys));packages/schema/src/mermaid-generator.ts (5)
49-61: Correct one-to-one cardinality symbols (optional side is reversed).For 1–1, the optionality should apply to the current (left) side when x.type.optional is true.
- } else { - //one to one - relation = currentType.optional ? '||--o|' : '|o--||'; - } + } else { + // one to one + const left = currentType.optional ? '|o' : '||'; + const right = oppositeType.optional ? 'o|' : '||'; + relation = `${left}--${right}`; + }
138-141: Include inherited fields when finding the opposite side.Use getModelFieldsWithBases for the opposite model; otherwise relations inherited from bases are missed.
- const oppositeField = oppositeModel.fields.find( - (field) => field.type.reference?.ref?.name === model.name - ); + const oppositeFields = getModelFieldsWithBases(oppositeModel); + const oppositeField = oppositeFields.find( + (field) => field.type.reference?.ref?.name === model.name + );
147-157: Unify relation symbol derivation and fix 1–1 optionality (same as generate).Keep symbol generation consistent across both methods.
- if (currentType.array && oppositeType.array) { - relation = '}o--o{'; - } else if (currentType.array && !oppositeType.array) { - relation = '||--o{'; - } else if (!currentType.array && oppositeType.array) { - relation = '}o--||'; - } else { - relation = currentType.optional ? '||--o|' : '|o--||'; - } + if (currentType.array && oppositeType.array) { + relation = '}o--o{'; + } else if (currentType.array && !oppositeType.array) { + relation = '||--o{'; + } else if (!currentType.array && oppositeType.array) { + relation = '}o--||'; + } else { + const left = currentType.optional ? '|o' : '||'; + const right = oppositeType.optional ? 'o|' : '||'; + relation = `${left}--${right}`; + }
128-161: De-duplicate relationships across both sides.Current Set still stores duplicates because the label includes the field name. Key by sorted model pair to add only one line per pair.
- const relationships = new Set<string>(); + const relationships = new Set<string>(); + const seenPairs = new Set<string>(); dataModels.forEach((model) => { @@ - if (oppositeField) { + if (oppositeField) { @@ - relationships.add(`"${model.name}" ${relation} "${oppositeModelName}": ${x.name}`); + const [a, b] = [model.name, oppositeModelName]; + const key = a <= b ? `${a}::${b}` : `${b}::${a}`; + if (!seenPairs.has(key)) { + seenPairs.add(key); + relationships.add(`"${model.name}" ${relation} "${oppositeModelName}": ${x.name}`); + }
99-101: Remove noisy console.log in library code.Use a logger behind a debug flag if needed.
- console.log('Generating comprehensive ER diagram...'); + // optionally log behind a debug flagpackages/schema/src/zenstack-auth-provider.ts (3)
102-105: Use crypto-grade randomness for state.Math.random is predictable; use crypto.randomBytes.
+import { randomBytes } from 'crypto'; @@ private generateState(): string { - return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); + return randomBytes(16).toString('hex'); }
44-49: Optional: filter sessions by scopes.If multiple sessions are stored, return those matching requested scopes.
- async getSessions(_scopes?: readonly string[]): Promise<vscode.AuthenticationSession[]> { + async getSessions(_scopes?: readonly string[]): Promise<vscode.AuthenticationSession[]> { // Check if we have stored sessions in VS Code's secret storage const storedSessions = await this.getStoredSessions(); - this._sessions = storedSessions; + this._sessions = storedSessions; return this._sessions; }Follow-up: if using scopes, intersect with _scopes before returning.
139-141: Avoid predictable fallback session IDs.Use crypto for id if jti is missing.
- id: claims.jti || Math.random().toString(36), + id: claims.jti || randomBytes(8).toString('hex'),packages/schema/src/language-server/main.ts (3)
38-45: Avoid magic number for severity.Use the enum for readability.
Apply:
- doc.diagnostics?.some((e) => e.severity === 1) + doc.diagnostics?.some((e) => e.severity === DiagnosticSeverity.Error)And add import (outside this hunk):
import { DiagnosticSeverity } from 'vscode-languageserver';
21-23: Drop redundant initial build.You build again with imported documents; this first build is unnecessary.
Apply:
- // Ensure the document is parsed and built - if (!document.parseResult) { - await shared.workspace.DocumentBuilder.build([document]); - } + // Parsing/build happens below together with imports
52-56: Prefer LSP logging channel.Use
connection.console.errorso messages surface properly in client logs.Apply:
- console.error('Error getting imported ZModel file:', error); + connection.console.error(`Error getting imported ZModel file: ${String(error)}`);packages/schema/src/zmodel-preview.ts (3)
189-197: Verify auth header format; use Bearer by default.Most APIs expect
Authorization: Bearer <token>. Current code sends the raw token.Apply:
- const apiResponse = await fetch('https://api.zenstack.dev/api/doc', { + const apiResponse = await fetch('https://api.zenstack.dev/api/doc', { method: 'POST', headers: { 'Content-Type': 'application/json', - authorization: session.accessToken, + Authorization: `Bearer ${session.accessToken}`, }, body: JSON.stringify(requestBody), });If backend intentionally expects a raw token, keep as-is and ignore this change.
72-89: Avoid double auth prompts: pass the session through.You call
requireAuth()twice. Reuse the session obtained inpreviewZModelFile.Apply:
- async () => { - const markdownContent = await this.generateZModelDocumentation(document); + async () => { + const markdownContent = await this.generateZModelDocumentation(document, session);- private async generateZModelDocumentation(document: vscode.TextDocument): Promise<string> { + private async generateZModelDocumentation( + document: vscode.TextDocument, + session: vscode.AuthenticationSession + ): Promise<string> {- // Fallback: fetch from API endpoint - const session = await requireAuth(); - if (!session) { - throw new Error('Authentication required to generate documentation'); - } + // Use existing authenticated sessionAlso applies to: 132-140, 164-169
221-255: Prefer in-memory preview; avoid temp file churn and TOCTOU window.Use an untitled document + markdown preview to remove FS writes and the delete timer.
Apply:
- const workspaceFolder = vscode.workspace.workspaceFolders?.[0]; - if (!workspaceFolder) { - vscode.window.showErrorMessage('No workspace folder found.'); - return; - } - - // Create a temporary markdown file with a descriptive name - const baseName = path.basename(originalFileName, '.zmodel'); - const tempFileName = `${baseName}-preview.md`; - const tempFile = vscode.Uri.joinPath(workspaceFolder.uri, tempFileName); - - try { - // Write the markdown content to the temp file - await vscode.workspace.fs.writeFile(tempFile, new TextEncoder().encode(markdownContent)); - - // Open the markdown preview side by side - await vscode.commands.executeCommand('markdown.showPreviewToSide', tempFile); - - // Optionally clean up the temp file after a delay - setTimeout(async () => { - try { - await vscode.workspace.fs.delete(tempFile); - } catch (error) { - // Ignore cleanup errors - console.log('Could not clean up temp file:', error); - } - }, 5000); // Clean up after 5 seconds + try { + const doc = await vscode.workspace.openTextDocument({ + content: markdownContent, + language: 'markdown', + }); + await vscode.window.showTextDocument(doc, { preview: true, viewColumn: vscode.ViewColumn.Two }); + await vscode.commands.executeCommand('markdown.showPreviewToSide', doc.uri);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/schema/package.jsonis excluded by!**/*.json
📒 Files selected for processing (10)
packages/schema/build/bundle.js(1 hunks)packages/schema/build/post-build.js(1 hunks)packages/schema/src/documentation-cache.ts(1 hunks)packages/schema/src/extension.ts(1 hunks)packages/schema/src/language-server/main.ts(1 hunks)packages/schema/src/mermaid-generator.ts(1 hunks)packages/schema/src/release-notes-manager.ts(1 hunks)packages/schema/src/release-notes.html(1 hunks)packages/schema/src/zenstack-auth-provider.ts(1 hunks)packages/schema/src/zmodel-preview.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/schema/build/bundle.js (1)
packages/schema/build/post-build.js (1)
fs(3-3)
packages/schema/src/language-server/main.ts (2)
packages/schema/src/language-server/zmodel-module.ts (1)
createZModelServices(129-138)packages/schema/src/cli/cli-util.ts (1)
eagerLoadAllImports(146-168)
packages/schema/build/post-build.js (1)
packages/schema/build/bundle.js (1)
fs(4-4)
packages/schema/src/zmodel-preview.ts (2)
packages/schema/src/documentation-cache.ts (1)
DocumentationCache(15-153)packages/schema/src/extension.ts (1)
requireAuth(14-34)
packages/schema/src/extension.ts (4)
packages/schema/src/zenstack-auth-provider.ts (2)
AUTH_PROVIDER_ID(11-11)ZenStackAuthenticationProvider(14-221)packages/schema/src/documentation-cache.ts (1)
DocumentationCache(15-153)packages/schema/src/zmodel-preview.ts (1)
ZModelPreview(12-294)packages/schema/src/release-notes-manager.ts (1)
ReleaseNotesManager(6-76)
packages/schema/src/mermaid-generator.ts (2)
packages/language/src/ast.ts (1)
DataModel(56-66)packages/sdk/src/utils.ts (4)
getModelFieldsWithBases(544-550)isRelationshipField(295-297)isIdField(255-290)isForeignKeyField(302-324)
⏰ 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). (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: dependency-review
🔇 Additional comments (4)
packages/schema/src/documentation-cache.ts (1)
57-65: Cache-key normalization trade-offStripping all whitespace can collide cases where whitespace is semantically meaningful (e.g., inside string literals or doc comments affecting LLM output). If that matters, normalize by collapsing runs of whitespace rather than removing all, or hash the raw plus a prettified variant.
Would collisions here change the generated documentation in practice for your inputs?
packages/schema/src/zenstack-auth-provider.ts (1)
29-42: Verify extension contributions and callback URI Ensure inpackage.jsonundercontributes.authenticationthat the provider ID matchesAUTH_PROVIDER_IDand that your OAuth redirect URI is set tovscode://<publisher>.<extensionName>/auth-callbackpackages/schema/src/extension.ts (2)
13-34: Auth flow wiring looks good.Session retrieval with non-creating attempt, prompt, and creating attempt is correct.
38-48: Activation lifecycle LGTM.Auth provider registration, client startup, and registrations for preview/cache/release-notes are properly disposed via
context.subscriptions.
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 (1)
packages/schema/src/extension.ts (1)
61-69: Include missing bundle assets in the extension distribution
- No files matching
bundle/language-server/main*were found; ensure the language-server bundle is copied into the extension’s output.zmodel-preview-release-notes.htmlexists underpackages/schema/src/resbut not atbundle/res; update your build or packaging config to place it underbundle/res.- Add a CI check (e.g., using
fd bundle/language-server/main*andfd bundle/res/zmodel-preview-release-notes.html) to prevent regressions.
♻️ Duplicate comments (8)
packages/schema/src/release-notes-manager.ts (1)
39-49: Add dev/packaged fallbacks for release-notes asset path.Reading only from
bundle/res/...will fail in dev (source) builds. Add a fallback tosrc/res/...(and optionallydist/res/...) to avoid file-not-found at runtime. This was noted previously and still applies.
[duplicated_comment]- // Read the release notes HTML file - const releaseNotesPath = vscode.Uri.joinPath( - this.extensionContext.extensionUri, - 'bundle/res/zmodel-preview-release-notes.html' - ); - - const htmlBytes = await vscode.workspace.fs.readFile(releaseNotesPath); + // Read the release notes HTML file (prefer bundled, fall back to source) + const candidates = [ + 'bundle/res/zmodel-preview-release-notes.html', + 'dist/res/zmodel-preview-release-notes.html', + 'src/res/zmodel-preview-release-notes.html', + ]; + let releaseNotesUri: vscode.Uri | undefined; + for (const rel of candidates) { + const uri = vscode.Uri.joinPath(this.extensionContext.extensionUri, rel); + try { + await vscode.workspace.fs.stat(uri); + releaseNotesUri = uri; + break; + } catch { + /* try next */ + } + } + if (!releaseNotesUri) { + throw new Error('zmodel-preview-release-notes.html not found in bundle/res, dist/res, or src/res'); + } + const htmlBytes = await vscode.workspace.fs.readFile(releaseNotesUri);packages/schema/src/zenstack-auth-provider.ts (4)
178-196: Fix JWT decoding: use Buffer with base64url;atobis undefined in Node.private parseJWTClaims(token: string): JWTClaims { try { // JWT tokens have 3 parts separated by dots: header.payload.signature const parts = token.split('.'); if (parts.length !== 3) { throw new Error('Invalid JWT format'); } - // Decode the payload (second part) - const payload = parts[1]; - // Add padding if needed for base64 decoding - const paddedPayload = payload + '='.repeat((4 - (payload.length % 4)) % 4); - const decoded = atob(paddedPayload); - - return JSON.parse(decoded); + // Decode the payload (second part) using base64url + const decoded = Buffer.from(parts[1], 'base64url').toString('utf8'); + return JSON.parse(decoded); } catch (error) { throw new Error(`Failed to parse JWT claims: ${error}`); } }
134-158: Validate state and read token from URI fragment.Implicit flows return params in the fragment; state must be validated.
- public async handleAuthCallback(callbackUri: vscode.Uri): Promise<void> { - const query = new URLSearchParams(callbackUri.query); - const accessToken = query.get('access_token'); + public async handleAuthCallback(callbackUri: vscode.Uri): Promise<void> { + const params = new URLSearchParams(callbackUri.fragment || callbackUri.query); + const accessToken = params.get('access_token'); + const state = params.get('state'); if (!this.pendingAuth) { console.warn('No pending authentication found'); return; } + if (!state || state !== this.pendingAuth.state) { + this.pendingAuth.reject(new Error('State mismatch')); + delete this.pendingAuth; + return; + } if (!accessToken) { this.pendingAuth.reject(new Error('No access token received')); delete this.pendingAuth; return; } try { // Create session from the access token - const session = await this.createSessionFromAccessToken(accessToken); + const session = await this.createSessionFromAccessToken(accessToken, this.pendingAuth.scopes); this.pendingAuth.resolve(session); delete this.pendingAuth;
80-129: OAuth flow: add redirect_uri/state/scopes and handle remote with asExternalUri.Without these, callback may not return (esp. remote) and CSRF protections are absent.
- private async performLogin(scopes: readonly string[]): Promise<vscode.AuthenticationSession> { + private async performLogin(scopes: readonly string[]): Promise<vscode.AuthenticationSession> { // Create the authentication promise return vscode.window.withProgress( @@ - return new Promise<vscode.AuthenticationSession>((resolve, reject) => { + return new Promise<vscode.AuthenticationSession>(async (resolve, reject) => { @@ - // Generate a unique state parameter for security - const state = this.generateState(); - // Construct the ZenStack sign-in URL for implicit flow (returns access_token directly) - const signInUrl = new URL('/sign-in', AUTH_URL); + // Generate state and construct redirect/callback URIs + const state = this.generateState(); + const extensionId = this._context.extension?.id ?? 'zenstackhq.zenstack'; + const callbackUri = vscode.Uri.parse(`${vscode.env.uriScheme}://${extensionId}/auth-callback`); + const externalCallbackUri = await vscode.env.asExternalUri(callbackUri); + // Construct the sign-in URL (implicit flow) + const signInUrl = new URL('/sign-in', AUTH_URL); + signInUrl.searchParams.set('response_type', 'token'); + signInUrl.searchParams.set('redirect_uri', externalCallbackUri.toString()); + signInUrl.searchParams.set('state', state); + if (scopes.length) signInUrl.searchParams.set('scope', scopes.join(' ')); @@ - // Store the state and resolve function for later use + // Store the state and resolve function for later use this.pendingAuth = { state, resolve, reject, scopes };
160-176: Propagate scopes and reject expired tokens when creating a session.- private async createSessionFromAccessToken(accessToken: string): Promise<vscode.AuthenticationSession> { + private async createSessionFromAccessToken( + accessToken: string, + scopes: readonly string[] + ): Promise<vscode.AuthenticationSession> { try { // Decode JWT to get claims const claims = this.parseJWTClaims(accessToken); + if (claims.exp && Date.now() / 1000 >= claims.exp) { + throw new Error('Access token expired'); + } return { id: claims.jti || Math.random().toString(36), accessToken: accessToken, account: { id: claims.sub || 'unknown', label: claims.email || 'unknown@zenstack.dev', }, - scopes: [], + scopes: [...scopes], }; } catch (error) { throw new Error(`Failed to create session from access token: ${error}`); } }packages/schema/src/zmodel-preview.ts (3)
108-113: Use onReady() instead of start() to avoid double-start race.Calling start() here can throw or race if the client is already started. Await readiness via onReady().
- // Ensure the language server is ready - await this.languageClient.start(); + // Ensure the language server is ready + await this.languageClient.onReady();
100-123: Serialize URIs as strings across LSP; parse on client, guard non-file schemes, and handle undefined.The LSP response won’t round-trip URI instances; assuming uri.path breaks on Windows and after JSON serialization. Expect string[], parse with vscode.Uri.parse(), skip non-file schemes, and default importedURIs to [] to avoid .map on undefined.
Type and cast fixes:
- private async getAllImportedZModelURIs(document: vscode.TextDocument): Promise<{ - hasSyntaxErrors: boolean; - importedURIs: URI[]; - }> { + private async getAllImportedZModelURIs(document: vscode.TextDocument): Promise<{ + hasSyntaxErrors: boolean; + importedURIs: string[]; + }> { ... - return result as { - hasSyntaxErrors: boolean; - importedURIs: URI[]; - }; + return result as { + hasSyntaxErrors: boolean; + importedURIs: string[]; + };Parse and guard when reading files:
- const importedURIs = astInfo?.importedURIs; + const importedURIs = (astInfo?.importedURIs ?? []) as string[]; // get vscode document from importedURIs const importedTexts = await Promise.all( - importedURIs.map(async (uri) => { + importedURIs.map(async (uriStr) => { try { - const fileUri = vscode.Uri.file(uri.path); + const fileUri = vscode.Uri.parse(uriStr); + if (fileUri.scheme !== 'file') { + console.warn(`Skipping non-file URI in ZModel imports: ${uriStr}`); + return null; + } const fileContent = await vscode.workspace.fs.readFile(fileUri); return Buffer.from(fileContent).toString('utf8'); } catch (error) { - console.warn(`Could not read file for URI ${uri}:`, error); + console.warn(`Could not read file for URI ${uriStr}:`, error); return null; } }) );Also applies to: 141-156
157-161: Narrow to string before .trim() to avoid potential null deref and TS errors.Current filter doesn’t narrow the union; .trim() can fail at compile/runtime.
- const zmodelContent = [document.getText(), ...importedTexts.filter((text) => text !== null)]; - - // Trim whitespace from each model string - const trimmedZmodelContent = zmodelContent.map((content) => content.trim()); + const nonNullImportedTexts = importedTexts.filter( + (t): t is string => typeof t === 'string' + ); + const zmodelContent: string[] = [document.getText(), ...nonNullImportedTexts]; + const trimmedZmodelContent = zmodelContent.map((content) => content.trim());
🧹 Nitpick comments (10)
packages/schema/src/documentation-cache.ts (3)
22-27: Avoid floating promise in constructor.
clearExpiredCache()is async; explicitly ignore its promise to satisfy linters and intent.- // clear expired cache entries on initialization - this.clearExpiredCache(); + // clear expired cache entries on initialization + void this.clearExpiredCache();
44-51: setKeysForSync can overwrite other features’ keys.Each call replaces the entire sync-key list. Consider centralizing sync management (e.g., once during activation) or building a union from
globalState.keys()to avoid clobbering keys set by ReleaseNotesManager.Would you like a small refactor PR to centralize sync-key registration during activation?
96-108: Guard against oversized entries and transient failures.GlobalState has practical size limits; caching very large docs repeatedly can evict other keys. Consider skipping cache for responses above a threshold and soft-failing updates.
- await this.extensionContext.globalState.update(cacheKey, cacheEntry); + // Avoid storing overly large entries (e.g., > 1MB) + if (Buffer.byteLength(cacheEntry.data, 'utf8') < 1_000_000) { + await this.extensionContext.globalState.update(cacheKey, cacheEntry); + } else { + console.warn('Skipping cache: entry exceeds size limit'); + }packages/schema/src/zenstack-auth-provider.ts (1)
44-49: Filter sessions by requested scopes.Return only sessions that satisfy the requested scopes (future-proof once scopes are used).
- async getSessions(_scopes?: readonly string[]): Promise<vscode.AuthenticationSession[]> { + async getSessions(requestedScopes: readonly string[] = []): Promise<vscode.AuthenticationSession[]> { // Check if we have stored sessions in VS Code's secret storage const storedSessions = await this.getStoredSessions(); - this._sessions = storedSessions; + this._sessions = requestedScopes.length + ? storedSessions.filter((s) => requestedScopes.every((r) => s.scopes.includes(r))) + : storedSessions; return this._sessions; }packages/schema/src/extension.ts (2)
17-36: Consider specifying scopes for auth.If the API expects scopes (e.g.,
openid profile email), pass them togetSessionso re-auth prompts are minimized and future checks are consistent.If scopes are required, what are they? I can wire them through
requireAuth()and the provider.
46-50: Centralize Settings Sync key registration.Both cache and release-notes call
setKeysForSync, which can overwrite each other. After constructing features, set a single union once during activation.const documentationCache = new DocumentationCache(context); context.subscriptions.push(documentationCache); context.subscriptions.push(new ZModelPreview(context, client, documentationCache)); context.subscriptions.push(new ReleaseNotesManager(context)); + // Centralize Settings Sync key management (avoid clobbering) + const allKeys = context.globalState.keys(); + context.globalState.setKeysForSync(allKeys);packages/schema/src/zmodel-preview.ts (4)
61-64: Make file-type check robust and case-insensitive.- if (!document.fileName.endsWith('.zmodel')) { + if (path.extname(document.fileName).toLowerCase() !== '.zmodel') {
66-71: Avoid double auth prompts; pass the session into generateZModelDocumentation.@@ - // Check authentication before proceeding - const session = await requireAuth(); + // Check authentication before proceeding + const session = await requireAuth(); if (!session) { return; } @@ - const markdownContent = await this.generateZModelDocumentation(document); + const markdownContent = await this.generateZModelDocumentation(document, session);- private async generateZModelDocumentation(document: vscode.TextDocument): Promise<string> { + private async generateZModelDocumentation( + document: vscode.TextDocument, + session: vscode.AuthenticationSession + ): Promise<string> {- const session = await requireAuth(); - if (!session) { - throw new Error('Authentication required to generate documentation'); - }Also applies to: 81-85, 132-132, 162-166
5-5: Remove unused import after switching to string[] URIs.-import { URI } from 'vscode-uri';
218-237: Use in-memory markdown document for preview instead of temp filesVS Code stable API since v1.9 supports
workspace.openTextDocument({ language, content }), so you can replace the temp-file approach with an in-memory document as shown. Note that VS Code’s automatic language detection may override the requested language in some cases.- private async openMarkdownPreview(markdownContent: string, originalFileName: string): Promise<void> { - // Create a temporary markdown file with a descriptive name in the system temp folder - const baseName = path.basename(originalFileName, '.zmodel'); - const tempFileName = `${baseName}-preview.md`; - const tempFilePath = path.join(os.tmpdir(), tempFileName); - const tempFile = vscode.Uri.file(tempFilePath); - - try { - // Write the markdown content to the temp file - await vscode.workspace.fs.writeFile(tempFile, new TextEncoder().encode(markdownContent)); - - // Open the markdown preview side by side - await vscode.commands.executeCommand('markdown.showPreviewToSide', tempFile); - } catch (error) { - console.error('Error creating markdown preview:', error); - throw new Error( - `Failed to create markdown preview: ${error instanceof Error ? error.message : String(error)}` - ); - } - } + private async openMarkdownPreview(markdownContent: string, _originalFileName: string): Promise<void> { + const doc = await vscode.workspace.openTextDocument({ language: 'markdown', content: markdownContent }); + await vscode.commands.executeCommand('markdown.showPreviewToSide', doc.uri); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/schema/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (6)
packages/schema/src/documentation-cache.ts(1 hunks)packages/schema/src/extension.ts(1 hunks)packages/schema/src/release-notes-manager.ts(1 hunks)packages/schema/src/res/zmodel-preview-release-notes.html(1 hunks)packages/schema/src/zenstack-auth-provider.ts(1 hunks)packages/schema/src/zmodel-preview.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/schema/src/res/zmodel-preview-release-notes.html
🧰 Additional context used
🧬 Code graph analysis (2)
packages/schema/src/zmodel-preview.ts (2)
packages/schema/src/documentation-cache.ts (1)
DocumentationCache(15-152)packages/schema/src/extension.ts (1)
requireAuth(14-36)
packages/schema/src/extension.ts (4)
packages/schema/src/zenstack-auth-provider.ts (2)
AUTH_PROVIDER_ID(11-11)ZenStackAuthenticationProvider(14-234)packages/schema/src/documentation-cache.ts (1)
DocumentationCache(15-152)packages/schema/src/zmodel-preview.ts (1)
ZModelPreview(12-278)packages/schema/src/release-notes-manager.ts (1)
ReleaseNotesManager(6-77)
⏰ 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). (5)
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
packages/schema/src/documentation-cache.ts (1)
56-64: Stable, format-insensitive cache keys: LGTM.Sorting and stripping whitespace before hashing prevents spurious cache misses. Good fix for ordering concerns.
packages/schema/src/zmodel-preview.ts (1)
187-195: Provide curl commands to verify auth format
Run these locally to compare responses and headers:TOKEN="<your_access_token>" # Without Bearer prefix curl -v -X POST https://api.zenstack.dev/api/doc \ -H "Content-Type: application/json" \ -H "Authorization: ${TOKEN}" \ -d '{}' # With Bearer prefix curl -v -X POST https://api.zenstack.dev/api/doc \ -H "Content-Type: application/json" \ -H "Authorization: Bearer ${TOKEN}" \ -d '{}'Check the HTTP status and any
WWW-Authenticateheader in each response to confirm if theBearerprefix is required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
packages/schema/src/zenstack-auth-provider.ts (8)
1-1: Import crypto for secure OAuth state generationUse cryptographically strong randomness for OAuth
state.import * as vscode from 'vscode'; +import { randomBytes } from 'crypto';
23-27: Track OAuth state in pendingAuthYou generate/validate
stateacross the flow; store it here.private pendingAuth?: { + state: string; resolve: (session: vscode.AuthenticationSession) => void; reject: (error: Error) => void; scopes: readonly string[]; };
89-97: Add redirect_uri/state/scopes; use asExternalUri; don’t log sensitive URLsCurrent flow lacks
redirect_uri,state, andscope, making it brittle and CSRF-prone; also log leaksstate. UseasExternalUrifor remotes/web.- return new Promise<vscode.AuthenticationSession>((resolve, reject) => { + return new Promise<vscode.AuthenticationSession>(async (resolve, reject) => { // Handle cancellation token.onCancellationRequested(() => { if (this.pendingAuth) { delete this.pendingAuth; } reject(new Error('User Cancelled')); }); - const isCursor = vscode.env.appName == 'Cursor'; - - let signInUrl = vscode.Uri.parse(new URL('/sign-in', AUTH_URL).toString()); - - if (isCursor) { - signInUrl = signInUrl.with({ - query: `redirect_url=${API_URL}/oauth/oauth_callback?vscodeapp=cursor`, - }); - } - - console.log('ZenStack sign-in URL:', signInUrl.toString()); - // Store the state and resolve function for later use - this.pendingAuth = { resolve, reject, scopes }; + const isCursor = vscode.env.appName === 'Cursor'; + // Generate state and build callback + const state = this.generateState(); + const extensionId = this._context.extension?.id ?? 'zenstackhq.zenstack'; + const callbackUri = vscode.Uri.parse(`${vscode.env.uriScheme}://${extensionId}/auth-callback`); + const externalCallbackUri = await vscode.env.asExternalUri(callbackUri); + + // Build sign-in URL (implicit flow: token in fragment) + const url = new URL('/sign-in', AUTH_URL); + url.searchParams.set('response_type', 'token'); + url.searchParams.set('redirect_uri', externalCallbackUri.toString()); + url.searchParams.set('state', state); + if (scopes.length) url.searchParams.set('scope', scopes.join(' ')); + + // Cursor-specific override if required by backend + if (isCursor) { + // keep state; server must echo it back + url.searchParams.set('redirect_url', `${API_URL}/oauth/oauth_callback?vscodeapp=cursor`); + } + + const signInUrl = vscode.Uri.parse(url.toString()); + + // Store the state and resolve function for later use + this.pendingAuth = { state, resolve, reject, scopes }; // Open the ZenStack sign-in page in the user's default browser - vscode.env.openExternal(signInUrl).then( + vscode.env.openExternal(signInUrl).then( () => { - console.log('Opened ZenStack sign-in page in browser'); + // avoid logging the full URL/state progress.report({ message: 'Waiting for return from browser...' }); },Also applies to: 98-106, 108-111
136-138: Generate OAuth state with crypto.randomBytes
Math.randomis not suitable for CSRF protection.private generateState(): string { - return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); + return randomBytes(16).toString('hex'); }
140-164: Validate state and read token from URI fragment; always clear pendingAuthImplicit flow returns params in
#fragment. Validatestate, handleerror, propagate scopes, and clearpendingAuthin all paths.// Handle authentication callback from ZenStack public async handleAuthCallback(callbackUri: vscode.Uri): Promise<void> { - const query = new URLSearchParams(callbackUri.query); - const accessToken = query.get('access_token'); - if (!this.pendingAuth) { - console.warn('No pending authentication found'); - return; - } - if (!accessToken) { - this.pendingAuth.reject(new Error('No access token received')); - delete this.pendingAuth; - return; - } - try { - // Create session from the access token - const session = await this.createSessionFromAccessToken(accessToken); - this.pendingAuth.resolve(session); - delete this.pendingAuth; - } catch (error) { - if (this.pendingAuth) { - this.pendingAuth.reject(error instanceof Error ? error : new Error(String(error))); - delete this.pendingAuth; - } - } + const params = new URLSearchParams((callbackUri.fragment || '').replace(/^#/, '') || callbackUri.query); + const accessToken = params.get('access_token'); + const state = params.get('state'); + const error = params.get('error'); + + if (!this.pendingAuth) { + console.warn('No pending authentication found'); + return; + } + try { + if (error) throw new Error(`Auth error: ${error}`); + if (!state || state !== this.pendingAuth.state) throw new Error('State mismatch'); + if (!accessToken) throw new Error('No access token received'); + const session = await this.createSessionFromAccessToken(accessToken, this.pendingAuth.scopes); + this.pendingAuth.resolve(session); + } catch (e) { + this.pendingAuth.reject(e instanceof Error ? e : new Error(String(e))); + } finally { + delete this.pendingAuth; + } }
166-178: Propagate scopes and reject expired tokens when creating a sessionAlso avoid empty scopes in stored sessions.
- private async createSessionFromAccessToken(accessToken: string): Promise<vscode.AuthenticationSession> { + private async createSessionFromAccessToken( + accessToken: string, + scopes: readonly string[] + ): Promise<vscode.AuthenticationSession> { try { // Decode JWT to get claims const claims = this.parseJWTClaims(accessToken); + if (claims.exp && Date.now() / 1000 >= claims.exp) { + throw new Error('Access token expired'); + } return { id: claims.jti || Math.random().toString(36), accessToken: accessToken, account: { id: claims.sub || 'unknown', label: claims.email || 'unknown@zenstack.dev', }, - scopes: [], + scopes: [...scopes], }; } catch (error) { throw new Error(`Failed to create session from access token: ${error}`); } }
184-202: Fix JWT decoding: use Buffer base64url; handle parse errors cleanly
atobisn’t available in Node; base64url must be handled.private parseJWTClaims(token: string): JWTClaims { try { // JWT tokens have 3 parts separated by dots: header.payload.signature const parts = token.split('.'); if (parts.length !== 3) { throw new Error('Invalid JWT format'); } - // Decode the payload (second part) - const payload = parts[1]; - // Add padding if needed for base64 decoding - const paddedPayload = payload + '='.repeat((4 - (payload.length % 4)) % 4); - const decoded = atob(paddedPayload); - - return JSON.parse(decoded); + // Decode the payload (second part) using base64url + const decoded = Buffer.from(parts[1], 'base64url').toString('utf8'); + return JSON.parse(decoded); } catch (error) { throw new Error(`Failed to parse JWT claims: ${error}`); } }
108-109: Remove URL logging to avoid leakingstateand identifiersDon’t log full auth URLs.
- console.log('ZenStack sign-in URL:', signInUrl.toString()); + // Do not log sign-in URL with sensitive query params
🧹 Nitpick comments (5)
packages/schema/src/zenstack-auth-provider.ts (5)
44-49: Respect requested scopes in getSessionsFilter stored sessions by scopes when provided.
- async getSessions(_scopes?: readonly string[]): Promise<vscode.AuthenticationSession[]> { + async getSessions(_scopes?: readonly string[]): Promise<vscode.AuthenticationSession[]> { // Check if we have stored sessions in VS Code's secret storage const storedSessions = await this.getStoredSessions(); - this._sessions = storedSessions; - return this._sessions; + this._sessions = storedSessions; + if (_scopes && _scopes.length) { + return this._sessions.filter((s) => _scopes.every((sc) => s.scopes.includes(sc))); + } + return this._sessions; }
51-64: Avoid duplicate sessions; reuse one that already satisfies scopesShort-circuit if an existing session already covers requested scopes.
async createSession(scopes: readonly string[]): Promise<vscode.AuthenticationSession> { - // Create a login flow + // Reuse existing session that already satisfies scopes + const existing = (await this.getSessions()).find((s) => scopes.every((sc) => s.scopes.includes(sc))); + if (existing) return existing; + // Create a login flow const session = await this.performLogin(scopes);
126-132: Consider a longer timeout (90–120s) for slower SSO hops60s can be tight with federated IdPs.
- }, 60000); + }, 120000);
214-221: De-duplicate sessions on store to avoid unbounded growthPersist unique sessions by
id.private async storeSession(session: vscode.AuthenticationSession): Promise<void> { try { const sessions = await this.getStoredSessions(); - sessions.push(session); + const idx = sessions.findIndex((s) => s.id === session.id); + if (idx >= 0) sessions[idx] = session; + else sessions.push(session); await this._context.secrets.store('zenstack-auth-sessions', JSON.stringify(sessions)); } catch (error) { console.error('Error storing session:', error); } }
11-13: Nit: provider id casingVS Code samples typically use lowercase ids (e.g.,
github,microsoft). Considerzenstackto avoid surprises for API consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/schema/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (3)
packages/schema/src/documentation-cache.ts(1 hunks)packages/schema/src/zenstack-auth-provider.ts(1 hunks)packages/schema/src/zmodel-preview.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/schema/src/documentation-cache.ts
- packages/schema/src/zmodel-preview.ts
⏰ 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). (5)
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
- GitHub Check: dependency-review
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
🔇 Additional comments (1)
packages/schema/src/zenstack-auth-provider.ts (1)
98-106: Cursor OAuth: add state & confirm redirect param
- Include a cryptographically-random
statein the Cursor sign-in URL and validate the echoedstateon the OAuth callback; without this the flow is vulnerable to CSRF and state validation can fail.- Confirm whether the server expects
redirect_uri(OAuth standard) orredirect_url(current code usesredirect_url) and update packages/schema/src/zenstack-auth-provider.ts (lines 98–106) to use the correct param.- Optional: centralize sign-in URL construction and state generation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
packages/schema/src/zenstack-auth-provider.ts (8)
124-124: Drop noisy sign-in URL log (or gate behind debug).- console.log('ZenStack sign-in URL:', signInUrl.toString()); + // optionally: vscode.window.showDebugMessage(`ZenStack sign-in URL: ${signInUrl.toString()}`);
96-151: Critical: OAuth flow lacks redirect_uri/state/scopes and remote-safe callback.Add state, scopes, and a VS Code deep-link callback using asExternalUri; pass state to the server and stash it for validation.
- private async performLogin(scopes: readonly string[]): Promise<vscode.AuthenticationSession> { + private async performLogin(scopes: readonly string[]): Promise<vscode.AuthenticationSession> { // Create the authentication promise return vscode.window.withProgress( { location: vscode.ProgressLocation.Notification, title: 'Signing in to ZenStack', cancellable: true, }, - async (progress, token) => { - return new Promise<vscode.AuthenticationSession>((resolve, reject) => { + async (progress, token) => { + return new Promise<vscode.AuthenticationSession>(async (resolve, reject) => { // Handle cancellation token.onCancellationRequested(() => { if (this.pendingAuth) { delete this.pendingAuth; } reject(new Error('User Cancelled')); }); const isCursor = vscode.env.appName == 'Cursor'; - let signInUrl = vscode.Uri.parse(new URL('/sign-in', AUTH_URL).toString()); + // Build callback + external URI for remote-safe redirect + const extensionId = this._context.extension?.id ?? 'zenstackhq.zenstack'; + const callbackUri = vscode.Uri.parse(`${vscode.env.uriScheme}://${extensionId}/auth-callback`); + const externalCallbackUri = await vscode.env.asExternalUri(callbackUri); + const state = this.generateState(); + + let signInUrl = vscode.Uri.parse(new URL('/sign-in', AUTH_URL).toString()); if (isCursor) { - signInUrl = signInUrl.with({ - query: `redirect_url=${API_URL}/oauth/oauth_callback?vscodeapp=cursor`, - }); + const q = new URLSearchParams(); + q.set('redirect_url', `${API_URL}/oauth/oauth_callback?vscodeapp=cursor`); + q.set('state', state); + if (scopes.length) q.set('scope', scopes.join(' ')); + signInUrl = signInUrl.with({ query: q.toString() }); + } else { + const q = new URLSearchParams(); + q.set('response_type', 'token'); + q.set('redirect_uri', externalCallbackUri.toString()); + q.set('state', state); + if (scopes.length) q.set('scope', scopes.join(' ')); + signInUrl = signInUrl.with({ query: q.toString() }); } - console.log('ZenStack sign-in URL:', signInUrl.toString()); // Store the state and resolve function for later use - this.pendingAuth = { resolve, reject, scopes }; + this.pendingAuth = { resolve, reject, scopes, state }; // Open the ZenStack sign-in page in the user's default browser vscode.env.openExternal(signInUrl).then( () => { - console.log('Opened ZenStack sign-in page in browser'); progress.report({ message: 'Waiting for return from browser...' }); }, (error) => { if (this.pendingAuth) { delete this.pendingAuth; } reject(new Error(`Failed to open sign-in page: ${error}`)); } ); // 2 minutes timeout setTimeout(() => { if (this.pendingAuth) { delete this.pendingAuth; } reject(new Error('Timeout')); }, 120000); }); } ); }
23-27: Plumb state through pendingAuth for CSRF protection.private pendingAuth?: { resolve: (session: vscode.AuthenticationSession) => void; reject: (error: Error) => void; scopes: readonly string[]; + state: string; };
157-181: Critical: Validate state and read token from URI fragment.Implicit/hybrid flows return params in the fragment; also enforce state check.
- public async handleAuthCallback(callbackUri: vscode.Uri): Promise<void> { - const query = new URLSearchParams(callbackUri.query); - const accessToken = query.get('access_token'); + public async handleAuthCallback(callbackUri: vscode.Uri): Promise<void> { + const params = new URLSearchParams(callbackUri.fragment?.replace(/^#/, '') || callbackUri.query); + const accessToken = params.get('access_token'); + const state = params.get('state'); if (!this.pendingAuth) { console.warn('No pending authentication found'); return; } + if (!state || state !== this.pendingAuth.state) { + this.pendingAuth.reject(new Error('State mismatch')); + delete this.pendingAuth; + return; + } if (!accessToken) { this.pendingAuth.reject(new Error('No access token received')); delete this.pendingAuth; return; } try { // Create session from the access token - const session = await this.createSessionFromAccessToken(accessToken); + const session = await this.createSessionFromAccessToken(accessToken, this.pendingAuth.scopes); this.pendingAuth.resolve(session); delete this.pendingAuth; } catch (error) { if (this.pendingAuth) { this.pendingAuth.reject(error instanceof Error ? error : new Error(String(error))); delete this.pendingAuth; } } }
183-199: Propagate scopes and reject expired tokens.- private async createSessionFromAccessToken(accessToken: string): Promise<vscode.AuthenticationSession> { + private async createSessionFromAccessToken( + accessToken: string, + scopes: readonly string[] + ): Promise<vscode.AuthenticationSession> { try { // Decode JWT to get claims const claims = this.parseJWTClaims(accessToken); + if (claims.exp && Date.now() / 1000 >= claims.exp) { + throw new Error('Access token expired'); + } return { id: claims.jti || Math.random().toString(36), accessToken: accessToken, account: { id: claims.sub || 'unknown', label: claims.email || 'unknown@zenstack.dev', }, - scopes: [], + scopes: [...scopes], }; } catch (error) { throw new Error(`Failed to create session from access token: ${error}`); } }
201-219: Critical: atob is undefined in Node and base64url must be handled.private parseJWTClaims(token: string): JWTClaims { - try { - // JWT tokens have 3 parts separated by dots: header.payload.signature - const parts = token.split('.'); - if (parts.length !== 3) { - throw new Error('Invalid JWT format'); - } - - // Decode the payload (second part) - const payload = parts[1]; - // Add padding if needed for base64 decoding - const paddedPayload = payload + '='.repeat((4 - (payload.length % 4)) % 4); - const decoded = atob(paddedPayload); - - return JSON.parse(decoded); - } catch (error) { - throw new Error(`Failed to parse JWT claims: ${error}`); - } + // JWT tokens have 3 parts separated by dots: header.payload.signature + const parts = token.split('.'); + if (parts.length !== 3) { + throw new Error('Invalid JWT format'); + } + try { + const decoded = Buffer.from(parts[1], 'base64url').toString('utf8'); + return JSON.parse(decoded); + } catch (error) { + throw new Error(`Failed to parse JWT claims: ${error instanceof Error ? error.message : String(error)}`); + } }
153-155: Use cryptographically strong randomness for OAuth state.private generateState(): string { - return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); + return randomBytes(16).toString('hex'); }
1-1: Import crypto for secure state generation.-import * as vscode from 'vscode'; +import * as vscode from 'vscode'; +import { randomBytes } from 'crypto';
🧹 Nitpick comments (3)
packages/schema/src/zenstack-auth-provider.ts (3)
48-53: Filter sessions by requested scopes.async getSessions(_scopes?: readonly string[]): Promise<vscode.AuthenticationSession[]> { // Check if we have stored sessions in VS Code's secret storage const storedSessions = await this.getStoredSessions(); this._sessions = storedSessions; - return this._sessions; + if (_scopes && _scopes.length) { + return this._sessions.filter((s) => _scopes.every((sc) => (s.scopes ?? []).includes(sc))); + } + return this._sessions; }
87-94: Await logout removals; forEach + async is not awaited.- (await this.getSessions()).forEach(async (s) => await this.removeSession(s.id)); - vscode.window.showInformationMessage('Successfully logged out of ZenStack.'); + const sessions = await this.getSessions(); + await Promise.all(sessions.map((s) => this.removeSession(s.id))); + vscode.window.showInformationMessage('Successfully logged out of ZenStack.');
231-239: Avoid duplicate sessions in secret storage.private async storeSession(session: vscode.AuthenticationSession): Promise<void> { try { const sessions = await this.getStoredSessions(); - sessions.push(session); + const idx = sessions.findIndex((s) => s.id === session.id); + if (idx >= 0) sessions[idx] = session; + else sessions.push(session); await this._context.secrets.store('zenstack-auth-sessions', JSON.stringify(sessions)); } catch (error) { console.error('Error storing session:', error); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/schema/package.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
packages/schema/src/documentation-cache.ts(1 hunks)packages/schema/src/zenstack-auth-provider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/schema/src/documentation-cache.ts
⏰ 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). (3)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
🔇 Additional comments (2)
packages/schema/src/zenstack-auth-provider.ts (2)
32-46: Register a dispose for the URI handler and command (already covered via from()). LGTM.Also applies to: 41-45
96-151: Verify package.json contributes: authentication id='ZenStack' and a URI handler for /auth-callbackpackages/schema/src/zenstack-auth-provider.ts registers AUTH_PROVIDER_ID = 'ZenStack' and a URI handler for path '/auth-callback'; confirm the VS Code extension package.json (the extension package that ships the VS Code extension) includes under "contributes" an "authentication" entry with id exactly "ZenStack" and a URI handler (or equivalent) that routes the browser auth callback to the extension.
No description provided.