8000 Add validation to `$PSStyle` to reject printable text as ANSI escape sequence by SteveL-MSFT · Pull Request #15825 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
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

Jump to bottom
Merged
merged 11 commits into from
Jul 27, 2021
Merged

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

merged 11 commits into from
Jul 27, 2021

Conversation

SteveL-MSFT
Copy link
Member
@SteveL-MSFT SteveL-MSFT commented Jul 23, 2021 •
edited
Loading

PR Summary

Currently, if you set custom formatting like $PSStyle.Formatting.Error and include printable characters (that is text other than an ANSI escape sequence), this can mess up the formatting which expects the formatting string to not take up any buffercells. The change is to add validation that the set text is only decoration and does not include characters that would print on screen as plain text.

Also added validation to Add() and Remove() to require period prefix which is used during formatting.

Also fixed an error in the formatting which incorrectly prefixed Verbose as Formatting.Verbose in the output.

PR Checklist

@SteveL-MSFT SteveL-MSFT force-pushed the ansi-validation branch 2 times, most recently from e4cd752 to 253a8a2 Compare July 23, 2021 22:42
iSazonov
iSazonov reviewed Jul 24, 2021
View reviewed changes
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jul 24, 2021
iSazonov
iSazonov approved these changes Jul 24, 2021
View reviewed changes
@SteveL-MSFT
Copy link
Member Author

The static analysis is unrelated to this PR, looks like CI picked up the changelog from preview 8 release bug not the spelling updates

iSazonov
iSazonov reviewed Jul 26, 2021
View reviewed changes
rjmholt
rjmholt reviewed Jul 26, 2021
View reviewed changes
rjmholt
rjmholt reviewed Jul 26, 2021
View reviewed changes
iSazonov
iSazonov reviewed Jul 27, 2021
View reviewed changes
src/System.Management.Automation/FormatAndOutput/common/PSStyle.cs
@@ -516,6 +684,17 @@ private PSStyle()
FileInfo = new FileInfoFormatting();
}

private static string ValidateNoContent(string text)
Copy link
Collaborator

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

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

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

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

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().

@rjmholt
Copy link
Collaborator
rjmholt commented Jul 27, 2021

/azp run

@azure-pipelines
Copy link
Azure Pipelines successfully started running 5 pipeline(s).

@rjmholt rjmholt enabled auto-merge (sq 6D40 uash) July 27, 2021 16:42
@rjmholt rjmholt merged commit 5e15f66 into PowerShell:master Jul 27, 2021
@SteveL-MSFT SteveL-MSFT deleted the ansi-validation branch July 27, 2021 17:54
@iSazonov iSazonov mentioned this pull request Aug 11, 2021
Closed
@ghost
Copy link
ghost commented Aug 23, 2021

🎉v7.2.0-preview.9 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link
ghost commented Sep 28, 2021

🎉v7.2.0-preview.10 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmholt rjmholt rjmholt left review comments

@iSazonov iSazonov iSazonov approved these changes

Assignees

@adityapatwardhan adityapatwardhan

Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SteveL-MSFT @rjmholt @iSazonov @adityapatwardhan
0