BUrl issues with certain URL strings

[humdinger]

This is hrev50563.

The app "Weather" keeps crashing when changing the location for the forecast, see attached report.

The crash happens in ​SelectionWindow::_FindId().

BUrlRequest* request =
	BUrlProtocolRoster::MakeRequest(BUrl(urlString.String()),
	&listener);

The BUrl(urlString.String()) doesn't appear to result in a valid BUrl, its UrlString() returns empty.

urlString.String() returns a url of the format:

https://query.yahooapis.com/v1/public/yql?q=select+woeid+from+geo.places(1)+where+text+="Munich,+Germany"&format=json

Looks like BUrl rejects the string. I did work in the past, so I did a binary search and found out that the regression happened between hrev50357 and hrev50448.

[humdinger]

crash of Weather when changing location

[vidrep]

There are a pair of related bug reports on the Haikuarchives page for Weather.

[humdinger]

​#14 of Weather's issue tracker is what's described here. ​#13 reports that same crash when putting the characters <, > and " into the location text view. BUrl apparently don't like those <> either.

[humdinger]

Apparently, it was Andrew's change in hrev50362. It introduced ​_Delimiter(), which fouls things up on among others <, > and ". There's a ​Todo for the missing error handling. :)

Shouldn't there be some sort of "escaping" to make the url compliant?

Anyway, I guess Weather can be changed to not use " in the url and filter out those pesky characters when entered in the location text view.

Close the ticket, or is it still valid to change the handling?

[pulkomandy]

These characters are currently not allowed in an URL. The restriction apparently comes from ​https://tools.ietf.org/html/rfc3986#appendix-C , but I don't see a reason why BUrl should reject the URLs because of that.

These checks were added to help HaikuDepot check and validate URLs, but enforcing them everywhere does not look like an useful feature.

Possible workaround on HaikuWeather side is to URL-Encode the request (there may be extra code to write, did not test this):

BUrlProtocolRoster::MakeRequest(BUrl(BUrl::UrlEncode(urlString)), &listener);

[axeld]

Replying to pulkomandy:

but I don't see a reason why BUrl should reject the URLs because of that.

Why should BUrl allow to create invalid URLs? If one isn't interested in validity, one should a string instead, no?

[pulkomandy]

In our API, it is allowed to build the URL from unescaped strings, and then percent-encode it (resulting in a valid, well formed URL string).

According to the RFC:

Under normal circumstances, the only time when octets within a URI
are percent-encoded is during the process of producing the URI from
its component parts.  This is when an implementation determines which
of the reserved characters are to be used as subcomponent delimiters
and which can be safely used as data.  Once produced, a URI is always
in its percent-encoded form.

This means we should remove UrlEncode, and instead have a parameter to most of the functions (constructor from string, and all the Set*) to specify if the component we are adding is already percent-encoded. And conversely, all getters should automatically percent-decode the components.

[khyati-agarwalss]

I think the angle brackets are not a problem. The actual problem was with the backslash. I replaced it with %5C because it is being used on most of the sites on the internet. Please correct me if I'm wrong.

[pulkomandy]

It is not possible to easily see what you changed if you just send the whole file. Please submit changes using Gerrit.

[khyati-agarwalss]

I have cloned the Weather directory in my system. Please tell me its exact location where it should be placed inside Haiku folder.

[pulkomandy]

I'm not sure what you want to do. Weather comes with its own makefile, so you go in the directory you cloned from Terminal and type 'make'. You should get a binary generated.

[khyati-agarwalss]

When I'm trying to push the change, it is pushing my code into github instead of gerrit.

[pulkomandy]

Yes, Weather is hosted and developed on Github, so you have to use the Github pull-request flow for it. You cannot send Weather patches on Gerrit.

[khyati-agarwalss]

In that process, I'm getting this error: "remote: Permission to HaikuArchives/Weather.git denied to khyati-agarwalss. fatal: unable to access '​https://github.com/HaikuArchives/Weather.git/': The requested URL returned error: 403 "

[khyati-agarwalss]

Please check this: ​https://github.com/khyati-agarwalss/Weather/pull/1/files

[pulkomandy]

There is a patch to solve this on BUrl side here: ​https://review.haiku-os.org/c/haiku/+/1193 I prefer this approach, instead of making changes in Weather to try to avoid limitations of the API.

[khyati-agarwalss]

There was an error in the Weather which I thought should be fixed. Otherwise, I guess athira has solved this problem.

[smallstepforman]

With hrev54954, the following string cannot be passed as an argument to BUrl constructor

BUrl url("​https://fonts.googleapis.com/css?family=Noto+Sans:400,400i,700,700i"); url.IsValid() will return false

Eg. From Haiku's own home webpage, we encounter the following: <link attr="stylesheet" href="​https://fonts.googleapis.com/css?family=Noto+Sans:400,400i,700,700i" </link>

Using Haiku's own BUrl class fails to parse a link from Haiku own homepage :)

[pulkomandy]

Patch ​https://review.haiku-os.org/c/haiku/+/1193 would solve this but it still has not been applied because it changes the ABI

The website could be fixed to use the urlencoded form of the URL (using % escaping to remove special characters).

[smallstepforman]

Thanks Adrien for quick response. I'm sure that you would agree that a class which cannot parse thousands of URL's in the wild eliminates its usefulness. I think that should influence any decision about whether to break ABI or not. No application developer can use a broken class anyway ...

[pulkomandy]

Yes, that's why the change is there. But it needs some coordination since the class is in the support kit and now used by many apps.

We will try to get it integrated in some way in beta3 if we can (probably similarly to the other ongoing changes to the network code, with some backwards compatibility system)