Skip to content

fix(OCS): Add IRequest::getFormat to determine the response Content-Type the same way everywhere #54627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
provokateurin merged 1 commit into from
Aug 28, 2025
Merged

fix(OCS): Add IRequest::getFormat to determine the response Content-Type the same way everywhere #54627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions build/psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3330,11 +3330,6 @@
<code><![CDATA[\OCP\IServerContainer]]></code>
</InvalidReturnType>
</file>
<file src="lib/private/AppFramework/Http/Dispatcher.php">
<NullArgument>
<code><![CDATA[null]]></code>
</NullArgument>
</file>
<file src="lib/private/AppFramework/Http/Output.php">
<InvalidReturnStatement>
<code><![CDATA[@readfile($path)]]></code>
Expand Down
9 changes: 1 addition & 8 deletions lib/private/AppFramework/Http/Dispatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,14 +210,7 @@ private function executeController(Controller $controller, string $methodName):

// format response
if ($response instanceof DataResponse || !($response instanceof Response)) {
// get format from the url format or request format parameter
$format = $this->request->getParam('format');

// if none is given try the first Accept header
if ($format === null) {
$headers = $this->request->getHeader('Accept');
$format = $controller->getResponderByHTTPHeader($headers, null);
}
$format = $this->request->getFormat();

if ($format !== null) {
$response = $controller->buildResponse($response, $format);
Expand Down
19 changes: 19 additions & 0 deletions lib/private/AppFramework/Http/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,4 +877,23 @@ private function fromTrustedProxy(): bool {

return \is_array($trustedProxies) && $this->isTrustedProxy($trustedProxies, $remoteAddress);
}

public function getFormat(): ?string {
Copy link
Member

@nickvergessen nickvergessen Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have upstreamed https://github.com/nextcloud/spreed/blob/38e56a3fe460f07e128c49be2759362592728a22/lib/Controller/AEnvironmentAwareOCSController.php#L60-L75 before it was cool :D

$format = $this->getParam('format');
if ($format !== null) {
return $format;
}

$prefix = 'application/';
$headers = explode(',', $this->getHeader('Accept'));
foreach ($headers as $header) {
$header = strtolower(trim($header));

if (str_starts_with($header, $prefix)) {
return substr($header, strlen($prefix));
Copy link
Member

@nickvergessen nickvergessen Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opposed to Controller::getResponderByHTTPHeader() which was used before, this here no longer checks if there is a responder registered and does not fall back to the default type correctly.
E.g. When you create a share for a file that can not have a preview (e.g. PDF without PDF being enabled), instead of having a proper 404, the response now is:

Technische Details

    Entfernte Adresse: ::1
    Anfragekennung: 3RuRvo3TEr52XxYi3pq6
    Typ: DomainException
    Code: 0
    Nachricht: No responder registered for format xhtml+xml!
    Datei: /home/nickv/Nextcloud/33/server/lib/public/AppFramework/Controller.php
    Zeile: 139


Trace

#0 /home/nickv/Nextcloud/33/server/lib/private/AppFramework/Http/Dispatcher.php(227): OCP\AppFramework\Controller->buildResponse(Object(OCP\AppFramework\Http\DataResponse), '...')
#1 /home/nickv/Nextcloud/33/server/lib/private/AppFramework/Http/Dispatcher.php(118): OC\AppFramework\Http\Dispatcher->executeController(Object(OCA\Files_Sharing\Controller\PublicPreviewController), '...')
#2 /home/nickv/Nextcloud/33/server/lib/private/AppFramework/App.php(153): OC\AppFramework\Http\Dispatcher->dispatch(Object(OCA\Files_Sharing\Controller\PublicPreviewController), '...')
#3 /home/nickv/Nextcloud/33/server/lib/private/Route/Router.php(321): OC\AppFramework\App::main('...', '...', Object(OC\AppFramework\DependencyInjection\DIContainer), Array)
#4 /home/nickv/Nextcloud/33/server/lib/base.php(1096): OC\Route\Router->match('...')
#5 /home/nickv/Nextcloud/33/server/index.php(25): OC::handleRequest()
#6 {main}

This works properly until Nextcloud 31.

Copy link
Member Author

@provokateurin provokateurin Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#55620

}
}

return null;
}
}
19 changes: 1 addition & 18 deletions lib/private/AppFramework/Middleware/OCSMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public function afterController($controller, $methodName, Response $response) {
* @return V1Response|V2Response
*/
private function buildNewResponse(Controller $controller, $code, $message) {
$format = $this->getFormat($controller);
$format = $this->request->getFormat() ?? 'xml';

$data = new DataResponse();
$data->setStatus($code);
Expand All @@ -122,21 +122,4 @@ private function buildNewResponse(Controller $controller, $code, $message) {

return $response;
}

/**
* @param Controller $controller
* @return string
*/
private function getFormat(Controller $controller) {
// get format from the url format or request format parameter
$format = $this->request->getParam('format');

// if none is given try the first Accept header
if ($format === null) {
$headers = $this->request->getHeader('Accept');
$format = $controller->getResponderByHTTPHeader($headers, 'xml');
}

return $format;
}
}
5 changes: 3 additions & 2 deletions lib/private/OCS/ApiHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ class ApiHelper {
*/
public static function respond(int $statusCode, string $statusMessage, array $headers = [], ?int $overrideHttpStatusCode = null): void {
$request = Server::get(IRequest::class);
$format = $request->getParam('format', 'xml');
$format = $request->getFormat() ?? 'xml';

if (self::isV2($request)) {
$response = new V2Response(new DataResponse([], $statusCode, $headers), $format, $statusMessage);
} else {
Expand Down Expand Up @@ -58,7 +59,7 @@ public static function respond(int $statusCode, string $statusMessage, array $he
* Based on the requested format the response content type is set
*/
public static function setContentType(?string $format = null): void {
$format ??= Server::get(IRequest::class)->getParam('format', 'xml');
$format ??= Server::get(IRequest::class)->getFormat() ?? 'xml';
if ($format === 'xml') {
header('Content-type: text/xml; charset=UTF-8');
return;
Expand Down
1 change: 1 addition & 0 deletions lib/public/AppFramework/Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public function __construct($appName,
* @return string the responder type
* @since 7.0.0
* @since 9.1.0 Added default parameter
* @deprecated 33.0.0 Use {@see \OCP\IRequest::getFormat} instead
*/
public function getResponderByHTTPHeader($acceptHeader, $default = 'json') {
$headers = explode(',', $acceptHeader);
Expand Down
10 changes: 10 additions & 0 deletions lib/public/IRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,4 +315,14 @@ public function getServerHost(): string;
* @since 32.0.0
*/
public function throwDecodingExceptionIfAny(): void;

/**
* Returns the format of the response to this request.
*
* The `Accept` header and the `format` query parameter control the format.
*
* @return string|null
* @since 33.0.0
*/
public function getFormat(): ?string;
}
Loading