microsoft / microsoft/winget-cli

[AppInstallerCLIE2ETests] If "StaticFileRootPath" in the .runsettings is not set before unit tests run, unit test run will try to remove user's entire %TEMP% directory

Open
#2,594 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Issue-Bug
Dominant language
C++
Stars
26.4k
Forks
1.8k
Avg merge
1d 11h
Merged PRs (30d)
15

Description

Brief description of your issue

When AppInstallerCLIE2ETests's setup code runs, it attempts to set a variable called TestCommon.StaticFileRootPath, first based on a parameter called StaticRootFilePath. If this is not set, it uses Path.GetTempPath:

https://github.com/microsoft/winget-cli/blob/3aa7aa444b862d7a186ad13cdc7322c3e6186f07/src/AppInstallerCLIE2ETests/SetUpFixture.cs#L85

Then, based on the output of this statement the setup will eventually attempt to entirely remove that directory and its contents:
https://github.com/microsoft/winget-cli/blob/082b9907644244c8697b4cdd4616ea111f15b73a/src/AppInstallerCLIE2ETests/TestIndexSetup.cs#L167

This presents a major problem if it falls through to the Path.GetTempPath()-derived value as it will try to remove the entirety of the user's %TEMP% directory.

This has a couple of obvious problems, the first being of course that it tries to remove the user's %TEMP% directory which doesn't seem like a desirable behavior. From a more practical standpoint, this path will very likely have files in it that are in use by some other process and there's no error tolerance for the directory cleanup. As a result will end up causing the unit test to fail during the setup step.

I discovered this by accident when I was trying to run a unit test here to perform some debugging into an issue and found that the setup step kept failing to remove a file in my %TEMP% directory. Digging further I found how it was trying to remove my entire %TEMP% directory since I was missing a setting in the .runsettings.

The README.md does note that StaticFileRoot must be set, so it seems reasonable that the unit test setup should fail right away rather than falling back to Path.GetTempPath(). However, if this is a desired fallback (even though this contradicts the documentation), it should Path.Combine(...) the output of Path.GetTempPath() with a sub-path so that is what gets cleaned up rather than the user's entire %TEMP% path.

Steps to reproduce

Run AppInstallerCLIE2ETests without setting a StaticFileRootPath in the runsettings.

Expected behavior

Not try to delete the entirety of the user's %TEMP% folder

Actual behavior

Tries to delete the entirety of the user's %TEMP% folder

Environment
Windows Package Manager (Preview) v1.4.2161-preview
Copyright (c) Microsoft Corporation. All rights reserved.

Windows: Windows.Desktop v10.0.19044.2130
System Architecture: X64
Package: Microsoft.DesktopAppInstaller v1.19.2161.0

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

Read src/AppInstallerCLIE2ETests/SetUpFixture.cs around line 85 and src/AppInstallerCLIE2ETests/TestIndexSetup.cs around line 167, then check the test setup instructions in README.md. Reproduce the run without StaticFileRootPath and verify that setup fails safely or confines cleanup to a dedicated temporary subdirectory rather than deleting the user's entire %TEMP% directory.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
cli, testing-qa
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.