Localization file (en only) for img_auth

attachment img_auth.i18n.php ignored as obsolete

Installation Instructions for testers:

1. Copy img_auth.php into ($IP) dir (overwrites existing)
2. Copy img_auth.i18.php into ($IP) dir (where img_auth.php) resides

Note: First "* See if could create the title object" in check order of description is redundant, and a typo. Real check is right before hook.

Couple quick notes:

First, the first parameter to wfForbidden() seems to always be the "access forbidden" message; it's probably cleaner to just call that from the function. :)

The debug log messages shouldn't be localized; those are internal messages which should be consistently readable by site administrators in a multilingual environment so they can debug issues.

With $wgImgAuthDetails on, input filenames are being passed into HTML error messages without validation or escaping; this is a script injection vuln. wfMsgHTML() escapes the text of the message, then replaces in your parameters -- the expectation being that your parameters are formatted HTML such as links.

Also we'd generally want config vars like this defined in DefaultSettings.php so they can be consistently located.

I'm a little vague on what the hook accomplishes; if meant for alternate file repository types, it'll fail as we've already dropped out a 403 result due to the file not existing in $wgUploadDirectory... It looks like the only thing it could do is reject access to local files which would otherwise have been allowed.

Probably if alternate source backends are desired here (say, database storage or a WebDAV storage backend), they'd need their own implementation on the repository class for checking path validity and doing the output streaming.

(In reply to comment #3)

Couple quick notes:
First, the first parameter to wfForbidden() seems to always be the "access
forbidden" message; it's probably cleaner to just call that from the function.
:)

There are two parts to the HTML 403 message, the header and the body.

• msg1 is header
• msg2 is body

Would be glad to change, but many gov sites have a standard header text
such as:

• We're big bad mothers so don't mess with us

Followed by standard body text

• The knock you're hearing on the door is our black suited, dark eyeglass goon

squad who are going to take you to a dark place and do terrible things to you.

I'd be glad to reduce the customization, but thought, what the heck.

The debug log messages shouldn't be localized; those are internal messages
which should be consistently readable by site administrators in a multilingual
environment so they can debug issues.

When I updated the messaging, I thought, hey, some people may want to give out
more detailed messages than "you can't get here", so I added that capability
with the localization. As long as that message was being sent to the user,
thought it might make sense to send the exact same message to the log file.

With $wgImgAuthDetails on, input filenames are being passed into HTML error
messages without validation or escaping; this is a script injection vuln.
wfMsgHTML() escapes the text of the message, then replaces in your parameters

• the expectation being that your parameters are formatted HTML such as links.

Good point about the script vunerability. Don't know how I missed that one.
Unless you have a better approach, I think I'll just take out the "$1" capability
in all the messages.

Should I be using straight wfMsg instead of wfMsgHTML too? Other suggestions
(or someone who might help without bothering you?)

Also we'd generally want config vars like this defined in DefaultSettings.php
so they can be consistently located.

I think that img_auth.php isn't used in the majority of MediaWiki installations.
Adding that parameter to the generic is something that would only be used if
img_auth (rewriting) is used. If you think it's better there, I'm all for it.
This way, only used when needed.

I'm a little vague on what the hook accomplishes; if meant for alternate file
repository types, it'll fail as we've already dropped out a 403 result due to
the file not existing in $wgUploadDirectory... It looks like the only thing it
could do is reject access to local files which would otherwise have been
allowed.
Probably if alternate source backends are desired here (say, database storage
or a WebDAV storage backend), they'd need their own implementation on the
repository class for checking path validity and doing the output streaming.

You saw straight through me on this one. Yes, I was intending to use it on
NSFileRepo, which does namespace validation. That implementation, based on
Tim Starling's recommendation, is an extension and alternate
"backend" source - and yes, I used a new repository class to do it.

With this hook, would be able to use this extension and possibly future acess
rejections without branching img_auth.php.

See http://www.mediawiki.org/wiki/Extension:NSFileRepo

(In reply to comment #3)

With $wgImgAuthDetails on, input filenames are being passed into HTML error
messages without validation or escaping; this is a script injection vuln.
wfMsgHTML() escapes the text of the message, then replaces in your parameters

Revisiting this one. I used wfMsgHTML() which has htmlspecialchars() escaping in it.
I may be displaying my ignorance here, but wouldn't that avoid any injection by
displaying it as a string versus allowing the injection of html links, javascript, etc.

This would actually allow the admin to view what the injection attack was, rather than
allow it to proceed. I'll admit I'm no expert here, so this might be dead wrong.

Would also need to inform hook users to do same in hook documentation.

(In reply to comment #3)

With $wgImgAuthDetails on, input filenames are being passed into HTML error
messages without validation or escaping; this is a script injection vuln.
wfMsgHTML() escapes the text of the message, then replaces in your parameters

Ah, stupid me - disregard previous comment. Would this solve that problem (wherever used)?

wfForbidden(wfMsgHTML('image_auth-accessdenied'),wfMsgHTML('image_auth-noread',htmlspecialchars($name)));

Patch File of final

Tested and ready for code review if acceptable

attachment img_auth.patch ignored as obsolete

Complete Tested code - ready for code review

attachment img_auth.php ignored as obsolete

Had an idea that would allow late loading of 18n file.

Instead of invoking wfMsgHTML before calling forbidden, could wait til after then add the 18n require_once in wfForbidden and wfPublicError.

That way the only time the 18n file would load would be when an error has occured. Down-side would be that hook extensions would have to use only message ID's, not actual messages - but I'm not sure that's bad.

You could use a wrapper function, eg:
function wfImgAuthMsg() {
static $fileloaded = false;
if (!$fileloaded) {

require_once( dirname( __FILE__ ) . '/img_auth.i18n.php' );
$fileloaded = true;

}

return call_user_func('wfMsgHTML', func_get_args() );

}

Updated img_auth.php using suggestions from code review

1. Only loads message file if error encountered (should be rare)
2. All logged messages are in english - user messages localized.
3. Errors return message index instead of error messages - actual interpretation of message in wfForbidden.
4. Eliminated all caching, compressed both error messages routines into a single routine.

attachment img_auth.patch ignored as obsolete

Complete Tested code - ready for final code review before commit to trunk

attachment img_auth.php ignored as obsolete

Additional Messages rqd for img_auth script

attachment MessagesEn.patch ignored as obsolete

Message Instructions for localization

attachment MessagesQqq.patch ignored as obsolete

Moved messages and default settings to language and DefaultSettings.php respectively

Will still have to add the following to DefaultSettings.php

$wgImgAuthDetails = false; /< defaults to false - only set to true if you use img_auth and want the user to see details on why access failed
$wgImgAuthPublicTest = true; /< defaults to true - if public read is turned on, no need for img_auth, config error unless other access is used

attachment img_auth.patch ignored as obsolete

Complete Tested Code ready for commit to trunk

attachment img_auth.php ignored as obsolete

Unified Diff for bug patches

Unified Diff for all required patches

attachment Bug19646.patch ignored as obsolete

(In reply to comment #13)

Created an attachment (id=6519) [details]
Additional Messages rqd for img_auth script

Is there some reason to use ` instead of " what is used elsewhere in MediaWiki?

(In reply to comment #17)

Created an attachment (id=6523) [details]

+if (!wfRunHooks( 'ImgAuthBeforeStream', array( &$title, &$path, &$name, &$result ) ) )
+ call_user_func_array('wfForbidden',merge_array(array($result[0],$result[1]),array_slice($result,2)));
Maybe a short comment here about the type of $result wouldn't hurt. If this is a new hook, it should be documented in docs/hooks.txt.

+'img-auth-accessdenied' => "[[Image Authorization]] Access Denied",
Is the link supposed to go somewhere? Documentation is also wikitext. It looks like you did not update maintenance/language/messages.inc, but hey, other developers forget to update it too.

I'm happy that you provided message documentation and that you pass around message keys instead of strings. It is a bit more flexible if you ever want to change something, like support magic words in the translations.

+'img-auth-nopathinfo' => "Missing PATH_INFO. Your server is not set up to pass this information - may be CGI-based and can't support img_auth. See Image Authorization on MediaWiki.",
Manual:x on MediaWiki.org?
+'img-auth-notindir' => "Requested path not in upload directory.",
is not in the upload directory?
+'img-auth-badtitle' => "Unable to construct a valid Title from $1.",
Title with lowercase, users don't need to care if it is Object for us or not.
+'img-auth-nologinnWL' => "Not logged in and $1 not in whitelist.",
"You are not logged in ... is not in the whitelist"
+'img-auth-nofile' => "$1 does not exist.",
Maybe prefix with File, to give a hint what is expected
+'img-auth-isdir' => "$1 is a directory.",
Same here, you could say we except a file, but directory was given.

Diff Patches for bug

1. Added to messages.inc,used miscellaneous3 block.
2. Followed Niklas's suggestions and modified messages

If all is OK, will Commit.

On Commit, will document:

1. ImgAuthBeforeStream hook
2. $wgImgAuthDetails
3. wgImgAuthPublicTest

attachment img_auth_all.patch ignored as obsolete

Hrm... this code is *really* nasty looking and needs to be reverted or cleaned up.

What's with the call_user_func_array() stuff everywhere? If you need to pass more variable parameters, just have it take an array... Don't try to squish random params onto wfMsgHtml() when you can simply use wfMsgExt() which will take an array of options and an array of message arguments.

Reverted in r56051.

Cleaned up code from revert

Cleaned up code:

1. Removed all call_user_func_array(), used $arg array
2. Removed all htmlspecialchars() for file name escaping
3. Reapplied DefaultSettings.php
4. Reapplied messages.inc
5. Reapplied MessagesEn.php and MessagesQqq.php
6. Reapplied RELEASE-NOTES
7. Tested

Don't know how to handle reapplying all the internationalization patches - whether that is automatic or whether I should do it manually, but will find out on IRC before committing.

attachment img_auth_all.patch ignored as obsolete

Much cleaner code. :) The one additional recommendation (posted this in IRC, not sure if you saw):

my one other recommendation is to change the wgMsgHtml() here to wfMsg() and do an htmlspecialchars() on the outside yourself (or possibly this can be done with one of the escaping options on wfMsgExt() -- we want to HTML-escape after argument replacement, whereas wfMsgHtml() does the escaping before argument replacement)

The rest of the localizations got reverted in r56053; you can re-merge them like so:

svn merge -r56053:56052 .

then do a commit as normal.

Final Touches and localisation files

1. Replaced wgMsgHtml() with htmlspecialchars(wfMsg()). Did not change wfMsgExt() because it is only used to output to log file - which cannot be injected.
2. Merged all localisation files back in.
3. Tested

Attached: