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

Modified URLs to use https rather than http #13

Closed
wants to merge 1 commit into from
Closed

Modified URLs to use https rather than http #13

wants to merge 1 commit into from

Conversation

tkeuning
Copy link

No description provided.

@tkeuning
Copy link
Author

This builds on the SSL fix in #12 by changing the URLs in use in the plugin from http to https

@mddepew
Copy link

mddepew commented Oct 23, 2018

I haven't had the chance to pull and test the change, but in theory it shouldn't be a problem to switch those URLs to https. It should be noted that the only way to make SSL work was to use older ciphers and limited certificate validation. The implementation in the current code is NOT secure. I don't think that's a problem for the type of traffic we have here. It's not like clients are sending financial information.

@tkeuning
Copy link
Author

Agreed about the security -- this is not ideal, but in light of the switch PBS kids seems to have made to enforce https, and the SSL capabilities in Plex, it seems to be a reasonable shortcut.

As for testing -- in my local testing, I was unable to get results with the state of the master code as-is; changing to https made it work for me. Would it be helpful if those test results (screenshots or logs) were attached to the PR?

@mddepew
Copy link

mddepew commented Oct 24, 2018

That's interesting. I'm running the master code on my system and it seems to be working. If you can post log files showing what errors you're getting with the current master code that would be helpful. That's mostly out of curiosity as to why the master code isn't working for you. I would recommend that the pull request be accepted as is though. I don't have write authority on this repo, so @sander1 will have to be the one to make the merge.

@tkeuning
Copy link
Author

And having gone back to review what I saw so I could add logs to the PR, I realize that my initial problems have disappeared... I must have been using an older code base unwittingly.

I'm going to close this my initial assessment of the need for change was unfounded.

Thanks!

@tkeuning tkeuning closed this Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants