Replace HTTP.download #273

oxinabox · 2018-07-06T15:24:14Z

rather than deleting it as in #271
this replace it with a new and better version.

It is very verbose, but I think the 0.7 way of handling that is to tell your logger to be quieter.
Also to tell your logger to display a progress bar instead, that should be possible with the new system.

Locally this was working when I had it as a separate package but it is screwing up now that I've merge it in and I'm at a lose as to why.

ERROR: DNSError: , unknown node or service (EAI_NONAME)

oxinabox · 2018-07-08T05:33:45Z

Fixed it, I think
Needed to be using Base.open

oxinabox · 2018-07-08T10:03:54Z

So this is failing on 0.6, because of usign the new @info.
with lots of kwargs.
I think using @info that way is good, because it will play nice with a Logger that implements progress bars.

codecov-io · 2018-07-08T14:42:52Z

Codecov Report

Merging #273 into master will increase coverage by 19.17%.
The diff coverage is 95.91%.

@@             Coverage Diff             @@
##           master     #273       +/-   ##
===========================================
+ Coverage   71.84%   91.01%   +19.17%     
===========================================
  Files          35       30        -5     
  Lines        1964     1258      -706     
===========================================
- Hits         1411     1145      -266     
+ Misses        553      113      -440

Impacted Files	Coverage Δ
src/compat.jl	`80% <ø> (+1.05%)`	⬆️
src/HTTP.jl	`100% <ø> (+22.72%)`	⬆️
src/client.jl	`61.36% <ø> (+5.97%)`	⬆️
src/download.jl	`95.91% <95.91%> (ø)`
src/TimeoutRequest.jl	`50% <0%> (-20%)`	⬇️
src/StreamRequest.jl	`91.17% <0%> (-2.95%)`	⬇️
src/debug.jl	`20% <0%> (-1.43%)`	⬇️
src/Parsers.jl	`96.15% <0%> (-0.57%)`	⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5630bf4...a6b4dba. Read the comment docs.

oxinabox · 2018-07-08T14:56:25Z

Demo of use (with OhMyLog.jl)

samoconnor · 2018-07-10T05:12:31Z

Regarding 0.6 compatibility. I would just hold of merging this until we update REQUIRE to say julia 0..7.

See comment here: #264 (comment)

"I would say that at this point, with 0.7 beta out, feature addition PRs for HTTP.jl could(should?) be 0.7-only. i.e. we should put julia 0.7 in REQUIRE soon anyway."

oxinabox · 2018-07-10T06:49:46Z

Fair, I am curious what it would take for 0.6 compat so I just made a commit (also I want to retrigger 0.7 CI which is being flaky).

oxinabox · 2018-07-10T11:42:31Z

While I remember: this should take a headers argument.
because that is required to work with OATH.

quinnj · 2018-07-10T12:25:15Z

Very nice demo; It'd be nice if the numbers could be pretty-printed so they were easier to glance at and grok. W/ regards to headers, it'd be nice if this basically matched some of the convenience methods like HTTP.get/HTTP.post in terms of what you can pass it (headers, body, etc).

Otherwise, I don't have a super strong preference for 0.6 vs. 0.7; if it works on 0.6, I'm happy to merge. I think we'll do one more release supporting both and then will go 0.7 only.

oxinabox · 2018-07-11T03:41:53Z

Ahhhh,
this has a Directory Traversal vulnerability I think.
Or at least needs to be checked carefully for one.

Consider that it lets the file set its own filepath.
And that

julia> joinpath("alocaldir", "/etc/passwd")
"/etc/passwd"

So I think we need to blacklist any filepath that starts with a /, or contains any ..
and probably some other things.
Does anyone know of a set of blacklist rules for this?

samoconnor · 2018-07-11T08:43:20Z

As a double-check, how about adding an assertion that the resulting path is actually under the download directory.

path = joinpath(download_dir, file_path)
@assert startswith(normpath(path), normpath(download_dir))

oxinabox · 2018-07-11T08:55:23Z

Ah, I didn't realize that normpath existed.
I was thinking about doing this using realpath,
but that has a variety of problems since symlinks make it not nesc true.

On the other hand, normpath literally just looked for .. and removes them.
Given we have banned .. outright (because you never want this in a downloaded file)
idk, it would be an assertion, and it isn't like it would take long per check.

EricForgy · 2018-07-11T11:13:23Z

I'm a little confused. I would expect something like HTTP.download to just download a specified file to some specified location.

oxinabox · 2018-07-11T12:57:29Z

I would expect something like HTTP.download to just download a specified file to some specified location.

It does. But the question is what does a specific location mean?
HTTP.download should work a lot like clicking download in a browser.
Only instead of a dialog appearing, it uses the second argument to give the specific location

if a specific location is path to a file* then yes, send it there. Easy Done.
- (* or technicall to a nonexistant location, which this PR currently assumes to be a file)
if a specific location is a directory, then we need to workout the filename to put in that directory.
- We need to get the filename right, because
- it is part of the file being downloaded -- intent of the file creator
- FileIO.jl (etc) need the file extension to know how to open it.
- If it is coming from a very simple HTTP server, then the file name is generally just the last part of the URL
- But HTTP lets it be specified in the header.
- A surfiently evil header on a compromised server might say that the filename of this is say ../../.shh/accept/evilkey.pub, and then break into your system

EricForgy · 2018-07-11T13:47:15Z

Why allow the case where the filename is located in the header? It would be reasonable, I think, to reject the download if the filename is not in the url :thinking_face:

oxinabox · 2018-07-11T13:54:18Z

There are a lot of files I want to download (millions)
That do not have it in the URL.
Anything on any modern filehosting service like Dropbox or google drive, anything on data.gov etc etc
I want to download them and I want their original filenames

KristofferC · 2018-08-01T13:09:18Z

Should this also accept headers?

oxinabox · 2018-08-01T13:21:58Z

Yes, it should.
And it will before being merged.

See also #273 (comment)

quinnj · 2018-08-04T03:13:07Z

@oxinabox, what's the status here? ready to review? still things you want to cleanup? just let me know. I'm going to target one final 0.6 release in the next few days and then we can rip out all the 0.6 compat stuff.

oxinabox · 2018-08-04T10:24:39Z

I want to add everything you suggested in
#273 (comment)
And I was kinda not rushing on it since can wait for 0.7

It won't take much to finish that off

oxinabox · 2018-08-07T08:29:21Z

Right, added headers and kwargs.
I didn't add body, because that doesn't seem reasonable.
I could be wrong, maybe someone uses it for auth.
But a GET request with a body; particularly for a download doesn't make sense to me.

I've not added human units, as I am of many minds and bikesheds over that.
So it seems better to add that later in another PR.

So this is ready for review, I hope.

The compat stuff can be stripped out, though I am not sure the best way to do that right now.
Can I just delete the compat.jl file?

oxinabox · 2018-09-13T07:45:45Z

Ok, this is all rebased for 0.7/1.0
~~and working~~
and working in 0.7

oxinabox · 2018-09-13T12:02:17Z

Assuming windows passes, I am done with this now.

src/download.jl

oxinabox · 2018-09-15T14:52:36Z

Bump

oxinabox · 2018-09-17T14:14:45Z

New video, with OhMyLog.

oxinabox · 2018-09-17T14:18:41Z

src/download.jl

+ - any additional keyword args (`kw...`) are passed on to the HTTP request.
+"""
+function download(url::AbstractString, local_path=nothing, headers=Header[]; update_period=1, kw...)
+    format_progress(x) = "$(round(100x, digits=2))%"


Possibly this should not be formatted.
idk what Juno is doing with progress.
OhMyLog doesn't mind if it is given a string or a float.
But something else doing progress bars might like it to be a float.

quinnj

This looks great @oxinabox ! Thanks for doing this.

oxinabox · 2018-09-17T23:59:07Z

Can we have a release tagged?
I want to Switch over to using this in my packages

oxinabox closed this Jul 11, 2018

oxinabox reopened this Jul 11, 2018

oxinabox mentioned this pull request Jul 11, 2018

Make chosing filename the job of the fetch_method oxinabox/DataDeps.jl#54

Merged

samoconnor mentioned this pull request Jul 27, 2018

Provide a download function #275

Closed

quinnj mentioned this pull request Sep 7, 2018

delete deadcode method #271

Closed

oxinabox added 5 commits September 13, 2018 14:24

=New download

d4db242

attach

101db9e

use Base.open

bb3b8bf

Fix urls for download test

5248e6b

Defend againt Directory Traversal Attacks

4896e40

add extra headers args

32bd298

oxinabox force-pushed the master branch from a6b4dba to 32bd298 Compare September 13, 2018 06:35

oxinabox added 2 commits September 13, 2018 15:19

=fix typos

94fac6f

=make work

d9535be

oxinabox added 3 commits September 13, 2018 16:11

=add pretty printing and fix 1.0

8fd807b

=disable quotes test on windows

bae91f1

=prettier printing

f4ab4d4

KristofferC reviewed Sep 13, 2018

View reviewed changes

src/download.jl Outdated Show resolved Hide resolved

=doc typos

7f8d0d4

oxinabox commented Sep 17, 2018

View reviewed changes

quinnj approved these changes Sep 17, 2018

View reviewed changes

quinnj merged commit 51fe562 into JuliaWeb:master Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace HTTP.download #273

Replace HTTP.download #273

oxinabox commented Jul 6, 2018

oxinabox commented Jul 8, 2018

oxinabox commented Jul 8, 2018

codecov-io commented Jul 8, 2018 •

edited

Loading

oxinabox commented Jul 8, 2018

samoconnor commented Jul 10, 2018

oxinabox commented Jul 10, 2018 •

edited

Loading

oxinabox commented Jul 10, 2018

quinnj commented Jul 10, 2018

oxinabox commented Jul 11, 2018

samoconnor commented Jul 11, 2018

oxinabox commented Jul 11, 2018

EricForgy commented Jul 11, 2018

oxinabox commented Jul 11, 2018 •

edited

Loading

EricForgy commented Jul 11, 2018

oxinabox commented Jul 11, 2018 •

edited

Loading

KristofferC commented Aug 1, 2018

oxinabox commented Aug 1, 2018

quinnj commented Aug 4, 2018

oxinabox commented Aug 4, 2018

oxinabox commented Aug 7, 2018

oxinabox commented Sep 13, 2018 •

edited

Loading

oxinabox commented Sep 13, 2018

oxinabox commented Sep 15, 2018

oxinabox commented Sep 17, 2018 •

edited

Loading

oxinabox Sep 17, 2018 •

edited

Loading

quinnj left a comment

oxinabox commented Sep 17, 2018

Replace HTTP.download #273

Replace HTTP.download #273

Conversation

oxinabox commented Jul 6, 2018

oxinabox commented Jul 8, 2018

oxinabox commented Jul 8, 2018

codecov-io commented Jul 8, 2018 • edited Loading

Codecov Report

oxinabox commented Jul 8, 2018

samoconnor commented Jul 10, 2018

oxinabox commented Jul 10, 2018 • edited Loading

oxinabox commented Jul 10, 2018

quinnj commented Jul 10, 2018

oxinabox commented Jul 11, 2018

samoconnor commented Jul 11, 2018

oxinabox commented Jul 11, 2018

EricForgy commented Jul 11, 2018

oxinabox commented Jul 11, 2018 • edited Loading

EricForgy commented Jul 11, 2018

oxinabox commented Jul 11, 2018 • edited Loading

KristofferC commented Aug 1, 2018

oxinabox commented Aug 1, 2018

quinnj commented Aug 4, 2018

oxinabox commented Aug 4, 2018

oxinabox commented Aug 7, 2018

oxinabox commented Sep 13, 2018 • edited Loading

oxinabox commented Sep 13, 2018

oxinabox commented Sep 15, 2018

oxinabox commented Sep 17, 2018 • edited Loading

oxinabox Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

quinnj left a comment

Choose a reason for hiding this comment

oxinabox commented Sep 17, 2018

codecov-io commented Jul 8, 2018 •

edited

Loading

oxinabox commented Jul 10, 2018 •

edited

Loading

oxinabox commented Jul 11, 2018 •

edited

Loading

oxinabox commented Jul 11, 2018 •

edited

Loading

oxinabox commented Sep 13, 2018 •

edited

Loading

oxinabox commented Sep 17, 2018 •

edited

Loading

oxinabox Sep 17, 2018 •

edited

Loading