Skip to content
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

Need to validate TypeScript types returned from VS Code before passing them to native process #11375

Closed
Colengms opened this issue Aug 28, 2023 · 4 comments
Labels
bug fixed Check the Milestone for the release in which the fix is or will be available. Language Service
Milestone

Comments

@Colengms
Copy link
Contributor

The repro I'm aware of involves invalid entries in files.associations. i.e.

	"files.associations": {
		"goodStringEntry": "cpp"
		"badBoolEntry": false
	}

If specified in a settings file, VS Code is giving us the same content, despite that it doesn't match what VS Code itself expects. Sending this in our LSP initialization message causes it to fail. The native JSON deserializer expects files.associations entries to be strings. The mismatch causes the deserializer to reject the request, preventing the initialization message from being processed. Lack of proper initialization causes subsequent messages to fail, triggering crashes due to unexpected state such as lack of any entry in the list of workspace folders (at least 1 is always expected, even in single file mode).

Any values that we received from VS Code (at least from user-editable settings) that we include in LSP JSONs messages to the native process, should first go through some sort of type validation.

@Colengms Colengms added this to the 1.18 milestone Aug 28, 2023
@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Aug 28, 2023

Yeah, I saw a user report of this scenario at #11192 (comment) , but the logging shows workbench.desktop.main.js:sourcemap:620 WARN Ignoring configured 'files.associations' for 'editor.trimAutoWhitespace' because its type is not a string but 'boolean', which implies that VS Code may be handling this for us? I don't remember reproing a cpptools failure when using that files.associations, but maybe I just didn't notice.

@Colengms
Copy link
Contributor Author

Should be addressed by: microsoft/vscode#125422 (Some day)

@sean-mcmanus
Copy link
Contributor

Another crash case is #11426

@Colengms Colengms added the fixed Check the Milestone for the release in which the fix is or will be available. label Sep 18, 2023
@Colengms
Copy link
Contributor Author

Resolving this as fixed as it refers to "before passing them to native process", though the issue has actually been addressed on the native side. There is still more potential work here to validate settings values read and used specifically on the TypeScript side.

@sean-mcmanus sean-mcmanus modified the milestones: 1.18, 1.18.0 Oct 12, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fixed Check the Milestone for the release in which the fix is or will be available. Language Service
Projects
None yet
Development

No branches or pull requests

2 participants