Discussion about new Curl URL API and ext/curl improvements

Hi internals,

Since its version 6.62.0 [1], libcurl features a brand new URL API [2] that
can be used to parse and generate URLs, using libcurl’s own parser. One of
the goals of this API is to tighten a problematic vulnerable area for
applications where the URL parser library would believe one thing and
libcurl another. This could and has sometimes led to security problems [3].
The new API is handled base and composed of 5 new functions [4] (and one
more since 7.80.0 to get more verbose information upon error).

I started to work on an implementation [5] to expose this API to PHP
userland in the curl extension so that PHP users can benefit from it. My
first reflex was to stay consistent on how the extension is currently
working and do a one to one mapping of the new curl functions. I got
feedback from both Ilija and Christoph saying that the API is, I quote, "a
bit clunky" which I can't disagree with.

For a long time, ext/curl worked with handles as "curl resources" and
functions mapped to the libcurl functions, but since PHP8.0 "curl
resources" were converted to opaque classes with no methods. So my main
goal here is to come up with a solution for the new Curl URL API, but maybe
also to be consistent, make some changes to existing curl classes like
CurlHandle to add methods and improve the current state of the extension.

Here is the solution I would propose :

• First of course keep all the current ext/curl functions as is not to
  break any compatibility (Seems obvious but just to be sure everybody is on
  the same page).
• For consistency expose the new Curl URL API as functions mapped one to
  one to libcurl functions :

function curl_url(?string $url = null): CurlUrl|false {}
function curl_url_set(CurlUrl $url, int $part, string $content, int $flags
= 0): bool {}
function curl_url_get(CurlUrl $url, int $part, int $flags = 0):
string|false {}
function curl_url_errno(CurlUrl $url): int {}
function curl_url_strerror(int $error_code): ?string {}

• Add methods to the CurlUrl object to make it less opaque and expose an
  object oriented style API. I would keep it minimal and let userlanAd API
  provide higher level APIs as guzzle for example. (You can see the current
  implementation [5])

final class CurlUrl implements Stringable
{
public function __construct(?string $url = null) {}
public function get(int $part, int $flags = 0): string {}
public function set(int $part, string $content, int $flags = 0): void
{}
public function getErrno(): int {}
public function __toString(): string {}
public function __clone() {}
}

• It would also be nice to add this object oriented API for existing
  CurlHandle, CurlMultiHandle and CurlShareHandle classes. For example the
  CurlHandle class would look like that (First implementation [6])

final class CurlHandle
{
public function __construct(?string $url = null) {}
public function setOpt(int $option, mixed $value): bool {}
public function getInfo(?int $option = null): mixed {}
public function exec(): string|bool {}
public function escape(string $string): string|false {}
public function unescape(string $string): string|false {}
public function pause(int $flags): int {}
public function getErrno(): int {}
public function reset(): void;
public function setOptArray(array $options): bool {}
public function upkeep(): bool {}
public function __clone() {}
}

As of right now I still have some unanswered questions like how should we
handle errors on the new CurlUrl API ?

• Throw CurlUrlException on both the procedural and object oriented style
  API (that's how current implementation works [5])
• Throw CurlUrlException with the oriented style API, but return for
  example boolean/null on errors when user uses the procedural API ?
• Always return null/booleans using both object oriented API and
  procedural API ?

The same question applies if we add a new object oriented style API on
existing classes. Current functions MUST stay unchanged but should we throw
CurlException when the user uses the object oriented style API ? (That's
what I would do) Or should we return the same result from OO and procedural
API ? Should we make the new CurlApi consistent with that ?

What are your thoughts about all this ? Any feedback on any of those
subjects are more than welcome.

Pierrick

[1]
https://daniel.haxhttps://
github.com/adoy/php-src/tree/curl-object-apix.se/blog/2018/10/31/curl-7-62-0-moar-stuff/
https://daniel.haxx.se/blog/2018/10/31/curl-7-62-0-moar-stuff/
[2] https://daniel.haxx.se/blog/2018/09/09/libcurl-gets-a-url-api/
[3]

https://www.blackhat.com/docs/us-17/thursday/us-17-Tsai-A-New-Era-Of-SSRF-Exploiting-URL-Parser-In-Trending-Programming-Languages.pdf
[4] https://github.com/curl/curl/blob/master/include/curl/urlapi.h
[5] https://github.com/php/php-src/pull/8770
[6] https://github.com/adoy/php-src/tree/curl-object-api

I understand why breaking changes would definitely be a no go here, but if
someone is adapting their code from procedural to OOP I would say they can
catch exceptions instead of comparing against false. To me it would even be
desirable. Any new code that didn't exist before show prefer throwing over
returning false, imo.

[...]

• For consistency expose the new Curl URL API as functions mapped one to
  one to libcurl functions :

function curl_url(?string $url = null): CurlUrl|false {}
function curl_url_set(CurlUrl $url, int $part, string $content, int $flags
= 0): bool {}
function curl_url_get(CurlUrl $url, int $part, int $flags = 0):
string|false {}
function curl_url_errno(CurlUrl $url): int {}
function curl_url_strerror(int $error_code): ?string {}

• Add methods to the CurlUrl object to make it less opaque and expose an
  object oriented style API. I would keep it minimal and let userlanAd API
  provide higher level APIs as guzzle for example. (You can see the current
  implementation [5])

final class CurlUrl implements Stringable
{
public function __construct(?string $url = null) {}
public function get(int $part, int $flags = 0): string {}
public function set(int $part, string $content, int $flags = 0): void {}
public function getErrno(): int {}
public function __toString(): string {}
public function __clone() {}
}

Hello,

When IntlDatePatternGenerator was implemented, I initially also supplied both
a procedural and OO-style API, but was told that this duplication was historical
and not necessary for new APIs.
See the thread following this message: [1]
I would argue that this also applies to curl, and that it might make sense to only
supply the OO-style API here as well.
(With curl_url_strerror being exposed as either a static method on CurlUrl or as getErrorMessage, I suppose.)

• It would also be nice to add this object oriented API for existing
  CurlHandle, CurlMultiHandle and CurlShareHandle classes. For example the
  CurlHandle class would look like that (First implementation [6])

I think this would be a good idea either way.

Regards,
Mel

[1] https://externals.io/message/114473#114658

Hi

As of right now I still have some unanswered questions like how should we
handle errors on the new CurlUrl API ?

• Throw CurlUrlException on both the procedural and object oriented style
  API (that's how current implementation works [5])
• Throw CurlUrlException with the oriented style API, but return for
  example boolean/null on errors when user uses the procedural API ?
• Always return null/booleans using both object oriented API and
  procedural API ?

Definitely not this one. I want Exceptions for an OO API. I don't
particularly care about the prodecural API, as it is not likely that I
would use it, given that it is more clunky.

The same question applies if we add a new object oriented style API on
existing classes. Current functions MUST stay unchanged but should we throw
CurlException when the user uses the object oriented style API ? (That's
what I would do) Or should we return the same result from OO and procedural
API ? Should we make the new CurlApi consistent with that ?

Same here: Exceptions for OO, please.

What are your thoughts about all this ? Any feedback on any of those
subjects are more than welcome.

Something else you didn't mention: Would it be possible to make the int $part parameter an enum?

Best regards
Tim Düsterhus

Hi internals,

Since its version 6.62.0 [1], libcurl features a brand new URL API [2] that
can be used to parse and generate URLs, using libcurl’s own parser. One of
the goals of this API is to tighten a problematic vulnerable area for
applications where the URL parser library would believe one thing and
libcurl another. This could and has sometimes led to security problems [3].
The new API is handled base and composed of 5 new functions [4] (and one
more since 7.80.0 to get more verbose information upon error).

I started to work on an implementation [5] to expose this API to PHP
userland in the curl extension so that PHP users can benefit from it. My
first reflex was to stay consistent on how the extension is currently
working and do a one to one mapping of the new curl functions. I got
feedback from both Ilija and Christoph saying that the API is, I quote, "a
bit clunky" which I can't disagree with.

For a long time, ext/curl worked with handles as "curl resources" and
functions mapped to the libcurl functions, but since PHP8.0 "curl
resources" were converted to opaque classes with no methods. So my main
goal here is to come up with a solution for the new Curl URL API, but maybe
also to be consistent, make some changes to existing curl classes like
CurlHandle to add methods and improve the current state of the extension.

Here is the solution I would propose :

• First of course keep all the current ext/curl functions as is not to
  break any compatibility (Seems obvious but just to be sure everybody is on
  the same page).
• For consistency expose the new Curl URL API as functions mapped one to
  one to libcurl functions :

function curl_url(?string $url = null): CurlUrl|false {}
function curl_url_set(CurlUrl $url, int $part, string $content, int $flags
= 0): bool {}
function curl_url_get(CurlUrl $url, int $part, int $flags = 0):
string|false {}
function curl_url_errno(CurlUrl $url): int {}
function curl_url_strerror(int $error_code): ?string {}

• Add methods to the CurlUrl object to make it less opaque and expose an
  object oriented style API. I would keep it minimal and let userlanAd API
  provide higher level APIs as guzzle for example. (You can see the current
  implementation [5])

final class CurlUrl implements Stringable
{
public function __construct(?string $url = null) {}
public function get(int $part, int $flags = 0): string {}
public function set(int $part, string $content, int $flags = 0): void {}
public function getErrno(): int {}
public function __toString(): string {}
public function __clone() {}
}

• It would also be nice to add this object oriented API for existing
  CurlHandle, CurlMultiHandle and CurlShareHandle classes. For example the
  CurlHandle class would look like that (First implementation [6])

final class CurlHandle
{
public function __construct(?string $url = null) {}
public function setOpt(int $option, mixed $value): bool {}
public function getInfo(?int $option = null): mixed {}
public function exec(): string|bool {}
public function escape(string $string): string|false {}
public function unescape(string $string): string|false {}
public function pause(int $flags): int {}
public function getErrno(): int {}
public function reset(): void;
public function setOptArray(array $options): bool {}
public function upkeep(): bool {}
public function __clone() {}
}

As of right now I still have some unanswered questions like how should we
handle errors on the new CurlUrl API ?

• Throw CurlUrlException on both the procedural and object oriented style
  API (that's how current implementation works [5])
• Throw CurlUrlException with the oriented style API, but return for
  example boolean/null on errors when user uses the procedural API ?
• Always return null/booleans using both object oriented API and
  procedural API ?

The same question applies if we add a new object oriented style API on
existing classes. Current functions MUST stay unchanged but should we throw
CurlException when the user uses the object oriented style API ? (That's
what I would do) Or should we return the same result from OO and procedural
API ? Should we make the new CurlApi consistent with that ?

What are your thoughts about all this ? Any feedback on any of those
subjects are more than welcome.

Pierrick

Like most of the responders so far, I would say skip the procedural API, just go OOP and be done with it. However, I would go a step further and say it should be a good OOP API, not just the Curl procedural API with funny syntax.

For example:

public function get(int $part, int $flags = 0): string {}

Absolutely not. :-) It should be public function getHost(), public function getScheme(), etc. The int $part bit is frankly bad design even by procedural standards, and has no place in a modern library. I don't even know what the $flags are or could be, which means that's also a bad design. They should either be omitted, handled via separate methods, or replaced with named arguments with reasonable defaults. (We can do that now, yay!) Translating those into Curl's nutty flag API is something that should be done once, in the wrapping extension/object, and no PHP user space developer should have to think about it.

Similarly, getErrno() should be replaced. If a URL cannot even be parsed at all, that's an exception. Not every error should be an exception (I recently wrote extensively on the subject here: https://peakd.com/hive-168588/@crell/much-ado-about-null), but in this case "I don't know WTF to do with this string you gave my constructor" is an exception. If instead of a constructor it had a static factory method (or multiple), then we could discuss those returning some other sentinel value instead to indicate failure, but "there was an error, go call this other thing to find out what it was" is a fundamentally flawed API design that should not be replicated.

In short, don't put a wrapper on the existing Curl API that happens to use ->. Design a good, usable, PHP-developer-friendly API, and put Curl behind it. Trying to just expose a crappy API and make it slightly less crappy is not a good use of time.

--Larry Garfield

For a long time, ext/curl worked with handles as "curl resources" and
functions mapped to the libcurl functions, but since PHP8.0 "curl
resources" were converted to opaque classes with no methods. So my main
goal here is to come up with a solution for the new Curl URL API, but maybe
also to be consistent, make some changes to existing curl classes like
CurlHandle to add methods and improve the current state of the extension.

This is so bizarre, I know I wrote an OOPified cURL extension some years
ago (called it curli), and now I can't find it anywhere. What universe am
I even in?

Anyway, +1 on making a whole new OOPified cURL implementation. $ch =
(new CurlHandle($uri))->setOpt(Curl::OPT_METHOD, 'post')->perform();
Let it throw exceptions, do proper type checks, all the goodness. Let the
procedural APIs behave differently than the object methods and that's okay.

cURL is a foundational extension in PHP and I respect the choices that were
made in its API design, but it's 2022, and we deserve a better ext/curl.

-Sara