Skip to content

Add validation to $PSStyle to reject printable text as ANSI escape sequence #15825

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.

Sign up for GitHub

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

Merged
rjmholt merged 11 commits into from
Jul 27, 2021
Merged

Add validation to $PSStyle to reject printable text as ANSI escape sequence #15825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5518541
Add validation to `$PSStyle` to reject printable text as ANSI escape …
SteveL-MSFT Jul 23, 2021
5a5c0ee
fix codefactor
SteveL-MSFT Jul 23, 2021
4465148
address Ilya's feedback
SteveL-MSFT Jul 24, 2021
4b45d5c
use modern get/set syntax and add comment
SteveL-MSFT Jul 24, 2021
0604327
add support for Add() and Remove() extensions without the priod prefix
SteveL-MSFT Jul 25, 2021
a671e7b
update validation of adding and removing extension to require period …
SteveL-MSFT Jul 26, 2021
c7af849
use char overload for StartsWith()
SteveL-MSFT Jul 26, 2021
8ea054d
change FileExtensionDictionary to encapsulate Dictionary privately
SteveL-MSFT Jul 26, 2021
baf55be
fix dictionary to be case-insensitive and validate extension for Cont…
SteveL-MSFT Jul 26, 2021
49b204b
don't have dictionary implement IEnumerable
SteveL-MSFT Jul 26, 2021
e745c86
add validation to ContainsKey()
SteveL-MSFT Jul 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...m.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2127,7 +2127,7 @@ private static IEnumerable<FormatViewDefinition> ViewsOf_System_Management_Autom
.AddItemScriptBlock(@"""$($_.ErrorAccent)$($_.ErrorAccent.Replace(""""`e"""",'`e'))$($PSStyle.Reset)""", label: "ErrorAccent")
.AddItemScriptBlock(@"""$($_.Error)$($_.Error.Replace(""""`e"""",'`e'))$($PSStyle.Reset)""", label: "Error")
.AddItemScriptBlock(@"""$($_.Warning)$($_.Warning.Replace(""""`e"""",'`e'))$($PSStyle.Reset)""", label: "Warning")
.AddItemScriptBlock(@"""$($_.Verbose)$($_.Verbose.Replace(""""`e"""",'`e'))$($PSStyle.Reset)""", label: "Formatting.Verbose")
.AddItemScriptBlock(@"""$($_.Verbose)$($_.Verbose.Replace(""""`e"""",'`e'))$($PSStyle.Reset)""", label: "Verbose")
.AddItemScriptBlock(@"""$($_.Debug)$($_.Debug.Replace(""""`e"""",'`e'))$($PSStyle.Reset)""", label: "Debug")
.EndEntry()
.EndList());
Expand Down
212 changes: 198 additions & 14 deletions src/System.Management.Automation/FormatAndOutput/common/PSStyle.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Collections;
using System.Collections.Generic;
using System.Management.Automation.Internal;

namespace System.Management.Automation
{
Expand Down Expand Up @@ -281,12 +283,33 @@ public sealed class ProgressConfiguration
/// <summary>
/// Gets or sets the style for progress bar.
/// </summary>
public string Style { get; set; } = "\x1b[33;1m";
public string Style
{
get => _style;
set => _style = ValidateNoContent(value);
}

private string _style = "\x1b[33;1m";

/// <summary>
/// Gets or sets the max width of the progress bar.
/// </summary>
public int MaxWidth { get; set; } = 120;
public int MaxWidth
{
get => _maxWidth;
set
{
// Width less than 18 does not render correctly due to the different parts of the progress bar.
if (value < 18)
{
throw new ArgumentOutOfRangeException(nameof(MaxWidth), PSStyleStrings.ProgressWidthTooSmall);
}

_maxWidth = value;
}
}

private int _maxWidth = 120;

/// <summary>
/// Gets or sets the view for progress bar.
Expand All @@ -307,37 +330,79 @@ public sealed class FormattingData
/// <summary>
/// Gets or sets the accent style for formatting.
/// </summary>
public string FormatAccent { get; set; } = "\x1b[32;1m";
public string FormatAccent
{
get => _formatAccent;
set => _formatAccent = ValidateNoContent(value);
}

private string _formatAccent = "\x1b[32;1m";

/// <summary>
/// Gets or sets the style for table headers.
/// </summary>
public string TableHeader { get; set; } = "\x1b[32;1m";
public string TableHeader
{
get => _tableHeader;
set => _tableHeader = ValidateNoContent(value);
}

private string _tableHeader = "\x1b[32;1m";

/// <summary>
/// Gets or sets the accent style for errors.
/// </summary>
public string ErrorAccent { get; set; } = "\x1b[36;1m";
public string ErrorAccent
{
get => _errorAccent;
set => _errorAccent = ValidateNoContent(value);
}

private string _errorAccent = "\x1b[36;1m";

/// <summary>
/// Gets or sets the style for error messages.
/// </summary>
public string Error { get; set; } = "\x1b[31;1m";
public string Error
{
get => _error;
set => _error = ValidateNoContent(value);
}

private string _error = "\x1b[31;1m";

/// <summary>
/// Gets or sets the style for warning messages.
/// </summary>
public string Warning { get; set; } = "\x1b[33;1m";
public string Warning
{
get => _warning;
set => _warning = ValidateNoContent(value);
}

private string _warning = "\x1b[33;1m";

/// <summary>
/// Gets or sets the style for verbose messages.
/// </summary>
public string Verbose { get; set; } = "\x1b[33;1m";
public string Verbose
{
get => _verbose;
set => _verbose = ValidateNoContent(value);
}

private string _verbose = "\x1b[33;1m";

/// <summary>
/// Gets or sets the style for debug messages.
/// </summary>
public string Debug { get; set; } = "\x1b[33;1m";
public string Debug
{
get => _debug;
set => _debug = ValidateNoContent(value);
}

private string _debug = "\x1b[33;1m";
}

/// <summary>
Expand All @@ -348,29 +413,137 @@ public sealed class FileInfoFormatting
/// <summary>
/// Gets or sets the style for directories.
/// </summary>
public string Directory { get; set; } = "\x1b[44;1m";
public string Directory
{
get => _directory;
set => _directory = ValidateNoContent(value);
}

private string _directory = "\x1b[44;1m";

/// <summary>
/// Gets or sets the style for symbolic links.
/// </summary>
public string SymbolicLink { get; set; } = "\x1b[36;1m";
public string SymbolicLink
{
get => _symbolicLink;
set => _symbolicLink = ValidateNoContent(value);
}

private string _symbolicLink = "\x1b[36;1m";

/// <summary>
/// Gets or sets the style for executables.
/// </summary>
public string Executable { get; set; } = "\x1b[32;1m";
public string Executable
{
get => _executable;
set => _executable = ValidateNoContent(value);
}

private string _executable = "\x1b[32;1m";

/// <summary>
/// Custom dictionary handling validation of extension and content.
/// </summary>
public sealed class FileExtensionDictionary
{
private static string ValidateExtension(string extension)
{
if (!extension.StartsWith('.'))
{
throw new ArgumentException(PSStyleStrings.ExtensionNotStartingWithPeriod);
}

return extension;
}

private readonly Dictionary<string, string> _extensionDictionary = new(StringComparer.OrdinalIgnoreCase);

/// <summary>
/// Add new extension and decoration to dictionary.
/// </summary>
/// <param name="extension">Extension to add.</param>
/// <param name="decoration">ANSI string value to add.</param>
public void Add(string extension, string decoration)
{
_extensionDictionary.Add(ValidateExtension(extension), ValidateNoContent(decoration));
}

/// <summary>
/// Remove an extension from dictionary.
/// </summary>
/// <param name="extension">Extension to remove.</param>
public void Remove(string extension)
{
_extensionDictionary.Remove(ValidateExtension(extension));
}

/// <summary>
/// Clear the dictionary.
/// </summary>
public void Clear()
{
_extensionDictionary.Clear();
}

/// <summary>
/// Gets or sets the decoration by specified extension.
/// </summary>
/// <param name="extension">Extension to get decoration for.</param>
/// <returns>The decoration for specified extension.</returns>
public string this[string extension]
{
get
{
return _extensionDictionary[ValidateExtension(extension)];
}

set
{
_extensionDictionary[ValidateExtension(extension)] = ValidateNoContent(value);
}
}

/// <summary>
/// Gets whether the dictionary contains the specified extension.
/// </summary>
/// <param name="extension">Extension to check for.</param>
/// <returns>True if the dictionary contains the specified extension, otherwise false.</returns>
public bool ContainsKey(string extension)
{
if (string.IsNullOrEmpty(extension))
{
return false;
}

return _extensionDictionary.ContainsKey(ValidateExtension(extension));
}

/// <summary>
/// Gets the extensions for the dictionary.
/// </summary>
/// <returns>The extensions for the dictionary.</returns>
public IEnumerable<string> Keys
{
get
{
return _extensionDictionary.Keys;
}
}
}

/// <summary>
/// Gets the style for archive.
/// </summary>
public Dictionary<string, string> Extension { get; }
public FileExtensionDictionary Extension { get; }

/// <summary>
/// Initializes a new instance of the <see cref="FileInfoFormatting"/> class.
/// </summary>
public FileInfoFormatting()
{
Extension = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
Extension = new FileExtensionDictionary();

// archives
Extension.Add(".zip", "\x1b[31;1m");
Expand Down Expand Up @@ -516,6 +689,17 @@ private PSStyle()
FileInfo = new FileInfoFormatting();
}

private static string ValidateNoContent(string text)
Copy link
Collaborator

@iSazonov iSazonov Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Validate" assumes a return true/false. Perhaps NormalizeContent is better name. The same for ValidateExtension.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normalize doesn't indicate it'll throw if the data isn't right.

Copy link
Collaborator

@iSazonov iSazonov Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not strong rule. If it is important we could name the method as NormalizeContentOrThrow.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think previously where I added the period if it was missing, then Normalize would make sense but since it's only checking if the period is there, it seems to me validate is more accurate than normalize. Perhaps we need another opinion here, @rjmholt ?

Copy link
Collaborator

@rjmholt rjmholt Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validate sounds more like an assertion that throws than a boolean to me — I think it fits here.

If it returned a boolean, I'd expect a predicate phrase like IsContentFree() or ContainsNoContent().

{
var decorartedString = new StringDecorated(text);
if (decorartedString.ContentLength > 0)
{
throw new ArgumentException(string.Format(PSStyleStrings.TextContainsContent, decorartedString.ToString(OutputRendering.PlainText)));
}

return text;
}

/// <summary>
/// Gets singleton instance.
/// </summary>
Expand Down
70 changes: 70 additions & 0 deletions src/System.Management.Automation/resources/PSStyleStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<xsd:schema id="root" xmlns="" xmlns:xsd="https://www.w3.org/2001/XMLSchema" xmlns:msdata="urn:schemas-microsoft-com:xml-msdata">
<xsd:import namespace="https://www.w3.org/XML/1998/namespace" />
<xsd:element name="root" msdata:IsDataSet="true">
<xsd:complexType>
<xsd:choice maxOccurs="unbounded">
<xsd:element name="metadata">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" />
</xsd:sequence>
<xsd:attribute name="name" use="required" type="xsd:string" />
<xsd:attribute name="type" type="xsd:string" />
<xsd:attribute name="mimetype" type="xsd:string" />
<xsd:attribute ref="xml:space" />
</xsd:complexType>
</xsd:element>
<xsd:element name="assembly">
<xsd:complexType>
<xsd:attribute name="alias" type="xsd:string" />
<xsd:attribute name="name" type="xsd:string" />
</xsd:complexType>
</xsd:element>
<xsd:element name="data">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
<xsd:element name="comment" type="xsd:string" minOccurs="0" msdata:Ordinal="2" />
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="required" msdata:Ordinal="1" />
<xsd:attribute name="type" type="xsd:string" msdata:Ordinal="3" />
<xsd:attribute name="mimetype" type="xsd:string" msdata:Ordinal="4" />
<xsd:attribute ref="xml:space" />
</xsd:complexType>
</xsd:element>
<xsd:element name="resheader">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="required" />
</xsd:complexType>
</xsd:element>
</xsd:choice>
</xsd:complexType>
</xsd:element>
</xsd:schema>
<resheader name="resmimetype">
<value>text/microsoft-resx</value>
</resheader>
<resheader name="version">
<value>2.0</value>
</resheader>
<resheader name="reader">
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="TextContainsContent" xml:space="preserve">
<value>The specified string contains printable content when it should only contain ANSI escape sequences: {0}</value>
</data>
<data name="ProgressWidthTooSmall" xml:space="preserve">
<value>The MaxWidth for the Progress rendering must be at least 18 to render correctly.</value>
</data>
<data name="ExtensionNotStartingWithPeriod" xml:space="preserve">
<value>When adding or removing extensions, the extension must start with a period.</value>
</data>
</root>
Loading