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

escape uri of path in get_file_path for http(s), ftp(s), dav(s) #792

Closed
wants to merge 6 commits into from

Conversation

omelettedufromagee
Copy link

@omelettedufromagee omelettedufromagee commented Nov 29, 2023

This change aims to import a working path into Kodi if the network shared path is using http, https, ftp, ftps, dav or davs protocol.
Currently, if a path contains a space, it is not replaced by %20 and becomes unreadable for Kodi.
requote_uri function ensures that the uri's special characters are properly encoded and passed safely.

…davs

This change aims to import a working path into Kodi if the network shared path is using http, https, ftp, ftps, dav or davs protocol.
Copy link

sonarcloud bot commented Nov 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@oddstr13
Copy link
Member

oddstr13 commented Feb 5, 2024

Could you please provide tests for this? A list of expected inputs -> outputs, both what's now which is incorrect, and what's correct would do, I can implement the tests myself. :)

@omelettedufromagee
Copy link
Author

omelettedufromagee commented Feb 5, 2024

Sure. Below some examples with https, it is also valid for protocols http, ftp, ftps, dav and davs.

Current behavior of path:
https://myserver.local/TV_Shows/My TV Show/My.TV.Show.S01E01.mkv
https://myserver.local/TV_Shows/Law&Github/Law&Github.S01E01.mkv

This results in Kodi trying to open and play "https://myserver.local/TV_Shows/My" or to use a literal ampersand in the URL which my break the call if you are using a webserver.

requote_uri passes the given URI through an unquote/quote cycle to ensure that it is fully and consistently quoted.

New behavior of path:
https://myserver.local/TV_Shows/My%20TV%20Show/My.TV.Show.S01E01.mkv
https://myserver.local/TV_Shows/Law%26Github/Law%26Github.S01E01.mkv

Moreover, I've been tested this for the past few months, I haven't got any issue. If anything, it strengthens the output to make sure a valid URL is generated for those protocols.
This resolves my issue of playing file through the Jellyfin websocket from Kodi, it is slower/buggier with big files. It works flawlessly when playing a file directly from the webdav or ftp server.

@omelettedufromagee
Copy link
Author

Btw, #767 is similar so you should be able to close it aswell with this PR.

@oddstr13
Copy link
Member

oddstr13 commented Feb 5, 2024

Btw, #767 is similar so you should be able to close it aswell with this PR.

Yes, I was planning to.

Could you test this with files that actually do contain escape sequences in the file names? Sounds like that might potentially cause issues.

@oddstr13 oddstr13 added Native Mode Issue is related to Native playback mode bugfix needs-testing Some testing is requested/required before merging labels Feb 6, 2024
@KyleSanderson

This comment was marked as abuse.

@KyleSanderson

This comment was marked as spam.

@oddstr13

This comment was marked as off-topic.

@KyleSanderson

This comment was marked as off-topic.

@mcarlton00

This comment was marked as off-topic.

Copy link

sonarcloud bot commented May 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 21.90%. Comparing base (6cc0bb6) to head (69f44d6).
Report is 14 commits behind head on master.

Current head 69f44d6 differs from pull request most recent head d5fb95d

Please upload reports for the commit d5fb95d to get more accurate results.

Files with missing lines Patch % Lines
jellyfin_kodi/helper/api.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #792   +/-   ##
=======================================
  Coverage   21.90%   21.90%           
=======================================
  Files          63       63           
  Lines        8634     8637    +3     
  Branches     1587     1588    +1     
=======================================
+ Hits         1891     1892    +1     
- Misses       6722     6724    +2     
  Partials       21       21           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented Sep 10, 2024

@KyleSanderson

This comment was marked as abuse.

@oddstr13

This comment was marked as off-topic.

@jellyfin jellyfin locked as too heated and limited conversation to collaborators Sep 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bugfix Native Mode Issue is related to Native playback mode needs-testing Some testing is requested/required before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants