Validate spec name before writing to the spec cache - #9690
Merged
Conversation
Gem::Source#fetch_spec built the local spec cache path directly from the name tuple returned by a remote index, which is never validated for use as a path component. A crafted gem name containing path separators or `..` could therefore make fetch_spec write the downloaded gemspec bytes outside Gem.spec_cache_dir. Reject any spec name that is not a plain basename before constructing the cache path. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Gem::Source#fetch_spec against path traversal by validating remote index-derived spec names before constructing on-disk cache paths, preventing writes outside Gem.spec_cache_dir.
Changes:
- Add a basename validation guard in
Gem::Source#fetch_specto reject malformed spec names before cache path construction. - Add a regression test covering path traversal via crafted gem names in remote index tuples.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/rubygems/source.rb | Adds spec-name path validation prior to building the spec cache file path. |
| test/rubygems/test_gem_source.rb | Adds a regression test intended to ensure no spec cache write can escape the cache directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+124
to
+135
| def test_fetch_spec_path_traversal | ||
| escape = File.expand_path(File.join(Gem.spec_cache_dir, "..", "owned.gemspec")) | ||
|
|
||
| name_tuple = tuple("../owned", Gem::Version.new(1), "ruby") | ||
|
|
||
| e = assert_raise Gem::Exception do | ||
| @source.fetch_spec name_tuple | ||
| end | ||
|
|
||
| assert_includes e.message, "malformed spec name" | ||
| refute File.exist?(escape), "spec must not be written outside the spec cache" | ||
| end |
Comment on lines
+112
to
+117
| # The name tuple comes from a remote index and is not otherwise | ||
| # validated, so refuse anything that would escape the spec cache | ||
| # directory when used as a path component. | ||
| if File.basename(spec_file_name) != spec_file_name | ||
| raise Gem::Exception, "malformed spec name: #{spec_file_name.inspect}" | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gem::Source#fetch_spec builds the local spec cache path directly from the name tuple returned by a remote index. That name is never validated for use as a path component, so a name containing path separators or
..makes fetch_spec write the downloaded gemspec bytes outsideGem.spec_cache_dir, before the Marshal payload is ever loaded.This adds a basename check at the single write site: any spec name that is not a plain basename is rejected before the cache path is constructed. Guarding here covers every crafted component (name, version, platform) at once, rather than filtering each index parser separately.