WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 22 months ago

Last modified 21 months ago

#37733 closed defect (bug) (fixed)

`cURL error 3: malformed` for remote requests since 4.6

Reported by: flixos90 Owned by: flixos90
Milestone: 4.6.1 Priority: normal
Severity: normal Version: 4.6
Component: HTTP API Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description (last modified by flixos90)

Since 4.6, one of my plugins has a problem with making a remote request. I get the error cURL error 3: malformed. The underlying codebase can be found here: https://github.com/felixarntz/wp-encrypt/blob/master/inc/WPENC/Core/Client.php#L384

This has worked correctly in previous WordPress versions, but fails now. The person who reported this bug to me (in the plugin support forums) notified me that other plugins see similar problems (for example https://wordpress.org/support/topic/curl-error-3-malformed-error?replies=3).

Attachments (1)

37733.diff (1.4 KB) - added by flixos90 22 months ago.

Download all attachments as: .zip

Change History (20)

#1 @flixos90
22 months ago

  • Description modified (diff)

#2 @swissspidy
22 months ago

From that support thread:

It did work with earlier versions of WP, but the new WP doesn't allow in an URL.

Do you perhaps have double slashes in your URLs (except for the protocol part, of course)?

Last edited 22 months ago by swissspidy (previous) (diff)

#3 @flixos90
22 months ago

@swissspidy: I thought I didn't, but now after some investigation I found out I indeed did. :)

For this ticket: there shouldn't be double slashes in URLs, but was that stated in dev notes? I guess this doesn't need to be changed on the Core side, but if it hasn't been posted yet, a note would be helpful.

Otherwise this can be closed.

#4 @swissspidy
22 months ago

Looks like @dd32 already filed an issue upstream: https://github.com/rmccue/Requests/issues/231.

#5 follow-up: @dd32
22 months ago

  • Milestone changed from Awaiting Review to 4.6.1

I'm considering that for 4.6.1 we might want to normalize the URLs to remove double slashes within WP_HTTP. I feel that there are enough plugins affected by incorrect url concatenation to fix this.

I don't think the upstream issue will be fixed in time, nor do I personally understand what the upstream undocumented logic is supposed to be.

#6 in reply to: ↑ 5 @jeremyfelt
22 months ago

  • Keywords needs-patch added

Replying to dd32:

I'm considering that for 4.6.1 we might want to normalize the URLs to remove double slashes within WP_HTTP.

I think this makes sense.

We already parse the URL in WP_Http::request() and can act directly on the path there. Or - would it make sense to have a static normalize_url or similar method available for use elsewhere?

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


22 months ago

#8 @jeremyfelt
22 months ago

  • Keywords needs-unit-tests added
  • Owner set to flixos90
  • Status changed from new to assigned

@flixos90
22 months ago

#9 @flixos90
22 months ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch needs-unit-tests removed

37733.diff replaces double slashes at the begin of the URL path by a single slash to fix this issue. A unit test is included.

#10 @dd32
22 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 38429:

HTTP: Handle an edgecase within the URI parsing library included in Requests, where if a double slash exists at the start of the path the URL is passed to cURL malformed.

Props flixos90 for initial patch.
Fixes #37733 for trunk.

#11 @dd32
22 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I took a slightly different route, as this is only going to be a temporary measure (hopefully).

re-opening for 4.6.1 merge

#12 @jeremyfelt
22 months ago

  • Keywords dev-feedback removed

The approach in [38429] looks and tests good.

#13 @jeremyfelt
22 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 38485:

HTTP: Handle an edgecase within the URI parsing library included in Requests, where if a double slash exists at the start of the path the URL is passed to cURL malformed.

Merge of [38429] to the 4.6 branch.

Props dd32, flixos90 for initial patch.
Fixes #37733.

#14 @dd32
22 months ago

I was going to hold off on a 4.6.1 commit here, as we might be able to just fix it properly within Requests, however it looks like that fix may not drop until next week now, so I expect trunk will get a proper fix for this, but 4.6.x can just keep this regex.

#15 @cklosows
22 months ago

I believe the goal here was to strip // from the path of URLs down to a single / however, it's also affecting any query string parameters, as seen in this example from the EDD Software Licensing parameters sent:

/app/public/wp-includes/class-http.php:368:string 'https://www.example.com?edd_action=activate_license&license=<redacted>&item_name=<redacted>&url=http:/one.submulti.dev' (length=151)

The Domain requesting the activation is http://one.submulti.dev, but the regex is stripping the additional slash here.

#16 @jeremyfelt
22 months ago

@cklosows Thanks for the report. You're correct in that other double slashes should not be stripped. As this ticket is closed against the 4.6.1 milestone, can you open a new ticket with that information so we can target for 4.6.2?

#17 @cklosows
22 months ago

@jeremyfelt Yep, arleady done: #38070 Thanks for getting back to me.

Last edited 21 months ago by SergeyBiryukov (previous) (diff)

#18 @dd32
21 months ago

In 38727:

HTTP: Update Requests to master (0048f3c) which fixes a number of outstanding issues.

Fixes #38070, #37733 by reverting part of [38429] and using the fix in Requests.
Fixes #37992 allowing for connecting to SSL resources on ports other than 443.
Fixes #37991 by not sending default ports in the Host: header.
Fixes #37839 to match and decode Chunked responses correctly.
Fixes #38232 allowing a SSL connection to ignore the hostname of the certificate when verification is disabled.

#19 @dd32
21 months ago

In 38728:

HTTP: Update Requests to master (0048f3c) which fixes a number of outstanding issues.

Merges [38727] to the 4.6 branch.

Fixes #38070, #37733 by reverting part of [38429] and using the fix in Requests.
Fixes #37992 allowing for connecting to SSL resources on ports other than 443.
Fixes #37991 by not sending default ports in the Host: header.
Fixes #37839 to match and decode Chunked responses correctly.
Fixes #38232 allowing a SSL connection to ignore the hostname of the certificate when verification is disabled.

Note: See TracTickets for help on using tickets.