Tim Starling has submitted this change and it was merged.
Change subject: noc: Refactor highlight.php to be simpler and <s>less</s> more
secure
......................................................................
noc: Refactor highlight.php to be simpler and <s>less</s> more secure
This is a second take at what I started 8 months ago (before this
was in version control) and followed-up on in I2d8a2a0ca8 but was
partially reverted in Idcec674abd due to bug 47112.
The main point of this refactor was to not mix logic with
symlinks and auto-guessting path destinations. Instead glob the
whitelist (the conf directory with symlinks) and only use that.
This was successfully implemented, but when I cleaned it up in
I2d8a2a0ca8 I made a stupid typo of a variable name: The foreach
loop variable $viewFilename of $viewFilenames conflicted with
the equally-named variable outside the loop that was supposed to
represent the sanitised version that is considered safe and in
the whitelist. As a result bug 47112.
I've now re-implemented this (without the typo, obviously) and
made it even stricter by using the whitelist exclusively for any
resolution. Not just to filter the user input, but also to
open the file (by reading the symlink).
This completely gets rid the logic that tries to find the file
in /home/wikipedia/common, which is why previously a simple typo
could cause bug 47112.
Also picked more descriptive variable names and added unit tests.
Change-Id: I56b455ebb8e3e083bb6390e03e350b69b902bc64
---
M docroot/noc/conf/highlight.php
A tests/noc-conf/highlightTest.php
2 files changed, 161 insertions(+), 43 deletions(-)
Approvals:
Tim Starling: Verified; Looks good to me, approved
jenkins-bot: Checked
diff --git a/docroot/noc/conf/highlight.php b/docroot/noc/conf/highlight.php
index 05553be..02446d1 100644
--- a/docroot/noc/conf/highlight.php
+++ b/docroot/noc/conf/highlight.php
@@ -1,81 +1,93 @@
<?php
// Only allow viewing of files of which there is a copy (or link)
// in noc/conf/* by the same name.
-$viewFilenames = array_map( 'basename', glob( __DIR__ . '/*' ) );
+$selectableFilepaths = glob( __DIR__ . '/*' );
-$srcFilename = $_GET['file'];
+// Name of file from user input
+$selectedFileName = $_GET['file'];
-$viewFilename = false;
-$srcDir = false;
+// Absolute path to file on disk
+$selectedFilePath = false;
-foreach ( $viewFilenames as $view ) {
- $src = substr( $view, -4 ) === '.txt'
- ? substr( $view, 0, -4 )
- : $view;
- if ( $srcFilename === $src ) {
- $viewFilename = $view;
+// Path to file in mediawiki-config repository
+$selectedFileRepoDirName = false;
+$selectedFileRepoPath = false;
+
+// Relative path to the symlink in conf/*
+$selectedFileViewRawUrl = false;
+
+foreach ( $selectableFilepaths as $filePath ) {
+ $fileName = basename( $filePath );
+ // Map .txt links to the original filename
+ if ( substr( $fileName, -4 ) === '.txt' ) {
+ $fileName = substr( $fileName, 0, -4 );
+ }
+ if ( $fileName === $selectedFileName ) {
+ $selectedFilePath = $filePath;
break;
}
}
+if ( PHP_SAPI !== 'cli' ) {
+ ob_start( 'ob_gzhandler' );
+ header( 'Content-Type: text/html; charset=utf-8' );
+}
-ob_start( 'ob_gzhandler' );
-header( 'Content-Type: text/html; charset=utf-8' );
-
-if ( !$viewFilename ) {
- # Secret site password distribution :-D
- # First implement access control
- if ( isset( $_SERVER['HTTP_REFERER'] ) && strpos(
$_SERVER['HTTP_REFERER'], 'google' ) !== false ) {
+if ( !$selectedFilePath ) {
+ if ( PHP_SAPI !== 'cli' ) {
header( "HTTP/1.1 404 Not Found" );
+ }
+ if ( isset( $_SERVER['HTTP_REFERER'] ) && strpos( strtolower(
$_SERVER['HTTP_REFERER'] ), 'google' ) !== false ) {
echo "File not found\n";
exit;
}
- # OK, authenticated developer, send password
+ // Easter egg
$hlHtml = highlight_string( '<'."?php\n\$secretSitePassword =
'jgmeidj28gms';\n", true );
+
} else {
- $baseSrcDir = '/home/wikipedia/common';
+ // Follow symlink
+ if ( !file_exists( $selectedFilePath ) ) {
+ $hlHtml = 'Whitelist contains inexistant entry. :(';
+ } elseif ( !is_link( $selectedFilePath ) ) {
+ $hlHtml = 'Whitelist must only contain symlinks.';
+ } else {
+ $selectedFileViewRawUrl = './' . urlencode( basename(
$selectedFilePath ) );
+ // Resolve symlink
+ // Don't use readlink since that will return a path relative to
where the symlink is.
+ // Which is a problem if our PWD is not the same dir (such as
in unit tests).
+ $selectedFilePath = realpath( $selectedFilePath );
+ // Figure out path to selected file in the mediawiki-config
repository
+ $selectedFileRepoPath = ( dirname( $selectedFilePath ) ===
'wmf-config' ? 'wmf-config/' : '' ) . $selectedFileName;
- // Find where it is
- if ( file_exists( "$baseSrcDir/wmf-config/$srcFilename" ) ) {
- $srcPath = "$baseSrcDir/wmf-config/$srcFilename";
- $srcDir = 'wmf-config/';
- } elseif ( file_exists( "$baseSrcDir/$srcFilename" ) ) {
- $srcPath = "$baseSrcDir/$srcFilename";
- $srcDir = '';
- }
+ if ( substr( $selectedFileName, -4 ) === '.php' ) {
- // How to display it
- if ( $srcDir !== false ) {
- if ( substr( $srcFilename, -4 ) === '.php' ) {
-
- $hlHtml = highlight_file( $srcPath, true );
+ $hlHtml = highlight_file( $selectedFilePath, true );
$hlHtml = str_replace( ' ', ' ', $hlHtml ); //
https://bugzilla.wikimedia.org/19253
$hlHtml = str_replace( ' ', "\t", $hlHtml ); //
convert 4 spaces to 1 tab character; bug #36576
} else {
- $hlHtml = htmlspecialchars( file_get_contents( __DIR__
. '/' . $srcFilename ) );
+ $hlHtml = htmlspecialchars( file_get_contents(
$selectedFilePath ) );
}
- } else {
- $hlHtml = 'Failed to read file. :(';
}
$hlHtml = "<pre>$hlHtml</pre>";
}
-$titleSrcFilename = htmlspecialchars( $srcFilename );
-$urlSrcFilename = htmlspecialchars( urlencode( $srcDir . $srcFilename ) );
-$urlViewFilename = htmlspecialchars( urlencode( $viewFilename ) );
+$selectedFileNameEsc = htmlspecialchars( $selectedFileName );
+$selectedFileRepoPathEsc = htmlspecialchars( $selectedFileRepoPath );
+$selectedFileViewRawUrlEsc = htmlspecialchars( $selectedFileViewRawUrl );
?>
<!DOCTYPE html>
<html lang="en">
<head>
- <title><?php echo $titleSrcFilename; ?></title>
- <link rel="stylesheet" href="/base.css">
+ <title><?php echo $selectedFileNameEsc; ?> - Wikimedia configuration
files</title>
+ <link rel="shortcut icon" href="//bits.wikimedia.org/favicon/wmf.ico">
+ <link rel="stylesheet" href="../base.css">
</head>
<body>
-<h1><a href="./">«</a> <?php echo $titleSrcFilename; ?></h1>
+<h1><a href="./">«</a> <?php echo $selectedFileNameEsc; ?></h1>
<?php
-if ( $srcDir !== false ) :
+if ( $selectedFilePath !== false ) :
?>
-<p>(<a
href="https://gerrit.wikimedia.org/r/gitweb?p=operations/mediawiki-config.git;a=history;f=<?php
echo $urlSrcFilename; ?>;hb=HEAD">version control</a> • <a
href="https://gerrit.wikimedia.org/r/gitweb?p=operations/mediawiki-config.git;a=blame;f=<?php
echo $urlSrcFilename; ?>;hb=HEAD">blame</a> • <a href="./<?php echo
$urlViewFilename; ?>">raw text</a>)</p>
+<p>(<a
href="https://gerrit.wikimedia.org/r/gitweb?p=operations/mediawiki-config.git;a=history;f=<?php
echo $selectedFileRepoPathEsc; ?>;hb=HEAD">version control</a> • <a
href="https://gerrit.wikimedia.org/r/gitweb?p=operations/mediawiki-config.git;a=blame;f=<?php
echo $selectedFileRepoPathEsc; ?>;hb=HEAD">blame</a> • <a href="<?php
echo $selectedFileViewRawUrlEsc; ?>">raw text</a>)</p>
<?php
endif;
?>
diff --git a/tests/noc-conf/highlightTest.php b/tests/noc-conf/highlightTest.php
new file mode 100644
index 0000000..ddd0a65
--- /dev/null
+++ b/tests/noc-conf/highlightTest.php
@@ -0,0 +1,106 @@
+<?php
+
+class NocConfHighlightTest extends PHPUnit_Framework_TestCase {
+ private $created = array();
+
+ protected function setUp() {
+ parent::setUp();
+
+ $wmfConfigDir = dirname( dirname( __DIR__ ) ) . '/wmf-config';
+ $this->nocConfDir = dirname( dirname( __DIR__ ) ) .
'/docroot/noc/conf';
+
+ // Created various files to test with
+ if ( !file_exists( "$wmfConfigDir/AdminSettings.php" ) ) {
+ $this->created[] = "$wmfConfigDir/AdminSettings.php";
+ file_put_contents( "$wmfConfigDir/AdminSettings.php",
'<?php $forbiddenFruit = "a";' );
+ }
+
+ if ( !file_exists( "$wmfConfigDir/PrivateSettings.php" ) ) {
+ $this->created[] = "$wmfConfigDir/PrivateSettings.php";
+ file_put_contents( "$wmfConfigDir/PrivateSettings.php",
'<?php $forbiddenFruit = "p";' );
+ }
+
+ $this->created[] = "$wmfConfigDir/ExampleValid.php";
+ file_put_contents( "$wmfConfigDir/ExampleValid.php", '<?php
$smoigel = "v";' );
+
+ $this->created[] = "$wmfConfigDir/ExampleInvalid.php";
+ file_put_contents( "$wmfConfigDir/ExampleInvalid.php", '<?php
$forbiddenFruit = "x";' );
+
+ $this->created[] = "{$this->nocConfDir}/ExampleContent.php.txt";
+ file_put_contents(
"{$this->nocConfDir}/ExampleContent.php.txt", '<?php forbiddenFruit =
"content";' );
+
+ $this->created[] = "{$this->nocConfDir}/ExampleContent.txt";
+ file_put_contents( "{$this->nocConfDir}/ExampleContent.txt",
'forbiddenFruit=txt-content' );
+
+ $this->created[] = "{$this->nocConfDir}/ExampleContent";
+ file_put_contents( "{$this->nocConfDir}/ExampleContent",
'forbiddenFruit=content' );
+
+ $this->created[] = "{$this->nocConfDir}/ExampleValid.php.txt";
+ symlink( "$wmfConfigDir/ExampleValid.php",
"{$this->nocConfDir}/ExampleValid.php.txt" );
+ }
+
+ protected function tearDown() {
+ foreach ( $this->created as $created ) {
+ unlink( $created );
+ }
+ parent::tearDown();
+ }
+
+ public static function provideValidCases() {
+ return array(
+ array( 'langlist', 'zh-classical', 'From root, without
extension' ),
+ array( 'all.dblist', 'enwiki', 'From root, dblist file'
),
+ array( 'ExampleValid.php', 'smoigel', 'From wmf-config,
dblist file' ),
+ );
+ }
+
+ /**
+ * @dataProvider provideValidCases
+ */
+ public function testValidCases( $q, $expect, $msg) {
+ $this->assertContains(
+ $expect,
+ $this->runHighlight( $q ),
+ "file=$q should work ($msg)"
+ );
+ }
+
+ public static function provideInvalidCases() {
+ return array(
+ array( 'search-redirect.php' ),
+ array( 'robots.txt' ),
+ array( 'README' ),
+ array( 'wmf-config/AdminSettings.php' ),
+ array( 'wmf-config/PrivateSettings.php' ),
+ array( 'wmf-config/ExampleFile.php' ),
+ array( 'AdminSettings.php' ),
+ array( 'PrivateSettings.php' ),
+ array( 'ExampleInvalid.php' ),
+ array( 'ExampleContent.php', 'must only contain
symlinks' ),
+ array( 'ExampleContent', 'must only contain symlinks' ),
+ array( 'ExampleContent.txt' ),
+ );
+ }
+
+ /**
+ * @dataProvider provideInvalidCases
+ */
+ public function testInvalidCases( $q, $expect = 'secretSitePassword' ) {
+ $this->assertContains(
+ $expect,
+ $this->runHighlight( $q ),
+ "file=$q should be 404"
+ );
+ }
+
+ /** @return string Page output */
+ protected function runHighlight( $q ) {
+ $_GET = array(
+ 'file' => $q
+ );
+ ob_start();
+ require( $this->nocConfDir . '/highlight.php' );
+ $out = ob_get_clean();
+ return $out;
+ }
+}
--
To view, visit https://gerrit.wikimedia.org/r/59034
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I56b455ebb8e3e083bb6390e03e350b69b902bc64
Gerrit-PatchSet: 7
Gerrit-Project: operations/mediawiki-config
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
Gerrit-Reviewer: Hashar <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: MZMcBride <[email protected]>
Gerrit-Reviewer: Tim Starling <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
