fix(apps/code-of-conduct): load provider icons over HTTPS#3765
fix(apps/code-of-conduct): load provider icons over HTTPS#3765josephperrott wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates favicon URLs to HTTPS in the Code of Conduct app and introduces a sanitization utility in ChildProcess to redact sensitive URL credentials from logs, command strings, and error messages. The review feedback correctly identifies several critical security and robustness issues: credentials can still leak via raw spawn error messages and silent-mode promise rejections, and stream-based sanitization may fail if a URL is split across chunk boundaries. Additionally, the feedback points out that spawnSync currently swallows execution errors (like ENOENT) by ignoring the error property, which in turn masks a weak assertion in the unit tests. Addressing these issues will significantly improve the security and reliability of the child process utility.
| } | ||
|
|
||
| throw new Error(stderr); | ||
| throw new Error(sanitize(stderr)); |
There was a problem hiding this comment.
When _spawnSync fails to execute (e.g., if the command does not exist, throwing ENOENT), Node's spawnSync returns an error property on the result object instead of populating stderr or setting a normal exit code.
Currently, the error property is ignored in the destructuring on line 97. As a result, when a spawn failure occurs, status is evaluated as -1, and the code throws new Error(sanitize(stderr)). Since stderr is empty/undefined in this case, it throws an empty Error(""), completely swallowing the original ENOENT error details and stack trace.
To fix this, update the destructuring of _spawnSync to include error, and throw it if present:
const {
status: exitCode,
signal,
stdout,
stderr,
error,
} = _spawnSync(command, args, {...options, env, encoding: 'utf8', stdio: 'pipe'});
if (error) {
error.message = sanitize(error.message);
throw error;
}| childProcess.on('error', (err) => { | ||
| reject(err); | ||
| }); |
There was a problem hiding this comment.
If the child process fails to spawn or encounters an error, the error event is emitted with an Error object. The err.message may contain the unsanitized command or arguments (which could include sensitive credentials/tokens).
We should sanitize the error message before rejecting the promise to prevent credential leaks in error logs or uncaught exception handlers.
childProcess.on('error', (err) => {
if (err && err.message) {
err.message = sanitize(err.message);
}
reject(err);
});| printFn(`Command "${command}" completed with ${exitDescription}.`); | ||
| printFn(`Process output: \n${logOutput}`); | ||
| printFn(`Command "${sanitize(command)}" completed with ${exitDescription}.`); | ||
| printFn(`Process output: \n${sanitize(logOutput)}`); |
There was a problem hiding this comment.
While logOutput is sanitized here for printing to the logs, note that if options.mode === 'silent', the promise is rejected with the raw, unsanitized logOutput on line 246:
reject(options.mode === 'silent' ? logOutput : undefined);If the caller of spawn or exec catches this rejection and logs or displays the error, the unsanitized credentials will be leaked. Consider sanitizing logOutput when rejecting as well:
reject(options.mode === 'silent' ? sanitize(logOutput) : undefined);| // we intentionally print all output to stderr as stdout should not be polluted. | ||
| if (options.mode === undefined || options.mode === 'enabled') { | ||
| process.stderr.write(message); | ||
| process.stderr.write(sanitize(String(message))); |
There was a problem hiding this comment.
Sanitizing stream chunks individually using a regular expression can be unreliable. If a URL containing credentials (e.g., https://user:pass@github.com) is split across two chunks during stream transmission (for example, chunk 1 ends with https://user:pa and chunk 2 starts with ss@github.com), the regex will fail to match on either chunk, resulting in the credentials being written to stderr unsanitized.
While stateful stream sanitization can be complex, it is important to be aware of this limitation. If absolute security is required for real-time console output, consider buffering the stream or using a stateful transform stream to sanitize across chunk boundaries.
| it('should handle spawn failures (e.g. command not found) without throwing TypeError', () => { | ||
| expect(() => { | ||
| ChildProcess.spawnSync('non-existent-command-12345', []); | ||
| }).toThrowError(); | ||
| }); |
There was a problem hiding this comment.
This test currently passes only because an error is thrown, but the thrown error is actually an empty Error("") due to the ignored error property in spawnSync.
Once spawnSync is updated to properly sanitize and rethrow the original error, this test should be updated to assert that the correct error (e.g., containing ENOENT) is thrown.
This PR resolves an insecure resource loading (mixed content) weakness in the Code of Conduct app by updating Google and GitHub favicon URLs to use HTTPS. Vulnerability: ac29b80c