search

jellyfin / jellyfin-plugin-nextpvr

https://jellyfin.org
MIT License
25 stars 9 forks source link

Increase sid security #19

Closed emveepee closed 3 years ago

emveepee commented 3 years ago

Add sid on Live TV. Allow files instead of URLs for recordings.

emveepee commented 3 years ago

@joshuaboniface still not clear on the release and review process . I submitted this in January and again zero feedback. This security change is required for the next version of NextPVR and it would be nice to see it included before the release

emveepee commented 3 years ago

The Sid is poorly named, but it is global to the addon and it is used for all API calls. It is created on login https://github.com/jellyfin/jellyfin-plugin-nextpvr/blob/c593a946f6493fc3e8505f2aca389f9438446c14/Jellyfin.Plugin.NextPVR/LiveTvService.cs#L103 and there is a heartbeat call the keeps it active while the server is up. https://github.com/jellyfin/jellyfin-plugin-nextpvr/blob/c593a946f6493fc3e8505f2aca389f9438446c14/Jellyfin.Plugin.NextPVR/LiveTvService.cs#L683

If the server is stopped the token is saved server side but there is additional reconnect logic to acquire a new one if it is invalid https://github.com/jellyfin/jellyfin-plugin-nextpvr/blob/c593a946f6493fc3e8505f2aca389f9438446c14/Jellyfin.Plugin.NextPVR/LiveTvService.cs#L79

cvium commented 3 years ago

No one asked or commented on the Sid name, intent or otherwise. You've added sid to the query parameters but forgot to add {3} after sid=.

Which I suppose also begs the question: Did you test it properly?

emveepee commented 3 years ago

No one asked or commented on the Sid name, intent or otherwise. You've added sid to the query parameters but forgot to add {3} after sid=.

Right I see now that is a good catch, a copy and paste issue. Updated.

Which I suppose also begs the question: Did you test it properly?

Clearly no. I only tested this in Emby and only recently where I actually do have it set correctly. In January it was unit tested but it was only on spec at the time and the missing sid was not a requirement.

Unfortunately the changes to the addon from core in 10.7.0 make it impossible to diff Emby and Jellyfin

emveepee commented 3 years ago

@oddstr13 @cvium I did submit the fix is more information required, the new version of NextPVR is going to be out soon