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

Escape control characters #456

Merged
merged 11 commits into from
Oct 9, 2022
Merged

Escape control characters #456

merged 11 commits into from
Oct 9, 2022

Conversation

ericcornelissen
Copy link
Owner

@ericcornelissen ericcornelissen commented Oct 8, 2022

Closes #347


TODO

Update the unix test fixtures to include samples of strings with the
control characters:
- `<backspace> ('\\b')`
- `<carriage return> ('\\r')`
- `<escape> ('\\u001B')`
- `<control sequence introducer> ('\\u009B')`
and how these should be escaped for each supported Unix shell. The tests
for `<end of line> ('\\n')` were also updated due to its relates to the
carriage return tests. Also, other test cases that include the carriage
return character were update according to how it should be escaped.

In most cases, these characters should be escaped because they could
allow for controller input outside the range of the input. The most
prominent example of this is the carriage return, which resets the
cursor to the start of the line.

The only exception to this is the exact sequence "\n\r" (newline
representation on Windows), which will be preserved as it's known safe
(even if it can be considered unnecessary on Unix).
@ericcornelissen ericcornelissen added the test Relates to testing label Oct 8, 2022
Update the Windows test fixtures to include samples of strings with the
control characters:
- `<backspace> ('\\b')`
- `<carriage return> ('\\r')`
- `<escape> ('\\u001B')`
- `<control sequence introducer> ('\\u009B')`
and how these should be escaped for each supported Windows shell. The
tests for `<end of line> ('\\n')` were also updated due to its relates
to the carriage return tests.

Note that for CMD, there's no actual changes in the expected behaviour
when it comes to <end of line> and <carriage return> because it is
already the case these will always be removed.

In most cases, these characters should be escaped because they could
allow for controlling input outside the range of the input. The most
prominent example of this is the carriage return, which resets the
cursor to the start of the line.

The only exception to this is the exact sequence "\n\r" (newline
representation on Windows), which will be preserved as it's known safe
(even if it can be considered unnecessary on Unix).
Update escaping for all Unix and all Windows shells to escape
- `<backspace> ('\\b')`
- `<carriage return> ('\\r')`
- `<escape> ('\\u001B')`
- `<control sequence introducer> ('\\u009B')`
in all situations with the soul exception of the carriage return
character when it appears in exactly as `\n\r` for Bash, Dash,
PowerShell, and Zsh (the carriage return character is already always
stripped for CMD).
@ericcornelissen ericcornelissen added the enhancement New feature or request label Oct 8, 2022
@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Merging #456 (cd85503) into main (d8c868d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #456   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          785       794    +9     
=========================================
+ Hits           785       794    +9     
Flag Coverage Δ
e2e-macos-latest 82.49% <20.00%> (-0.70%) ⬇️
e2e-ubuntu-latest 81.98% <20.00%> (-0.69%) ⬇️
e2e-windows-latest 82.36% <20.00%> (-0.69%) ⬇️
integration-macos-latest 89.16% <60.00%> (-0.26%) ⬇️
integration-ubuntu-latest 89.29% <60.00%> (-0.26%) ⬇️
integration-windows-latest 86.27% <40.00%> (-0.61%) ⬇️
property 96.97% <100.00%> (+0.03%) ⬆️
unit 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/unix.js 100.00% <100.00%> (ø)
src/win.js 100.00% <100.00%> (ø)

Allow control characters in regular expression as this project sometimes
require matching control characters (because they need to be escaped or
removed from strings when escaping)
@ericcornelissen ericcornelissen force-pushed the 347-control-chars branch 5 times, most recently from fd667c9 to c8899fc Compare October 9, 2022 09:55
@ericcornelissen ericcornelissen force-pushed the 347-control-chars branch 2 times, most recently from a0bb9fc to d54f12d Compare October 9, 2022 10:17
Update the fuzz normalization for Unix shells and PowerShell in
alignment with the changes to the newline substitution in the source
code.
Update the macro title for the test that runs fixutures to account for
the newly introduced control characters.
@ericcornelissen ericcornelissen marked this pull request as ready for review October 9, 2022 11:02
@ericcornelissen ericcornelissen enabled auto-merge (squash) October 9, 2022 11:12
@ericcornelissen ericcornelissen merged commit 1da3708 into main Oct 9, 2022
@ericcornelissen ericcornelissen deleted the 347-control-chars branch October 9, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test Relates to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support escaping control characters
1 participant