Vinnie Falco wrote:

On Tue, Oct 12, 2021 at 10:17 AM Ivan Matek <libbooze@gmail.com> wrote:

...

I see examples work with std::cout, would it work (in C++20 compilers) with fmt::format?

Good question! I'm not sure... the library provides operator<< for all important types. If fmt::format uses operator<< internally, then I suppose it should work.

No, it doesn't. std::format has its own customization mechanism.

On Oct 12, 2021, at 12:17 PM, Vinnie Falco <vinnie.falco@gmail.com> wrote:

On Tue, Oct 12, 2021 at 10:57 AM Alex Christensen <achristensen@apple.com> wrote:

...

Why did you use RFC 3986 as your specification?

Well, this one seems to be the latest RFC regarding URLs, modulo a bit of HTTP-specific stuff like authority-form appearing in rfc7230. Is there something newer? I would say that the WhatWG URL specification is that something newer, but I sympathize. It is difficult to get started with.

...

Do you have any general plan for a strategy for handling non-ASCII input?

Yes, the plan is to reject such input. Strings containing non-ASCII characters are not valid URLs. And even some ASCII characters are not allowed to appear in a URL, for example all control characters. That is certainly a choice you can make, but at some point you may run into issues with people trying to give your library input like http://example.com/💩 <http://example.com/%F0%9F%92%A9> and expecting the URL parser to normalize it to http://example.com/%F0%9F%92%A9 <http://example.com/%F0%9F%92%A9> for you like it does in some other URL libraries. I see Punycode encoding and decoding doesn’t seem to be in the scope of this library, and for your use cases that may be fine and for others that might not be fine. It seems like you’re aware of this design choice, though.

On Tue, Oct 12, 2021 at 1:02 PM Alex Christensen <achristensen@apple.com> wrote:

I would say that the WhatWG URL specification is that something newer, but I sympathize. It is difficult to get started with.

I plan to pick through the WhatWG specification and see if there are any tidbits that could have value. The procedural exposition (append this character, execute this algorithm, output this string) makes it very difficult to grasp the higher level semantics of the thing. The BNF in the RFC is way easier to grok, e,g: scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) and in fact this is exactly how I have decomposed the parsing, into each individual named element described by the RFC.

That is certainly a choice you can make, but at some point you may run into issues with people trying to give your library input like http://example.com/💩

I'm not sure how someone would give the library that input. Is this expressible in a string_view?

I see Punycode encoding and decoding doesn’t seem to be in the scope of this library, and for your use cases that may be fine and for others that might not be fine.

I actually have all the punycode algorithms ready including tests: <https://github.com/CPPAlliance/url/tree/punycode> But after giving it some thought, I couldn't see the use-case for it. Callers who want to perform name resolution on a host can't pass a utf-8 string they have to pass the punycode-encoded string. The only use-case that I can discern for punycode is for display purposes which is out of scope. Unless, do you know of any other use-case?

It seems like you’re aware of this design choice, though.

Yes Actually, come to think of it - there is a use-case for punycode encoding and that is to take an international domain name string in utf-8 encoding and apply puny-code encoding. I think... I have no experience with this so some guidance would be helpful. -- Regards, Vinnie Follow me on GitHub: https://github.com/vinniefalco

On Oct 12, 2021, at 3:15 PM, Vinnie Falco <vinnie.falco@gmail.com> wrote:

On Tue, Oct 12, 2021 at 1:02 PM Alex Christensen <achristensen@apple.com> wrote:

...

at some point you may run into issues with people trying to give your library input like http://example.com/💩 and expecting the URL parser to normalize it to http://example.com/%F0%9F%92%A9

Okay, I think what you're saying is that you will have this string literal:

string_view s = "http://example.com/\xf0\x9f\x92\xa9";

Unfortunately, this is not a valid URL and I don't think that the library should accept this input. It is perfectly valid input that some URL libraries I work with accept and percent encode, and some URL libraries I work with reject it as an invalid URL. I think it’s a valid URL parser input that ought to produce a valid URL, but not everyone agrees on this yet.

However, you could write this:

url u = parse_uri( "http://example.com" ).value();

u.set_path( "/\xf0\x9f\x92\xa9" );

This will produce:

assert( u.encoded_url() == "http://example.com/%f0%9f%92%a9" );

Is this what you meant? Welcome to the crazy world of URL parsing!

I'm guessing that the turd emoji is inserted into the C++ source file as a utf-8 encoded code point, so that's what you get in the string literal.

Thanks

On Tue, Oct 12, 2021 at 6:06 PM Vinnie Falco <vinnie.falco@gmail.com> wrote:

What you are thinking of as a "valid URL parser input" is actually an Internationalized Resource Identifier, which supports the broader universal character set instead of just ASCII and is abbreviated by the even more obscure acronym "IRI." It is covered by rfc3987:

<https://datatracker.ietf.org/doc/html/rfc3987>

We looked over this RFC and I think, it would be possible to support IRIs simply by providing a new set of parsing functions, for example void parse_iri ( string_view, error_code&, url& ); void parse_irelative_ref ( string_view, error_code&, url& ); void parse_absolute_iri ( string_view, error_code&, url& ); void parse_iri_reference ( string_view, error_code&, url& ); It wouldn't be possible to parse into a url_view, since UTF-8 encoded characters have to be converted to percent-encoded escapes. But this could be made to work, and it fits neatly into the current implementation. There would be an additional function to take a url and convert it back into its IRI string, which is mostly just decoding percent-escaped characters. The library would disallow invalid UTF-8. Thanks

On Tue, Oct 12, 2021 at 3:28 PM Gavin Lambert via Boost <boost@lists.boost.org> wrote:

I note in the docs that there is also a set_encoded_path, which implies that set_path might be "unencoded", which might explain it -- but set_path has no documentation, so this is unclear.

The docs could use work as usual. Yes, there are two functions set_encoded_path set_path For almost all functions that set a string, there is the "encoded" version which requires a valid percent-encoded string, and the plain version which accepts any string and then escapes the reserved characters as needed.

On an unrelated note, set_query_part seems wrong, it should have a leading question mark but the BNF specifies a leading hash. Also it claims its input is an encoded string, which seems inconsistently named with set_encoded_query (which in turn mentions fragments in its docs, which also seems wrong).

Little bit of under-construction going on there, it will be resolved today, thanks for looking! Regards

On 13/10/2021 18:56, Julien Blanc wrote:

And then there's the issue of the '/' character in set_path if you take unencoded strings (not sure how this should be handled...)

Exactly, that's a fundamental problem with the concept of accepting not-yet-encoded strings -- the URL specification does not have uniform encoding; it is not possible to take an arbitrary URL and round-trip it between encoded and decoded forms as a whole (even ignoring equivalent variants like %20 vs + or %-encoding more characters than strictly required). Where a / occurs inside a single path-component, it must be escaped so as to not be seen as a path-component-separator. And as such, it's not possible to (reliably) pass multiple unencoded path-components as a single string. The same problems occur with query parameters and &= characters, or with ?# characters appearing anywhere. (Where the authority component is not a DNS hostname there may be issues with :@/ characters appearing there too.) I think the only sane choice here is (if supported at all) to only provide access to unencoded values at the smallest possible unit (i.e. only for a single parameter or a single path component, or a collection of such, but never as a single string). (But to still provide easy access to the encoder/decoder for arbitrary external usage.)

On Wed, Oct 13, 2021 at 8:14 AM Vinnie Falco <vinnie.falco@gmail.com> wrote:

...

Yeah you all uncovered some lurking problems so I am going back to the drawing board to come up with a comprehensive set of fixes. Thanks!!!

Le 2021-10-13 16:54, Vinnie Falco a écrit :

On Tue, Oct 12, 2021 at 11:24 PM Julien Blanc via Boost

...

And then there's the issue of the '/' character in set_path if you take unencoded strings (not sure how this should be handled...)

Well, there's not much of an issue there. set_path() treats slashes as segment separators. If that's not what you want, you can set the individual unencoded segments through the container returned by url::segments(). The same goes for the query parameters (call url::params()).

I'm not at ease with that. If i build a mailto: uri, and call set_path with a email address containing a / character in it (such as valid/email@example.com), it will result in a uri containing two segments, which is nonsense for a mailto scheme (and obviously not what i expected). I understand the api is fitted toward web uris, but i see there a potential for hard to find bugs due to api misuse. Maybe it's just the naming that needs to be improved. Regards, Julien

On Wed, Oct 13, 2021 at 7:10 AM Phil Endecott via Boost <boost@lists.boost.org> wrote:

Have you looked at the RFC 3986 errata at https://www.rfc-editor.org/errata_search.php?rfc=3986&rec_status=0

ruh roh.. I have not gotten to that yet but yeah. I see problems :) Not difficult problems, but there are relevant errata that must be fixed before any review. Thanks!

? For example, I note that your rule for relative-part on the "Parsing" page doesn't seem to include the path-abempty alternative added in erratum 5428.

Thanks I need to update the javadocs and exposition. Although, I think the parsing is still correct despite not showing path-abempty. I will add more tests.

I would be interested to see how compile time, run time, and lines-of-code compare between your implementation, a Spirit parser, and a regexp.

Someone volunteered to do that so I might explore it at one point.

You also have the choice of (a) parsing and storing pointers to each component as I believe you do,

Right, the library keeps a small table of offsets to various parts. For the modifiable containers I plan to also keep a table for the segments and query parameters, to allow O(1) access of indexed elements (right now its linear). I think this is a good balance for accessing parts of the URL and helping speed up mutation operations.

(b) could be better if you're mostly just passing around complete URLs, storing them in containers, etc.

Well, in url the table adds overhead. If you don't want that overhead then... just store the URL string :) and parse it again when you need it, this is equivalent to your (b), and less impact on the library.

There's also the question of worst-case complexity; does your parser backtrack? If so, is there a pathological input that causes it to take exponential time?

I have done no profiling or optimization to speak of except some minor design work to facilitate the use of character-based SIMD algorithms later. I don't have a monolithic "parser" per-se, there are classes called "bnf elements" which can be used in the generic bnf::parse function. These elements call parse on child elements recursively. I do backtrack. For example, URI-reference is defined as: URI-reference = URI / relative-ref The class uri_reference_bnf implements this by first attempting to parse the scheme and colon, and if that fails then it backtracks and attempts parsing a "relative-ref." You can see this here: https://github.com/CPPAlliance/url/blob/680f6db547ffc3367e1ba93bae29e4c9278f... I'm not really sure how this can be improved, you need to know if there's a scheme and that can only be determined by scanning forward until reaching a colon or an invalid character for scheme. In theory this could scan the entire input. Thus I have not put any effort into making this better.

Can you add a complexity guarantee to the docs? (I posted about this in the context of regular expression libraries a few weeks ago.)

Maybe... I did it for JSON because performance is critical and the difference between a poorly performing library and a greatly performing library is massive. I'm not sure that URL parsing is the same. URLs tend to be short and it is sort of difficult to really get bad performance out of parsing them even if you go a character at a time. Unlike JSON you don't need to convert between base 2 and base 10 for numbers (which is a headache). Mutations always give the strong exception safety guarantee and it also guarantees only one memmove, I will probably document that. I can give you a better answer in the future when it is at the point where benchmarks are explored.

auto url = parse_url(str);

The problem is that url is a view of the string parameter, but nothing warns the user about that and...

Well, it is documented profusely. I agree there is some danger here but with great danger comes great power. Or something about great responsibility?

..now url is dangling. Am I right, or is there something that prevents this?

No you are right, in the statement below `uv` references the string and does not take ownership: url_view uv = parse_uri( "https://example.com/" ).value(); However, url is constructible from url_view and url DOES make a copy with ownership, so this statement also works and doesn't have the problem of dangling: url u = parse_uri( "https://example.com/" ).value();

I believe that functions that returns views should be named so that that is obvious, and in cases like this the function with the simplest name should be the safer version that returns a value-type.

These algorithms only return views, because there are a few different owning URL containers and they can all be constructed from the views. Overloading on return type doesn't really work and some containers may have template parameters or allocators which makes APIs to parse into them awkward. If we're debating the name though, it means we have accepted the general design of the library :)

Also I note that parse_uri() returns a result<> and doesn't throw. My preference would be to name that e.g. try_parse_uri() and for the simpler-named function to be a wrapper that returns a plain uri or throws on error. My rationale is to make the simplest-named functions provide the easiest-to-use functionality, so that *teaching C++ to a Javascript programmer is not so embarrassing*, while retaining the more performant API variants for those who need them.

Well the whole point of boost::system::result (the new type that Boost.URL uses) is to cut down on those extra names and overloads. Your suggestion would keep the additional complexity of result and also increase the number of overloads which isn't sounding like a clear cut win to me... although, this "result" return type is sort of new.

(Is it parse_uri() or parse_url()? I thought you had decided on URL.)

I use the term URL generally, but I use the precise wording when referring to grammar. "parse_uri" uses the URI grammar specified in rf3986, modulo any errata fixes which I have yet to apply.

"pct" doesn't immediately mean "percent" to me.

The grammar says "pct-encode", I followed that. I think there is sufficient use of the abbreviation that it is justified but I could be convinced otherwise if there is new data.

It doesn't look like your url type is comparable. I think it should be. The comparisons need to be careful about case-sensitivity.>

And it should be hashable.

You can track those issues here: <https://github.com/CPPAlliance/url/issues/64> <https://github.com/CPPAlliance/url/issues/65>

I recently had to implement request signing for AWS. This requires canonicalisation of URLs. There are some subtleties in percent encoding: you need to be able to control exactly which characters are escaped (I think you allow this); you need to be able to control whether + is used to escape space (do you?)

Yes and yes.

and you need to be able to control whether the hex characters are upper or lower case (do you?).

But no to this... we can add it though, there's a "pct_encode_opts" structure: <https://master.url.cpp.al/url/ref/boost__urls__pct_encode_opts.html>

This code also needed to sort the query parameters by key; that raises the question of whether the query is a sequence or an associative container of key-value pairs, and if an associative container, what the comparison function is.

The library treats the query parameters as a ForwardRange (url_view) or RandomAccessRange (url) of key/value pairs, allowing for duplicate keys. The functions find/find_next can be used to visit all values with the same key. If you are using the "encoded" params container (by calling encoded_params()) then comparisons are bitwise. If you are using the "plain" params container (by calling params()) which decodes everything for you, then the comparison is made using notional percent-decoding. That is, your decoded key is compared against each container key as-if it had percent-decoding applied.

I would hope that the path would be similar to a std::filesystem::path i.e. it should use the same vocabulary where possible (or could even actually be a std::filesystem::path, on C++17).

OOF I hope never to see this. std::filesystem is kind of a mess. These are the path containers: https://master.url.cpp.al/url/ref/boost__urls__segments.html https://master.url.cpp.al/url/ref/boost__urls__segments_encoded.html https://master.url.cpp.al/url/ref/boost__urls__segments_view.html https://master.url.cpp.al/url/ref/boost__urls__segments_encoded_view.html "view" containers are read-only and reference the character buffer without ownership, these containers are obtained by calling functions on url_view. The other containers allow modification (the iterators are still read-only, because a proxy-assignment implementation proved to be clumsy) and these containers are obtained by calling functions on url.

Conversion between std::filesystem::path and file: URLs would be nice to have.

Yeah something like that is possible, falling back to boost::filesystem on C++11.

In your table 1.7, "Return the type of host specified, if any." should I think say "Return the host, if any."

hmm, yeah - the function "host_type()" is missing from that list (it returns the type of host).

Your percent-decoding methods return strings. I'm a bit surprised that you've not tried to return lazy decoding views, at least as an option.

Now that is a very interesting idea! I didn't think of it. But, its hard to see the use case, what would you do with a lazy view other than unroll it into a linear buffer? Thanks

On Wed, Oct 13, 2021 at 3:52 PM Gavin Lambert via Boost <boost@lists.boost.org> wrote:

Bear in mind that most of those are Reported and not Verified, so they should be treated as non-authoritative comments.

Never a dull moment!!

For example, the suggested amendment for parsing too many .. path components seems incorrect and the original is more correct. (Unmatched .. components should not be left in a canonical URL.)

I'm still working on that part. But I think that the reg-name change is legit? The addition of path-abempty doesn't actually change anything it seems, but I have updated the documentation. Thanks

On Wed, Oct 13, 2021 at 4:09 PM Gavin Lambert via Boost <boost@lists.boost.org> wrote:

...

Given the following path: /path/to/file.txt What should iterating the segments return? { "path", "to", "file.txt" } or { "/path", "/to", "/file.txt" } or something else? Given this path: my/download/folder/ What should iterating the segments return? { "my", "download", "folder", "" } or { "my/", "download/", "folder/" } or { "my", "/download", "/folder", "/" } or something else? Given an empty relative-ref (empty string) in a url u, what is the encoded path after executing this statement? u.segments().push_back( "index.htm" ); Anyone feel free to answer! Thanks

On 10/14/21 3:25 AM, Andrey Semashev wrote:

On 10/14/21 2:52 AM, Vinnie Falco via Boost wrote:

...

On Wed, Oct 13, 2021 at 4:09 PM Gavin Lambert via Boost <boost@lists.boost.org> wrote:

...

...

Given the following path:

/path/to/file.txt

What should iterating the segments return?

{ "path", "to", "file.txt" } or { "/path", "/to", "/file.txt" }

or something else?

Given this path:

my/download/folder/

What should iterating the segments return?

{ "my", "download", "folder", "" } or { "my/", "download/", "folder/" } or { "my", "/download", "/folder", "/" }

or something else?

Given an empty relative-ref (empty string) in a url u, what is the encoded path after executing this statement?

u.segments().push_back( "index.htm" );

Anyone feel free to answer!

If std::filesystem::path is an example to follow then path elements should not contain path separators, except the root directory. The trailing separator is indicated by an empty final element. So:

/my/download/folder/

would correspond to:

{ "/", "my", "download", "folder", "" }

and

/path/to/file.txt

to

{ "/", "path", "to", "file.txt" }

I think, URL paths must always have the root directory as the leading slash is a necessary separator from the host and port, so you could probably omit it from the list of elements.

Two corner cases to consider is when you have no path at all, and when you have a root directory as a path. Those two cases have to be distinct in terms of observed sequence of elements. I.e. https:://example.com?foo=42 should be different from https:://example.com/?foo=42 in terms of observed elements of the path.

On 10/14/21 4:15 AM, Vinnie Falco wrote:

On Wed, Oct 13, 2021 at 5:32 PM Andrey Semashev via Boost <boost@lists.boost.org> wrote:

...

https:://example.com?foo=42

should be different from

https:://example.com/?foo=42

Well... in your specific case not, because normalization of the https scheme inserts "/" when the path is zero length. Although this is scheme-specific. Generally speaking, there is always a path, it can be zero length. This is different from the query and fragment - which may or may not exist. A query can exist and be zero length, and the query can not exist.

Ok. But do other schemes make the same requirement? And does Boost.URL support those? My point is that if there is a case supported by Boost.URL where an empty path and a path of "/" are considered distinct, Boost.URL has to recognize and expose that distinction in its path elements sequence. For example, an empty path would correspond to an empty sequence, and the path of "/" would correspond to a single element sequence, with the element being "".

On 10/14/21 4:13 AM, Vinnie Falco wrote:

On Wed, Oct 13, 2021 at 5:25 PM Andrey Semashev via Boost <boost@lists.boost.org> wrote:

...

{ "/", "my", "download", "folder", "" } ... { "/", "path", "to", "file.txt" }

These are not really great because then elements are no longer homogeneous. That is, "/" is allowed in the front element but not the others. So

u.segments().insert( u.segments().begin(), "/" );

is okay, but these are not:

u.segments().insert( u.segments().begin(), "home" );

u.segments().push_back( "/" );

As I said, you're probably fine omitting the root directory.

On Wed, Oct 13, 2021 at 5:56 PM Gavin Lambert via Boost <boost@lists.boost.org> wrote:

(On that note, url::resolve is undocumented and doesn't seem to have the correct signature -- having both a base and relative parameter only makes sense if it's static or free, but it appears to be declared as instance. Unless the instance is only used as a completely-replaced output, but that seems weird and a free function with return value makes more sense to me.)

Yes `this` is used as a "completely-replace output." A free function with a return value doesn't work, because there are a few different flavors of url (url, static_url, and possibly a couple more if users request them). They differ in how they obtain their storage. But they use url as a base class. So having `url::resolve` a as member function makes it all work out.

On Wed, Oct 13, 2021 at 6:19 PM Vinnie Falco <vinnie.falco@gmail.com> wrote:

...

Thinking out loud here, it seems that the segments container models the path as vector< string > When it actually needs to model the container as struct { bool leading_slash; vector< string > }; Should I just add this function void segments::set_absolute( bool path_should_be_absolute ); and keep bool segments::is_absolute() const noexcept; ?

Vinnie Falco wrote:

On Wed, Oct 13, 2021 at 7:10 AM Phil Endecott via Boost <boost@lists.boost.org> wrote:

...

There's also the question of worst-case complexity; does your parser backtrack? If so, is there a pathological input that causes it to take exponential time?

I have done no profiling or optimization to speak of except some minor design work to facilitate the use of character-based SIMD algorithms later. I don't have a monolithic "parser" per-se, there are classes called "bnf elements" which can be used in the generic bnf::parse function. These elements call parse on child elements recursively. I do backtrack. For example, URI-reference is defined as:

URI-reference = URI / relative-ref

The class uri_reference_bnf implements this by first attempting to parse the scheme and colon, and if that fails then it backtracks and attempts parsing a "relative-ref." You can see this here:

https://github.com/CPPAlliance/url/blob/680f6db547ffc3367e1ba93bae29e4c9278f...

I'm not really sure how this can be improved, you need to know if there's a scheme and that can only be determined by scanning forward until reaching a colon or an invalid character for scheme. In theory this could scan the entire input. Thus I have not put any effort into making this better.

...

Can you add a complexity guarantee to the docs? (I posted about this in the context of regular expression libraries a few weeks ago.)

Maybe... I did it for JSON because performance is critical and the difference between a poorly performing library and a greatly performing library is massive. I'm not sure that URL parsing is the same. URLs tend to be short and it is sort of difficult to really get bad performance out of parsing them even if you go a character at a time.

data: URLs are a good example of long URLs that actually exist in practice. But what I'm really thinking about is guarding against malicious malformed input. You don't want your parser to have N^2 let alone exp(N) complexity when the input could legitimately be a data: URL of a few kbytes. And there's no need for it to have worse than linear complexity, because I'm pretty sure that the URL syntax is a Regular Language (in the computer science sense, not in the "regular expression is a synonym for search pattern" I'm-a-perl-programmer sense), and all regular languages can by definition be parsed in linear time. Regarding your example, i.e. "http://foo.com/" vs the relative URL "http.html", the point is that when the parser has reached the 'p' it will be in a state meaning "either this is a scheme or the first segment of the path of a relative URL". The transformation from BNF to a no-backtracking state machine of that sort is what tools like flex and libraries like RE2 do. Now having said all that, I do suspect that the particular grammar of URLs can be parsed even by a backtracking parser in linear time (with some constant factor) - but it will depend on the rules. I think you aspire to making the library "secure", and if that includes not having pathological performance in the presence of malicious input, you need to understand this stuff!

...

auto url = parse_url(str);

The problem is that url is a view of the string parameter, but nothing warns the user about that and...

Well, it is documented profusely. I agree there is some danger here but with great danger comes great power. Or something about great responsibility?

I don't think this is acceptable. What do others think?

...

I would hope that the path would be similar to a std::filesystem::path i.e. it should use the same vocabulary where possible (or could even actually be a std::filesystem::path, on C++17).

OOF I hope never to see this. std::filesystem is kind of a mess.

Well it's not my top favourite library either. But it's familiar, and the authors have considered many of the issues that have been mentioned in other threads i.e. whether path components include directory separators, distinguishing absolute and relative paths, etc. Having mentioned data: URLs above, I have tried to understand how these would be handled by your library. (mailto: also.) Note that a data: URL will generally contains /s, e.g. in the content type e.g. image/jpeg and also in the base64 data but these are not directory separators. I think what's really needed is a specific getter/setter for the body of an "unconventional" URL scheme like these. It may be possible to use your set_encoded_path() but it's not obvious. Please fix the docs for set[_encoded]_query() which currently seem confused with the operations for fragments. Regards, Phil.

On Thu, Oct 14, 2021 at 6:02 AM Dominique Devienne via Boost <boost@lists.boost.org> wrote:

You pay for copies only if you really want to. Maybe having parse_url_as_view(str) and a safer parse_url(str) using the former but not returning a view would satisfy you?

Remember that there are potentially many url types that have ownership of the buffer: url static_url dynamic_url<Allocator> pmr_url Which one would the safe_parse_url() return? Thanks

On Fri, Oct 15, 2021 at 5:15 AM Phil Endecott via Boost <boost@lists.boost.org> wrote:

Exactly, it's like returning a string_view from a function - which you should never do, at least not without an obvious indication...

All functions in url_view which return encoded parts have a string_view return type...

...

Maybe having parse_url_as_view(str) and a safer parse_url(str) using the former but not returning a view would satisfy you?

Yes, that's exactly what I suggested.

How would you convert this code to use "safer parse_url(str)"? url_view u1 = parse_relative_ref( "/" ).value(); url u2 = parse_relative_ref( "/" ).value(); static_url<1024> u3 = parse_relative_ref( "/" ).value(); pmr_url u4 = parse_relative_ref( "/" ).value(); dynamic_url< std::allocator<char>> u5 = parse_relative_ref( "/" ).value();

The alternative is to try to find some way to return something that can't be assigned to auto. This may be possible with something like a private_url_view class that wraps a view and makes everything private:

This sounds like an awful lot of try-hard. string_view is one of those controversial types which is going to split people into two camps. I'm of the camp that you have to be careful how you hold the knife. Rather than put the library through contortions with questionable degrees of effectiveness at solving the "problem" I think it is just easier and more straightforward to say that ownership of strings is simply not transferred using parse functions or url_view. Thanks

On Fri, Oct 15, 2021 at 9:04 AM Rainer Deyke via Boost <boost@lists.boost.org> wrote:

The following would do the trick:

auto u1 = parse_relative_ref<url_view>( "/" ).value(); auto u2 = parse_relative_ref<url>( "/" ).value(); auto u3 = parse_relative_ref<static_url<1024>>( "/" ).value(); auto u4 = parse_relative_ref<pmr_url>( "/" ).value(); auto u5 = parse_relative_ref<dynamic_url< std::allocator<char>>>( "/" ).value();

Yeah, kinda. This is not expert friendly but also, does not accommodate this use-case: struct connection { url u_; void process( string_view s ) { u_ = parse_uri( s ); ... } }; In the code above, the capacity for u_ is reused for each I/O cycle. Thanks

Vinnie Falco wrote:

On Fri, Oct 15, 2021 at 5:15 AM Phil Endecott via Boost <boost@lists.boost.org> wrote:

...

Exactly, it's like returning a string_view from a function - which you should never do, at least not without an obvious indication...

All functions in url_view which return encoded parts have a string_view return type...

Right. I think it's OK for a type whose name is *_view to have getters that return views. Actually Arthur O'Dwyer's blog post that I linked to before suggested that it would be OK for a getter method of any object to return a view whose lifetime is the same as the object - I'm not sure that I agree with that: auto old_host = url.host(); url.set_host( new_host ); // oops, old_host is invalid because it was a view.

How would you convert this code to use "safer parse_url(str)"?

url_view u1 = parse_relative_ref( "/" ).value(); url u2 = parse_relative_ref( "/" ).value(); static_url<1024> u3 = parse_relative_ref( "/" ).value(); pmr_url u4 = parse_relative_ref( "/" ).value(); dynamic_url< std::allocator<char>> u5 = parse_relative_ref( "/" ).value();

There are plenty of possibilities, including: 1. Verbose names that indicate the return type. 2. Out parameters for the result. 3. Template parameter for the result type. 4. Make these constructors for the url_* types, not a free function. These all have pros and cons, none are perfect. Here's another idea that has just occurred to me: 5. Base the return type on the input string type. I.e. if the input is a string_view, return a url_view; if the input is a string with a particular allocator, return a url with the same allocator etc. (Does this avoid all the possible nasty surprises?) In particular, if the input is a string&&, you can move that into the string stored by the url: url parse_url(std::string&& s) { url u; u.str = std::move(s); ... parse u.str ... return u; } In this case, if the caller writes: auto u = parse_url( get_string_from_somewhere() ); then there is no allocation, and u is a "safe" non-view type. This seems to work for this example:

struct connection { url u_;

void process( string_view s ) { u_ = parse_uri( s ); ... } };

s is a string_view, so the result of parse is a url_view, which you can assign to the url without re-allocation (subject to capacity). Regards, Phil

Vinnie Falco wrote:

To all, thanks for all the feedback. I'm not ignoring it, but we discovered a super duper annoying problem with how the path segments and query parameters containers represent their respective parts of the URL

I would be very interested to see a comparison of how your decomposition of paths into segments, and the reverse, compares to what std::filesystem::path does, and rationale for the differences.

abs("/././/", { ".", "", "" });

Well that's an interesting one. Why is the middle element "" not "."? Can you clarify? I might perhaps hope that something like this would work: for (auto dir: path.segments) { chdir(dir.c_str()); } i.e. I'd end up in the same directory as chdir(path.c_str()) would get me to. But chdir("") is an error. I believe that std::filesystem::path merges adjacent directory separators, so the "" segments disappear. Regards, Phil.

On 17/10/2021 07:53, Vinnie Falco wrote:

What should the resulting encoded URL be? Well a naive algorithm might leave us with "https://index.htm" but this is clearly wrong, as index.htm is now the host! To fix this, the library prepends "/." to the path (this is guidance from rfc3986):

assert( u.encoded_url() == "https:/.//index.htm" );

I assume this was intended to be "https://./index.htm"?

There are a handful of other cases like this (for example, removing the scheme from a relative-ref whose first segment has a colon). Coming back to:

abs("/././/", { ".", "", "" });

We treat a leading "/." as not appearing in the segments, to make the behavior of the library doing these syntactic adjustments transparent and satisfy the rule that assignments from segments produce the same result when iterated.

If you're stripping leading ./ then shouldn't the result just be "/" alone? Same reason that "/../../foo/../bar/" should become "/bar/".

On 18/10/2021 13:01, Vinnie Falco wrote:

...

...

assert( u.encoded_url() == "https:/.//index.htm" );

I assume this was intended to be "https://./index.htm"?

Nope, it was correct as I wrote it. You managed to produce an authority with a single dot :)

Yes, that was my intent, from your description of replacing the authority with a dot. Though I see the issue now. I was reading the input as: "https://example.com/index.htm" For which removing the authority should result in: "https:/index.htm" (Although this is unusual; typically relative URIs will omit the scheme as well.) For the input: "https://example.com//index.htm" Then it does make sense at a purely-URL-level to transform this to: "https:/.//index.htm" (Although most web servers would treat either as illegal, but you could envisage some not-HTTP protocol that requires such syntax.) Adding the authority back to this URL should result in "https://example.com/.//index.htm", however -- it should not be "ignoring" the prefix once it exists. At least not until the URL is normalised. (Unless you're documenting that URLs are always stored in normalised form, or that setters will automatically normalise.)

We treat a leading "/." as not appearing in the segments, to make the behavior of the library doing these syntactic adjustments transparent and satisfy the rule that assignments from segments produce the same result when iterated.

So what about the input "https://example.com/./index.htm"? Unless you're documenting automatic normalisation, this should still iterate the "." and "index.htm" path components separately.

On 17/10/2021 07:53, Vinnie Falco wrote:

On Sat, Oct 16, 2021 at 11:10 AM Phil Endecott wrote:

...

I would be very interested to see a comparison of how your decomposition of paths into segments, and the reverse, compares to what std::filesystem::path does, and rationale for the differences.

To be honest I have no idea, I have never used std::filesystem to any meaningful extent.

Getting back to this point, as I mentioned in another branch it looks like {std,boost}::filesystem breaks down "/foo/bar/baz.htm" to { "/", "foo/", "bar/", "baz.htm" }. Like it or hate it, this is the standard now, and it's likely that people would expect that Boost.Url would use similar segmentation. On a related note, you should strongly consider cross-compatibility with {std,boost}::filesystem::path anyway, since inevitably an URL-parsing library will hit one of the following scenarios sooner or later: - conversion of an absolute file:/// uri to a filesystem path and back - conversion of a relative uri path (only) to a relative filesystem path and back (or perhaps better: conversion of [absolute uri, base uri, base path] to absolute path) While you could get away with doing the second kind of interop only with complete strings, having an explicit method for it is better as it allows you to encourage security checks such as considering it illegal to use a relative path that would escape the base path.

On 18/10/2021 15:06, Vinnie Falco wrote:

On Sun, Oct 17, 2021 at 7:04 PM Gavin Lambert wrote:

...

Getting back to this point, as I mentioned in another branch it looks like {std,boost}::filesystem breaks down "/foo/bar/baz.htm" to { "/", "foo/", "bar/", "baz.htm" }.

I'm not sure that's viable or correct for URL paths though.

Actually, sorry, I was getting my wires crossed. The above is how .NET's System.Uri class behaves. (So Microsoft evidently thinks it's viable for URLs.) {std,boost}::filesystem will break down "/foo/bar/baz.htm" to { "/", "foo", "bar", "baz.htm" } (and "/foo/bar/" to { "/", "foo", "bar", "" } -- except boost:: returns "." for the final element). Filesystems perhaps have a slight advantage here as AFAIK no filesystem accepts an escaped path separator within a filename (modulo Linux allowing backslashes; including WSL, but the latter actually stores a different character), while there's no rule against %2F appearing inside an URL path (although it's more common in a query string). But this isn't insurmountable. You can perhaps argue that the leading "/" element is not needed if there is another method to indicate whether the URI is absolute or not; Filesystem includes it for lexicographic sorting & comparison purposes, but those don't entirely apply since we're only considering part of the URL here anyway. So that is more justifiable. With that in mind, it's probably reasonable for this iteration: - "/foo/bar/baz.htm" => { "foo", "bar", "baz.htm" } - "/foo/bar/" => { "foo", "bar", "" } - "/foo/bar" => { "foo", "bar" } - "/.//foo/bar" => { ".", "", "foo", "bar" } But as I said before, this makes it impossible to convert between relative and absolute paths via the path-segment API alone (as the initial "/" isn't exposed, and instead a different method must be used). Whether this is a feature or a bug is in the eye of the beholder.

...

On a related note, you should strongly consider cross-compatibility with {std,boost}::filesystem::path anyway, since inevitably an URL-parsing library will hit one of the following scenarios sooner or later:

Yeah, I agree that there needs to be something to facilitate working with paths that are formed from URLs. I haven't given it any thought yet because I am still trying to finish the baseline functionality.

It's worthwhile considering these things from the start, as they can inform design of your baseline (such as compatibility of path segment iteration).

On Sun, Oct 17, 2021 at 8:56 PM Gavin Lambert via Boost

...

It's worthwhile considering these things from the start, as they can inform design of your baseline (such as compatibility of path segment iteration).

Segment iteration is not going to be compatible. In addition to adding an initial "/" segment for absolute paths, Filesystem also collapses consecutive / separators. So iterating "/foo//bar//baz///" produces "/" │ "foo" │ "bar" │ "baz" │ "" (https://godbolt.org/z/EsjKzc5f1) A design goal of URL seems to be that the information that the accessors give accurately reflects the contents of the string (and that there's no hidden metadata that the string doesn't reflect.) So the segments of the above path are { "foo", "", "bar", "", "baz", "", "", "" } because otherwise the segments of the above and "/foo/bar/baz/" will be the same, which means that it won't be possible to reconstruct the string from the information the URL accessors give. That is, if you have a string, and you construct an URL object from it, then take its state as returned by the accessors, copy it into another empty URL, the resultant string should be the same as the original. And similarly, if you take an empty URL, set its parts to some values, take the string, create another URL object from the string, the accessors should give the original values. Destructive transformations such as what Filesystem does above cannot work in this model.

Peter Dimov wrote:

...

On Sun, Oct 17, 2021 at 8:56 PM Gavin Lambert via Boost

...

It's worthwhile considering these things from the start, as they can inform design of your baseline (such as compatibility of path segment iteration).

Segment iteration is not going to be compatible. In addition to adding an initial "/" segment for absolute paths, Filesystem also collapses consecutive / separators. So iterating "/foo//bar//baz///" produces

"/" ? "foo" ? "bar" ? "baz" ? ""

(https://godbolt.org/z/EsjKzc5f1)

A design goal of URL seems to be that the information that the accessors give accurately reflects the contents of the string (and that there's no hidden metadata that the string doesn't reflect.)

So the segments of the above path are

{ "foo", "", "bar", "", "baz", "", "", "" }

because otherwise the segments of the above and "/foo/bar/baz/" will be the same, which means that it won't be possible to reconstruct the string from the information the URL accessors give.

Right. But why has it chosen that goal, rather than the alternatives? What's the rationale? It seems to me that a URL with redundant /s (e.g. http://foo.com/path/////to/file) is either (a) malicious or erroneous input, or (b) equivalent to the versions without the redundant /s. So a user might want to (a) get an exception or error, or (b) ignore the redundant segments. Under what circumstances would a user want to see the empty segments between those /s? Here's an alternative: - Skip over duplicate adjacent '/' when iterating segments. - Return "/" as the first segment for absolute paths. - Return "" as the last segment for paths with a trailing "/". - Give p.push_back(s) a precondition that s must not be empty if p.back() is empty. I think this gives pretty sane behaviour. The invariant that push_back()ing a series of segments and then iterating returns the same strings holds. Vinnie Falco wrote:

note that the "absoluteness" of the path is a property of the URL which is reflected in the url API and not the segments:

You're saying this because that's what the BNF says. Your URL api doesn't have to exactly mirror the BNF. If it would make sense for "absoluteness" to be a property of the path rather than the URL from the point of view of the library user, you can do that. Two other things to consider: - What is your operator== going to do about redundant /s? - How does this all work with data: and mailto: URLs? Regards, Phil.

On 19/10/2021 05:10, Vinnie Falco wrote:

...

- "/.//foo/bar" => { ".", "", "foo", "bar" }

The list looks fine except for the above, which I think has to be { "", "foo", "bar" } for the reason that assigning the path should give you back the same results when iterated:

url u = parse_uri( "http:" ).value();

u.segments() = { "", "foo", "bar" };

assert( u.encoded_url() == "http:/.//foo/bar" ); assert( u.segments() == { "", "foo", "bar" } ); // same list

Again, what about the case where the original input URL contained that leading dot? You can't argue "we must report it unchanged" when by definition there are conditions when you are changing it. The only mechanism that seems sane to me is that encoded_url() and friends are documented to normalise (or at least to partially normalise, limited to adding/removing the path prefix) the URL before returning a string, at which point segments() may change content. (But it's important that it doesn't break if you push_back each segment individually instead of assigning it all at once.)

If we then remove the scheme, I think the library needs to remove the prefix that it added. Not a full "normalization" (that's a separate member function that the user calls explicitly). The rationale is that if the library can add something extra to make things consistent, it should strive to remove that something extra when it can do so. Yes this means that if the user adds the prefix themselves and then performs a mutation, the library could end up removing the thing that the user added. I think that's ok though, because the (up to 2 character) path prefixes that the library can add are all semantic no-ops even in the filesystem cases.

I don't disagree with this, but I do disagree with the iteration methods trying to "hide" elements that are actually present in the URL. On 19/10/2021 08:37, Peter Dimov wrote:

Segment iteration is not going to be compatible. In addition to adding an initial "/" segment for absolute paths, Filesystem also collapses consecutive / separators. So iterating "/foo//bar//baz///" produces

"/" │ "foo" │ "bar" │ "baz" │ "" Fair point; I hadn't considered that one. That's unfortunate. I agree that URL cannot collapse adjacent separators.

Ahmed Charles wrote:

On Friday, October 15th, 2021 at 5:14 AM, Phil Endecott via Boost <boost@lists.boost.org> wrote:

...

Exactly, it's like returning a string_view from a function - which you should never do, at least not without an obvious indication...

My primary objection to this line of thinking is that the standard begin/end functions have the same dangerous implications as this function does. The input can be a string and the output is something that becomes invalid when that string is destroyed.

begin/end don't have this problem in practice because you need to call both, and that's hard to do on the same temporary. c_str does, but virtually nobody makes this mistake for some reason.

Gavin Lambert wrote:

And the static analysis tooling is a bit better now at spotting such things, though not perfect.

Not really. https://godbolt.org/z/P8xb4Ern4 https://godbolt.org/z/Y56KnxGeT https://godbolt.org/z/M7aGb7jq8 https://pdimov.github.io/blog/2020/05/17/state-of-c-static-analysis-circa-20... https://godbolt.org/z/Pba64YjYv Well, there is https://godbolt.org/z/nz9Pvzfoj, I suppose. Oh, -fanalyzer has improved: https://godbolt.org/z/xhnr88ah1 It wasn't very useful for C++ last I checked, but it works now.

On 21/10/2021 14:13, Peter Dimov wrote:

Gavin Lambert wrote:

...

And the static analysis tooling is a bit better now at spotting such things, though not perfect.

Not really.

Yes, this was admittedly a small value of "a bit".

Ahmed Charles wrote:

On Friday, October 15th, 2021 at 5:14 AM, Phil Endecott via Boost <boost@lists.boost.org> wrote:

...

Exactly, it's like returning a string_view from a function - which you should never do, at least not without an obvious indication...

My primary objection to this line of thinking is that the standard begin/end functions have the same dangerous implications as this function does. The input can be a string and the output is something that becomes invalid when that string is destroyed.

I'd be happy with the parsing function returning a view if its name reflected that. My complaint is that the short name parse_url() doesn't give the user that "obvious indication". There are a couple of differences with begin()/end(). Firstly, "everyone knows" that begin() and end() return iterators so the names do give the required clue. Secondly, these are methods, not free functions; you can argue that it's OK to return iterator/pointer/view types from getter methods if the lifetime of the pointee is the same as the object. (Yes I'm conveniently overlooking the begin()/end() free functions.) I'd also suggest that the existence of foot-guns already in the language is not an excuse for adding more! These are not absolute, yes/no decisions; there are trade-offs to be made. The only benefit of naming this view-returning function parse_url() is less typing. I don't think that's worthwhile. Regards, Phil.

On Wed, Oct 13, 2021 at 7:10 AM Phil Endecott via Boost <boost@lists.boost.org> wrote:

Have you looked at the RFC 3986 errata at https://www.rfc-editor.org/errata_search.php?rfc=3986&rec_status=0?

Okay, this is taken care of now!!! Thanks

On Thu, Oct 14, 2021 at 11:35 AM Robert Ramey via Boost <boost@lists.boost.org> wrote:

Too me, all this argues for the usage of boost spirit as a url parser.

Never going to happen. I would never take a dependency on Spirit for anything network related or that will have security reviews. And users don't want this dependency either. Thanks

On Thu, Oct 14, 2021 at 11:00 PM Robert Ramey via Boost < boost@lists.boost.org> wrote:

On 10/14/21 11:38 AM, Vinnie Falco via Boost wrote:

...

On Thu, Oct 14, 2021 at 11:35 AM Robert Ramey via Boost <boost@lists.boost.org> wrote:

...

Too me, all this argues for the usage of boost spirit as a url parser.

Never going to happen. I would never take a dependency on Spirit for anything network related or that will have security reviews. And users don't want this dependency either.

Hmmm - how about this idea. Create all your tests and examples first. Basically this will consist of a long list of urls - each one more elaborate and indecipherable then the last. Write the spirit parse to run in these tests. When you get all this working, write your "hand-rolled" parser (this will take a while) and test against the same list of urls and verify that that the parsing matches the spirit generated ones. Then you'll have 2 things:

a) verifiable, maintainable, confirmation that the test parsing is very correct b) test results which show that your hand rolled parser is either correct or difficult to prove otherwise.

I think maybe even better is to take some other implementations from other languages(if they exist), maybe C# or Rust have official/unofficial equivalents of Boost.URL so one could do a "diff" against them.

Robert Ramey wrote:

On 10/13/21 7:09 AM, Phil Endecott via Boost wrote:

...

Back in 2005-ish I decided to write a URL parser based on the RFC BNF using Boost.Spirit. At that time the most recent RFCs were RFC2616 (1999) and RFC2396 (1998). I did a fairly direct translation of the BNF to Spirit and ... it didn't work. It turned out that the BNF in the RFCs was wrong, and had been for 6 years at that point. So I advise that you are careful about directly using the RFC BNF. ...

Too me, all this argues for the usage of boost spirit as a url parser. Maintenance, Verification, etc. Of a protocol with the problems you mention is much easier when the specification of the syntax is separated from the code which does the parsing.

Spirit is also a backtracking parser, so can't promise linear complexity - unless you can show that it does for the particular grammar that you are using. This isn't a performance issue, it's a security issue i.e. you don't want your application to suffer denial of service when it encounters malicious input. Regards, Phil.