Uh oh!
There was an error while loading. Please reload this page.
fix value escaping in codeql-env.sh - #477
Conversation
4a4a2ac to
44ebca1Compare| // Some vars contain ${LIB} that we do not want to be expanded when executing this script | ||
| .map( | ||
| ([key, value]) => `export ${key}="${value.replace(/\$/g, "\\$")}"` | ||
| ([key, value]) => "export " + key + "='" + value.replace(/'/g, "'\"'\"'") + "'" |
There was a problem hiding this comment.
| ([key,value])=>"export "+key+"='"+value.replace(/'/g,"'\"'\"'")+"'" | |
| ([key,value])=>`export ${key}='${value.replace(/'/g,"'\"'\"'")}'` |
44ebca1 to
e7e64d5Compare
aeisenberg
left a comment
There was a problem hiding this comment.
Yes, it's safer to use single quotes instead of escaping the $.
alexet
commented
May 5, 2021
I know there are consumers converting the windows output to sh (as they use bash on windows) but I don't know about users converting the sh output. |
Uh oh!
There was an error while loading. Please reload this page.
aeisenberg
commented
May 5, 2021
Arrrrgh....also need to update the compile files. Let me just push a change up. |
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
11f9ba6 to
a6ebb19Comparehmakholm
commented
May 5, 2021
Do we need a change note warning users of the different quoting? |
aeisenberg
commented
May 5, 2021
Based on how we are doing things now, the answer is no. And as we discussed, this might change, but I don't think we should be holding this PR up until we make any process changes. |
aeisenberg
commented
May 6, 2021
@hmakholm, merged this for you because you mentioned earlier that you kept having merge conflicts if you waited too long. |
hmakholm
commented
May 6, 2021
Thank you! |
CodeQL itself complained (correctly) that the escaping here would fail if the value contains backslashes.
Switch to single-quoting, which is less tricky to escape.
Hmmm, how confident are we that there are not consumers that try to parse
codeql-env.shthemselves and will break if they don't find double quotes? They should be using the JSON output instead, but still ...Merge / deployment checklist