Repository navigation
Make LSP server load content mappers only from node_modules inside workspace folders - #64837
Conversation
Adapt the rebased workspace policy to the typed-path constructors and component splitting required by current main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-from-workspace-folders
There was a problem hiding this comment.
🟡 Changes recommended
Workspace-folder additions and removals leave the content-mapper allowlist stale until the server restarts.
1 open finding
What changed in this PR
Restricts language-server content mappers to packages resolved through node_modules inside workspace folders, preventing loading from unrelated directories.
Changes:
- Pass initial workspace roots into content-mapper resolution policy.
- Report a diagnostic for rejected packages.
- Add workspace-boundary and symlink tests; update existing test initialization.
| File | Description |
|---|---|
| tsc/internal/tsoptions/contentmappers.go | Check package policy before loading manifests. |
| tsc/internal/project/snapshotfs.go | Enforce workspace containment. |
| tsc/internal/project/session.go | Add initial workspace roots option. |
| tsc/internal/project/contentmapper_test.go | Test allowed and rejected package locations. |
| tsc/internal/project/configfileregistrybuilder.go | Apply workspace roots to config resolution. |
| tsc/internal/lsp/server.go | Extract and pass workspace roots. |
| tsc/internal/lsp/server_test.go | Test workspace-root extraction. |
| tsc/internal/lsp/server_contentmapper_test.go | Supply a workspace root for integration testing. |
| tsc/internal/fourslash/fourslash.go | Supply a root for external-code tests. |
| tsc/internal/diagnostics/diagnostics_generated.go | Register the rejection diagnostic. |
| tsc/internal/diagnostics/diagnosticMessages.json | Define the rejection message. |
| tsc/internal/diagnostics/diagnosticMessages.generated.json | Include the generated diagnostic entry. |
Files not reviewed (1)
- tsc/internal/diagnostics/diagnostics_generated.go: Generated file
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| DebounceDelay: 500 * time.Millisecond, | ||
| PushDiagnosticsEnabled: !disablePushDiagnostics, | ||
| RunExternalCode: runExternalCode, | ||
| InitialWorkspaceRoots: workspaceRoots, |
There was a problem hiding this comment.
This was intentionally deferred because we just literally don't do anything correctly with workspace folders. I wasn't intending to add full support here, and "remove a folder from a workspace that was previously open in a trusted window" doesn't mean you stopped trusting it.
|
For that last commit, why can't we check this after tsoptions is done instead of having to reintroduce the config host? |
|
We could of course check afterwards, but (1) every other content mapper config diagnostic is issued right there, and (2) doing it during parsing lets us return a zero manifest so there's no chance that later code forgets to do validation and executes it. It made the most sense to me to bail out at resolution time if you're resolving something you're not allowed to load. |
|
That's fair, just lamenting a bit reintroducing that layer when I just got rid of it 😄 |
| } | ||
|
|
||
| // NewDefaultParseConfigHost leaves external code authorization to the runExternalCode option. | ||
| func NewDefaultParseConfigHost(fs vfs.FS) ParseConfigHost { |
There was a problem hiding this comment.
Perhaps this should be named more like NewParseConfigHostAllowingContentMappers?
It's probably fine, jsut don't want someone to accidentally do this
| InitializationOptions: initializationOptions, | ||
| }, | ||
| } | ||
| if options.RunExternalCode { |
There was a problem hiding this comment.
When does fourslash ever do this?
There was a problem hiding this comment.
All the content mapper fourslash tests do, in fact. I suppose it's not necessary, just kind of inherited from // @runExternalCode: true in compiler tests, which is useful for testing what the diagnostic looks like when it's not there.
When you open a file from an untrusted directory into a trusted VS Code window, VS Code asks you if you really want to do that. But that warning can be permanently suppressed, and users might find it surprising that by trusting the file, TypeScript's language server will find its tsconfig and potentially load content mappers that it resolves from the node_modules belonging to that untrusted directory. This PR prevents that by only allowing the LSP server to load content mappers resolved to a node_modules directory contained by an open workspace folder.