r109227 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r109226‎ | r109227 | r109228 >
Date:22:00, 17 January 2012
Author:aaron
Status:resolved (Comments)
Tags:filebackend, swift 
Comment:
Added php-cloudfiles (verbatim) with a thin MediaWiki extension wrapper around it. Custom modifications will also be maintained here.
Modified paths:
  • /trunk/extensions/SwiftCloudFiles (added) (history)
  • /trunk/extensions/SwiftCloudFiles/SwiftCloudFiles.i18n.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/SwiftCloudFiles.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10 (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/.gitignore (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/AUTHORS (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/COPYING (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/Changelog (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/README (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/cloudfiles.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/cloudfiles_exceptions.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/cloudfiles_http.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/changelog (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/compat (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/control (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/copyright (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/dirs (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/docs (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/examples (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/install (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/debian/rules (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/classtrees_php-cloudfiles.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/elementindex.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/elementindex_php-cloudfiles.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/errors.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/index.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/li_php-cloudfiles.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/php-cloudfiles (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/php-cloudfiles/CF_Authentication.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/php-cloudfiles/CF_Connection.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/php-cloudfiles/CF_Container.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/php-cloudfiles/CF_Object.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/docs/php-cloudfiles/_cloudfiles.php.html (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/phpdoc.ini (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/phpunit.xml (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/share (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/share/cacert.pem (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/share/magic (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/share/magic.mgc (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/share/magic.mime (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/share/magic.mime.mgc (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/tests (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/tests/Authentication.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/tests/Comprehensive.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/tests/FileDetection.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/tests/General.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/tests/UTF8.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/tests/cloudfiles_ini.php (added) (history)
  • /trunk/extensions/SwiftCloudFiles/php-cloudfiles-1.7.10/tests/common.php (added) (history)

Diff [purge]

The diff is too large to display.

Follow-up revisions

RevisionCommit summaryAuthorDate
r109434r109227: Consistency tweaks in preparation for adding extension to translatew...raymond21:23, 18 January 2012
r109437r109227: Register extension for translatewiki.net.raymond21:26, 18 January 2012
r109984r109227: Use dirname( __FILE__ ) in php-cloudfiles for code inclusion stateme...aaron02:21, 25 January 2012
r110020r109227: prevent tests from running via web requestsaaron19:42, 25 January 2012
r110439Follow-up r109227: use dirname() in another placeaaron22:15, 31 January 2012

Comments

#Comment by Tim Starling (talk | contribs)   22:22, 24 January 2012

I think CloudFiles' use of finfo opens up a broad attack surface for security vulnerabilities. MediaWiki should specify a content type, using the same algorithm as that used by StreamFile.php.

#Comment by Aaron Schulz (talk | contribs)   23:04, 24 January 2012
#Comment by Aaron Schulz (talk | contribs)   23:07, 24 January 2012

Gah, that was the md5sum problem actually.

#Comment by Tim Starling (talk | contribs)   22:36, 24 January 2012

CF_Object::compute_md5sum() acts as a single entry point for hashing both files (by path) and strings. It does is_file() to decide which action to take. So if the file is missing for some reason, the result will be counterintuitive, and if you use it to hash a string, it will attempt to do syscalls to check if huge bogus paths exist.

#Comment by Tim Starling (talk | contribs)   22:42, 24 January 2012

define()s at the top of cloudfiles.php and cloudfiles_http.php may conflict with other extensions, especially USER_AGENT, DESTINATION, AUTH_TOKEN. Suggest a CF_ prefix on all. The author does not appear to be aware of the global scope of constants in PHP, as opposed to the file scope of such constants in C.

#Comment by Aaron Schulz (talk | contribs)   23:05, 24 January 2012

The require() statements should also use dirname( __FILE__ ) to avoid the "current directory" pitfall.

#Comment by Tim Starling (talk | contribs)   23:07, 24 January 2012

Also the tests directory might be slightly scary if the .php files can be executed from the web. Otherwise OK.

Status & tagging log