fix: fixes OTP leak in case of error #413
Conversation
|
No linked issues found. Please add the corresponding issues in the pull request description. |
src/utils/execWithOutput.js
Outdated
| function removeConfidentialArguments(args) { | ||
| let skipItem = false | ||
|
|
||
| return (args ?? []).filter(arg => { | ||
| if (skipItem) { | ||
| skipItem = false | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| skipItem = CONFIDENTIAL_KEYWORDS_FOR_REDACTION.includes(arg?.toString().toLocaleUpperCase()) | ||
|
|
||
| return !skipItem | ||
| }) | ||
| } |
There was a problem hiding this comment.
it's not very easy to follow what this code is doing, plus there are not tests for this function outside of the edge cases. let's please write this in a more idiomatic way so that understanding what it does it easier for a reader
| @@ -111,3 +112,43 @@ tap.test('passes env vars excluding `INPUT_*` env vars', async t => { | |||
| // Its value will vary by test runner, so just check it is present. | |||
| t.hasProp(envInExec, 'NODE') | |||
| }) | |||
|
|
|||
| tap.test('Invalid arguments inputs should not fail', async t => { | |||
There was a problem hiding this comment.
we need to test for normal cases here, not just edge cases
test/execWithOutput.test.js
Outdated
| t.equal(redactedBlankArray?.length, 0) | ||
| t.equal(redactedUndefinedArray?.length, 0) | ||
| t.equal(redactedNullArray?.length, 0) |
There was a problem hiding this comment.
the conditional operator is unnecessary, you have already tested that it's an array
test/execWithOutput.test.js
Outdated
| t.ok(errorMessage1.message.indexOf('--otp') === -1 && errorMessage1.message.indexOf('publish') > -1) | ||
| t.ok(errorMessage2.message.indexOf('--otp') === -1 && errorMessage2.message.indexOf('publish') > -1) | ||
| t.ok(errorMessage3.message.indexOf('--otp') === -1 && errorMessage3.message.indexOf('publish') > -1) | ||
| t.ok(errorMessage4.message.indexOf('publish') > -1) |
There was a problem hiding this comment.
ideally test using the built in assertions, which will give you meaningful error messages when they fail. if you test way, the error message won't tell you anything about the nature of the failure, as it will only show you true or false
What does this PR do?
Since the OTP is a confidential information, it should not be visible in the logs.
This PR fixes the leak and adds the code to redact the confidential arguments so it doesn't print on the Build Error Logs. The supporting tests have been added as well.