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

CURLE_URL_MALFORMAT returned if Location has malformed url #3340

Closed
pps83 opened this issue Dec 4, 2018 · 10 comments

Comments

@pps83
Copy link
Contributor

commented Dec 4, 2018

I did this

I do http get using libcurl and returned 302 response intentionally has malformed url in the Location header (this is a unit test). I requested libcurl not to follow redirects. libcurl used to always return CURL_OK in this case without checking contents of the Location header, but starting from 7.62 unit tests started to fail and it appears that now libcurl returns CURLE_URL_MALFORMAT in this case. I think this is wrong, because request actually succeeded and response was received. CURLE_URL_MALFORMAT means that the request that I made was invalid and had malformed url. Now it's not even clear if CURLE_URL_MALFORMAT resulted from the url0 that I requested, or after fetching url0 and checking Location header that contained malformed url1. At least with previous versions of curl I was able to extract value of locations header. It was clear if CURLE_URL_MALFORMAT resulted from url0, and I was able to get malformed url1 to inspect/log it. Now I cannot even get the url1. CURLINFO_REDIRECT_URL returns NULL, and CURLINFO_REDIRECT_COUNT also returns null.
I've seen that url api code added in 7.62, I suspect that now curl tries to validate Location while it was requested not to follow it and effectively fail as it tried to follow the location.

I expected the following

curl/libcurl version

[curl -V output]

operating system

@bagder bagder added HTTP URL labels Dec 4, 2018

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Isn't this a duplicate of #3210 and thus fixed in #3215 already?

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Thanks for a quick reply. I took 2c5ec33 commit and I can confirm that this doesn't not affect outcome in any way. I still get CURLE_URL_MALFORMAT, I still cannot retrieve (malformed) Location header, and I still cannot even know if the url0 was malformed or url1 and most importantly I don't even know that the request was actually completed.

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

in my case I have a unit test testing invalid urls and I try to fetch them directly and through a redirect to verify that API produces consistent results. Unfortunately with latest libcurl this test now fails

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

Another related change from url-api commit: previously if the server replied with corrupt Location that contained spaces (Location: test?msg=hello world) curl would follow test?msg=hello world, now it follows test?msg=hello which is wrong imo: it should have accepted entire location (like it used to before 7.62) or should have rejected it as invalid (because url in Loaction header doesn't follow http spec that requires the spaces to be percent encoded).
MS's XmlHttp for example follows full url (just like curl used to do before 7.62)

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

I still cannot retrieve (malformed) Location header

Right, that's a side-effect of the new logic... I don't think we have to do it like this. Care you write up a PR that fixes this? Maybe with a test that verifies it as well?

curl would follow "test?msg=hello world"

Please don't append separate problems in the same issue, file it as a separate one. I note that test42 verifies Location following with spaces in the URL so you're probably talking about spaces exactly in the query part?

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Now I cannot even get the url1. CURLINFO_REDIRECT_URL returns NULL, and CURLINFO_REDIRECT_COUNT also returns null

Can you help us with a source code to a small application that reproduces this problem and what http response headers it would need to trigger?

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

#include <stdio.h>
#include <curl/curl.h>

void testCurl()
{
    HttpServer srv;
    srv.use("/test", [](Request& req, Response& res) {
        res.setHeader("Location", "http://1.2 .4.5/test");
        res.writeHead(302);
        res.end();
    });
    srv.listen(0, "127.0.0.1");
    std::string redirectToInvalidLocationUrlX = "http://" + srv.getServerHost() + "/test";

    const char* invalidUr = "http://1.2 .4.5/test";
    const char* redirectToInvalidLocationUrl = redirectToInvalidLocationUrlX.c_str();

    for (int i = 0; i < 2; ++i)
    {
        CURL *curl;
        CURLcode res;
        const char* url = ((i & 1) == 0) ? invalidUr : redirectToInvalidLocationUrl;

        curl = curl_easy_init();
        if (curl)
        {
            curl_easy_setopt(curl, CURLOPT_URL, url);
            curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 0L);
            
            res = curl_easy_perform(curl);

            long curlResponseCode;
            curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &curlResponseCode);
            long curlRedirectCount;
            curl_easy_getinfo(curl, CURLINFO_REDIRECT_COUNT, &curlRedirectCount);
            char* effectiveUrl = NULL;
            curl_easy_getinfo(curl, CURLINFO_EFFECTIVE_URL, &effectiveUrl);
            char* redirectUrl = NULL;
            curl_easy_getinfo(curl, CURLINFO_REDIRECT_URL, &redirectUrl);

            printf("res:%d status:%d, redirects:%d, effectiveurl:%s redirecturl:%s\n", (int)res, (int)curlResponseCode, (int)curlRedirectCount, effectiveUrl, redirectUrl ? redirectUrl : "?");

            /* always cleanup */
            curl_easy_cleanup(curl);
        }
    }
}

this is the output that I get:

res:3 status:0, redirects:0, effectiveurl:http://1.2 .4.5/test redirecturl:?
res:3 status:302, redirects:0, effectiveurl:http://127.0.0.1:60305/test redirecturl:?

Note, I have a simple HttpServer to be able to simulate whatever I want, you can use whatever you use for that kind of stuff and set redirectToInvalidLocationUrl accordingly. As you can see in both cases outcome looks as though the url that was used was malformed even though in second case url wasn't malformed, and the request completed successfully. The only "hint" that there is something different is that the status is 302 in second case.

Before url-api change res would be OK in second case and I'd be able to retrieve redirect url.

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

@pps83 it would be great if you could verify this patch asap as then we might be able to get this merged before the next release, which ships tomorrow morning...

bagder added a commit that referenced this issue Dec 11, 2018
Curl_follow: extract the Location: header field unvalidated
... when not actually following the redirect. Otherwise we return error
for this and an application can't extract the value.

Test 1518 added to verify.

Reported-by: Pavel Pavlov
Fixes #3340
@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

I tried this patch, and all if I apply it all my tests pass as before.

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Thanks for confirming!

@bagder bagder closed this in 435402c Dec 12, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Mar 12, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.