nodejs/node

Add testcases for all documented safeguards

Open

#22492 opened on Aug 23, 2018

View on GitHub
 (8 comments) (6 reactions) (0 assignees)JavaScript (117,218 stars) (35,535 forks)batch import
help wantedsecuritytest

Description

To clarify: I am not blaming anyone. I view this as a problem of the process that we used, the aim of this issue is to seek how we can actually improve things. I myself LGTMd the change that introduced the initial safeguard without a testcase.

This is a post-mortem of https://github.com/nodejs-private/security/issues/202 (see 40a7beeddac9b9ec9ef5b49157daaf8470648b08).

That itself was a minor issue which is very unlikely to be exploitable (more unlikely than the second one fixed in the same release), but it revealed what I think is a process problem.

That issue was introduced in #18790 which removed the corresponding safeguard from the code while optimizing performance. The safeguard was initially introduced in #4682, and was documented in the code — that didn't save it from being removed:

This prevents accidentally sending in a number that would be interpretted as a start offset.

My opinion is that a testcase should have been introduced at the same time when the comment was. Or, perhaps, even instead of the comment — the comment turned out to be not effective.

So, the proposal is to:

  1. Collect a list of similar comments in the codebase that warn against changing some code or explain why is it needed — that hints that the common usecase might work with that code removed, but doing so is expected to break some rare usecase or cause a potential security issue.
  2. Try removing/modifying the safegaurds and see if any tests break, make a list of safeguards that do not break tests when are tampered with.
  3. Add missing tests.
  4. Update to try spotting such things (i.e. untested expectations) in the future while reviewing PRs. Should be fairly easy to spot things that were worth a comment in the code and to question if those are tested?

There might be none — but I would prefer if we re-checked that.

Side note: code coverage is not an effective measure to collect the data here or prevent the issue.

/cc @nodejs/security-wg

Contributor guide