|
[
Permalink
| « Hide
]
Eloy Lafuente (stronk7) added a comment - 07/Dec/07 12:51 AM
+5 for this. Sam is familiar with it.
 Just guessing, also... if we should make timezones information download to use componentlib instead of own code.
I have coded fixes for these using Snoopy and $CFG->proxyhost/port, but am currently unable to test as our developer server has just gone down.
Once it comes back (either later this evening, or tomorrow, I should think) I'll test & commit to 1.9 once the code works. Just in case you haven't seen it as I think its fairly new, there is download_file_content in filelib which I fixed recently to work properly though proxy.
I have changed this as described, tested (I can install language packs now! Yay!), and committed to 1.9 and HEAD.
Eloy's suggestion about using componentlib might be correct but I am leaving that as an exercise for the viewer oops, the download_file_content() is the right way, it uses either curl extension or PHP emulation
my -1 for using snoopy, looking at the code there should be an empty() test to prevent potential warnings during upgrades/installs my SoC student was supposed to get rid of magpie&snoopy and replace it with download_file_content() and simplepie, unfortunately he did not even start Uhm... I really don't know why Snoopy is so bad... is it?
Anyway, if the UNIFIED solution is to use download_file_content() everywhere, oki, let's go. This also makes me think what we have the curl extension in the environmental checks is there is one pure-php alternative. Any insight? Hi,
the pure PHP alternatives have limitation - both snoopy and that curl emulation stuff - speed, url fopen, socket read, ssl, ect. The emulation is not good enough for mnet, that is why it is why the real curl is tested in environmnet check. Curl is official PHP extension which is supposed to do this kind of stuff that is why I originally worked on the integration of emulation and a wrapper function. I will change to use download_file_content, certainly it is better to do things the same way.
Unfortunately, the very helpful comment pointing out this function, which I didn't know about, crossed over with me checking it in and going home. OK, done. I have changed all three places to use download_file_content. (This is much nicer, I was actually thinking there should be a function for it, even if using snoopy...) Only thing I don't like about it is the name, I wish it was 'contents'.
 Anyway.
I retested (made it download timezone file, imported a language pack.) It seems to work. Thanks Sam and Dan!
...and Eloy too :-P Currently in 1.9, timezone download is working OK for me but language packs are not. Is this a regression?
— Notice: Undefined offset: 3 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 4 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 5 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 6 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 7 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 8 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 9 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 10 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 11 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 12 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 13 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 14 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 15 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 16 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 17 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 18 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Notice: Undefined offset: 19 in /moodle/19/lib/libcurlemu/libcurlexternal.inc.php on line 535 Downloaded file check failed. Ah, this failure is on PHP4 only ... my PHP5 servers work fine. Perhaps a PHP5-ism has crept in to MOODLE_19_STABLE?
maybe there is a curl PHP extension in your PHP5
I really think that the key here is the nature of files being downloaded:
and it seems that the libcurl-emu (when executing external OS curl executable), returns information "inline" with some extra info attached (sizes, times... are concatenated to the binary result). I guess that curl-emu code then tries to strip that attached info and it causes the binary contents to become garbled. So perhaps, when using external executable, we should separate both that statistical info and the binary file itself to prevent that garbled files. Once done, those .zip binary files should arrive perfectly to Moodle. In fact, that would be perfect because I've another bug ( Ciao Hmm. I think that its screwing with the data and altering the hash even when not using the OS executable
Ahh.. and the native implemententation of libcurl-emu doesn't support proxies!
 How about something like this patch to use snoopy when curl isn't available for the time being (since the curl emulated proxy support isn't good)
Well, if I'm not wrong, the target was to avoid using Snoopy completely and move any file-download to download_file_content.() that will use curl exclusively, in any of its 3 variants:
1- CURL PHP Extension. I think Petr was going to take a look to this (in order to have 2 & 3 working better than now) because it severely breaks lang & environment updates under 1.9 (and timezones soon too). So let's wait if he finds the way inside 100% curl of we need to add something else to the 1), 2), 3) above. Ciao Hi,
if anybody has need for proxy he/she should install curl in the first place, as a bonus curl extension works with several types of proxies. Another problem is exploits of url fopen for remote file inclusions, we should not recommend it enabled - url fopen and register globals on is a fatal combination
I should be back tomorrow working full time on this problem... I agree. Is it too much to require curl extension? For 2.0?
The component downloading is not essential IMO, you can always workaround it by uploading of files manually.
It should work for majority of ppl out of the box, but proxy is not used in majority of installs. I think we can make it rock solid if curl installed and usable if not in 1.9. Yep, with curl-extension installed, everything will be rock-solid.
But we must guarantee that download_file_content() works (in a limited way, agree) for normal servers. Right now it's broken for binary files in normal servers. I think we all share this objective, just to confirm it, ok? About making the extension mandatory, I wouldn't do that. Just highly recommended. Nothing else. Perhaps in the admin->proxy settings we can re-recommend the extension. Just that. And only if the OS command curl emulation doesn't provide it (I think it does). my +1 Eloy, in fact I already started tweaking the admin settings (adding CURL warning, password, username, proxy type)
1/ binary safe emulation After hours of trying to fix issues in curl emulation library I have to agree with Dan: using snoopy when cURL extension not present is much, much easier and works in majority of cases.
I have removed the cURL emulation library from CVS, used snoopy if cURL not present, added all admin settings with some explanation, added new debug messages and did some other improvements. Everything tested with anonymous proxy http and SOCKS5 servers + local squid installation. Going to review all uses of fopne, snoopy and new download_file_content() now... Oki, so then, the fallback for download_file_content() is, simply:
1) PHP Curl extension correct? I think we have a proxy lying around in the office with NTLM auth setup on it so I'll give it a bit of testing next week.
Any reason for the proxy password not to be an unmask field?
yep, the reason is I forgot to add that :-D
I'd like to suggest we also use the http 1.0 response codes for error messages rather than the 1.1 messages (RFC1945 / 2616)
We have a transparent proxy which returns 302 to be interpreted as 'Moved Temporarily' and that error is much more of an error than 'Found'! But furthermore we can't assume things support 1.1 and also most apps I use tend to default to 1.0 errors because of this [Read: wget moans to me about Moved Temporarily and I know to explicitly set the proxy, found seems a bit.. eh if its found give it me] Feel free to submit patches for better error messages, I am going to work on other parts now
Its only 302 which conflicts with anything else in 1.0, so i've fixed that in CVS
So, then.... this can be considered resolved yep?
If so, great work! Else, too! :-D Ciao reclosing, thanks everybody
please reopen again in case of any problems reopenening again, going to add snoopy emulation using curl extension and patch magpie to use it, that should solve RSS+proxy
reclosing again, please test - I hope there will not be any regressions
changes:
eh, reopening - have to fix curl related security issues
reclosing again, the download_file_content() should be much more secure now
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug
Resolved
Minor
Fix various components that don't respect http proxy settings
 |
|
