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 characters in configs #2354

Open
patrick96 opened this issue Jan 16, 2021 · 5 comments
Open

Escape characters in configs #2354

patrick96 opened this issue Jan 16, 2021 · 5 comments

Comments

@patrick96
Copy link
Member

Currently, everything in the config is read as-is. There is no way to escape characters with special meaning.

This isn't really an issue right now because the config recognizes special characters only if the value starts with ${ and ends with } (references).

However, if we want to allow something like key = foo ${section.key} bar, we need to have a way to escape literal uses of ${ to not irreparably break existing configs.

I would propose the backslash (\) as the escape character because it's the most commonly used escape character.

Having an escape character here would also allow us to implement multiline config options by putting the backslash as the last character in a line (like in bash). This would also make #2207 obsolete.

Unfortunately, just making \ an escape character may break existing configs that use it as a literal character.
I propose a multi-step process to implement this functionality, to give users time to escape their literal backslashes:

1. Add warning or error to config parser for single backslashes

Add an error to the config parser when there is a single backslash in a config value and tell the user to escape their backslashes.
Since the backslash isn't used for escaping anything yet, all single backslashes in a config are literal backslashes.
We should still treat single backslashes as literal backslashes for backwards compatibility.

Because we tell the user to escape their backslashes, we also need to treat two backslashes (\\) as a single literal backslash.
This is the only breaking change here: If a user uses two consecutive literal backslashes, they will now be interpreted as a single literal backslash.
I think the risk of this is quite low.

At this point, the backslash is a proper escape character, it's just not used for anything else than escaping other backslashes.

2. Using the backslash to create multi-line config values

Any config value that ends in a backslash is joined together with the next line (the value will not contain a newline character).
Here, we need to take care to recognize both \n and \r\n as valid line-endings.

For users that have now properly escaped their literal backslashes, this should not break anything.
Otherwise it will break configs where there are an odd number of backslashes at the end of the line.

This would make #2207 obsolete

3+. Introduce constructs with special meanings to config values

We can now start adding things like config values where we can mix references and text.

Each time we do that, it may break certain configs that used that construct in a literal sense (for example someone using ${ as a literal string).


I think we can target 1 for a release and 2 & 3 for the release after that.

The error introduced in 1 should stay and we should also keep interpreting single backslashes that don't escape anything as literal backslashes.

I doubt that many people will be affected by any one of these steps. But I'm sure that there are some people that the backslash as a separator, so we should properly communicate what we're doing.

@Filip62
Copy link
Contributor

Filip62 commented Jan 17, 2021

I will try to implement this.

@patrick96
Copy link
Member Author

@Filip62 Thanks 😃

Filip62 added a commit to Filip62/polybar that referenced this issue Jan 21, 2021
Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

 - Warn the user of any unescaped backslashes, as they will not be
   treated as a literal character in the future

 - For now, still treat a single backslash as a literal character

 - Treat two consecutive backslashes as a single properly escaped
   literal backslash

Resolves: First step in polybar#2354
Filip62 added a commit to Filip62/polybar that referenced this issue Jan 26, 2021
author Filip Banák <filip.banak.62@gmail.com> 1611238053 +0100
committer Filip Banák <filip.banak.62@gmail.com> 1611677221 +0100
gpgsig -----BEGIN PGP SIGNATURE-----

 iQIzBAABCAAdFiEEz8tzNwOTJ5CaZ2CTeWyhOG/JAtcFAmAQPiUACgkQeWyhOG/J
 AtcQBQ//bcfJtEGp5uK5/OgInm0RPQIWZLlgk64Y6KujH2aLKS84kLlwjke8gHU6
 c46OQz94bS+fM0RtB1RcGtev2Q3tV1K+pPaUHg0IO79ZcL9YqR7+ktyItJtCIaSz
 57AKughcqVTLHqkZaR9AV/cXOHfMXbwlVNYfolT5JqLb4HvTsvzgqCw157uM1pkk
 bMBsQUVRqjpr6hPGUbUyFPDkrToW1Qc70JaQy88iEYvbaTo2/NJapOg+hsYuzXHp
 U9sHsZuElOOonvkXEcFIVDgxHTUmGB0h6x4TKXCiTdM3TC98B2ohAL+uxROI4k80
 MFhTxB15aQAHne7OnlUj3LS6gm/yjIuPQMNPtST9trR0lNvZ6RrddEj+prNfr1mZ
 jqQcqo7XKDP7yPULi0/fNFeuOfS0HBputRpij9BX3zGUHqvbR+8C9xnVoj2P1u6s
 pgbS39/iCmnhiUpaC1mle/LgOVAQZZwQ5k8v5ln8wDPunAguN1Psy80IXqHWdtIw
 e+PVvurOa+UJeitHohSH+n3CSLrShZJREzUIkx/HS8khTvaJIkcM8z3Xdgxkxv8r
 TK1DgrbwjebxGD/Dxeex/IT6PR+AWq8RECjr078CR58E6zGDdNEZGF6yJ/0k8qA8
 qUrVPspm1Fh7Ht65s124andiLZelHCVNzjjXBSgEVzL3Q7dysjM=
 =HmWZ
 -----END PGP SIGNATURE-----

Add initial support for an escape character

Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

  - Warn the user of any unescaped backslashes, as they will not be
    treated as a literal character in the future

  - For now, still treat a single backslash as a literal character

  - Treat two consecutive backslashes as a single properly escaped
    literal backslash

Also:
  - Add documentation about the escape character to polybar(5) manpage
  - Add info about the escape character to changelog
  - Add testcases for ParseLineKeyTest
  - Add new test ParseEscapedValueTest

Resolves: First step in polybar#2354

Improve value parsing

 - Take value arg in as an rvalue reference and move parsed value back
 - Remove unnecessary if statement
 - Rename function
 - Improve error message
 - Improve function description
 - Format

Add escape character documentation to manpages

Add information about the escape character to the polybar(5) manpage.

Add info about the esacape character to changelog

Add test cases for ParseLineKeyTest

Fix ParseLineKeyTest test cases

Also make config parser method parse_escaped_value private.

Add tests for escaped_value_parser method

Also remove unsued include statement

Simplify parse_escaped_value in config_parser

Remove unnecessary escaped value generation, so we do not have to keep
track of index differences.

Fix ParseEscapedValueTest test cases

Fix parse_escaped_value

Add more test cases for ParseLineKeyTest

Update CHANGELOG.md

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

Adress review

 - Adjust documentation
 - Small code changes

Improve parse_escaped_value
Filip62 added a commit to Filip62/polybar that referenced this issue Jan 26, 2021
Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

 - Warn the user of any unescaped backslashes, as they will not be
   treated as a literal character in the future

 - For now, still treat a single backslash as a literal character

 - Treat two consecutive backslashes as a single properly escaped
   literal backslash

Resolves: First step in polybar#2354
patrick96 pushed a commit to Filip62/polybar that referenced this issue Jan 26, 2021
Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

  - Warn the user of any unescaped backslashes, as they will not be
    treated as a literal character in the future

  - For now, still treat a single backslash as a literal character

  - Treat two consecutive backslashes as a single properly escaped
    literal backslash

Also:
  - Add documentation about the escape character to polybar(5) manpage
  - Add info about the escape character to changelog
  - Add testcases for ParseLineKeyTest
  - Add new test ParseEscapedValueTest

Resolves: First step in polybar#2354

Improve value parsing

 - Take value arg in as an rvalue reference and move parsed value back
 - Remove unnecessary if statement
 - Rename function
 - Improve error message
 - Improve function description
 - Format

Add escape character documentation to manpages

Add information about the escape character to the polybar(5) manpage.

Add info about the esacape character to changelog

Add test cases for ParseLineKeyTest

Fix ParseLineKeyTest test cases

Also make config parser method parse_escaped_value private.

Add tests for escaped_value_parser method

Also remove unsued include statement

Simplify parse_escaped_value in config_parser

Remove unnecessary escaped value generation, so we do not have to keep
track of index differences.

Fix ParseEscapedValueTest test cases

Fix parse_escaped_value

Add more test cases for ParseLineKeyTest

Update CHANGELOG.md

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

Adress review

 - Adjust documentation
 - Small code changes

Improve parse_escaped_value

Add initial support for an escape character

Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

 - Warn the user of any unescaped backslashes, as they will not be
   treated as a literal character in the future

 - For now, still treat a single backslash as a literal character

 - Treat two consecutive backslashes as a single properly escaped
   literal backslash

Resolves: First step in polybar#2354

Improve value parsing

 - Take value arg in as an rvalue reference and move parsed value back
 - Remove unnecessary if statement
 - Rename function
 - Improve error message
 - Improve function description
 - Format

Add info about the esacape character to changelog

Add test cases for ParseLineKeyTest

Fix ParseLineKeyTest test cases

Also make config parser method parse_escaped_value private.

Add tests for escaped_value_parser method

Also remove unsued include statement

Simplify parse_escaped_value in config_parser

Remove unnecessary escaped value generation, so we do not have to keep
track of index differences.

Fix ParseEscapedValueTest test cases

Add more test cases for ParseLineKeyTest

Adress review

 - Adjust documentation
 - Small code changes

Remove duplicate testcase from ParseLineKeyTest

Add initial support for an escape character

Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

 - Warn the user of any unescaped backslashes, as they will not be
   treated as a literal character in the future

 - For now, still treat a single backslash as a literal character

 - Treat two consecutive backslashes as a single properly escaped
   literal backslash

Resolves: First step in polybar#2354

Improve value parsing

 - Take value arg in as an rvalue reference and move parsed value back
 - Remove unnecessary if statement
 - Rename function
 - Improve error message
 - Improve function description
 - Format

Fix ParseLineKeyTest test cases

Also make config parser method parse_escaped_value private.

Remove duplicate testcase from ParseLineKeyTest
patrick96 pushed a commit that referenced this issue Jan 26, 2021
Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

  - Warn the user of any unescaped backslashes, as they will not be
    treated as a literal character in the future

  - For now, still treat a single backslash as a literal character

  - Treat two consecutive backslashes as a single properly escaped
    literal backslash

Also:
  - Add documentation about the escape character to polybar(5) manpage
  - Add info about the escape character to changelog
  - Add testcases for ParseLineKeyTest
  - Add new test ParseEscapedValueTest

Resolves: First step in #2354

Improve value parsing

 - Take value arg in as an rvalue reference and move parsed value back
 - Remove unnecessary if statement
 - Rename function
 - Improve error message
 - Improve function description
 - Format

Add escape character documentation to manpages

Add information about the escape character to the polybar(5) manpage.

Add info about the esacape character to changelog

Add test cases for ParseLineKeyTest

Fix ParseLineKeyTest test cases

Also make config parser method parse_escaped_value private.

Add tests for escaped_value_parser method

Also remove unsued include statement

Simplify parse_escaped_value in config_parser

Remove unnecessary escaped value generation, so we do not have to keep
track of index differences.

Fix ParseEscapedValueTest test cases

Fix parse_escaped_value

Add more test cases for ParseLineKeyTest

Update CHANGELOG.md

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

Adress review

 - Adjust documentation
 - Small code changes

Improve parse_escaped_value

Add initial support for an escape character

Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

 - Warn the user of any unescaped backslashes, as they will not be
   treated as a literal character in the future

 - For now, still treat a single backslash as a literal character

 - Treat two consecutive backslashes as a single properly escaped
   literal backslash

Resolves: First step in #2354

Improve value parsing

 - Take value arg in as an rvalue reference and move parsed value back
 - Remove unnecessary if statement
 - Rename function
 - Improve error message
 - Improve function description
 - Format

Add info about the esacape character to changelog

Add test cases for ParseLineKeyTest

Fix ParseLineKeyTest test cases

Also make config parser method parse_escaped_value private.

Add tests for escaped_value_parser method

Also remove unsued include statement

Simplify parse_escaped_value in config_parser

Remove unnecessary escaped value generation, so we do not have to keep
track of index differences.

Fix ParseEscapedValueTest test cases

Add more test cases for ParseLineKeyTest

Adress review

 - Adjust documentation
 - Small code changes

Remove duplicate testcase from ParseLineKeyTest

Add initial support for an escape character

Add a config parser method which, for now, deals only with escaping the
literal backslash character and logs an error message to inform the user
of the coming change.

The error message includes a properly escaped value for the user.

As a result of introducing an escape character('\'):

 - Warn the user of any unescaped backslashes, as they will not be
   treated as a literal character in the future

 - For now, still treat a single backslash as a literal character

 - Treat two consecutive backslashes as a single properly escaped
   literal backslash

Resolves: First step in #2354

Improve value parsing

 - Take value arg in as an rvalue reference and move parsed value back
 - Remove unnecessary if statement
 - Rename function
 - Improve error message
 - Improve function description
 - Format

Fix ParseLineKeyTest test cases

Also make config parser method parse_escaped_value private.

Remove duplicate testcase from ParseLineKeyTest
@patrick96 patrick96 added this to the 3.8.0 milestone Feb 3, 2021
@minuq
Copy link

minuq commented Aug 3, 2023

The current implementation results in
error: /home/minuq/.config/polybar/modules.ini:1435: Value '"upower --dump | grep G403 -A 11 | grep percentage | head -n 1 | sed 's/percentage.*\([[:digit:]][[:digit:]]%\).*/\1/' | awk '{$1=$1};1'"' of key 'exec' contains one or more unescaped backslashes, please prepend them with the backslash escape character.
for a perfectly valid command due to how it's looking for single backslashes. I assume this is the correct issue for that false-positive, since it's referenced in the relevant commits.

@Filip62
Copy link
Contributor

Filip62 commented Aug 3, 2023

The current implementation results in
error: /home/minuq/.config/polybar/modules.ini:1435: Value '"upower --dump | grep G403 -A 11 | grep percentage | head -n 1 | sed 's/percentage.*\([[:digit:]][[:digit:]]%\).*/\1/' | awk '{$1=$1};1'"' of key 'exec' contains one or more unescaped backslashes, please prepend them with the backslash escape character.
for a perfectly valid command due to how it's looking for single backslashes. I assume this is the correct issue for that false-positive, since it's referenced in the relevant commits.

I think this is expected behaviour.
The value does indeed contain unescaped backslashes.

The backslash character in a config value has the meanig of a special escape character. Together with the character following right after it, an escape sequence is formed. Which gets internally replaced by polybar by the character this sequence escapes.

As of now (I haven't checked in a while though, so I may be wrong) there is only one supported escape sequence: \\

Which gets replaced by a single backslash.

The following character sequences in your config value: \( and \) do not form any supported escape sequence, thus polybar assumes these sequences should be taken literally, as they are. So the error you reported is logged to make you aware of this.

Because in the future, the character sequence \( might be a valid escape sequence, that will get replaced by (, which would change the meaning of your command.

If you want this configuration value to be taken literally, without interpreting the above mentioned character sequences as escape sequences, you need to replace them in the config value with \\( and \\). The sequence \\ gets internally replaced by a single backslash and the rest of the value is taken literally.

Which is exactly what the error message tells you to do. That is expected behaviour.

@minuq
Copy link

minuq commented Aug 3, 2023

Makes sense, thanks for your explanation. I missed the part about it getting replaced with a single \ during parsing.

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

No branches or pull requests

3 participants