r98430 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98429‎ | r98430 | r98431 >
Date:19:00, 29 September 2011
Author:btongminh
Status:ok (Comments)
Tags:
Comment:
(bug 30202) Restrict file names on upload to 240 bytes, because wfTimestamp( TS_MW ) . '!' . $fileName should fit in oi_archive_name, which is 255 bytes, and also the maximum file name length on many file systems is 255 bytes.

Commit to fix UploadTest to use @dataProvider will follow
Modified paths:
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/includes/upload/UploadBase.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/upload/UploadTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/upload/UploadBase.php
——@@ -631,6 +631,14 @@
632632 }
633633 $this->mFilteredName = $nt->getDBkey();
634634
 635+ # oi_archive_name is max 255 bytes, which include a timestamp and an
 636+ # exclamation mark, so restrict file name to 240 bytes. Return
 637+ # ILLEGAL_FILENAME, just like would have happened for >255 bytes
 638+ if ( strlen( $this->mFilteredName ) > 240 ) {
 639+ $this->mTitleError = self::ILLEGAL_FILENAME;
 640+ return $this->mTitle = null;
 641+ }
 642+
635643 /**
636644 * We'll want to blacklist against *any* 'extension', and use
637645 * only the final one for the whitelist.
Index: trunk/phase3/tests/phpunit/includes/upload/UploadTest.php
——@@ -60,6 +60,16 @@
6161 $this->assertUploadTitleAndCode( '.jpg',
6262 null, UploadBase::MIN_LENGTH_PARTNAME,
6363 'upload title without basename' );
 64+
 65+ /* A title that is longer than 255 bytes */
 66+ $this->assertUploadTitleAndCode( str_repeat( 'a', 255 ) . '.jpg',
 67+ null, UploadBase::ILLEGAL_FILENAME,
 68+ 'upload title longer than 255 bytes' );
 69+
 70+ /* A title that is longer than 240 bytes */
 71+ $this->assertUploadTitleAndCode( str_repeat( 'a', 240 ) . '.jpg',
 72+ null, UploadBase::ILLEGAL_FILENAME,
 73+ 'upload title longer than 240 bytes' );
6474
6575 }
6676 /**
——@@ -134,6 +144,7 @@
135145 public function testTitleValidation( $name ) {
136146 $this->mTitle = false;
137147 $this->mDesiredDestName = $name;
 148+ $this->mTitleError = UploadBase::OK;
138149 $this->getTitle();
139150 return $this->mTitleError;
140151 }
Index: trunk/phase3/RELEASE-NOTES-1.19
——@@ -107,6 +107,8 @@
108108 * Per page edit-notices now work in namespaces without subpages enabled.
109109 * (bug 30245) Use the correct way to construct a log page title
110110 * (bug 31081) $wgEnotifUseJobQ causes many unnecessary jobs to be queued
 111+* (bug 30202) File names are now restricted on upload to 240 bytes, because of
 112+ restrictions on some of the database fields.
111113
112114 === API changes in 1.19 ===
113115 * (bug 19838) siprop=interwikimap can now use the interwiki cache.

Follow-up revisions

RevisionCommit summaryAuthorDate
r99224Follow-up r98430, use dedicated error message for filename too long error. Ad...btongminh18:20, 7 October 2011

Comments

#Comment by Nikerabbit (talk | contribs)   05:20, 30 September 2011

And now we have y10k problem ;)

#Comment by Hashar (talk | contribs)   17:30, 28 November 2011

What happens now with old files that used more than 240 chars ?

#Comment by Bryan (talk | contribs)   17:39, 28 November 2011

They continue to work, but can't be uploaded or overwritten anymore, and deleting them will result in undefined behavior. Perhaps we have to write a maintenance script to truncate files with too long names.

#Comment by TheDJ (talk | contribs)   22:12, 9 January 2012

Does it break "upload a new version", for these older files ? Do we have lists of how many files currently have names with over 240 byte names ?

#Comment by Bryan (talk | contribs)   08:31, 10 January 2012

"upload a new version" would break indeed.

#Comment by Hashar (talk | contribs)   14:27, 19 January 2012

Marking "new" again. We have too look at the breakage reported above. The files made invalid can not be overwritten, deleted anymore.

Maybe oi_archive_name (from r1284 ) should be made longer? :-)

#Comment by Bryan (talk | contribs)   14:38, 19 January 2012

Well, they could not be properly overwritten before anyway. Presumably before this revision if you would overwrite an affected file, your original file would just get lost. I don't know what the original behaviour for deletion was, but I suspect that it was already broken. The only functionality that we should support on files longer than 240 bytes is moving and redirecting imho. So, if it can be confirmed that that still is possible, I believe that this revision is ok.

#Comment by MarkAHershberger (talk | contribs)   20:44, 24 January 2012

> The only functionality that we should support on files longer than 240 bytes is moving and redirecting imho. So, if it can > be confirmed that that still is possible, I believe that this revision is ok.

Could you confirm this and then mark it "new"?

#Comment by Bryan (talk | contribs)   21:43, 27 January 2012

Can't test this, because either my file system or the file backend refuses to create files with filenames larger than 240 bytes.

#Comment by Hashar (talk | contribs)   16:15, 31 January 2012

You could lower the hardcoded 240 bytes limit to something lower. Maybe 64 ? :-D

#Comment by Bryan (talk | contribs)   20:41, 5 February 2012

Confirmed that this still works.

#Comment by Duplicatebug (talk | contribs)   19:53, 22 January 2012

What is about file moves from shorter to longer names over 240 bytes? It is better to restrict that also.

Someone with db connection can select all files now and add a section to the local village pump on each wiki. The local community can than fix this problem by moving the files (when the images are unused or bad quality, they should first move and than delete). After this revision is deployed that select should run a second time to find new uploaded files in the mean time.

#Comment by Bryan (talk | contribs)   21:08, 22 January 2012

Good point about the file moves. That reminds me that the checks from Title::isValidFileMoveOperation() should really be merged with those from UploadBase::verifyTitle() etc.

Status & tagging log