New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate the names of directory children, and normalize output directory/file names #10850
Validate the names of directory children, and normalize output directory/file names #10850
Conversation
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…hould enforce some path constraints. [ci skip-build-wheels]
Commits are useful to review independently. |
So probably no need for my short-term fix in #10849 then? |
Happy to land both of them. Yours is likely to go green first, but this change will normalize that path in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Good call to use RelativePath
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I ran the example-python reproduction of the bug with this change and issue is fixed.
Good to land both. This will ensure normalization while my change will ensure no need to normalize in that spot. |
[ci skip-build-wheels]
8ce3d6b
to
438f390
Compare
Problem
As explained in #10802 (comment), Pants does not currently normalize
Process::{output_files,output_directories}
as relative paths, which caused us to send an invalid path to the server. Additionally, the server managed to return a "nearly valid" output where a child path for a directory had an empty name, which we should validate for in order to fail faster.Solution
In two commits: 1) validate that
Directory
protos returned by remoting do not contain empty child names, 2) switch theProcess::{output_files,output_directories}
fields to holdingRelativePath
s, which are normalized to strip trailing slashes, and validated as not escaping their root.Result
Fixes #10802. Opened bazelbuild/remote-apis#175 to track upstreaming the additional "must be non-empty" constraint.
[ci skip-build-wheels]