ensure $HOME and Dir.tmpdir are writable #5954
Conversation
|
So i have caused some confusion here, i'm really sorry for that 🙇. In regards to checking the permissions of the |
|
No worries, I will remove the validation on |
| # Fastly ignores Range when Accept-Encoding: gzip is set | ||
| headers["Accept-Encoding"] = "gzip" | ||
| end | ||
| Bundler::SharedHelpers.filesystem_access(Dir.tmpdir, :write) do |
There was a problem hiding this comment.
Dir.tmpdir and Dir.mktmpdir are different folders, see:
› irb
irb(main):001:0> require 'tmpdir'
=> true
irb(main):002:0> Dir.tmpdir
=> "/var/folders/8q/r499202j3pvgyhpm1nq8n5fw0000gn/T"
irb(main):003:0> Dir.mktmpdir("bundler-compact-index") { |dir| puts dir }
/var/folders/8q/r499202j3pvgyhpm1nq8n5fw0000gn/T/bundler-compact-index20170820-60920-8s1n97
=> nil
This is important because Bundler typically prints the path to the folder in the error that is given back to the user:
› dbundler install
There was an error while trying to write to `foo`. There was insufficient space remaining on the
device.
There was a problem hiding this comment.
In that case I can create the temp dir with Dir.mktmpdir("bundler-compact-index-") before calling filesystem_access, and then pass it to filesystem_access.
There was a problem hiding this comment.
Actually I don't think that will work. If Dir.mktmpdir fails outside of the filesystem_access block, the exception won't be caught.
I could just rescue the Errno::EACCES exception when it's raised by Dir.mktmpdir, and re-raisse it inside the filesystem_access block.
Update: the above solution seems to be a catch-22. I can't call filesystem_access without a directory, and I can't create that temp directory without calling Dir.mktmpdir. If the bundler process doesn't have permission to create that temp directory, it raises Errno::EACCES. I think the best thing I can do in this case is to not use filesystem_access at all, and just rescue Errno::EACCES.
ajwann
force-pushed
the
check-permissions-for-client-index-dir
branch
3 times, most recently
from
224ce87
to
1813716
Compare
ajwann
force-pushed
the
check-permissions-for-client-index-dir
branch
from
1813716
to
008537d
Compare
|
|
||
| response = @fetcher.call(remote_path, headers) | ||
| return nil if response.is_a?(Net::HTTPNotModified) | ||
| local_temp_dir = create_tmpdir |
There was a problem hiding this comment.
moving away from the block form to this will no longer clean up the tmpdir, can we ensure that we remove the directory if it exists?
There was a problem hiding this comment.
In that case I think it would be best if I still called Dir.mktmpdir with the block, and just rescued Errno::EACCES within the update method if the call to Dir.mktmpdir raises an exception.
|
Awesome! |
|
📌 Commit 932ea90 has been approved by |
…r=segiddins ensure $HOME and Dir.tmpdir are writable Fixes #5518 ### What was the end-user problem that led to this PR? A user had issues installing gems due to a permissions issue on the temp dir, because Bundler was not checking for proper permissions on the temp directory or $HOME. ### What was your diagnosis of the problem? After discussing the issue with @colby-swandale, the solution was to check permissions on $HOME and the ```tmpdir```. ### What is your fix for the problem, implemented in this PR? The creation of the [temp dir](https://github.com/bundler/bundler/blob/master/lib/bundler/compact_index_client/updater.rb#L31) is now wrapped in the block passed to ```filesystem_access```, so ```filesystem_access``` will rescue the ```Errno::EACCES``` exception which is thrown if the effective user of the bundler process doesn't have sufficient permissions to create the temp dir. ### Why did you choose this fix out of the possible options? I chose this fix because it's what was discussed in the original issue thread.
|
☀️ Test successful - status-travis |
Fixes #5518
What was the end-user problem that led to this PR?
A user had issues installing gems due to a permissions issue on the temp dir, because
Bundler was not checking for proper permissions on the temp directory or $HOME.
What was your diagnosis of the problem?
After discussing the issue with @colby-swandale, the solution was to check permissions on $HOME and the
tmpdir.What is your fix for the problem, implemented in this PR?
The creation of the temp dir is now wrapped in the block passed to
filesystem_access, sofilesystem_accesswill rescue theErrno::EACCESexception which is thrown if the effective user of the bundler process doesn't have sufficient permissions to create the temp dir.Why did you choose this fix out of the possible options?
I chose this fix because it's what was discussed in the original issue thread.