Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

improvement: include response body on fetch_http error #7148

Merged
merged 3 commits into from
Nov 14, 2023
Merged

improvement: include response body on fetch_http error #7148

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de4189a
improvement: include response body on fetch_http error
Paul-Bob Nov 9, 2023
75d3d61
Merge branch 'master' into improvement/include_response_body_on_fetch…
Paul-Bob Nov 10, 2023
0bcb46a
Merge branch 'master' into improvement/include_response_body_on_fetch…
duckinator Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rubygems/remote_fetcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ def fetch_http(uri, last_modified = nil, head = false, depth = 0)

fetch_http(location, last_modified, head, depth + 1)
else
raise FetchError.new("bad response #{response.message} #{response.code}", uri)
raise FetchError.new("bad response #{response.message} #{response.code} #{response.body.strip}", uri)
Copy link
Member

Choose a reason for hiding this comment

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

This can be suuuuuuuuuuuuper messy IMHO when remote provides like 404 HTML page. Would it make sense to limit this to first X characters at least or scan for friendly response and report back only when recognized format is returned? What's returned by RubyGems.org in here?

Copy link
Member

Choose a reason for hiding this comment

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

Referencing a gem that doesn't exist doesn't result in this message being displayed.

I'm not opposed to adding a character limit, though.

puppy@orthrus:~/test$ ls -a
./       ../      Gemfile
puppy@orthrus:~/test$ cat Gemfile
source 'https://rubygems.org'

gem 'asdfasdfsadfihgjklsadkjfhakljfhsaf-this-gem-does-not-exist'
puppy@orthrus:~/test$ bundle install
Fetching gem metadata from https://rubygems.org/.
Could not find gem 'asdfasdfsadfihgjklsadkjfhakljfhsaf-this-gem-does-not-exist' in rubygems repository
https://rubygems.org/ or installed locally.
puppy@orthrus:~/test$

Copy link
Member

Choose a reason for hiding this comment

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

There are other gem servers which can return super long 500 HTML page for example. I would suggest to revert this and instead build some kind of "protocol" to let the error message propagate to client. IMHO it is not good idea to propagate licensing error back to clients using HTTP message on gem install. There is no guarantee how RubyGems handles HTTP errors and IMHO it should not be mis-used to handing over messages to client.

Copy link
Member

Choose a reason for hiding this comment

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

I would be happy to keep this output, but only in kind of "verbose" mode and that is most likely against the original need of @adrianthedev.

Copy link
Member

Choose a reason for hiding this comment

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

We have also sometimes needed this in Bundler, for example when rate-limiting certain requests. I like the idea of this being officially supported, but I agree that an unlimited message size is messy and unhelpful.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, I misunderstood! I thought you were saying only responses not 4xx or 5xx. 🙃 Sounds good to me.

Copy link
Member

Choose a reason for hiding this comment

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

@adrianthedev would that work for you? ^ Are you in control of returned HTTP headers for 403 on your gemserver?

Copy link

Choose a reason for hiding this comment

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

Yes @simi, we are in control of the returned HTTP headers. Our gem server is operated by a sinatra app.

Not completely sure how to send the message.
Is it something like this?

halt 403, {'Content-Type' => 'application/vnd.rubygems.text'}, { message: 'some text' }.to_json

Our message is something like this. That's 164 characters. We might play around with it so the number of chars might increase.

It seems that you don't have a valid GEM_SERVER_TOKEN for Avo (https://bundler.dev/). Please read this for more information
https://docs.avohq.io/3.0/some-path-here

Copy link
Member

Choose a reason for hiding this comment

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

Did I understand correctly that we'd rather revert this PR and go with the cleaner solution that @simi proposed?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if possible, don't release this. 🙏

end
end

Expand Down