Skip to content
2 changes: 2 additions & 0 deletions bundler/lib/bundler/man/bundle-config.1
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ The following is a list of all configuration keys and their purpose\. You can le
.IP "\(bu" 4
\fBconsole\fR (\fBBUNDLE_CONSOLE\fR): The console that \fBbundle console\fR starts\. Defaults to \fBirb\fR\.
.IP "\(bu" 4
\fBcredential\.helper\fR (\fBBUNDLE_CREDENTIAL_HELPER\fR): The path to a credential helper to use for fetching credentials from a remote gem server\.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
\fBcredential\.helper\fR (\fBBUNDLE_CREDENTIAL_HELPER\fR): The path to a credential helper to use for fetching credentials from a remote gem server\.
\fBcredential\.helper\fR (\fBBUNDLE_CREDENTIAL_HELPER\fR): The path to a credential helper to use to fetch credentials for a remote gem server\.

.IP "\(bu" 4
\fBdefault_install_uses_path\fR (\fBBUNDLE_DEFAULT_INSTALL_USES_PATH\fR): Whether a \fBbundle install\fR without an explicit \fB\-\-path\fR argument defaults to installing gems in \fB\.bundle\fR\.
.IP "\(bu" 4
\fBdeployment\fR (\fBBUNDLE_DEPLOYMENT\fR): Disallow changes to the \fBGemfile\fR\. When the \fBGemfile\fR is changed and the lockfile has not been updated, running Bundler commands will be blocked\.
Expand Down
3 changes: 3 additions & 0 deletions bundler/lib/bundler/man/bundle-config.1.ronn
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ learn more about their operation in [bundle install(1)](bundle-install.1.html).
`bundle install`.
* `console` (`BUNDLE_CONSOLE`):
The console that `bundle console` starts. Defaults to `irb`.
* `credential.helper` (`BUNDLE_CREDENTIAL_HELPER`):
The path to a credential helper to use for fetching credentials from a
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
The path to a credential helper to use for fetching credentials from a
The path to a credential helper to use to fetch credentials for a

remote gem server.
* `default_install_uses_path` (`BUNDLE_DEFAULT_INSTALL_USES_PATH`):
Whether a `bundle install` without an explicit `--path` argument defaults
to installing gems in `.bundle`.
Expand Down
33 changes: 32 additions & 1 deletion bundler/lib/bundler/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ class Settings
system_bindir
trust-policy
version
credential.helper
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The credential.helper key doesn't have a value, but credential.helper.#{host} keys have string values. So how about adding credential.helper. prefix to is_string method instead of adding credential.helper here? https://github.com/atpons/rubygems/blob/5b91e1db720cb6b8a0ddeeb9952ea2e8a04811c9/bundler/lib/bundler/settings.rb#L369-L372

].freeze

DEFAULT_CONFIG = {
Expand Down Expand Up @@ -197,7 +198,7 @@ def mirror_for(uri)
end

def credentials_for(uri)
self[uri.to_s] || self[uri.host]
credentials_from_helper(uri) || self[uri.to_s] || self[uri.host]
end

def gem_mirrors
Expand Down Expand Up @@ -595,5 +596,35 @@ def self.key_to_s(key)
end
end
end

def credentials_from_helper(uri)
helper_key = "credential.helper.#{uri.host}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: This key differs from the one written in the RFC: https://github.com/rubygems/rfcs/pull/59/changes#diff-30acc2f7e5e276ac49ae593cd2b11138320356f6ebee37c3ca655624e7f4ea4eR33

But I think both credential.helper and credential-helper are acceptable.

helper_path = self[helper_key]
return unless helper_path

begin
require "shellwords"
command = Shellwords.shellsplit(helper_path)
command[0] = if command[0].start_with?("/", "~")
command[0]
else
"bundler-credential-#{command[0]}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure why we need a constant prefix here. Isn't it enough to simply run it with the name as is even when the command is specified by a relative path? We can directly write bundler-credential-foobar to the config.

end

output = Bundler.with_unbundled_env { IO.popen(command, &:read) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit - lets check $? after IO.popen

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks, db1e5ab fixed checking Process.last_status (use $? difficult to write specs)

unless Process.last_status.success?
Bundler.ui.warn "Credential helper failed with exit status #{$?.exitstatus}"
return nil
end
output = output.to_s.strip
output.empty? ? nil : output
rescue Errno::ENOENT, ArgumentError => e
Bundler.ui.warn "Credential helper #{helper_path} not available: #{e.message}"
nil
rescue StandardError => e
Bundler.ui.warn "Credential helper failed: #{e.message}"
nil
Comment on lines +624 to +626
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

IMHO: I think this rescue clause is unnecessary because failures of the executed command are caught by Process.last_status.success?, and failures of IO.popen such as Errno::EPIPE should be treated as errors.

end
end
end
end
42 changes: 42 additions & 0 deletions bundler/spec/bundler/settings_spec.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require "stringio"
require "bundler/settings"

RSpec.describe Bundler::Settings do
Expand Down Expand Up @@ -263,6 +264,47 @@
expect(settings.credentials_for(uri)).to eq(credentials)
end
end

context "with credential helper configured" do
let(:helper_path) { "/path/to/helper" }
let(:uri) { Gem::URI("https://gemserver.example.org") }

before do
settings.set_local "credential.helper.gemserver.example.org", helper_path
allow(Process).to receive(:last_status).and_return(double(success?: true, exitstatus: 0))
end

it "uses the credential helper when configured" do
expect(IO).to receive(:popen).with([helper_path]).and_yield(StringIO.new("username:password\n"))
expect(settings.credentials_for(uri)).to eq("username:password")
end

it "fallback to config when helper fails" do
expect(IO).to receive(:popen).with([helper_path]).and_raise(StandardError, "Helper failed")
expect(Bundler.ui).to receive(:warn).with("Credential helper failed: Helper failed")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe we want to have more info on which helper failed for which host?

settings.set_local "gemserver.example.org", "fallback:password"
expect(settings.credentials_for(uri)).to eq("fallback:password")
end

it "returns nil when helper fails and no fallback config exists" do
expect(IO).to receive(:popen).with([helper_path]).and_yield(StringIO.new(""))
expect(settings.credentials_for(uri)).to be_nil
end

context "with relative helper path and options" do
let(:helper_path) { "custom-helper --foo=bar" }

before do
settings.set_local "credential.helper.gemserver.example.org", helper_path
allow(Process).to receive(:last_status).and_return(double(success?: true, exitstatus: 0))
end

it "prepends bundler-credential- to the helper name" do
expect(IO).to receive(:popen).with(["bundler-credential-custom-helper", "--foo=bar"]).and_yield(StringIO.new("username:password\n"))
expect(settings.credentials_for(uri)).to eq("username:password")
end
end
end
end

describe "URI normalization" do
Expand Down
Loading