Skip to content

fix(apps/code-of-conduct): load provider icons over HTTPS#3765

Open
josephperrott wants to merge 2 commits into
angular:mainfrom
josephperrott:fix/sec-coc-insecure-resource-ac29b80c
Open

fix(apps/code-of-conduct): load provider icons over HTTPS#3765
josephperrott wants to merge 2 commits into
angular:mainfrom
josephperrott:fix/sec-coc-insecure-resource-ac29b80c

Conversation

@josephperrott
Copy link
Copy Markdown
Member

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

@josephperrott josephperrott added the action: merge The PR is ready for merge by the caretaker label Jun 6, 2026
@josephperrott josephperrott requested a review from alan-agius4 June 6, 2026 02:42
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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;
    }

Comment on lines +195 to +197
childProcess.on('error', (err) => {
reject(err);
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

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)}`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-medium medium

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)));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-medium medium

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.

Comment on lines +65 to +69
it('should handle spawn failures (e.g. command not found) without throwing TypeError', () => {
expect(() => {
ChildProcess.spawnSync('non-existent-command-12345', []);
}).toThrowError();
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant