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

Try to use .netrc if available and enable session cookies by default #98

Merged
merged 7 commits into from
Apr 14, 2021

Conversation

evetion
Copy link
Member

@evetion evetion commented Feb 14, 2021

Regarding cookies, see https://curl.se/libcurl/c/CURLOPT_COOKIEFILE.html:

Given an empty or non-existing file or by passing the empty string ("") to this option, you can enable the cookie engine without reading any initial cookies.

Regarding netrc, see https://curl.se/libcurl/c/CURLOPT_NETRC.html:

The use of the ~/.netrc file is optional, and information in the URL is to be preferred. The file will be scanned for the host and user name (to find the password only) or for the host only, to find the first user name and password after that machine, which ever information is not specified.

evetion and others added 2 commits February 14, 2021 20:29
See https://curl.se/libcurl/c/CURLOPT_NETRC.html for a description.

If this is not wanted as default, I will make an issue to enable .netrc in the `download` function, but making this PR was just as fast.
@evetion evetion changed the title Try to use .netrc if available Try to use .netrc if available and enable session cookies by default Feb 26, 2021
@StefanKarpinski
Copy link
Member

Can you add tests that these options actually work? I'm wondering whether we should enable this by default or allow the user to opt into these features.

@StefanKarpinski
Copy link
Member

Bump. This seems like a good change but needs tests.

@evetion
Copy link
Member Author

evetion commented Apr 13, 2021

I'll add tests, still working on the cookie one.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Apr 13, 2021

Thinking about this, I think we should just do this by default and I'm not even convinced we need an option to turn it off. If someone has bothered to create a .netrc file, presumably they'd want to honor it.

Similarly, if a website sets a cookie during a request, we should serve them back their cookie — we don't have the privacy concerns that browsers do because we would only serve cookies back during the same request, i.e. when there's a redirect from one location to another one.


# This url will redirect to /cookies, which echoes the set cookies as json
set_cookie_url = "$server/cookies/set?k1=v1&k2=v2"
cookies = download_json(set_cookie_url, downloader=downloader)
Copy link
Member Author

@evetion evetion Apr 13, 2021

Choose a reason for hiding this comment

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

I'd like to have a control test for this, but it seems there's no way to actually disable the cookie engine once set.

Copy link
Member

Choose a reason for hiding this comment

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

What about setopt(easy, CURLOPT_COOKIEFILE, C_NULL)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't seem to work, this fails:

# Setup control (disable cookie engine)
downloader = Downloader()
easy_hook = (easy, info) -> begin
    Downloads.Curl.setopt(
        easy,
        Downloads.Curl.CURLOPT_COOKIEFILE, C_NULL)
end
downloader.easy_hook = easy_hook
cookies = download_json(set_cookie_url, downloader=downloader)
@test isempty(cookies)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. That's a shame. I guess we don't strictly need the control. I may file an issue with libcurl.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, that's quite productive. Do you want to wait for a fix (and subsequent release?).

Copy link
Member

Choose a reason for hiding this comment

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

No, we should proceed without it since it's a bunch of effort to update the library version (new BinaryBuilder release, bumping version in Julia's makefiles, checksum updates — it's a whole process).

@StefanKarpinski
Copy link
Member

I've tried to see if httpbingo has a way to set a cookie and redirect to another domain to see if cross-domain cookie leaks are possible, but I couldn't figure out a way to test that short of writing our own HTTP end point. We don't necessarily need that in the test suite, but I did kind of want to test it to make sure that we can't accidentally leak auth cookies in the case of a redirect. I'm guessing libcurl is too smart to do that, but I wanted to see for myself before turning this feature on for everyone.

@StefanKarpinski
Copy link
Member

Nice! Tests passing.

test/runtests.jl Outdated

# Setup .netrc
servername = split(server, "/")[end] # strip https://
open(".netrc", "w") do io
Copy link
Member

Choose a reason for hiding this comment

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

This should be a temporary file, shouldn't create files in the current directory.

test/runtests.jl Outdated Show resolved Hide resolved
@test resp.status == 200 # succesful authentication

# Cleanup
rm(netrc) # isn't cleaned automatically on Julia 1.3
Copy link
Member

Choose a reason for hiding this comment

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

Best practice to explicitly delete it anyway.

Co-authored-by: Stefan Karpinski <[email protected]>
@evetion
Copy link
Member Author

evetion commented Apr 13, 2021

I've tried to see if httpbingo has a way to set a cookie and redirect to another domain to see if cross-domain cookie leaks are possible, but I couldn't figure out a way to test that short of writing our own HTTP end point. We don't necessarily need that in the test suite, but I did kind of want to test it to make sure that we can't accidentally leak auth cookies in the case of a redirect. I'm guessing libcurl is too smart to do that, but I wanted to see for myself before turning this feature on for everyone.

Always good to double check these possible security issues. I think this is not possible, as a domain will always be set, either by the website or by curl. For our test the domain is explicitly added.

* Added cookie k1="v1" for domain httpbingo.julialang.org, path /cookies/, expire 0

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Great work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants