ecleese / plexWatchWeb

A web front-end for plexWatch.
255 stars 41 forks source link

Use separate CURLOPT_USERNAME and CURLOPT_PASSWORD for auth #154

Closed caelor closed 9 years ago

caelor commented 9 years ago

This allows complex passwords to not be mangled by CURL's USERPWD handling.

Arcanemagus commented 9 years ago

Remove the newline at the end of the file, as it currently stands this will break all images proxied from Plex.

I have no issue with splitting it up into the individual headers, I'm just curious what problems you running into that this fixes?

caelor commented 9 years ago

Newline now removed in commit bab7189 - it had been automatically added by vi, and I'd missed when checking the diff.

On a number of the systems that I look after for others, the primary account holders have switched to using complex passwords on their accounts following the recent Plex forum breach. In some cases, those passwords contained the ":" character, and it seems that using this in the password then confuses CURL's separation of the username and password (for some combinations of PHP/CURL, but not for others).

The intended CURL behaviour of CURLOPT_USERPWD is that it does a forward search on the combined string for ":", and hence this would only normally be an issue if the username contained a colon. I'm not sure if a valid Plex username could contain a colon. I'm not sure what other parsing CURL may do of the USERPWD option, but there's also a chance that characters from a strong (e.g. including symbols that are often used as separators and escapes) password could cause problems.

Explicitly passing the username and password as separate CURLOPT_ options gave me consistent behaviour across a range of PHP and CURL versions.

See http://stackoverflow.com/questions/4753648/problems-with-username-or-pass-with-colon-when-setting-curlopt-userpwd for somebody reporting the same issue (although they later reported they identified their root cause to be using an incorrect auth scheme - I don't know if they also upgraded away from a problematic php/curl combination before finding their auth-scheme cause).

Arcanemagus commented 9 years ago

I had seen that as well when trying to find examples of this and ignored it as a different issue :wink:

Is this working on your system? I'm getting errors that those constants aren't defined, and I can't find them on http://php.net/manual/en/function.curl-setopt.php...

caelor commented 9 years ago

Yes, it's working fine here - the only warning I get is regarding ANSI_X3.4-1968 not being supported on line 649 of settings.php.

I'm running PHP 5.5.14, CURL 7.32.0 (CentOS packaged builds) on my primary dev system.

I agree, I also don't see them on the PHP page, but they are defined upstream in libcurl: http://curl.haxx.se/libcurl/c/CURLOPT_USERNAME.html and http://curl.haxx.se/libcurl/c/CURLOPT_PASSWORD.html.

I wonder if I've been lucky with the versions of php-curl that I've been testing with, and that they've included constants over and above the documented set.

On my system, CURLOPT_USERNAME==10173, and CURLOPT_PASSWORD==10174.

Would you prefer a (slightly more complex) change that falls back to using CURLOPT_USERPWD if CURLOPT_USERNAME or CURLOPT_PASSWORD are not defined?

Arcanemagus commented 9 years ago

Since those constants don't exist on my system (5.4) and I've run into issues before where I have been using things too new for some of the users, that is probably a good idea.

Testing this now.

Arcanemagus commented 9 years ago

Put the comment on the next line (inside the if) so it's not all the way out at 115 characters and I'll merge it :wink:.

caelor commented 9 years ago

Thanks, commit 07b5b8a moves the comment to the next line.

Arcanemagus commented 9 years ago

Thanks for getting this working!

Arcanemagus commented 9 years ago

Btw, the error you are seeing in settings.php shouldn't be happening, by any chance is your default_charset not set (correctly)?

http://php.net/manual/en/ini.core.php#ini.default-charset

If it isn't something obvious can you file an issue on that so it can get fixed?

caelor commented 9 years ago

@Arcanemagus quite right, that warning is due to default_charset not being set. No issue to resolve there (other than fixing my php.ini)!