Fix and consolidate markup escaping in CLI#14749
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14749Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14749" |
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #22531201402 |
There was a problem hiding this comment.
Pull request overview
This PR addresses a security and usability issue where external content containing Spectre.Console markup characters (like brackets []) would cause exceptions or unintended rendering in CLI output. The solution adds auto-escaping by default to display methods and introduces a separate emojiName parameter to cleanly separate emoji formatting from message text.
Changes:
- Added auto-escaping by default to
DisplayMessage,DisplaySuccess,DisplaySubtleMessage, andShowStatusAsync/ShowStatusmethods with an optionalallowMarkupparameter to opt-in to markup rendering - Introduced
emojiNameparameter toShowStatusAsync/ShowStatusto handle emoji separately from status text, enabling proper auto-escaping while preserving emoji rendering - Added
EmojiWidthutility class to calculate terminal cell widths for emoji, handling Unicode presentation properties for consistent emoji alignment - Consolidated
TableExtensions.AddBoldColumnoverloads into a single method with optional parameters - Updated all call sites to use the new signatures, with several removing manual
.EscapeMarkup()calls - Added comprehensive test coverage for the escaping behavior
- Added debug-only
RenderCommandfor testing CLI rendering
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Cli/Utils/EmojiWidth.cs |
New utility class for calculating emoji display widths based on Unicode Emoji_Presentation property |
tests/Aspire.Cli.Tests/Utils/EmojiWidthTests.cs |
Tests for emoji width calculation |
src/Aspire.Cli/Interaction/IInteractionService.cs |
Updated interface signatures to add emojiName and allowMarkup parameters |
src/Aspire.Cli/Interaction/ConsoleInteractionService.cs |
Implemented auto-escaping logic and emoji prefix formatting |
src/Aspire.Cli/Interaction/ExtensionInteractionService.cs |
Updated wrapper to forward new parameters |
tests/Aspire.Cli.Tests/Interaction/ConsoleInteractionServiceTests.cs |
Comprehensive tests for auto-escaping, allowMarkup, and emojiName behavior |
tests/Aspire.Cli.Tests/Utils/ConsoleActivityLoggerTests.cs |
Tests for markup escaping in activity logger output |
src/Aspire.Cli/Utils/TableExtensions.cs |
Consolidated three AddBoldColumn overloads into one with optional parameters |
src/Aspire.Cli/Utils/ConsoleActivityLogger.cs |
Added escaping for failure reasons and pipeline summary keys/values |
| Multiple command files | Updated to use emojiName parameter and removed manual .EscapeMarkup() calls where appropriate |
src/Aspire.Cli/Commands/RenderCommand.cs |
New DEBUG-only command for testing CLI rendering |
| Test service mocks | Updated to match new interface signatures |
c3a33cf to
56209ca
Compare
Description
The CLI interaction service has methods like
DisplayMessageandDisplaySuccessthat are rendered using markup. That means callers must escape external content or get problems when content contains markup characters. This is non-obvious and it's very easy to include external content in these methods.This PR:
DisplayMarkupShowStatusAsync. Allows message to be escaped by defaultRenderCommand. It's a test bed for smoke testing render output. Hidden and excluded from non-debug builds.KnownEmojis. List of emojis used in the CLI. Fix some invalid emojis.Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: