Skip to content
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.

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

Ensure log does not contain ANSI sequences or control codes #1604

Merged
merged 2 commits into from Jan 15, 2024

Conversation

rmartin16
Copy link
Member

Changes

  • ANSI escape sequences and console control codes are stripped from text before being written to the log

Notes

  • This was originally discussed as part of Colorize ADB logcat by default #1572
    • Relatedly, I think that PR just needs a little direction to keep going; that is, should the regexes accommodate the ANSI sequences? Or could they be stripped for the purposes of Android log filtering?

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

+1 to the broad idea; one minor quibble inline.

@@ -399,7 +412,8 @@ def _build_log(self, command):
# build log header and export buffered log from Rich
uname = platform.uname()
sanitized_env_vars = "\n".join(
f" {env_var}={value if not SENSITIVE_SETTING_RE.search(env_var) else '********************'}"
f"\t{env_var}="
f"{sanitize_text(value) if not SENSITIVE_SETTING_RE.search(env_var) else '********************'}"
Copy link
Member

Choose a reason for hiding this comment

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

Is this advisable? Environment variables are something that we really need to know verbatim - any manipulation could lead us to believe that one value has been set, when it's actually a different value.

To be clear - I can't think of a reason why the user's PS1 setting could cause a problem, but then, that's likely a lack of imagination on my part.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah; that's fair....and it's really of marginal value to remove these anyway.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good - and I'm going to need the cleansing utility to get #1606 working, so this is really timely :-)

@freakboy3742 freakboy3742 merged commit 307c2ac into beeware:main Jan 15, 2024
44 checks passed
@rmartin16 rmartin16 deleted the clean-log branch January 15, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants