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
--remote-header-name doesn't work when a scheme is missing from a URL #760
Comments
I don't think it is related to the missing scheme, I think it is the redirect that causes it. Hm, but why does the scheme-using one work then... ? |
We don't honor the content disposition filename unless the scheme is explicit and http or https. How about we relax it a little: diff --git a/src/tool_operate.c b/src/tool_operate.c
index c8bf12b..94a9658 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -1290,7 +1290,10 @@ static CURLcode operate_do(struct GlobalConfig *global,
if(config->content_disposition
&& (urlnode->flags & GETOUT_USEREMOTE)
&& (checkprefix("http://", this_url) ||
- checkprefix("https://", this_url)))
+ checkprefix("https://", this_url) ||
+ (!strstr(this_url, "://") &&
+ (!config->proto_default ||
+ checkprefix("http", config->proto_default)))))
hdrcbdata.honor_cd_filename = TRUE;
else
hdrcbdata.honor_cd_filename = FALSE; |
I completely forgot that piece of logic. I'm thinking perhaps we can skip the protocol check completely. There are (or at least these once existed) for example users who'd get FTP:// URLs over a HTTP proxy that can also get HTTP headers like this. To me, it seems quite harmless to just remove that check and have all protocols get this ability. Thoughts? |
born in a884ffe. If we remove the scheme check could there be a vulnerability if some other protocol sends a content disposition header with the filename? |
I don't see how any other protocol would, and we only check headers if asked for it with the specific command line option so it seems like a very small risk that would ever happen. |
I say please land that patch and close this issue! |
I landed your patch with a test that verifies it. Thanks! |
|
It was your patch, I hope you like it! =) It is still a small risk AFAICS because of libcurl's protocol "guessing" logic based on the host name prefix. If you specify "curl ftp.funet.fi", it will speak FTP... |
Oops I forgot about that, I guess I'll eat my words. How about this: diff --git a/src/tool_cb_hdr.c b/src/tool_cb_hdr.c
index 5be02aa..f7d8355 100644
--- a/src/tool_cb_hdr.c
+++ b/src/tool_cb_hdr.c
@@ -48,6 +48,7 @@ size_t tool_header_cb(void *ptr, size_t size, size_t nmemb, vo
const char *str = ptr;
const size_t cb = size * nmemb;
const char *end = (char*)ptr + cb;
+ char *url = NULL;
/*
* Once that libcurl has called back tool_header_cb() the returned value
@@ -88,7 +89,9 @@ size_t tool_header_cb(void *ptr, size_t size, size_t nmemb, vo
*/
if(hdrcbdata->honor_cd_filename &&
- (cb > 20) && checkprefix("Content-disposition:", str)) {
+ (cb > 20) && checkprefix("Content-disposition:", str) &&
+ !curl_easy_getinfo(outs->config->easy, CURLINFO_EFFECTIVE_URL, &url) &&
+ url && (checkprefix("http://", url) || checkprefix("https://", url))) {
const char *p = str + 20;
/* look for the 'filename=' parameter
diff --git a/src/tool_operate.c b/src/tool_operate.c
index ab29c00..5f49efb 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -1295,12 +1295,7 @@ static CURLcode operate_do(struct GlobalConfig *global,
my_setopt_flags(curl, CURLOPT_REDIR_PROTOCOLS, config->proto_redir);
if(config->content_disposition
- && (urlnode->flags & GETOUT_USEREMOTE)
- && (checkprefix("http://", this_url) ||
- checkprefix("https://", this_url) ||
- (!strstr(this_url, "://") &&
- (!config->proto_default ||
- checkprefix("http", config->proto_default)))))
+ && (urlnode->flags & GETOUT_USEREMOTE))
hdrcbdata.honor_cd_filename = TRUE;
else
hdrcbdata.honor_cd_filename = FALSE; |
Oh, clever. Should work just fine indeed! |
- Move the existing scheme check from tool_operate. In the case of --remote-header-name we want to parse Content-disposition for a filename, but only if the scheme is http or https. A recent adjustment 0dc4d8e was made to account for schemeless URLs however it's not 100% accurate. To remedy that I've moved the scheme check to the header callback, since at that point the library has already determined the scheme. Bug: #760 Reported-by: Kai Noda
Thanks, landed in b9728bc. |
I did this
I expected the following
I expected to successfully download
rustup-setup.exe
according tobut I got the following error:
curl/libcurl version
operating system
The text was updated successfully, but these errors were encountered: