Skip to content
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

Fix #65069: GlobIterator incorrect handling of open_basedir check #9120

Closed
wants to merge 1 commit into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Jul 24, 2022

Proper checking and filtering of glob stream paths when open_basedir set.

This is a rebase with some minor changes of PR #398 that I did 9 years ago that got somehow sidetracked discussion about the correct glob behavior. After thinking about this, the PR still makes sense to me as it fixes the reported problem even though it doesn't do anything about the inconsistency which is however already present in glob so there is no reason to not apply this.

@bukka bukka force-pushed the glob_wrapper_open_basedir branch 3 times, most recently from ec34cfa to d96c168 Compare July 24, 2022 21:41
@bukka
Copy link
Member Author

bukka commented Jul 24, 2022

So after a bit of thinking I went for solution with empty array as it is really the only one that makes sense to me.

@bukka bukka force-pushed the glob_wrapper_open_basedir branch from d96c168 to 960d127 Compare July 25, 2022 10:47
@mvorisek
Copy link
Contributor

Will/can this PR address also https://3v4l.org/EYVGd?

@bukka bukka changed the base branch from PHP-8.0 to master July 25, 2022 16:55
@bukka bukka force-pushed the glob_wrapper_open_basedir branch from 960d127 to f03676c Compare July 25, 2022 16:55
@bukka
Copy link
Member Author

bukka commented Jul 25, 2022

@mvorisek This PR is just for glob wrapper (glob://... or GlobIterator that uses that) so I don't think it can address the issue you mentioned. From a quick look, I'm not sure why this actually doesn't work as it is under /in but haven't checked the code. Even if it is supposed to be like that, it might be worth to create a separate issue for it because it might be at least a documentation issue. Can you check if there is already a bug for this by any chance?

@mvorisek
Copy link
Contributor

Can you check if there is already a bug for this by any chance?

https://bugs.php.net/bug.php?id=52065

@bukka bukka force-pushed the glob_wrapper_open_basedir branch from f03676c to 7953a39 Compare July 25, 2022 19:15
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the warning is IMHO good, can't really comment on the implementation as I'm far from being familiar of the stream layer

ext/spl/tests/bug65069.phpt Outdated Show resolved Hide resolved
ext/spl/tests/bug65069.phpt Outdated Show resolved Hide resolved
ext/spl/tests/bug65069.phpt Show resolved Hide resolved
@bukka bukka force-pushed the glob_wrapper_open_basedir branch from 7953a39 to 76eb3ba Compare July 26, 2022 12:25
@bukka
Copy link
Member Author

bukka commented Jul 26, 2022

@cmb69 I saw your comments in the related PR https://bugs.php.net/bug.php?id=77085 so it would be great if you could have a quick look. I target master as there is that potential minor BC break that you mentioned in that bug for glob function. I would actually like to introduce the same behavior for glob as it just doesn't make sense to do open basedir check on pattern. I think it should be still fine for beta. What do you think?

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks generally good to me. I made some suggestions for your consideration.

ext/spl/tests/bug65069.phpt Outdated Show resolved Hide resolved
main/streams/glob_wrapper.c Outdated Show resolved Hide resolved
main/streams/glob_wrapper.c Outdated Show resolved Hide resolved
@bukka bukka force-pushed the glob_wrapper_open_basedir branch from 76eb3ba to 3235c92 Compare July 26, 2022 17:22
@bukka
Copy link
Member Author

bukka commented Jul 28, 2022

Merged via 1a9e689

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants