Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[url] don't use # as separator (fixes #16066) #7326

Merged
merged 1 commit into from Jul 3, 2015

Conversation

Projects
None yet
6 participants
@mkortstiege
Copy link
Member

commented Jun 22, 2015

I am not actually sure if # is a valid separator. Fixes an issue reported in http://trac.kodi.tv/ticket/16066.
@arnova, @Paxxi for view and comments please.

@Montellese

This comment has been minimized.

Copy link
Member

commented Jun 22, 2015

# is a valid hierarchical separator in an URI just like / and ?, see https://tools.ietf.org/html/rfc3986#section-1.2.3. The part of the URI after a # is called fragment. As an example HTML pages use them for anchor links.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2015

OK, so i have to check the darn Parser :)

@mkortstiege

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2015

@Montellese do you happen to know why it's used for zip and apk? Instead of messing around with the Parser that is known to work, we could move zip and apk to the block where only ? is considered as a valid separator.

@mkortstiege mkortstiege force-pushed the mkortstiege:fix-cbz branch from 909ce91 to c8ea1da Jun 22, 2015

@mkortstiege

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2015

Updated. Now all our internal archive paths are handled the same.

@arnova

This comment has been minimized.

Copy link
Member

commented Jun 22, 2015

This feels like deja-vu. I think Montellese and I had the exact same conversation a few months ago about a simular (same?) issue. I think we need more intelligent ways of detecting separators in urls.

@Paxxi

This comment has been minimized.

Copy link
Member

commented Jun 22, 2015

@Montellese yes it is technically a separator but does it count as one for our use cases? In http it's never sent to a server and only used for in-page navigation by the browser. I don't know of any other protocols that use it.

@mkortstiege I don't see anything wrong with this change but I'd rather defer it for J** since it touches a sensitive part of the code and it's hard to be sure about how safe the change is.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2015

Any objections to shove this one in?

@arnova

This comment has been minimized.

Copy link
Member

commented Jul 3, 2015

Seems safe enough.

mkortstiege added a commit that referenced this pull request Jul 3, 2015

Merge pull request #7326 from mkortstiege/fix-cbz
[url] don't use # as separator (fixes #16066)

@mkortstiege mkortstiege merged commit e836b9f into xbmc:master Jul 3, 2015

@mkortstiege mkortstiege deleted the mkortstiege:fix-cbz branch Jul 3, 2015

@MartijnKaijser MartijnKaijser added this to the Isengard 16.0-alpha1 milestone Jul 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.