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.

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

Use Net:HTTP instead of Typhoeus #26

Merged
merged 10 commits into from
May 14, 2018
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
2 changes: 1 addition & 1 deletion jekyll-remote-theme.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ Gem::Specification.new do |s|

s.add_dependency "jekyll", "~> 3.5"
s.add_dependency "rubyzip", ">= 1.2.1", "< 3.0"
s.add_dependency "typhoeus", ">= 0.7", "< 2.0"
s.add_development_dependency "jekyll-theme-primer", "~> 0.5"
s.add_development_dependency "jekyll_test_plugin_malicious", "~> 0.2"
s.add_development_dependency "pry", "~> 0.11"
s.add_development_dependency "rspec", "~> 3.0"
s.add_development_dependency "rubocop", "~> 0.4", ">= 0.49.0"
s.add_development_dependency "webmock", "~> 3.0"
end
4 changes: 1 addition & 3 deletions lib/jekyll-remote-theme.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
require "fileutils"
require "tempfile"
require "addressable"
require "typhoeus"
require "net/http"
require "zip"

$LOAD_PATH.unshift(File.dirname(__FILE__))
Expand Down Expand Up @@ -32,5 +32,3 @@ def self.init(site)
Jekyll::Hooks.register :site, :after_reset do |site|
Jekyll::RemoteTheme.init(site)
end

Ethon.logger = Jekyll.logger.writer
66 changes: 39 additions & 27 deletions lib/jekyll-remote-theme/downloader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ module RemoteTheme
class Downloader
HOST = "https://codeload.github.com".freeze
PROJECT_URL = "https://github.com/benbalter/jekyll-remote-theme".freeze
TYPHOEUS_OPTIONS = {
:headers => {
:user_agent => "Jekyll Remote Theme/#{VERSION} (+#{PROJECT_URL})",
},
:verbose => (Jekyll.logger.level == :debug),
}.freeze
USER_AGENT = "Jekyll Remote Theme/#{VERSION} (+#{PROJECT_URL})".freeze
MAX_FILE_SIZE = 1 * (1024 * 1024 * 1024) # Size in bytes (1 GB)
Copy link

Choose a reason for hiding this comment

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

Do you have any thoughts on whether this should be configurable? Maybe my host’s disks aren’t quite so large?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Assume you're talking about the max file size? I put it in largely as a guard against abuse. Not sure how a malicious user might create a never-ending zip since we control the source server, but figured it'd be better to be safe than sorry.

NET_HTTP_ERRORS = [
Timeout::Error, Errno::EINVAL, Errno::ECONNRESET, EOFError,
Net::HTTPBadResponse, Net::HTTPHeaderSyntaxError, Net::ProtocolError,
].freeze

attr_reader :theme
private :theme
Expand All @@ -27,8 +27,6 @@ def run

download
unzip

@downloaded = true
end

def downloaded?
Expand All @@ -38,16 +36,40 @@ def downloaded?
private

def zip_file
@zip_file ||= Tempfile.new([TEMP_PREFIX, ".zip"])
@zip_file ||= Tempfile.new([TEMP_PREFIX, ".zip"], :binmode => true)
end

def download
Jekyll.logger.debug LOG_KEY, "Downloading #{zip_url} to #{zip_file.path}"
request = Typhoeus::Request.new zip_url, TYPHOEUS_OPTIONS
request.on_headers { |response| raise_if_unsuccessful(response) }
request.on_body { |chunk| zip_file.write(chunk) }
request.on_complete { |response| raise_if_unsuccessful(response) }
request.run
Net::HTTP.start(zip_url.host, zip_url.port, :use_ssl => true) do |http|
http.request(request) do |response|
raise_unless_sucess(response)
enforce_max_file_size(response.content_length)
Copy link

Choose a reason for hiding this comment

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

The Content-Length header won't be set for chunked responses, right? Seems like you may also want to enforce your size limits by summing up chunk sizes.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I looked into this and there is not a clean way to do this with our clean implementation. We don't have any limit on master, so what's currently implemented should be some protection, and we can look into adding better chunk support, if necessary, in a subsequent pass.

response.read_body do |chunk|
zip_file.write chunk
end
end
end
@downloaded = true
rescue *NET_HTTP_ERRORS => e
raise DownloadError, e.message
end

def request
return @request if defined? @request
@request = Net::HTTP::Get.new zip_url.request_uri
@request["User-Agent"] = USER_AGENT
@request
end

def raise_unless_sucess(response)
return if response.is_a?(Net::HTTPSuccess)
raise DownloadError, "#{response.code} - #{response.message}"
Copy link

Choose a reason for hiding this comment

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

Would it be helpful to include the URL that failed to return a helpful response?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It would theoretically be in the log output immediately above.

end

def enforce_max_file_size(size)
return unless size && size > MAX_FILE_SIZE
raise DownloadError, "Maximum file size of #{MAX_FILE_SIZE} bytes exceeded"
end

def unzip
Expand All @@ -59,13 +81,14 @@ def unzip
Zip::File.open(zip_file) do |archive|
archive.each { |file| file.extract path_without_name_and_ref(file.name) }
end

ensure
zip_file.close
zip_file.unlink
end

# Full URL to codeload zip download endpoint for the given theme
def zip_url
Addressable::URI.join(
@zip_url ||= Addressable::URI.join(
HOST, "#{theme.owner}/", "#{theme.name}/", "zip/", theme.git_ref
).normalize
end
Expand All @@ -78,17 +101,6 @@ def theme_dir_empty?
Dir["#{theme.root}/*"].empty?
end

def raise_if_unsuccessful(response)
if response.timed_out?
raise DownloadError, "Request timed out"
elsif response.code.zero?
raise DownloadError, response.return_message
elsif response.code != 200
msg = "Request failed with #{response.code} - #{response.status_message}"
raise DownloadError, msg
end
end

# Codeload generated zip files contain a top level folder in the form of
# THEME_NAME-GIT_REF/. While requests for Git repos are case insensitive,
# the zip subfolder will respect the case in the repository's name, thus
Expand Down
38 changes: 32 additions & 6 deletions spec/jekyll-remote-theme/downloader_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,39 @@
end
end

context "with an invalid URL" do
let(:zip_url) { "https://codeload.github.com/benbalter/_invalid_/zip/master" }
before { allow(subject).to receive(:zip_url) { zip_url } }
context "with zip_url stubbed" do
before { allow(subject).to receive(:zip_url) { Addressable::URI.parse zip_url } }

it "raises a DownloadError" do
msg = "Request failed with 404 - Not Found"
expect { subject.run }.to raise_error(Jekyll::RemoteTheme::DownloadError, msg)
context "with an invalid URL" do
let(:zip_url) { "https://codeload.github.com/benbalter/_invalid_/zip/master" }
before do
WebMock.disable_net_connect!
stub_request(:get, zip_url).to_return(:status => [404, "Not Found"])
end

after { WebMock.allow_net_connect! }

it "raises a DownloadError" do
msg = "404 - Not Found"
expect { subject.run }.to raise_error(Jekyll::RemoteTheme::DownloadError, msg)
end
end

context "with a large file" do
let(:zip_url) { "https://codeload.github.com/benbalter/_invalid_/zip/master" }
let(:content_length) { 10 * 1024 * 1024 * 1024 }
let(:headers) { { "Content-Length" => content_length } }
before do
WebMock.disable_net_connect!
stub_request(:get, zip_url).to_return(:headers => headers)
end

after { WebMock.allow_net_connect! }

it "raises a DownloadError" do
msg = "Maximum file size of 1073741824 bytes exceeded"
expect { subject.run }.to raise_error(Jekyll::RemoteTheme::DownloadError, msg)
end
end
end
end
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
require "fileutils"
require "open3"
require "pathname"
require "webmock/rspec"
WebMock.allow_net_connect!

RSpec.configure do |config|
config.example_status_persistence_file_path = "spec/examples.txt"
Expand Down