This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Thanks @SpenserJ !
@edodusi would you mind having a look?

Sure @escapedcat let me add my review

Hey @edodusi, thank you for the feedback. I'd be happy to make that change, and am wondering if you'd prefer it to be hardcoded or configurable, and if its configurable what config name would you like it to use?

@SpenserJ I think if we leave it configurable we can open the same security hole, so I would probably go with hardcode (also the regexp itself is an hardcoded blacklist).

At the moment I can only think of scenarios that involve CI and NODE_ENV, if you think we should allow something else feel free to add them!

@edodusi I'm not sure I fully understand the security hole and would love it if you could teach me more about the potential risks. I assume most security-conscious repos are already restricting automatic CI runs to trusted contributors or reviewed PRs (like commitlint does), and an upstream supply chain attack would either be able to access it directly or shouldn't be able to change the configuration for commitlint.

My team isn't currently using any other env variables for commitlint, but I'm not sure thats a good reason to hardcode it since the original PR didn't account for my needs and we may not be accounting for someone else's legitimate needs by hardcoding either. That said, I can't think of a case where you'd only want to ignore a commit message outside of CI (I can't even think of a scenario where you'd only ignore in a certain NODE_ENV), so I'll go ahead with hardcoding for now and leave it to someone with more unique requirements to propose a solution that works for them if that ever arises 👍

@SpenserJ so my concern (and the reason for my PR #4258) is that users can include commitlint rules provided by third parties, say for example @commitlint/config-conventional. In this third party package an attacker may include the ignores rule with malicious code, and since the party is implicitly trusted the function will get executed in CI, so we can only prevent it by running static analysis.

This is to summarize, and to answer your question: if we let the author of the is-ignored function also list the allowed properties in process.env, then the safety net is lost because they can also include say API_KEYS in that list and it will pass the check.

You may say that the end user could simply look at the code they are including in extends, but this basically never happens, especially with automated updates, thus the supply chain attack opportunity.

I agree with you that here we are deciding that certain use cases are valid and others are not, but honestly I cannot come up with a better idea ATM

@edodusi Thank you, that makes sense 👍 While working on this, I did notice a few issues with the overall implementation:

• Its checking for process.exec, but exec/spawn/etc exist on child_process
• Its not checking for fork, or explicitly checking for execSync/execFileSync/spawnSync, and those are only being caught in the tests because of process.
• Its blocking require/import inside the ignores function, but that doesn't prevent an attacker from importing at the config root
• Destructuring can also bypass the checks

Doing a simple string check will catch a basic attack, but its very easy to get around and you can confirm this by extending a config containing:

// Import at the top level isn't caught by checking the `ignores` function
// Using `as` lets us alias the import to further hide from checks
import { execSync as bypass } from 'child_process';
export default {
  ignores: [
    // Able to call child_process.execSync
    commit => { console.log(bypass('ls', { encoding: 'utf8' })); return true; },
  ],
};

Next to using an AST to scan the whole config, I can't think of a good way of preventing bypasses like this. The checks are still useful for blocking naive exploitation and supply chain attacks, but they're insufficient for preventing a more experienced attacker.

Even if we ignore spawning processes, it would be very easy to bypass most string-based environment guards checking for process.env as well:

export default {
  ignores: [
    commit => {
      // Destructuring avoids any checks for `process.`
      const { env: { API_KEY: exploited } } = process;
      console.log(exploited);
      return true;
    },
  ],
};

Although I agree with trying to prevent a supply chain attack like this, I don't think this approach is sufficient and I'm not even sure that a more advanced approach like using an AST to track variable assignments and destructuring would prevent most bypasses without being extremely complicated and expensive to maintain ☹️

@SpenserJ totally agree here: this level of protection is very basic, however at least it's something?

My concern is that since this is a library that validates commits it could be perceived as "innocent" by end users, and because of that the level of attention is lower. But this is a script that people executes on their local env and in CI, so to me it's maximum alert 😄

Do you have any other idea on how to improve the security level here? Could the ignores feature be disabled by default and opt-in, so that only the end-user can allow a specific package to executes functions? Something similar to what pnpm did with lifecycle scripts.

cc @escapedcat

Nor really my expertise to be honest. I'd rather not make something opt-in. Maybe if people know what they are doing they can disable it?

@escapedcat and what about, instead of a function, letting users provide a regexp? If the regexp matches then the commit is ignored, which I think is probably the most common use case.

You could also provide other options like ignore if a certain env variable equals a value, both provided by the user.

@edodusi @escapedcat I see it from the opposite side, where very basic protection that is easy to bypass is worse than no protection at all, since its a false sense of security. No matter how innocent the dependency, end users should be responsible for knowing what they're running and taking adequate steps to protect their repos and secrets. I don't disagree that most users won't look close enough (or at all) to understand the risks, but I don't think its reasonable or possible to prevent every possible supply chain attack, and an easily bypassed check is more dangerous than "Run untrusted code at your own risk"

The only ways I can think of implementing this check more effectively would be either AST scanning the entire package that is being extended (technically and computationally expensive, and better left to SAST and vulnerability scanning tools) or limiting configs/presets/plugins/etc to a syntax like JSON that can't execute code (which greatly limits users and preset maintainers from doing legitimate things).

Letting users provide a regexp instead of a function seems like a nice improvement for usability since I agree that its the most common use-case, but that doesn't prevent the supply chain concern you mentioned.

My suggestion would be to remove the easily bypassed check since it prevents legitimate uses and fails to prevent the intended risks, and potentially remind users that running untrusted code is dangerous (which everyone should already understand). For now I'll be working around it in our repos by destructuring const { CI } = process.env outside of the ignores function

@SpenserJ by providing only a regexp the most dangerous supply chain risk will be prevented (executing arbitrary code), I'm thinking about removing ignores entirely and replacing it with just a string like ignore: /regExp

Anyway, I get your point, I don't know if no security is better than low security, I will leave this to another reviewer but thank you for your very clear explanation

@edodusi That doesn't prevent the most dangerous supply chain risk if presets are still able to run JS outside of the ignores function though

import { exec } from 'child_process';
exec('ls'); // Supply chain attack in the preset

export default {
  ignores: [
    /^wip: /, // Only allow regexp in ignores
  ],
};

@SpenserJ oh yes, yes you are right. So at this point do you think it's better to just revert my PR and leave it to the end user completely?

@edodusi As much as I hate to see contributions reversed, I would recommend reverting it unless someone can find a more complete solution. It could be helpful to check through other libraries that use config presets like ESLint/Babel/etc and see what they do. I'm just a random contributor that ran into a limitation with this approach though, so that is a better question for the maintainers (cc @escapedcat).

Regardless of the outcome, kudos to you for trying to improve the security 👏 I wish users could be trusted to do their own research, and its refreshing to see someone making security conscious improvements to protect users!

Thank you @SpenserJ. I'm leaving the decision to @escapedcat

@escapedcat Any thoughts on this? Tl;dr is that the added security around execution in ignores isn't actually preventing upstream presets from executing processes or accessing environment variables, but it is making legitimate use of env variables much harder.

Ugh, I kinda tried to sit this out, sorry.
Sounds like we merge this in?

@escapedcat I'm of the opinion that #4258 should be reverted, since it gives a false sense of security and prevents legitimate use without actually preventing the intended security risks. This PR relaxes the check slightly, but its still extremely easy to work around the check as demonstrated in #4282 (comment), so I'm not sure those checks are worth the complexity ☹️ I hate suggesting that contributions are reverted so I don't say this lightly, but the PR doesn't accomplish its intended goal

Alright, thanks for your feedback! Appreciate both of your comments.
@edodusi I'll revert your PR within the next days and publish a new release. Thanks for your efforts!

Thanks for the update. In that case I'll close this PR, but I'll leave #4281 open in case anyone runs into it until the new version is released