-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: prevent C# LSP crashes from excessively long URIs #4576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
abc2378
5b8d172
ed73805
8fba446
9409490
6d4dc1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "roo-cline": patch | ||
| --- | ||
|
|
||
| Fix C# LSP crashes caused by excessively long URIs in diff views and checkpoints. Added URI length validation to prevent crashes when working with large files by truncating content that would exceed safe URI length limits. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import { ClineSayTool } from "../../shared/ExtensionMessage" | |
| import { Task } from "../../core/task/Task" | ||
|
|
||
| import { DecorationController } from "./DecorationController" | ||
| import { createSafeContentUri } from "../../utils/uri" | ||
|
|
||
| export const DIFF_VIEW_URI_SCHEME = "cline-diff" | ||
|
|
||
|
|
@@ -310,14 +311,14 @@ export class DiffViewProvider { | |
| indentBy: "", | ||
| suppressEmptyNode: true, | ||
| processEntities: false, | ||
| tagValueProcessor: (name, value) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was there a reason to change name to _name or did the AI just decide to "help"
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Reverted the change. The |
||
| tagValueProcessor: (_name, value) => { | ||
| if (typeof value === "string") { | ||
| // Only escape <, >, and & characters | ||
| return value.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">") | ||
| } | ||
| return value | ||
| }, | ||
| attributeValueProcessor: (name, value) => { | ||
| attributeValueProcessor: (_name, value) => { | ||
| if (typeof value === "string") { | ||
| // Only escape <, >, and & characters | ||
| return value.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">") | ||
|
|
@@ -444,9 +445,7 @@ export class DiffViewProvider { | |
|
|
||
| vscode.commands.executeCommand( | ||
| "vscode.diff", | ||
| vscode.Uri.parse(`${DIFF_VIEW_URI_SCHEME}:${fileName}`).with({ | ||
| query: Buffer.from(this.originalContent ?? "").toString("base64"), | ||
| }), | ||
| createSafeContentUri(DIFF_VIEW_URI_SCHEME, fileName, this.originalContent ?? ""), | ||
| uri, | ||
| `${fileName}: ${fileExists ? "Original ↔ Roo's Changes" : "New File"} (Editable)`, | ||
| { preserveFocus: true }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| import * as vscode from "vscode" | ||
| import { createSafeContentUri, MAX_SAFE_URI_LENGTH } from "../uri" | ||
|
|
||
| // Mock vscode.Uri to avoid VS Code dependency in tests | ||
| jest.mock("vscode", () => ({ | ||
| Uri: { | ||
| parse: jest.fn((uriString: string) => ({ | ||
| toString: () => uriString, | ||
| with: jest.fn(({ query }: { query: string }) => ({ | ||
| toString: () => `${uriString}?${query}`, | ||
| scheme: uriString.split(":")[0], | ||
| path: uriString.split(":")[1], | ||
| query, | ||
| })), | ||
| })), | ||
| }, | ||
| })) | ||
|
|
||
| describe("uri utilities", () => { | ||
| describe("createSafeContentUri", () => { | ||
| const scheme = "test-scheme" | ||
| const path = "test-file.txt" | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| it("should create a normal URI for small content", () => { | ||
| const content = "small content" | ||
| const result = createSafeContentUri(scheme, path, content) | ||
|
|
||
| expect(vscode.Uri.parse).toHaveBeenCalledWith(`${scheme}:${path}`) | ||
| expect(result.query).toBe(Buffer.from(content).toString("base64")) | ||
| }) | ||
|
|
||
| it("should truncate content when URI would exceed safe length", () => { | ||
| // Create content that would result in a very long URI | ||
| const longContent = "x".repeat(10000) // 10KB of content | ||
| const result = createSafeContentUri(scheme, path, longContent) | ||
|
|
||
| // Verify the URI was created | ||
| expect(vscode.Uri.parse).toHaveBeenCalledWith(`${scheme}:${path}`) | ||
|
|
||
| // Decode the base64 query to check if content was truncated | ||
| const decodedContent = Buffer.from(result.query, "base64").toString() | ||
| expect(decodedContent).toContain("... [Content truncated to prevent LSP crashes]") | ||
| expect(decodedContent.length).toBeLessThan(longContent.length) | ||
|
|
||
| // Verify the total URI length is within safe limits | ||
| const totalUriLength = result.toString().length | ||
| expect(totalUriLength).toBeLessThanOrEqual(MAX_SAFE_URI_LENGTH) | ||
| }) | ||
|
|
||
| it("should handle empty content", () => { | ||
| const content = "" | ||
| const result = createSafeContentUri(scheme, path, content) | ||
|
|
||
| expect(result.query).toBe(Buffer.from(content).toString("base64")) | ||
| }) | ||
|
|
||
| it("should handle content exactly at the safe limit", () => { | ||
| // Calculate content size that would result in URI exactly at limit | ||
| const baseUri = `${scheme}:${path}` | ||
| const overhead = baseUri.length + 50 | ||
| const maxBase64Length = MAX_SAFE_URI_LENGTH - overhead | ||
| const maxContentLength = Math.floor((maxBase64Length * 3) / 4) | ||
|
|
||
| const content = "x".repeat(maxContentLength) | ||
| const result = createSafeContentUri(scheme, path, content) | ||
|
|
||
| const totalUriLength = result.toString().length | ||
| expect(totalUriLength).toBeLessThanOrEqual(MAX_SAFE_URI_LENGTH) | ||
| }) | ||
|
|
||
| it("should handle invalid characters gracefully", () => { | ||
| // Test with potentially problematic content that might cause issues | ||
| const problemContent = "\uFFFE\uFFFF\x00\x01\x02" | ||
| const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation() | ||
|
|
||
| const result = createSafeContentUri(scheme, path, problemContent) | ||
|
|
||
| // Should still create a valid URI (may log error but shouldn't crash) | ||
| expect(result).toBeDefined() | ||
| expect(result.query).toBeDefined() | ||
|
|
||
| consoleErrorSpy.mockRestore() | ||
| }) | ||
|
|
||
| it("should use provided scheme and path correctly", () => { | ||
| const customScheme = "cline-diff" | ||
| const customPath = "src/components/Button.tsx" | ||
| const content = "test content" | ||
|
|
||
| createSafeContentUri(customScheme, customPath, content) | ||
|
|
||
| expect(vscode.Uri.parse).toHaveBeenCalledWith(`${customScheme}:${customPath}`) | ||
| }) | ||
|
|
||
| it("should preserve content when within safe limits", () => { | ||
| const content = "This is some test content that should not be truncated" | ||
| const result = createSafeContentUri(scheme, path, content) | ||
|
|
||
| const decodedContent = Buffer.from(result.query, "base64").toString() | ||
| expect(decodedContent).toBe(content) | ||
| expect(decodedContent).not.toContain("... [Content truncated to prevent LSP crashes]") | ||
| }) | ||
|
|
||
| it("should calculate base64 expansion correctly", () => { | ||
| // Base64 encoding expands content by ~4/3 | ||
| const content = "x".repeat(1000) | ||
| const base64Content = Buffer.from(content).toString("base64") | ||
|
|
||
| // Verify our calculation assumption | ||
| expect(base64Content.length).toBeCloseTo((content.length * 4) / 3, -1) // Within 10% | ||
| }) | ||
| }) | ||
|
|
||
| describe("MAX_SAFE_URI_LENGTH constant", () => { | ||
| it("should be set to a reasonable value", () => { | ||
| expect(MAX_SAFE_URI_LENGTH).toBe(8192) | ||
| expect(MAX_SAFE_URI_LENGTH).toBeGreaterThan(2000) // Above minimum | ||
| expect(MAX_SAFE_URI_LENGTH).toBeLessThan(32768) // Below typical maximum | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import * as vscode from "vscode" | ||
|
|
||
| // Maximum safe URI length to avoid crashes in language servers | ||
| // Most systems have limits between 2KB-32KB, using conservative 8KB limit | ||
| export const MAX_SAFE_URI_LENGTH = 8192 | ||
|
|
||
| /** | ||
| * Safely creates a URI with content encoded in the query parameter. | ||
| * If the resulting URI would be too long, truncates the content to avoid LSP crashes. | ||
| * | ||
| * @param scheme - The URI scheme (e.g., "cline-diff") | ||
| * @param path - The URI path/identifier | ||
| * @param content - Content to encode as base64 in the query parameter | ||
| * @returns A safe URI that won't exceed system limits | ||
| */ | ||
| export function createSafeContentUri(scheme: string, path: string, content: string): vscode.Uri { | ||
| try { | ||
| const base64Content = Buffer.from(content).toString("base64") | ||
| const baseUri = `${scheme}:${path}` | ||
| const uri = vscode.Uri.parse(baseUri).with({ query: base64Content }) | ||
|
|
||
| if (uri.toString().length <= MAX_SAFE_URI_LENGTH) { | ||
| return uri | ||
| } | ||
|
|
||
| // Calculate available space for content after accounting for URI overhead | ||
| const overhead = baseUri.length + 100 // Extra buffer for URI encoding and VS Code overhead | ||
| const maxBase64Length = Math.max(0, MAX_SAFE_URI_LENGTH - overhead) | ||
|
|
||
| // Truncate content to fit within safe URI length | ||
| const maxContentLength = Math.floor((maxBase64Length * 3) / 4) // Base64 is ~4/3 the size | ||
| const truncatedContent = | ||
| content.length > maxContentLength | ||
| ? content.substring(0, maxContentLength) + "\n... [Content truncated to prevent LSP crashes]" | ||
| : content | ||
|
|
||
| const truncatedBase64 = Buffer.from(truncatedContent).toString("base64") | ||
| return vscode.Uri.parse(baseUri).with({ query: truncatedBase64 }) | ||
| } catch (error) { | ||
| console.error(`Failed to create safe content URI for ${path}:`, error) | ||
| // Fallback to empty content if all else fails | ||
| return vscode.Uri.parse(`${scheme}:${path}`).with({ | ||
| query: Buffer.from("").toString("base64"), | ||
| }) | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.