Skip to content

Commit f5c8977

Browse files
joshtrichardsjoshtrichards
authored
refactor(setup): improve htaccess update handling and code clarity
Optimize the .htaccess and data-dir protection updates and more defensive. - make the write idempotent so we only update file when the generated block actually changes - more robust read/write/existence checks - improve low disk space guard - additional docblocks/comment clarity Signed-off-by: Josh <[email protected]>
1 parent bae24eb commit f5c8977

File tree

1 file changed

+194
-104
lines changed

1 file changed

+194
-104
lines changed

‎lib/private/Setup.php‎

Lines changed: 194 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -215,85 +215,102 @@ public function getSystemInfo(bool $allowAllDatabases = false): array {
215215
];
216216
}
217217

218-
public function createHtaccessTestFile(string $dataDir): string|false {
219-
// php dev server does not support htaccess
220-
if (php_sapi_name() === 'cli-server') {
218+
/**
219+
* Create a temporary htaccess test file for isHtaccessWorking().
220+
*
221+
* Writes "htaccesstest.txt" into $dataDir and returns its content, or false if skipped.
222+
*
223+
* @return string|false The test content written, or false if the test was skipped
224+
* @throws \OCP\HintException If the test file cannot be created or written
225+
* @internal
226+
*/
227+
private function createHtaccessTestFile(string $dataDir): string|false {
228+
$testFile = $dataDir . '/htaccesstest.txt';
229+
if (file_exists($testFile)) { // unexpected; possible recursive call
221230
return false;
222231
}
223232

224-
// testdata
225-
$fileName = '/htaccesstest.txt';
226233
$testContent = 'This is used for testing whether htaccess is properly enabled to disallow access from the outside. This file can be safely removed.';
227-
228-
// creating a test file
229-
$testFile = $dataDir . '/' . $fileName;
230-
231-
if (file_exists($testFile)) {// already running this test, possible recursive call
232-
return false;
234+
$written = @file_put_contents($testFile, $testContent);
235+
if ($written === false) {
236+
throw new \OCP\HintException(
237+
'Can\'t create htaccess test file to verify .htaccess protection.',
238+
'Make sure the web server user can write to the data directory (default: /data).'
239+
);
233240
}
234241

235-
$fp = @fopen($testFile, 'w');
236-
if (!$fp) {
237-
throw new \OCP\HintException('Can\'t create test file to check for working .htaccess file.',
238-
'Make sure it is possible for the web server to write to ' . $testFile);
239-
}
240-
fwrite($fp, $testContent);
241-
fclose($fp);
242-
243242
return $testContent;
244243
}
245244

246245
/**
247-
* Check if the .htaccess file is working
246+
* Check whether the .htaccess protection is effective for the given data directory.
248247
*
249-
* @param \OCP\IConfig $config
250-
* @return bool
251-
* @throws Exception
252-
* @throws \OCP\HintException If the test file can't get written.
248+
* Creates a temporary file (htaccesstest.txt) under $dataDir and performs an HTTP
249+
* probe. Bypassed under some scenarios (see code) when unnecessary or to avoid false
250+
* negatives.
251+
*
252+
* @return bool True when .htaccess protection appears to work, false otherwise.
253+
* @throws \OCP\HintException If the test file cannot be created.
253254
*/
254-
public function isHtaccessWorking(string $dataDir) {
255-
$config = Server::get(IConfig::class);
255+
public function isHtaccessWorking(string $dataDir): bool {
256256

257-
if (\OC::$CLI || !$config->getSystemValueBool('check_for_working_htaccess', true)) {
257+
// Skip quietly to avoid false negatives since web server state unknown in CLI mode
258+
if (\OC::$CLI) {
258259
return true;
259260
}
260261

262+
// Skip quietly if explicitly configured to do so
263+
if (!(bool)$this->config->getValue('check_for_working_htaccess', true)) {
264+
return true;
265+
}
266+
267+
// Don't bother probing; we already know PHP's dev server does not support
268+
if (PHP_SAPI === 'cli-server') {
269+
return false;
270+
}
271+
272+
// Create a temporary htaccess test file
261273
$testContent = $this->createHtaccessTestFile($dataDir);
262-
if ($testContent === false) {
274+
if ($testContent === false) { // File already exists for some reason
275+
// Note: createHtaccessTestFile() passes up a HintException for most real-world
276+
// failure scenarios which we currently expect our caller to handle.
263277
return false;
264278
}
265279

266-
$fileName = '/htaccesstest.txt';
267-
$testFile = $dataDir . '/' . $fileName;
280+
$testFile = $dataDir . '/htaccesstest.txt';
268281

269-
// accessing the file via http
270-
$url = Server::get(IURLGenerator::class)->getAbsoluteURL(\OC::$WEBROOT . '/data' . $fileName);
271-
try {
272-
$content = Server::get(IClientService::class)->newClient()->get($url)->getBody();
273-
} catch (\Exception $e) {
274-
$content = false;
275-
}
282+
// TODO: consider supporting non-default datadirectory
283+
$url = Server::get(IURLGenerator::class)->getAbsoluteURL(\OC::$WEBROOT . '/data/htaccesstest.txt');
276284

277-
if (str_starts_with($url, 'https:')) {
278-
$url = 'http:' . substr($url, 6);
279-
} else {
280-
$url = 'https:' . substr($url, 5);
281-
}
285+
$client = Server::get(IClientService::class)->newClient();
286+
$fetch = function (string $target) use ($client, $testContent): string|false {
287+
try {
288+
$resp = $client->get($target);
289+
$body = $resp->getBody();
282290

283-
try {
284-
$fallbackContent = Server::get(IClientService::class)->newClient()->get($url)->getBody();
285-
} catch (\Exception $e) {
286-
$fallbackContent = false;
287-
}
291+
if (is_resource($body)) {
292+
$max = strlen($testContent) + 1024; // small margin
293+
return stream_get_contents($body, $max);
294+
}
288295

289-
// cleanup
290-
@unlink($testFile);
296+
return (string)$body;
297+
} catch (\Exception $e) {
298+
return false;
299+
}
300+
};
301+
302+
try {
303+
$content = $fetch($url);
304+
// Probe both schemes for full coverage
305+
$fallbackUrl = str_starts_with($url, 'https:') ? 'http:' . substr($url, 6) : 'https:' . substr($url, 5);
306+
$fallbackContent = $fetch($fallbackUrl);
291307

292-
/*
293-
* If the content is not equal to test content our .htaccess
294-
* is working as required
295-
*/
296-
return $content !== $testContent && $fallbackContent !== $testContent;
308+
// .htaccess likely works if content of probes !== the test content
309+
return $content !== $testContent && $fallbackContent !== $testContent;
310+
} finally {
311+
// Always cleanup
312+
@unlink($testFile);
313+
}
297314
}
298315

299316
/**
@@ -540,12 +557,29 @@ private static function findWebRoot(SystemConfig $config): string {
540557
}
541558

542559
/**
543-
* Append the correct ErrorDocument path for Apache hosts
560+
* Update the default (installation provided) .htaccess by inserting or overwriting
561+
* the non-static section (ErrorDocument and optional front end controller) while
562+
* preserving all static (install artifact) content above the preservation marker.
563+
*
564+
* Runs regardless of web server in use, but only effective on Apache web servers.
544565
*
545-
* @return bool True when success, False otherwise
546-
* @throws \OCP\AppFramework\QueryException
566+
* TODO: Make this no longer static (looks easy; few calls)
567+
*
568+
* @return bool True on success; False if not
547569
*/
548570
public static function updateHtaccess(): bool {
571+
$setupHelper = Server::get(\OC\Setup::class);
572+
$htaccessPath = $setupHelper->pathToHtaccess();
573+
574+
// The distributed .htaccess file is required
575+
if (!is_writable($htaccessPath)
576+
|| !is_readable($htaccessPath)
577+
) {
578+
// cannot update .htaccess (bad permissions or it is missing)
579+
return false;
580+
}
581+
582+
// We're a static method; cannot use DI $this->config
549583
$config = Server::get(SystemConfig::class);
550584

551585
try {
@@ -554,63 +588,114 @@ public static function updateHtaccess(): bool {
554588
return false;
555589
}
556590

557-
$setupHelper = Server::get(\OC\Setup::class);
558-
559-
if (!is_writable($setupHelper->pathToHtaccess())) {
591+
// Read original content
592+
$original = @file_get_contents($htaccessPath);
593+
// extra check for good measure
594+
if ($original === false) {
595+
// bad permissions or installation provided .htaccess is missing
560596
return false;
561597
}
562598

563-
$htaccessContent = file_get_contents($setupHelper->pathToHtaccess());
564-
$content = "#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####\n";
565-
$htaccessContent = explode($content, $htaccessContent, 2)[0];
599+
$preservationBoundary = "#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####\n";
600+
601+
// Preserve everything above the boundary line; drop the rest (if any)
602+
$parts = explode($preservationBoundary, $original, 2);
603+
$preservedContent = $parts[0];
566604

567-
//custom 403 error page
568-
$content .= "\nErrorDocument 403 " . $webRoot . '/index.php/error/403';
605+
// New section must start with the boundary marker
606+
$newContent = $preservationBoundary;
569607

570-
//custom 404 error page
571-
$content .= "\nErrorDocument 404 " . $webRoot . '/index.php/error/404';
608+
// Handle 403s/404s via primary front controller under all installation scenarios
609+
// ErrorDocument path must be relative to the VirtualHost DocumentRoot
610+
$newContent .= "\nErrorDocument 403 " . $webRoot . '/index.php/error/403';
611+
$newContent .= "\nErrorDocument 404 " . $webRoot . '/index.php/error/404';
612+
613+
// RewriteBase tells mod_rewrite the URL base for the rules in this
614+
// .htaccess file. It is required when Nextcloud is served from a subpath (so the
615+
// rewrite rules generate and match the correct prefixed request paths). It
616+
// also enables "pretty" URLs by routing most requests to the primary front
617+
// controller (index.php).
618+
//
619+
// When served from the document root, RewriteBase is usually not required,
620+
// though some specific server setups may still need it. In Nextcloud, setting
621+
// htaccess.RewriteBase to '/' (instead of leaving it empty or unconfigured) is
622+
// the trigger that causes updateHtaccess() to write the bundled rewrite rules
623+
// and thus enable "pretty" URLs for root installs.
572624

573-
// Add rewrite rules if the RewriteBase is configured
574625
$rewriteBase = $config->getValue('htaccess.RewriteBase', '');
626+
// Notes:
627+
// - Equivalent handling may be provided by the web server (e.g. nginx location
628+
// / Apache vhost blocks) even without this.
629+
// - This is not the entire Nextcloud .htaccess file; these are merely appended
630+
// to the base file distributed with each release.
631+
// TODO: Document these rules/conditions
575632
if ($rewriteBase !== '') {
576-
$content .= "\n<IfModule mod_rewrite.c>";
577-
$content .= "\n Options -MultiViews";
578-
$content .= "\n RewriteRule ^core/js/oc.js$ index.php [PT,E=PATH_INFO:$1]";
579-
$content .= "\n RewriteRule ^core/preview.png$ index.php [PT,E=PATH_INFO:$1]";
580-
$content .= "\n RewriteCond %{REQUEST_FILENAME} !\\.(css|js|mjs|svg|gif|ico|jpg|jpeg|png|webp|html|otf|ttf|woff2?|map|webm|mp4|mp3|ogg|wav|flac|wasm|tflite)$";
581-
$content .= "\n RewriteCond %{REQUEST_FILENAME} !/core/ajax/update\\.php";
582-
$content .= "\n RewriteCond %{REQUEST_FILENAME} !/core/img/(favicon\\.ico|manifest\\.json)$";
583-
$content .= "\n RewriteCond %{REQUEST_FILENAME} !/(cron|public|remote|status)\\.php";
584-
$content .= "\n RewriteCond %{REQUEST_FILENAME} !/ocs/v(1|2)\\.php";
585-
$content .= "\n RewriteCond %{REQUEST_FILENAME} !/robots\\.txt";
586-
$content .= "\n RewriteCond %{REQUEST_FILENAME} !/(ocs-provider|updater)/";
587-
$content .= "\n RewriteCond %{REQUEST_URI} !^/\\.well-known/(acme-challenge|pki-validation)/.*";
588-
$content .= "\n RewriteCond %{REQUEST_FILENAME} !/richdocumentscode(_arm64)?/proxy.php$";
589-
$content .= "\n RewriteRule . index.php [PT,E=PATH_INFO:$1]";
590-
$content .= "\n RewriteBase " . $rewriteBase;
591-
$content .= "\n <IfModule mod_env.c>";
592-
$content .= "\n SetEnv front_controller_active true";
593-
$content .= "\n <IfModule mod_dir.c>";
594-
$content .= "\n DirectorySlash off";
595-
$content .= "\n </IfModule>";
596-
$content .= "\n </IfModule>";
597-
$content .= "\n</IfModule>";
598-
}
599-
600-
// Never write file back if disk space should be too low
601-
if (function_exists('disk_free_space')) {
602-
$df = disk_free_space(\OC::$SERVERROOT);
603-
$size = strlen($content) + 10240;
604-
if ($df !== false && $df < (float)$size) {
605-
throw new \Exception(\OC::$SERVERROOT . ' does not have enough space for writing the htaccess file! Not writing it back!');
633+
$newContent .= "\n<IfModule mod_rewrite.c>";
634+
$newContent .= "\n Options -MultiViews";
635+
$newContent .= "\n RewriteRule ^core/js/oc.js$ index.php [PT,E=PATH_INFO:$1]";
636+
$newContent .= "\n RewriteRule ^core/preview.png$ index.php [PT,E=PATH_INFO:$1]";
637+
$newContent .= "\n RewriteCond %{REQUEST_FILENAME} !\\.(css|js|mjs|svg|gif|ico|jpg|jpeg|png|webp|html|otf|ttf|woff2?|map|webm|mp4|mp3|ogg|wav|flac|wasm|tflite)$";
638+
$newContent .= "\n RewriteCond %{REQUEST_FILENAME} !/core/ajax/update\\.php";
639+
$newContent .= "\n RewriteCond %{REQUEST_FILENAME} !/core/img/(favicon\\.ico|manifest\\.json)$";
640+
$newContent .= "\n RewriteCond %{REQUEST_FILENAME} !/(cron|public|remote|status)\\.php";
641+
$newContent .= "\n RewriteCond %{REQUEST_FILENAME} !/ocs/v(1|2)\\.php";
642+
$newContent .= "\n RewriteCond %{REQUEST_FILENAME} !/robots\\.txt";
643+
$newContent .= "\n RewriteCond %{REQUEST_FILENAME} !/(ocs-provider|updater)/";
644+
$newContent .= "\n RewriteCond %{REQUEST_URI} !^/\\.well-known/(acme-challenge|pki-validation)/.*";
645+
$newContent .= "\n RewriteCond %{REQUEST_FILENAME} !/richdocumentscode(_arm64)?/proxy.php$";
646+
$newContent .= "\n RewriteRule . index.php [PT,E=PATH_INFO:$1]";
647+
$newContent .= "\n RewriteBase " . $rewriteBase;
648+
$newContent .= "\n <IfModule mod_env.c>";
649+
$newContent .= "\n SetEnv front_controller_active true";
650+
$newContent .= "\n <IfModule mod_dir.c>";
651+
$newContent .= "\n DirectorySlash off";
652+
$newContent .= "\n </IfModule>";
653+
$newContent .= "\n </IfModule>";
654+
$newContent .= "\n</IfModule>";
655+
}
656+
657+
// Assemble new file contents
658+
$assembled = $preservedContent . $newContent . "\n";
659+
660+
// Only write if changed
661+
if ($original !== $assembled) {
662+
// Guard against disk space being too low to safely update
663+
if (function_exists('disk_free_space')) {
664+
$df = disk_free_space(\OC::$SERVERROOT);
665+
$size = strlen($assembled) + 10240;
666+
if ($df !== false && $df < (float)$size) {
667+
throw new \Exception(\OC::$SERVERROOT . ' does not have enough storage space for writing the updated .htaccess file! Giving up!');
668+
}
606669
}
670+
// TODO: Consider atomic write (write to tmp + rename)
671+
$written = @file_put_contents($htaccessPath, $assembled);
672+
return ($written !== false);
607673
}
608-
//suppress errors in case we don't have permissions for it
609-
return (bool)@file_put_contents($setupHelper->pathToHtaccess(), $htaccessContent . $content . "\n");
674+
675+
return true;
610676
}
611677

678+
/**
679+
* Prevents direct HTTP access to user files (high security risk if the
680+
* data directory were web-accessible).
681+
*
682+
* - Prevents directory listing of the data directory.
683+
* - Provides a safe default protection for Apache installs (where .htaccess is honored).
684+
*/
612685
public static function protectDataDirectory(): void {
613-
//Require all denied
686+
687+
$defaultDataDir = \OC::$SERVERROOT . '/data';
688+
$dataDir = Server::get(IConfig::class)->getSystemValueString('datadirectory', $defaultDataDir);
689+
690+
// Ensure data directory exists and is writable
691+
if (!is_dir($dataDir) || !is_writable($dataDir)) {
692+
throw new \Exception("Unable to write to data directory ($dataDir) to protect it! Giving up!");
693+
}
694+
695+
$dataDirHtaccess = $dataDir . '/.htaccess';
696+
$dataDirIndex = $dataDir . '/index.html';
697+
698+
// Content for the .htaccess file that locks down (most) Apache environments
614699
$now = date('Y-m-d H:i:s');
615700
$content = "# Generated by Nextcloud on $now\n";
616701
$content .= "# Section for Apache 2.4 to 2.6\n";
@@ -637,9 +722,14 @@ public static function protectDataDirectory(): void {
637722
$content .= " IndexIgnore *\n";
638723
$content .= '</IfModule>';
639724

640-
$baseDir = Server::get(IConfig::class)->getSystemValueString('datadirectory', \OC::$SERVERROOT . '/data');
641-
file_put_contents($baseDir . '/.htaccess', $content);
642-
file_put_contents($baseDir . '/index.html', '');
725+
// Create an empty index.html to prevent simply browsing
726+
$writtenIndex = file_put_contents($dataDirIndex, '');
727+
// Create the .htaccess file
728+
$writtenHtaccess = file_put_contents($dataDirHtaccess, $content);
729+
730+
if ($writtenHtaccess === false || $writtenIndex === false) {
731+
throw new \Exception("Failed to write $dataDirHtaccess or $dataDirIndex");
732+
}
643733
}
644734

645735
private function getVendorData(): array {

0 commit comments

Comments
 (0)