wit-bindgen icon indicating copy to clipboard operation
wit-bindgen copied to clipboard

Restrict `generate!` paths to paths within crate boundaries

Open rvolosatovs opened this issue 2 years ago • 6 comments

Crates using generate! with paths leading outside the crate boundary cannot be dependent upon in cargo vendor use cases.

E.g. see https://github.com/rvolosatovs/wasi-vendor-repro for a reproducer

Refs https://github.com/bytecodealliance/preview2-prototyping/issues/133

A draft PR will follow

rvolosatovs avatar Apr 07 '23 18:04 rvolosatovs

Note, that the fix enabling cargo vendor use cases is actually trivial - symlinks https://github.com/bytecodealliance/preview2-prototyping/pull/134

rvolosatovs avatar Apr 07 '23 19:04 rvolosatovs

Thanks for the report! I'm not sure if there's much that can be done within this crate, though, about that?

alexcrichton avatar Apr 10 '23 14:04 alexcrichton

Thanks for the report! I'm not sure if there's much that can be done within this crate, though, about that?

The proposal here is to disallow path references like this https://github.com/bytecodealliance/preview2-prototyping/blob/083879cb955d7cc719eb7fa1b59c6096fcc97bbf/wasi/src/lib.rs#L7, which lead outside the crate

generate! would fail with a descriptive error in this case. I have a WIP PR ready for it, but adapting tests, which rely heavily on this feature in this repository is quite time-consuming

rvolosatovs avatar Apr 10 '23 14:04 rvolosatovs

I'm not sure that disallowing outside-root paths is a way to handle this, though, since not all crates will make their way to crates.io which would cause this to be an issue. Personally I feel like it's better to be more flexible with the macro, but still have facilities available for those publishing to crates.io.

alexcrichton avatar Apr 10 '23 15:04 alexcrichton

I'm not sure that disallowing outside-root paths is a way to handle this, though, since not all crates will make their way to crates.io which would cause this to be an issue. Personally I feel like it's better to be more flexible with the macro, but still have facilities available for those publishing to crates.io.

Well, this does not break the crates being published, rather it breaks all downstream consumers of these crates using the standard cargo functionality: cargo vendor. Whether those crates downstream consumers depend upon are published to a particular crate registry or just consumed via git is irrelevant here.

rvolosatovs avatar Apr 10 '23 15:04 rvolosatovs

I don't think I quite agree with that because if a crate was cargo publish'd using out-of-tree paths then it would still be broken on crates.io. If depended on with Cargo via a git dependency it would still work, which I say falls under the umbrell of "standard cargo functionality".

I don't dispute that the vendor use case is broken, but what I'm saying is that I don't think it's the right solution to require all uses to be vendor-ready. It should definitely be possible to be vendor-ready, but I don't think it's worthwhile to require that of everyone.

alexcrichton avatar Apr 11 '23 19:04 alexcrichton