Exclude development scripts from published package

[weiznich]

During a dependency review we noticed that the autocfg crate includes various development scripts. These development scripts shouldn't be there as they might, at some point become problematic. As of now they prevent any downstream user from enabling the [bans.build.interpreted] option of cargo deny.

I opted for using an explicit include list instead of an exclude list to prevent these files from being included in the published packages to make sure that everything that's included is an conscious choice.

[cuviper]

During a dependency review we noticed that the autocfg crate includes various development scripts.

What exactly? Is it just tests/wrap_ignored?

These development scripts shouldn't be there as they might, at some point become problematic. As of now they prevent any downstream user from enabling the [bans.build.interpreted] option of cargo deny.

cargo deny is over-zealous if it flags this script as a build issue. This crate has no build scripts and uses no proc macros, so there's no chance for any script to get invoked in this build regardless.

They also caution that "the compile time crate linting supplied by cargo-deny does NOT protect you from actively malicious code."

https://embarkstudios.github.io/cargo-deny/checks/bans/cfg.html#the-build-field-optional

I opted for using an explicit include list instead of an exclude list to prevent these files from being included in the published packages to make sure that everything that's included is an conscious choice.

I do object to excluding tests though, and this script is needed by one of those. Tests are used both by downstream packagers (like Linux distros) as well as Rust's own crater for release checks -- it would raise awareness if some upcoming change caused a regression in autocfg testing.

Note that the test could write an equivalent script as a temporary file when it runs, but I think that would look even more suspicious. Either way, obfuscating or omitting tests, we shouldn't let audit tooling push us toward worse behavior!

[weiznich]

What exactly? Is it just tests/wrap_ignored?

Yes, only tests/wrap_ignored is reported for autocfg.

cargo deny is over-zealous if it flags this script as a build issue. This crate has no build scripts and uses no proc macros, so there's no chance for any script to get invoked in this build regardless.

I don't think it's that simple. The code in this crate can (and in my particular example is) used as build-dependency of another crate. Given that all the code in this crate can run at built-time. This theoretically includes the script as well. An example of an crate using autocfg as build dependency is bigdecimal.

They also caution that "the compile time crate linting supplied by cargo-deny does NOT protect you from actively malicious code."

Thanks for highlighting. I'm well aware of that, and to be clear cargo deny is only a tool to make my live a bit easier. Ultimately we nevertheless look at all our dependencies code, so yes cargo deny doesn't protect you from anything, but it can make spotting potential problematic places easier. For the record: I also reported several other issues to other crates spotted during reviews over the last year or so. This includes cases that were obviously not detected by any automation.

And to be clear here: Yes the current code is not malicious, but it is still a potential vector for anyone to introduce malicious code.

I do object to excluding tests though, and this script is needed by one of those. Tests are used both by downstream packagers (like Linux distros) as well as Rust's own crater for release checks -- it would raise awareness if some upcoming change caused a regression in autocfg testing.

Note that the test could write an equivalent script as a temporary file when it runs, but I think that would look even more suspicious. Either way, obfuscating or omitting tests, we shouldn't let audit tooling push us toward worse behavior!

If you feel that there is any value in including the tests I'm happy to change the PR to include them as well. I still would like to avoid the script. To argue with your own arguments here: The shell script is just not portable and will make the tests fail in environments that doesn't provide bash (e.g. windows). So maybe there is a solution to run the test without the shell script? Or just exclude that particular test from publishing?

Another transparency section: I opted for excluding the tests as that is what other larger projects like rustls or reqwest do as well. I have no hard feelings for tests being there, it's just that I believe it's uneccessary to have them for almost all consumers of the crates.io package. As for crater: I'm well aware of that. You should also know that it also uses github repositories as source, so it can (and likely will?) use the tests from there.

[weiznich]

@cuviper What is missing to move this forward. Does the response above address your concerns?

[cuviper]

I would rather not exclude tests. If you'd like to try reworking that particular test without the script, e.g. using itself as the wrapper (and harness = false), that might be feasible.

[weiznich]

@cuviper I updated the PR according to your requests. It now replaces the shell script with a small binary compiled by rustc doing the same thing. Tests are also included in the published package now. Hopefully that satisfies your needs, if that's not the case just let me know.

[cuviper]

I was thinking more like #80 -- let me know if that works for you!