fix value escaping in codeql-env.sh - #477
aeisenberg merged 5 commits into
Conversation
Sorry, something went wrong.
| // 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, "'\"'\"'")}'` |
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
Yes, it's safer to use single quotes instead of escaping the $.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Arrrrgh....also need to update the compile files. Let me just push a change up. |
Sorry, something went wrong.
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
|
Do we need a change note warning users of the different quoting? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@hmakholm, merged this for you because you mentioned earlier that you kept having merge conflicts if you waited too long. |
Sorry, something went wrong.
|
Thank you! |
Sorry, something went wrong.
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.sh themselves and will break if they don't find double quotes? They should be using the JSON output instead, but still ...
Merge / deployment checklist