Properly escape query parameters sent to Kodi

[anohren]

Currently neither the raw URL nor the optional youtube-dl options (including separating whitespace) are percent encoded in the iOS app afaik. That would require URLComponents to which I couldn't find a reference. This is probably not done in the other clients either.

This can create complications before the parameters reach the plugin.

It also requires changes in the plugin which is why I mention it here.

[anohren]

@nullket

I think I continued the discussion completely in the wrong issue. Here's the right place for it.

Sure:

So, currently the video URL is sent to Kodi (in a Json string as part of Kodi's JSON RPC API) on this format: plugin://plugin.video.sendtokodi/?video url non-escaped space non-escaped JSON string containing "extra parameters"

By "non-escaped" I mean not percent encoded.

The video url isn't properly escaped for a url query parameter, the space isn't escaped at all, and neither is the following json containing YouTube-do options.

My idea is that we should support a single properly formatted URL as input, instead of this pseudo-command line splitting of arguments with whitespace. I imagine it should maybe be possible without breakage if we simply parse the whole input properly as a url iff there's no literal space character in the input string we get from Kodi.

I pinged you so that you could catch any potential oversight with this idea.

The only worry I can think of is if the (completely unescaped) video URL (sent from an old app) itself has query parameters with the same name as the parameter names we decide to use when implementing this. I think that the risk of that is negligible, if kept in mind.

[anohren]

And the problem I experience is that the non-percent encoded parameter string (in JSON format) might contain the sequence \/ which is just a JSON escaped / (it's not even necessary to escape that character, but still allowed).

E.g. the ytdl format string best[ext=mp4]/best relies on using /.

By the time the plug-in JSON decodes the parameters this substring has for some reason changed to //. Sometimes ytdl/ytdlp chokes on that.

My hypothesis is that when Kodi sees certain sequences containing backslash on unix it's trying to help by converting it before forwarding what it thinks is a URL...? I doubt that'd happen if we used a valid URL.

[anohren]

You basically describe this issue in your comment: #54 (comment)

[nullket]

That makes totally sense.

We could still implement a mechanism to identify and parse the „old way“ like now. Maybe even show a deprecated warning in case the new way works well and can be integrated in other tools (kore, chrome extension etc).

I do not fully understand the Kodi API yet, but instead of building a complicated string, can we not just use the JSON RPC directly and nicely separate everything in the „params“ section of the ExecuteAddon function instead of building a long URL?

[anohren]

We could still implement a mechanism to identify and parse the „old way“ like now.

Yeah, I thought that since we already have the parsing part, the main trap is the "identify" part.

I do not fully understand the Kodi API yet, but instead of building a complicated string, can we not just use the JSON RPC directly and nicely separate everything in the „params“ section of the ExecuteAddon function instead of building a long URL?

I subconsciously avoid the API docs 😄 so I can't answer straight away, but you make a good point that I've only focused on what I know we control in the request (the item field) and haven't taken a step back to look at any alternative.

I'll take a look at that function and see if I can make heads and tails of it. Thanks for the suggestion.

[anohren]

Ok, I made a shortcut calling this function instead. Preliminary test:

1. I could make a video resolve without code change on addon side (didn't play, but might be unrelated)
2. Unfortunately when using this function the main Kodi GUI is affected: it enters the non-existent menu tree node that this plugin represents.

We're basically now being bitten by the fact that we're a plugin, when we probably should be a script. That's another topic we've discussed before 🙂

I don't know but I'd guess that it's not possible for us to tell Kodi to not enter the menu node that our plugin constitutes, upon execution of ExecuteAddon.

Edit:
Note to myself, because I always forget: parameter 2 is the query part of the invocation URL:

The query string passed to your add-on, e.g. '?foo=bar&baz=quux'

(This detail doesn't really change the concerns expressed so far)

[anohren]

Okay, the results are:

1. Can't play video. setResolvedUrl probably has no effect when Kodi isn't explicitly calling us as part of a playback operation. I also tried to execute the built in PlayMedia, but that resulted in the"known issue" "two concurrent busydialogs", which in true Kodi tradition means: "The application will exit"
2. Entering the plugin in the GUI is a really bad side-effect, not least because any time you navigate (up) between these fake menu nodes the plugin will run again...

So I think it's not feasible until after conversion from plugin to script.

[nullket]

I think 1. can maybe somehow be solved with changes to the python script. Nonetheless, the 2. issue is not acceptable.

I assume a conversion would required manually removing the old version and reinstalling the addon again. I don’t know if that is really user friendly.

While not my favorite solution, we might should stick to the current way and need to find a good pattern how the plugin-url could look like to include the media-url (with page specific parameters given by the page) as well as plugin parameters (including yt-dlp options). In the end that should be solvable by a good encoding. Maybe the media-url should be just the last element so we do not get in conflict with page specific parameters which might be present in the media url.

[anohren]

The user friendliness of installing is already not the best, so I think it's safe to say that those who have jumped through that hoop already are capable of doing so again in the future if someone decides to convert this and release it with a new addon ID. The bigger problem is that it doesn't sound dev friendly to convert it just for a marginally cleaner RPC call — there'd have to be a list of benefits to be worth it :) for both sides. Those mentioned so far in the repo seem marginal.

But it's not like it would hurt anyone just to try it if it sounds like an interesting experiment. It would create the freedom to do...anything you can think of, like resolving URLs proactively in the background (another thing we've talked about I think), etc.

/Someone who has never written a script addon

Yes, I don't think you have to worry about the media URL interfering — it'll have its special characters percent encoded. Its parameters are only a concern when identifying whether to use new or old parsing, but that should be solvable simply by requiring the new input url to have certain non-optional parameters with SendToKodi-specific names. Missing parameter = old format.

[firsttris]

The addon now supports a proper query-parameter format using url-encoded values, including yt-dlp options via the dedicated yt-dlp-options parameter.

Example:
plugin://plugin.video.sendtokodi/?url=<urlencoded_media_url>&yt-dlp-options=<urlencoded_json_object>

Legacy input is still supported for backward compatibility (including ydlOpts), but the recommended format is now the encoded query-param style above.

[anohren]

Nice. I took a look, but since there were no PR I had to choose the commit that looked the most relevant – maybe you've changed it since then.

I found it difficult to read the handling of addon parameters because everything was called query, query_params, query_part, parse, parse, parse, etc, combined with my doubt at every point of whether "query" referred to the query part of the plugin URL, or whether it referred to the query part of the video URL (since I knew of at least one use of video URL parameters), but here's a try:

1. You introduced the addon query parameter url, which is, I imagine, a not entirely uncommon query parameter name. What happens if you receive this legacy format: plugin://plugin.video.sendtokodi/?http://example.com/channel/myVideo?p1=v1&url=example.com/someUnrelatedGarbage? Which URL will it choose?
2. Related to 1. The following function seems to treat both legacy parameters and new properly percent encoded parameters the same:
   

   plugin.video.sendtokodi/core/addon_params.py

   Lines 55 to 62 in 9561e41

   	def parse_query_params(paramstring):
   	if not paramstring or not paramstring.startswith("?"):
   	return {}
   	query_part = paramstring[1:]
   	if " " in query_part:
   	query_part = query_part.split(" ", 1)[0]
   	return urllib.parse.parse_qs(query_part)

   
   It looks as if you intend to send both the entire legacy unencoded video URL and the new properly encoded query string to parse_qs. Maybe it works – I'm not that familiar with it – but it doesn't seem right to me. Surely that function is only supposed to accept either a full URL or only the query component of an URL, but not both?