Opened 3 years ago

Last modified 8 months ago

#12983 assigned bug

BUrl issues with certain URL strings

Reported by: humdinger Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: Kits/Network Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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.

Attachments (2)

Weather-1170-debug-03-10-2016-10-04-21.report (20.1 KB ) - added by humdinger 3 years ago.
crash of Weather when changing location
SelectionWindow.cpp (3.8 KB ) - added by khyati-agarwalss 8 months ago.
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.

Download all attachments as: .zip

Change History (18)

by humdinger, 3 years ago

crash of Weather when changing location

comment:1 by vidrep, 3 years ago

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

comment:2 by humdinger, 3 years ago

#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.

comment:3 by humdinger, 3 years ago

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?

comment:4 by pulkomandy, 3 years ago

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);

comment:5 by axeld, 3 years ago

Owner: changed from axeld to nobody
Status: newassigned

in reply to:  4 comment:6 by axeld, 3 years ago

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?

comment:7 by pulkomandy, 3 years ago

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.

by khyati-agarwalss, 8 months ago

Attachment: SelectionWindow.cpp added

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.

comment:8 by pulkomandy, 8 months ago

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

comment:9 by khyati-agarwalss, 8 months ago

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

comment:10 by pulkomandy, 8 months ago

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.

comment:11 by khyati-agarwalss, 8 months ago

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

comment:12 by pulkomandy, 8 months ago

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.

comment:13 by khyati-agarwalss, 8 months ago

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 "

comment:15 by pulkomandy, 8 months ago

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.

comment:16 by khyati-agarwalss, 8 months ago

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

Note: See TracTickets for help on using tickets.