Skip to content

Escape control characters in message sign preview.#2509

Open
fnando wants to merge 2 commits intomainfrom
message-sign-control-sequence
Open

Escape control characters in message sign preview.#2509
fnando wants to merge 2 commits intomainfrom
message-sign-control-sequence

Conversation

@fnando
Copy link
Copy Markdown
Member

@fnando fnando commented Apr 21, 2026

What

Escape control characters in message sign preview.

Why

Close https://github.com/stellar/stellar-cli-internal/issues/8

Known limitations

N/A

Copilot AI review requested due to automatic review settings April 21, 2026 22:49
@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX Apr 21, 2026
@fnando fnando requested a review from mootz12 April 21, 2026 22:49
@fnando fnando moved this from Backlog (Not Ready) to Needs Review in DevX Apr 21, 2026
@fnando fnando self-assigned this Apr 21, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the message sign UX by sanitizing the human-readable message preview so terminal control characters (e.g., ANSI escape sequences) aren’t emitted to the user’s terminal, addressing the linked issue.

Changes:

  • Sanitize the Message: preview line in message sign output to escape control characters.
  • Add an integration test ensuring the preview output does not contain raw ESC bytes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cmd/soroban-cli/src/commands/message/sign.rs Sanitizes the message preview before printing to stderr.
cmd/crates/soroban-test/tests/it/message.rs Adds an integration test that verifies ESC bytes are not present in the preview output.

Comment thread cmd/soroban-cli/src/commands/message/sign.rs Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
String::from_utf8_lossy(&message_bytes).to_string()
};
print.infoln(format!("Message: {message_display}"));
print.infoln(format!("Message: {}", sanitize(&message_display)));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's odd to call the soroban_spec_tools::sanitize fn from a package that provides contract spec utilities when the message sign command has nothing to do with contracts or contract specs. The sanitize function is just a light wrap around the escape_bytes crate, so I think we can just use that here?

Suggested change
print.infoln(format!("Message: {}", sanitize(&message_display)));
let escaped = escape_bytes::escape(s.as_bytes()).map(char::from).collect();
print.infoln(format!("Message: {}", sanitize(&message_display)));

Given the byte <> str shuffling it'd be nice if the escape-bytes crate had a fn that took on the responsibility of guaranteeing that you can convert bytes or a string to a utf8 safe string:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this package might be too strict, as it obfuscates the output string when using chars that take up more than 1 byte.

For example:

$ stellar message sign '¡Hola Mundo!' --sign-with-key SAKICEVQLYWGSOJS4WW7HZJWAHZVEEBS527LHK5V4MLJALYKICQCJXMW
ℹ️  Signer: GBXFXNDLV4LSWA4VB7YIL5GBD7BVNR22SGBTDKMO2SBZZHDXSKZYCP7L
ℹ️  Message: \xc2\xa1Hola Mundo!
sH3cuFuzVmfgXuMsJEH+iTryYo1zeQdNbHUFBZry3MRSNgEP2kg2SYuChVmoWMjgJdGyKQKK3tIzGPk3ft9WDw==

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah the escape-bytes package outputs printable ASCII only, not printable UTF8. Maybe not what you want here.

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

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

4 participants