Check correct paths are writable in requires_sudo? by jhawthorn · Pull Request #6318 · rubygems/bundler

jhawthorn · 2018-03-01T22:31:35Z

What was the end-user problem that led to this PR?

Bundler was attempting to use sudo, despite having permissions to create bundle_path (which didn't yet exist).

For some reason this became an issue in recent versions of bundler, where it wasn't one before. I'm not sure what change caused this problem to be exposed since requires_sudo? has not changed in a while.

What was your diagnosis of the problem?

requires_sudo? checks that bundle_path, and directories that bundler may need to write within it are writable. If bundle_path itself does not exist, we instead find the nearest existent parent directory, and check that that exists.

Unfortunately, when these two rules were applied together, we got the wrong result. If bundle_path did not exist, bundler would check that the nearest parent directory and everything within that parent directory were writable. This could lead to false positives for requires_sudo? when bundle_path did not yet exist.

What is your fix for the problem, implemented in this PR?

This commit fixes the issue by always checking the writability of $bundle_path/* instead of $parent_path/*.

Why did you choose this fix out of the possible options?

If we are able to create bundle_path, we know that we can write to anything within it. Changing the test to use Dir["$bundle_path/*"] is a succinct way to implement this since it will return [] if bundle_path does not yet exist.

ghost · 2018-03-01T22:31:37Z

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

segiddins · 2018-03-03T03:20:37Z

Thanks for the PR! Could you please look into adding a test for this fix?

colby-swandale · 2018-03-09T05:04:38Z

ping @jhawthorn

jhawthorn · 2018-04-03T06:52:14Z

@segiddins @colby-swandale I finally got around to this.

There are now specs for the basic behaviour of requires_sudo? (there were none previously) and a regression spec exposing the issue this PR fixes.

EDIT: I don't believe failures are related

segiddins

Thanks! We'll look into fixing those failures on master, then we'll just be able to 👍 and merge

segiddins · 2018-04-07T03:35:28Z

+      allow(Bundler).to receive(:which).with("sudo").and_return("/usr/bin/sudo")
+
+      require "tmpdir"
+      @tmpdir = Dir.mktmpdir


please use let instead of ivars

This previously had no specs

This method's goal is to check that bundle_path, and directories that bundler may need to write within it are writable. If bundle_path itself does not exist, we instead find the nearest existent parent directory, and check that that exists. However these two rules were combined incorrectly. If bundle_path did not exist, bundler would check that the nearest parent directory _and everything within that parent directory_ were writable. This could lead to false positives for requires_sudo? when bundle_path did not yet exist. This commit fixes the issue by always checking the writability of $bundle_path/* instead of $parent_path/*.

segiddins · 2018-04-21T18:37:27Z

Thanks!

@bundlerbot r+

bundlerbot · 2018-04-21T18:37:28Z

📌 Commit d8bb345 has been approved by segiddins

bundlerbot · 2018-04-21T18:37:40Z

⌛ Testing commit d8bb345 with merge 9487710...

…r=segiddins Check correct paths are writable in requires_sudo? ### What was the end-user problem that led to this PR? Bundler was attempting to use `sudo`, despite having permissions to create `bundle_path` (which didn't yet exist). For some reason this became an issue in recent versions of bundler, where it wasn't one before. I'm not sure what change caused this problem to be exposed since `requires_sudo?` has not changed in a while. ### What was your diagnosis of the problem? `requires_sudo?` checks that bundle_path, and directories that bundler may need to write within it are writable. If bundle_path itself does not exist, we instead find the nearest existent parent directory, and check that that exists. Unfortunately, when these two rules were applied together, we got the wrong result. If `bundle_path` did not exist, bundler would check that the nearest parent directory **and everything within that parent directory** were writable. This could lead to false positives for `requires_sudo?` when `bundle_path` did not yet exist. ### What is your fix for the problem, implemented in this PR? This commit fixes the issue by always checking the writability of `$bundle_path/*` instead of `$parent_path/*`. ### Why did you choose this fix out of the possible options? If we are able to create `bundle_path`, we know that we can write to anything within it. Changing the test to use `Dir["$bundle_path/*"]` is a succinct way to implement this since it will return `[]` if `bundle_path` does not yet exist.

bundlerbot · 2018-04-21T19:09:45Z

☀️ Test successful - status-travis
Approved by: segiddins
Pushing 9487710 to master...

…r=segiddins Check correct paths are writable in requires_sudo? Bundler was attempting to use `sudo`, despite having permissions to create `bundle_path` (which didn't yet exist). For some reason this became an issue in recent versions of bundler, where it wasn't one before. I'm not sure what change caused this problem to be exposed since `requires_sudo?` has not changed in a while. `requires_sudo?` checks that bundle_path, and directories that bundler may need to write within it are writable. If bundle_path itself does not exist, we instead find the nearest existent parent directory, and check that that exists. Unfortunately, when these two rules were applied together, we got the wrong result. If `bundle_path` did not exist, bundler would check that the nearest parent directory **and everything within that parent directory** were writable. This could lead to false positives for `requires_sudo?` when `bundle_path` did not yet exist. This commit fixes the issue by always checking the writability of `$bundle_path/*` instead of `$parent_path/*`. If we are able to create `bundle_path`, we know that we can write to anything within it. Changing the test to use `Dir["$bundle_path/*"]` is a succinct way to implement this since it will return `[]` if `bundle_path` does not yet exist. (cherry picked from commit 9487710)

…r=segiddins Check correct paths are writable in requires_sudo? ### What was the end-user problem that led to this PR? Bundler was attempting to use `sudo`, despite having permissions to create `bundle_path` (which didn't yet exist). For some reason this became an issue in recent versions of bundler, where it wasn't one before. I'm not sure what change caused this problem to be exposed since `requires_sudo?` has not changed in a while. ### What was your diagnosis of the problem? `requires_sudo?` checks that bundle_path, and directories that bundler may need to write within it are writable. If bundle_path itself does not exist, we instead find the nearest existent parent directory, and check that that exists. Unfortunately, when these two rules were applied together, we got the wrong result. If `bundle_path` did not exist, bundler would check that the nearest parent directory **and everything within that parent directory** were writable. This could lead to false positives for `requires_sudo?` when `bundle_path` did not yet exist. ### What is your fix for the problem, implemented in this PR? This commit fixes the issue by always checking the writability of `$bundle_path/*` instead of `$parent_path/*`. ### Why did you choose this fix out of the possible options? If we are able to create `bundle_path`, we know that we can write to anything within it. Changing the test to use `Dir["$bundle_path/*"]` is a succinct way to implement this since it will return `[]` if `bundle_path` does not yet exist. (cherry picked from commit 9487710)

* 1-17-stable: (38 commits) Version 1.17.0 with changelog Merge #6754 Version 1.17.0.pre.2 with changelog fix failing bundle remove specs Still document the `--force` option scope specs testing bundler 2 deprecations to bundler 1 only Merge #6718 Merge #6707 Merge #6702 Merge #6316 Auto merge of #6447 - agrim123:agr-update-error-message, r=segiddins Auto merge of #6513 - agrim123:agr-bundler-remove, r=indirect Auto merge of #6318 - jhawthorn:fix_incorrect_test_in_requires_sudo, r=segiddins Auto merge of #6450 - bundler:segiddins/bundle-binstubs-all, r=colby-swandale Auto merge of #6024 - gwerbin:custom-user-bundle-dir, r=colby-swandale Version 1.17.0.pre.1 with changelog Auto merge of #5964 - bundler:colby/deprecate-viz-command, r=segiddins Auto merge of #5986 - bundler:seg-jobs-count, r=indirect Auto merge of #5995 - bundler:seg-gvp-major, r=indirect Auto merge of #5803 - igorbozato:path_relative_to_pwd, r=indirect ...

colby-swandale added the status: user feedback required label Mar 9, 2018

jhawthorn force-pushed the fix_incorrect_test_in_requires_sudo branch from 97d1afd to 6b23ad6 Compare April 3, 2018 06:45

segiddins approved these changes Apr 7, 2018

View reviewed changes

jhawthorn added 2 commits April 20, 2018 11:33

Add some specs against requires_sudo?

8cc446c

This previously had no specs

jhawthorn force-pushed the fix_incorrect_test_in_requires_sudo branch from 6b23ad6 to d8bb345 Compare April 20, 2018 18:33

bundlerbot merged commit d8bb345 into rubygems:master Apr 21, 2018

bundlerbot mentioned this pull request Apr 21, 2018

Display reason to require sudo #6316

Merged

colby-swandale added this to the 1.17.1 milestone Sep 24, 2018

colby-swandale modified the milestones: 1.16.6, 1.17.0 Oct 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Check correct paths are writable in requires_sudo?#6318

Check correct paths are writable in requires_sudo?#6318
bundlerbot merged 2 commits intorubygems:masterfrom
jhawthorn:fix_incorrect_test_in_requires_sudo

jhawthorn commented Mar 1, 2018 •

edited

Loading

Uh oh!

ghost commented Mar 1, 2018

Uh oh!

segiddins commented Mar 3, 2018

Uh oh!

colby-swandale commented Mar 9, 2018

Uh oh!

jhawthorn commented Apr 3, 2018 •

edited

Loading

Uh oh!

segiddins left a comment

Uh oh!

segiddins Apr 7, 2018

Uh oh!

jhawthorn Apr 20, 2018

Uh oh!

segiddins commented Apr 21, 2018

Uh oh!

bundlerbot commented Apr 21, 2018

Uh oh!

bundlerbot commented Apr 21, 2018

Uh oh!

bundlerbot commented Apr 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jhawthorn commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was the end-user problem that led to this PR?

What was your diagnosis of the problem?

What is your fix for the problem, implemented in this PR?

Why did you choose this fix out of the possible options?

Uh oh!

ghost commented Mar 1, 2018

Uh oh!

segiddins commented Mar 3, 2018

Uh oh!

colby-swandale commented Mar 9, 2018

Uh oh!

jhawthorn commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

segiddins left a comment

Choose a reason for hiding this comment

Uh oh!

segiddins Apr 7, 2018

Choose a reason for hiding this comment

Uh oh!

jhawthorn Apr 20, 2018

Choose a reason for hiding this comment

Uh oh!

segiddins commented Apr 21, 2018

Uh oh!

bundlerbot commented Apr 21, 2018

Uh oh!

bundlerbot commented Apr 21, 2018

Uh oh!

bundlerbot commented Apr 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jhawthorn commented Mar 1, 2018 •

edited

Loading

jhawthorn commented Apr 3, 2018 •

edited

Loading