Skip to content
This repository has been archived by the owner on Oct 27, 2022. It is now read-only.

Feat/allow escaped equals #32

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mtenrero
Copy link
Contributor

@mtenrero mtenrero commented Jan 5, 2021

Refactor of config file parser in order to allow equals characters on the keys escaping them with a backslash.

Needed for allowing declaring jwt:kid certificates whose kid contains equal signs.

Related with these two bugs:
apache/couchdb#3319
apache/couchdb#2188

@mtenrero
Copy link
Contributor Author

mtenrero commented Apr 4, 2021

I've merged the main branch into this one, because contains the changes I had made for fixing the CI and it wasn't allowing the pipeline be green for integrating this fix.

Copy link
Member

@rnewson rnewson left a comment

Choose a reason for hiding this comment

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

The principle of allowing escaped equals characters in the key seems safe to me after some consideration. however I don't think this patch is quite right yet. below is an example of the problem;

re:run("a\=\=b = b\=c",<<"(.*(\\\\=)?\\S)(\\s?=\\s?)(.*)">>, [{capture,[1,4],list}]).
{match,["a==b = b","c"]}

despite the clear intention of the input string, the regex has misinterpreted the division between key and value (correct answer should be "a==b" and "b=c" (assuming we decide not to unescape on the value side, which could be argued either way.

@@ -403,8 +403,9 @@ parse_ini_file(IniFile) ->
";" ++ _Comment ->
{AccSectionName, AccValues};
Line2 ->
case re:split(Line2, "\s?=\s?", [{return, list}]) of
Copy link
Member

@rnewson rnewson Sep 20, 2021

Choose a reason for hiding this comment

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

would re:split(Line2, "\s?(?<!\\\\)=\s?", [{return, list}]) work here? (that is, split the string only on = when not immediately preceded by \.

@rnewson
Copy link
Member

rnewson commented Sep 20, 2021

finally noting this is not a refactor as it changes the behaviour of the software.

@nickva
Copy link
Contributor

nickva commented Oct 26, 2022

Note: config application was moved to the main repo. This PR can probably be closed or moved to the main repo

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants