Netocrat wrote:
On Mon, 28 Nov 2005 12:40:56 -0800, Brion Vibber wrote:
Netocrat wrote:
[incorrectly patching Title.php/getLocalURL() and getFullURL() to encode ampersands]
This patch is incorrect, and will cause broken URLs to be output throughout the wiki.
Too hasty - I didn't notice that there were already escaped versions of these functions and that unescaped versions were necessary.
As a general rule, escaping should be done as close as possible to the actual use. It's part of the file format or communications protocol in your output, not a part of your data, so you want to keep them separate to avoid unnecessarily hobbling code that should be more flexible.
URLs may go to different outputs, not always HTML; for instance HTTP headers, plaintext output, generated JavaScript code, etc, will all have different requirements from XML/HTML text.
Early escaping is also dangerous, since additional processing or just forgetting where you got some value can end up producing either corrupt data (such as double-escaping) or security vulnerabilities from missing escaping (SQL injection, HTML/JavaScript injection, etc).
Instead, you should locate the individual *output* of the bad URL that you found and patch *that* to properly HTML-encode its output.
I based part of the extension on code from BoardVote.php, which doesn't use the escaped url function to generate the action of a form. So the patch is not very significant anyhow but this is what it should have been:
[patch snipped]
Thanks, I've applied it to CVS.
-- brion vibber (brion @ pobox.com)