[Bug]: headers_sent inappropriate method to validate config output

[martin-rueegg]

⚠️ This issue respects the following points: ⚠️

• This is a bug, not a question or a configuration/webserver/proxy issue.
• This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
• Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
• I agree to follow Nextcloud's Code of Conduct.

Bug description

Context

The server configuration file is loaded by the OC\Config:readData() method.

That method evaluates if the config file has any content before the <?php token. It does this by verifying if headers have already be sent after the file is read using the \headers_sent() return value. It does however not verify the $filename is actually the config $file included.

Problem

The test actually tests the wrong thing, namely if headers are sent, NOT if the config file includes data before the opening php token. this is problematic in tow ways:

• the reported error message is misleading
• if headers MUST NOT have been sent (which is entirely irrelevant to the method in question) it's a mixture of concerns.

If any function of the code is responsible e.g. to build and and headers, THAT's where this should be tested - NOT while reading the configuration.
The test is an undocumented side effect of loading the configuration.

Why does it matter?

Loading lib/base.php from an auxiliary script to leverage the server's services (e.g. move multiple files files to efficiently, rather that to have to invoke a new php instance for each file when using the occ files:move command) and that script may indeed already have sent e.g. debug output of it's own concern, loading the OC code-space fails for no good reason.

Solution

A better test would actually be either to use output_buffer and test if there is any content after the include of the config file. This is standard procedure for such a test.

If for whatever reason this is not possible, then the test should make sure that the config file is the actual culprit by using:

if (!defined('PHPUNIT_RUN') && headers_sent($filename) && $filename === $file) {

happy to provide a PR.

Relevant Code

(also applicable to previous versions of NC Server)

server/lib/private/Config.php

Lines 248 to 255 in fb27fd5

	if (!defined('PHPUNIT_RUN') && headers_sent()) {
	// syntax issues in the config file like leading spaces causing PHP to send output
	$errorMessage = sprintf('Config file has leading content, please remove everything before "<?php" in %s', basename($file));
	if (!defined('OC_CONSOLE')) {
	print(Util::sanitizeHTML($errorMessage));
	}
	throw new \Exception($errorMessage);
	}

Steps to reproduce

1. create a CLI file that echos enough data to make the output buffer flush
2. require OC's lib/base.php

Expected behavior

OC base properly loaded

Nextcloud Server version

35 (master)

Operating system

Debian/Ubuntu

PHP engine version

PHP 8.4

Web server

None

Database engine version

None

Is this bug present after an update or on a fresh install?

None

Are you using the Nextcloud Server Encryption module?

None

What user-backends are you using?

• Default user-backend (database)
• LDAP/ Active Directory
• SSO - SAML
• Other

Configuration report

not relevant. but happy to deliver if actually required.

List of activated Apps

not relevant. but happy to deliver if actually required.

Nextcloud Signing status

not relevant. but happy to deliver if actually required.

Nextcloud Logs

not relevant. but happy to deliver if actually required.

Additional info

not relevant. but happy to deliver if actually required.