microsoft / microsoft/WinAppVSCE

XAML: constrain document-link targets to allowed roots

Open
#210 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
13
Forks
3
Avg merge
6d 1h
Merged PRs (30d)
11

Description

Follow-up from the PR #50 review (finding M1).

server/src/WinUiXaml.LanguageServer/XamlDocumentLinks.cs Combine() (lines 134-152) resolves a link target with Path.GetFullPath and then probes File.Exists, with no containment check against the workspace/project root. A crafted Source="C:\..." or a ..-heavy relative path in a XAML file therefore produces a clickable document link to an arbitrary local path.

Severity is limited because:

  • package.json declares untrustedWorkspaces: "limited", so the server does not run in untrusted workspaces.
  • The result is a link the user must click, not an automatic read of file contents.

The reason this was not fixed in PR #50: server/test/WinUiXaml.LanguageServer.Tests/XamlDocumentLinksTests.cs lines 265-272 explicitly asserts that an out-of-root absolute path such as C:\other\Colors.xaml resolves to a link. Tightening Combine() would break a blessed test, so this needs a product decision first.

Suggested work:

  • Decide whether absolute out-of-root link targets are intended (they are legitimate for some shared-resource-dictionary layouts).
  • If not, constrain resolution to the project/workspace roots and update XamlDocumentLinksTests.cs accordingly.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with Combine() in server/src/WinUiXaml.LanguageServer/XamlDocumentLinks.cs and the out-of-root case in server/test/WinUiXaml.LanguageServer.Tests/XamlDocumentLinksTests.cs. Resolve the product decision about absolute targets first; done means the chosen containment policy is implemented and the affected test assertions match it.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
security, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.