Skip to content

Escape non-printable characters in user output #7831

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

Merged
merged 5 commits into from
Jan 14, 2021

Conversation

gammazero
Copy link
Contributor

Replaces control characters and non-printable characters with escape sequences, in any fields that are printed by the CLI, which could have been user input.

Output from ipfs cat is unchanged.

Replaces control characters and non-printable characters with escape sequences, in any fields that are printed by the CLI, which could have been user input.

Output from `ipfs cat` is unchanged.
@gammazero gammazero force-pushed the fix/escape-nonprintable-chars branch from 065f5c0 to 7193f95 Compare December 18, 2020 02:16
@gammazero gammazero requested a review from aschmahmann January 13, 2021 03:46
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

LGTM, left a few small comments it'd be nice to fix up before merging.

Comment on lines 75 to 76
// EscNonPrint converts non-printable characters and backslash into Go
// escape sequences, if the given string contains any.
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about adding a comment that we're trying to mimic the escape quoting style from https://www.gnu.org/software/coreutils/manual/html_node/Formatting-the-file-names.html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The link says essentially what the comment here says, so I don't think it adds much, other than it hints that we are doing this for the same reason. Also, I fear that people will misread that and think it means identical content.

Added brief description of purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, if/when we add other escaping options we can add more descriptions

@aschmahmann aschmahmann merged commit fb0a9ac into master Jan 14, 2021
@gammazero gammazero deleted the fix/escape-nonprintable-chars branch January 14, 2021 19:52
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.

2 participants