Skip to content

Commit 6c69825

Browse files
alericksonCopilot
alerickson
and
Copilot
committed
Add suggestions from code review
Co-authored-by: Copilot <copilot@github.com>
1 parent 7da6e3b commit 6c69825

6 files changed

Lines changed: 89 additions & 83 deletions

File tree

‎src/code/FindHelper.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1196,7 +1196,6 @@ internal void FindDependencyPackagesHelper(ServerApiCall currentServer, Response
11961196
debugMsgs.Enqueue($"Finding dependency '{dep.Name}' version range '{dep.VersionRange}'");
11971197
FindDependencyPackageVersion(dep, currentServer, currentResponseUtil, currentPkg, repository, errorMsgs, warningMsgs, debugMsgs, verboseMsgs);
11981198
});
1199-
// TODO: what is perf if parallel.ForEach is always run?
12001199
}
12011200
else
12021201
{

‎src/code/InstallHelper.cs‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -770,22 +770,18 @@ private ConcurrentDictionary<string, Hashtable> BeginPackageInstall(
770770
ConcurrentDictionary<string, Hashtable> updatedPackagesHash = packagesHash;
771771

772772
// -WhatIf processing.
773-
// TODO::
774773
if (_savePkg && !_cmdletPassedIn.ShouldProcess($"Package to save: '{pkgToInstall.Name}', version: '{pkgVersion}'"))
775-
{
776-
if (!updatedPackagesHash.ContainsKey(pkgToInstall.Name))
777-
{
778-
updatedPackagesHash.TryAdd(pkgToInstall.Name, new Hashtable(StringComparer.InvariantCultureIgnoreCase)
779-
{
780-
{ "isModule", "" },
781-
{ "isScript", "" },
782-
{ "psResourceInfoPkg", pkgToInstall },
783-
{ "tempDirNameVersionPath", tempInstallPath },
784-
{ "pkgVersion", "" },
785-
{ "scriptPath", "" },
786-
{ "installPath", "" }
787-
});
788-
}
774+
{
775+
updatedPackagesHash.TryAdd(pkgToInstall.Name, new Hashtable(StringComparer.InvariantCultureIgnoreCase)
776+
{
777+
{ "isModule", "" },
778+
{ "isScript", "" },
779+
{ "psResourceInfoPkg", pkgToInstall },
780+
{ "tempDirNameVersionPath", tempInstallPath },
781+
{ "pkgVersion", "" },
782+
{ "scriptPath", "" },
783+
{ "installPath", "" }
784+
});
789785
}
790786
else if (!_cmdletPassedIn.ShouldProcess($"Package to install: '{pkgToInstall.Name}', version: '{pkgVersion}'"))
791787
{

‎src/code/LocalServerApiCalls.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public override FindResults FindAll(bool includePrerelease, ResourceType type, o
7272
public override FindResults FindTags(string[] tags, bool includePrerelease, ResourceType _type, out ErrorRecord errRecord)
7373
{
7474
// TODO: pass in ConcurrentQueue to write out debug message.
75-
//_cmdletPassedIn.WriteDebug("In LocalServerApiCalls::FindTags()");
75+
_cmdletPassedIn.WriteDebug("In LocalServerApiCalls::FindTags()");
7676
FindResults tagFindResults = FindTagsHelper(tags, includePrerelease, out errRecord);
7777
if (tagFindResults.IsFindResultsEmpty())
7878
{

‎src/code/Utils.cs‎

Lines changed: 76 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1398,85 +1398,97 @@ private static bool TryReadPSDataFile(
13981398
}
13991399
string contents = System.IO.File.ReadAllText(filePath);
14001400

1401+
// Validate that the file content conforms to restricted language rules before execution.
1402+
// This parses the content into an AST and statically validates it only contains data-file-safe constructs (hashtables, arrays, literals, etc).
1403+
// It throws a ParseException if anything disallowed is found, before any code is run.
1404+
ScriptBlock scriptBlock = ScriptBlock.Create(contents);
1405+
scriptBlock.CheckRestrictedLanguage(allowedCommands, allowedVariables, allowEnvironmentVariables);
1406+
14011407
// Parallel.ForEach calls into this method.
14021408
// Each thread needs its own runspace created to provide a separate environment for operations to run independently.
1403-
Runspace runspace = RunspaceFactory.CreateRunspace();
1404-
runspace.Open();
1405-
runspace.SessionStateProxy.LanguageMode = PSLanguageMode.ConstrainedLanguage;
1406-
1407-
// Save and set the default runspace for the current thread to prevent
1408-
// stale DefaultRunspace from a prior operation on this reused thread-pool thread.
1409-
Runspace previousDefaultRunspace = Runspace.DefaultRunspace;
1410-
try
1409+
using (Runspace runspace = RunspaceFactory.CreateRunspace())
14111410
{
1412-
Runspace.DefaultRunspace = runspace;
1411+
runspace.Open();
1412+
runspace.SessionStateProxy.LanguageMode = PSLanguageMode.ConstrainedLanguage;
14131413

1414-
using (System.Management.Automation.PowerShell pwsh = System.Management.Automation.PowerShell.Create())
1414+
// Save and set the default runspace for the current thread to prevent
1415+
// stale DefaultRunspace from a prior operation on this reused thread-pool thread.
1416+
Runspace previousDefaultRunspace = Runspace.DefaultRunspace;
1417+
try
14151418
{
1416-
pwsh.Runspace = runspace;
1419+
Runspace.DefaultRunspace = runspace;
14171420

1418-
var cmd = new Command(
1419-
command: contents,
1420-
isScript: true,
1421-
useLocalScope: true);
1422-
cmd.MergeMyResults(
1423-
myResult: PipelineResultTypes.Error | PipelineResultTypes.Warning | PipelineResultTypes.Verbose | PipelineResultTypes.Debug | PipelineResultTypes.Information,
1424-
toResult: PipelineResultTypes.Output);
1425-
pwsh.Commands.AddCommand(cmd);
1426-
1427-
try
1421+
using (System.Management.Automation.PowerShell pwsh = System.Management.Automation.PowerShell.Create())
14281422
{
1429-
// Invoke the pipeline and retrieve the results
1430-
var results = pwsh.Invoke();
1431-
1432-
if (results[0] is PSObject pwshObj)
1423+
pwsh.Runspace = runspace;
1424+
1425+
var cmd = new Command(
1426+
command: contents,
1427+
isScript: true,
1428+
useLocalScope: true);
1429+
cmd.MergeMyResults(
1430+
myResult: PipelineResultTypes.Error | PipelineResultTypes.Warning | PipelineResultTypes.Verbose | PipelineResultTypes.Debug | PipelineResultTypes.Information,
1431+
toResult: PipelineResultTypes.Output);
1432+
pwsh.Commands.AddCommand(cmd);
1433+
1434+
try
14331435
{
1434-
switch (pwshObj.BaseObject)
1435-
{
1436-
case ErrorRecord err:
1437-
//_cmdletPassedIn.WriteError(error);
1438-
break;
1439-
1440-
case WarningRecord warning:
1441-
//cmdlet.WriteWarning(warning.Message);
1442-
break;
1443-
1444-
case VerboseRecord verbose:
1445-
//cmdlet.WriteVerbose(verbose.Message);
1446-
break;
1436+
// Invoke the pipeline and retrieve the results
1437+
var results = pwsh.Invoke();
14471438

1448-
case DebugRecord debug:
1449-
//cmdlet.WriteDebug(debug.Message);
1450-
break;
1451-
1452-
case InformationRecord info:
1453-
//cmdlet.WriteInformation(info);
1454-
break;
1455-
1456-
case Hashtable result:
1457-
dataFileInfo = result;
1458-
return true;
1439+
if (results[0] is PSObject pwshObj)
1440+
{
1441+
switch (pwshObj.BaseObject)
1442+
{
1443+
case ErrorRecord err:
1444+
//_cmdletPassedIn.WriteError(error);
1445+
break;
1446+
1447+
case WarningRecord warning:
1448+
//cmdlet.WriteWarning(warning.Message);
1449+
break;
1450+
1451+
case VerboseRecord verbose:
1452+
//cmdlet.WriteVerbose(verbose.Message);
1453+
break;
1454+
1455+
case DebugRecord debug:
1456+
//cmdlet.WriteDebug(debug.Message);
1457+
break;
1458+
1459+
case InformationRecord info:
1460+
//cmdlet.WriteInformation(info);
1461+
break;
1462+
1463+
case Hashtable result:
1464+
dataFileInfo = result;
1465+
return true;
1466+
}
14591467
}
14601468
}
1469+
catch (Exception ex)
1470+
{
1471+
error = ex;
1472+
}
14611473
}
1462-
catch (Exception ex)
1463-
{
1464-
error = ex;
1465-
}
1474+
// Return false to indicate "we couldn't parse a valid Hashtable from this .psd1 file."
1475+
// The only success path is the 'case Hashtable result' branch above which does return true.
1476+
return false;
1477+
}
1478+
finally
1479+
{
1480+
// Always restore the previous default runspace for the current thread.
1481+
Runspace.DefaultRunspace = previousDefaultRunspace;
14661482
}
1467-
// Return false to indicate "we couldn't parse a valid Hashtable from this .psd1 file."
1468-
// The only success path is the 'case Hashtable result' branch above which does return true.
1469-
return false;
1470-
}
1471-
finally
1472-
{
1473-
// Always restore the previous default runspace and close/dispose
1474-
// the per-thread runspace, even on success (return true) or exception paths.
1475-
Runspace.DefaultRunspace = previousDefaultRunspace;
1476-
runspace.Close();
1477-
runspace.Dispose();
14781483
}
14791484
}
1485+
catch (System.Management.Automation.ParseException parseEx)
1486+
{
1487+
error = new InvalidDataException(
1488+
$"The file '{filePath}' cannot be parsed as a PowerShell data file. It contains disallowed language elements: {parseEx.Message}",
1489+
parseEx);
1490+
return false;
1491+
}
14801492
catch (Exception ex)
14811493
{
14821494
error = ex;

‎src/code/V2ServerAPICalls.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,6 @@ public override FindResults FindName(string packageName, bool includePrerelease,
409409
/// </summary>
410410
public override async Task<FindResults> FindNameAsync(string packageName, bool includePrerelease, ResourceType type, ConcurrentQueue<ErrorRecord> errorMsgs, ConcurrentQueue<string> warningMsgs, ConcurrentQueue<string> debugMsgs, ConcurrentQueue<string> verboseMsgs)
411411
{
412-
// TODO: pass all debug output into a list that can be written to console later.
413412
debugMsgs.Enqueue("In V2ServerAPICalls::FindNameAsync()");
414413
// Make sure to include quotations around the package name
415414

‎src/code/V3ServerAPICalls.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -732,7 +732,7 @@ private Stream InstallName(string packageName, out ErrorRecord errRecord)
732732
private Stream InstallVersion(string packageName, string version, out ErrorRecord errRecord)
733733
{
734734
// TODO: pass in ConcurrentQueue to write out debug message.
735-
//_cmdletPassedIn.WriteDebug("In V3ServerAPICalls::InstallVersion()");
735+
_cmdletPassedIn.WriteDebug("In V3ServerAPICalls::InstallVersion()");
736736
if (!NuGetVersion.TryParse(version, out NuGetVersion requiredVersion))
737737
{
738738
errRecord = new ErrorRecord(

0 commit comments

Comments
 (0)