Use Net:HTTP instead of Typhoeus #26
Conversation
| request.run | ||
| io = URI(zip_url).open(OPTIONS) | ||
| IO.copy_stream io, zip_file | ||
| OpenURI::Meta.init zip_file, io |
There was a problem hiding this comment.
Hm, I couldn't find this method in Dash from the ruby stdlib, and rubydoc.info didn't have any docs for the method I did find. What does this do?
There was a problem hiding this comment.
Had the same question... https://apidock.com/ruby/OpenURI/Meta/init/class
|
@ptoomey3, @mastahyeti I know open uri has a bad history, in terms of security. Wondering if you have a minute to share any thoughts one way or the other with using it in a Jekyll plugin? We build the URL ourselves, normalize it with Addressable::URI, and sanitize the repository name + owner with Edit: Also to note we're calling |
|
It seems like it would be better to just use a purpose built HTTP client than to use something with a bunch of "extra features". If you're concerned with platform compatibility issues, you could maybe use Faraday, which would allow you to select different adapters based on the platform. |
|
@mastahyeti thanks for the 👀, as always. Given that we're making a single, predictable call to download a (potentially large) file to a known location, I was able to implement everything using native @parkr mind taking another look? (tests are passing). |
benbalter
changed the title
|
|
||
| def raise_unless_sucess(response) | ||
| return if response.is_a?(Net::HTTPSuccess) | ||
| raise DownloadError, "#{response.code} - #{response.message}" |
There was a problem hiding this comment.
Would it be helpful to include the URL that failed to return a helpful response?
There was a problem hiding this comment.
It would theoretically be in the log output immediately above.
| :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) |
There was a problem hiding this comment.
Do you have any thoughts on whether this should be configurable? Maybe my host’s disks aren’t quite so large?
There was a problem hiding this comment.
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.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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This PR moves the project to use
OpenURINet:HTTP to download themes, rather than Typhoeus. Typhoeus is a wrapper for Libcurl, which has Windows compatibility issues (#9, #18).OpenURINet:HTTP is native Ruby, and thus shouldn't suffer from the same problems.This implementation is largely based on https://twin.github.io/improving-open-uri/ and to a lesser extent https://gist.github.com/janko-m/7cd94b8b4dd113c2c193. I looked at using @janko-m's Down which would provide a nice abstraction layer, but didn't want to add a dependency on a project with only 3 contributors (although it looks to be a great project).For the most part, this approach appears to be a drop-in replacement, with tests passing with only minor modifications.
@parkr would you be able to review this?