Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 33 additions & 16 deletions lib/rubygems/commands/sources_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,8 @@ def initialize
end

def add_source(source_uri) # :nodoc:
check_rubygems_https source_uri

source = Gem::Source.new source_uri

check_typo_squatting(source)
source = build_new_source(source_uri)
source_uri = source.uri.to_s

begin
if Gem.sources.include? source
Expand All @@ -76,11 +73,8 @@ def add_source(source_uri) # :nodoc:
end

def append_source(source_uri) # :nodoc:
check_rubygems_https source_uri

source = Gem::Source.new source_uri

check_typo_squatting(source)
source = build_new_source(source_uri)
source_uri = source.uri.to_s

begin
source.load_specs :released
Expand All @@ -103,11 +97,8 @@ def append_source(source_uri) # :nodoc:
end

def prepend_source(source_uri) # :nodoc:
check_rubygems_https source_uri

source = Gem::Source.new source_uri

check_typo_squatting(source)
source = build_new_source(source_uri)
source_uri = source.uri.to_s

begin
source.load_specs :released
Expand Down Expand Up @@ -141,6 +132,19 @@ def check_typo_squatting(source)
end
end

def normalize_source_uri(source_uri) # :nodoc:
# Ensure the source URI has a trailing slash for proper RFC 2396 path merging
# Without a trailing slash, the last path segment is treated as a file and removed
# during relative path resolution (e.g., "/blish" + "gems/foo.gem" = "/gems/foo.gem")
# With a trailing slash, it's treated as a directory (e.g., "/blish/" + "gems/foo.gem" = "/blish/gems/foo.gem")
uri = Gem::URI.parse(source_uri)
uri.path = uri.path.gsub(%r{/+\z}, "") + "/" if uri.path && !uri.path.empty?
uri.to_s
rescue Gem::URI::Error
# If parsing fails, return the original URI and let later validation handle it
source_uri
end

def check_rubygems_https(source_uri) # :nodoc:
uri = Gem::URI source_uri

Expand Down Expand Up @@ -273,7 +277,8 @@ def execute
end

def remove_source(source_uri) # :nodoc:
source = Gem::Source.new source_uri
source = build_source(source_uri)
source_uri = source.uri.to_s

if configured_sources&.include? source
Gem.sources.delete source
Expand Down Expand Up @@ -328,4 +333,16 @@ def configured_sources
def config_file_name
Gem.configuration.config_file_name
end

def build_source(source_uri)
source_uri = normalize_source_uri(source_uri)
Gem::Source.new(source_uri)
end

def build_new_source(source_uri)
source = build_source(source_uri)
check_rubygems_https(source.uri.to_s)
check_typo_squatting(source)
source
end
end
148 changes: 131 additions & 17 deletions test/rubygems/test_gem_commands_sources_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,82 @@ def test_execute_add
assert_equal "", @ui.error
end

def test_execute_add_without_trailing_slash
setup_fake_source("https://rubygems.pkg.github.com/my-org")

@cmd.handle_options %W[--add https://rubygems.pkg.github.com/my-org]

use_ui @ui do
@cmd.execute
end

assert_equal [@gem_repo, "https://rubygems.pkg.github.com/my-org/"], Gem.sources

expected = <<-EOF
https://rubygems.pkg.github.com/my-org/ added to sources
EOF

assert_equal expected, @ui.output
assert_equal "", @ui.error
end

def test_execute_add_multiple_trailing_slash
setup_fake_source("https://rubygems.pkg.github.com/my-org/")

@cmd.handle_options %W[--add https://rubygems.pkg.github.com/my-org///]

use_ui @ui do
@cmd.execute
end

assert_equal [@gem_repo, "https://rubygems.pkg.github.com/my-org/"], Gem.sources

expected = <<-EOF
https://rubygems.pkg.github.com/my-org/ added to sources
EOF

assert_equal expected, @ui.output
assert_equal "", @ui.error
end

def test_execute_append_without_trailing_slash
setup_fake_source("https://rubygems.pkg.github.com/my-org")

@cmd.handle_options %W[--append https://rubygems.pkg.github.com/my-org]

use_ui @ui do
@cmd.execute
end

assert_equal [@gem_repo, "https://rubygems.pkg.github.com/my-org/"], Gem.sources

expected = <<-EOF
https://rubygems.pkg.github.com/my-org/ added to sources
EOF

assert_equal expected, @ui.output
assert_equal "", @ui.error
end

def test_execute_prepend_without_trailing_slash
setup_fake_source("https://rubygems.pkg.github.com/my-org")

@cmd.handle_options %W[--prepend https://rubygems.pkg.github.com/my-org]

use_ui @ui do
@cmd.execute
end

assert_equal ["https://rubygems.pkg.github.com/my-org/", @gem_repo], Gem.sources

expected = <<-EOF
https://rubygems.pkg.github.com/my-org/ added to sources
EOF

assert_equal expected, @ui.output
assert_equal "", @ui.error
end

def test_execute_append
setup_fake_source(@new_repo)

Expand Down Expand Up @@ -530,17 +606,14 @@ def test_execute_add_https_rubygems_org

@cmd.handle_options %W[--add #{https_rubygems_org}]

ui = Gem::MockGemUi.new "n"

use_ui ui do
assert_raise Gem::MockGemUi::TermError do
@cmd.execute
end
use_ui @ui do
@cmd.execute
end
Comment on lines -533 to 611
Copy link
Member

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 the existing passed with Gem::MockGemUi.new "n"...
It seems that https://rubygems.org/ passes both of check_typo_squatting and check_rubygems_https...


assert_equal [@gem_repo], Gem.sources
assert_equal [@gem_repo, https_rubygems_org], Gem.sources

expected = <<-EXPECTED
#{https_rubygems_org} added to sources
EXPECTED

assert_equal expected, @ui.output
Expand All @@ -554,17 +627,14 @@ def test_execute_append_https_rubygems_org

@cmd.handle_options %W[--append #{https_rubygems_org}]

ui = Gem::MockGemUi.new "n"

use_ui ui do
assert_raise Gem::MockGemUi::TermError do
@cmd.execute
end
use_ui @ui do
@cmd.execute
end

assert_equal [@gem_repo], Gem.sources
assert_equal [@gem_repo, https_rubygems_org], Gem.sources

expected = <<-EXPECTED
#{https_rubygems_org} added to sources
EXPECTED

assert_equal expected, @ui.output
Expand All @@ -583,7 +653,7 @@ def test_execute_add_bad_uri
assert_equal [@gem_repo], Gem.sources

expected = <<-EOF
beta-gems.example.com is not a URI
beta-gems.example.com/ is not a URI
EOF

assert_equal expected, @ui.output
Expand All @@ -602,7 +672,26 @@ def test_execute_append_bad_uri
assert_equal [@gem_repo], Gem.sources

expected = <<-EOF
beta-gems.example.com is not a URI
beta-gems.example.com/ is not a URI
EOF

assert_equal expected, @ui.output
assert_equal "", @ui.error
end

def test_execute_prepend_bad_uri
@cmd.handle_options %w[--prepend beta-gems.example.com]

use_ui @ui do
assert_raise Gem::MockGemUi::TermError do
@cmd.execute
end
end

assert_equal [@gem_repo], Gem.sources

expected = <<-EOF
beta-gems.example.com/ is not a URI
EOF

assert_equal expected, @ui.output
Expand Down Expand Up @@ -778,6 +867,31 @@ def test_execute_remove_redundant_source_trailing_slash
Gem.configuration.sources = nil
end

def test_execute_remove_without_trailing_slash
source_uri = "https://rubygems.pkg.github.com/my-org/"

Gem.configuration.sources = [source_uri]

setup_fake_source(source_uri)

@cmd.handle_options %W[--remove https://rubygems.pkg.github.com/my-org]

use_ui @ui do
@cmd.execute
end

assert_equal [], Gem.sources

expected = <<-EOF
#{source_uri} removed from sources
EOF

assert_equal expected, @ui.output
assert_equal "", @ui.error
ensure
Gem.configuration.sources = nil
end

def test_execute_update
@cmd.handle_options %w[--update]

Expand Down Expand Up @@ -888,6 +1002,6 @@ def setup_fake_source(uri)
Marshal.dump specs, io
end

@fetcher.data["#{uri}/specs.#{@marshal_version}.gz"] = specs_dump_gz.string
@fetcher.data["#{uri.chomp("/")}/specs.#{@marshal_version}.gz"] = specs_dump_gz.string
Copy link
Member

Choose a reason for hiding this comment

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

Can we revert this change if we always use URI that has a trailing slash for setup_fake_source?
We don't need accept non trailing slash URI in setup_fake_source, right?

Copy link
Member

Choose a reason for hiding this comment

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

Without chomp, passing "https://example.com/path/" produces "https://example.com/path//specs...".

It's difficult to check for the presence or absence of slashes in every call to the helper, so I think it's reasonable to remove them here.

end
end
Loading