Files Sharing API breaks in Nc 28+

[DanRiess]

Explain the Problem

There is a related issue with pretty much the exact same problem, but it was last active a year ago, so I figured I open a new one. The files sharing API seems to be broken in Nextcloud Version 28 and 29 and throws CORS errors. Authorized file access is not affected and works as intended.

The files sharing API works fine in my local 26 version and it used to work in our production environment until we upgraded to v28. I'm not 100% sure however if our live Nextcloud was upgraded over time or all at once to v28 at some point, so there's a slight chance the breaking change might have been introduced in v27 instead.

Steps to Reproduce

I used the test suite that @aleixq has provided in the related issue about CORS errors in files sharing (https://gitlab.com/communia/nc-webapppassword-share-test).
I created the required test file and added the origin to both webdav access and files sharing in the settings.
The webdav access outputs directory metadata, the share test fails. The other 2 cases fail as expected.
In my local environment with NC 26, the share test passes.

Contents of nextcloud/data/nextcloud.log

{
"reqId":"IX66HIjeyKVHhi4y9ff7",
"level":3,"time":"2024-06-12T09:45:25+00:00",
"remoteAddr":"172.17.0.1",
"user":"--",
"app":"index",
"method":"OPTIONS",
"url":"/index.php/apps/webapppassword/api/v1/shares",
"message":"Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string",
"userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36",
"version":"29.0.1.1",
"exception":{"Exception":"TypeError","Message":"Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string","Code":0,"Trace":[{"function":"__construct","class":"OCA\\Files_Sharing\\Controller\\ShareAPIController","type":"->","args":["webapppassword",["OC\\AppFramework\\Http\\Request"],["OC\\Share20\\Manager"],["OC\\Group\\Manager"],["OC\\User\\Manager"],["OC\\Files\\Node\\LazyRoot"],["OC\\URLGenerator",["OC\\User\\Session"]],["OC\\L10N\\LazyL10N"],["OC\\AllConfig"],["OC\\App\\AppManager"],["OC\\AppFramework\\DependencyInjection\\DIContainer"],["OC\\UserStatus\\Manager"],["OC\\PreviewManager"],["OC\\DateTimeZone"],["OC\\AppFramework\\ScopedPsrLogger"],null]},{"file":"/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php","line":83,"function":"newInstanceArgs","class":"ReflectionClass","type":"->","args":[["webapppassword",["OC\\AppFramework\\Http\\Request"],["OC\\Share20\\Manager"],["OC\\Group\\Manager"],["OC\\User\\Manager"],"And 11 more entries, set log level to debug to see all entries"]]},{"file":"/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php","line":128,"function":"buildClass","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":[["ReflectionClass","OCA\\WebAppPassword\\Controller\\ShareAPIController"]]},{"file":"/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php","line":146,"function":"resolve","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController"]},{"file":"/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":470,"function":"query","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController"]},{"file":"/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":442,"function":"queryNoFallback","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController"]},{"file":"/var/www/html/lib/private/AppFramework/App.php","line":163,"function":"query","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController"]},{"file":"/var/www/html/lib/private/Route/Router.php","line":338,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\WebAppPassword\\Controller\\ShareAPIController","preflightedCors",["OC\\AppFramework\\DependencyInjection\\DIContainer"],["webapppassword.shareapi.preflighted_cors"]]},{"file":"/var/www/html/lib/base.php","line":1050,"function":"match","class":"OC\\Route\\Router","type":"->","args":["/apps/webapppassword/api/v1/shares"]},{"file":"/var/www/html/index.php","line":49,"function":"handleRequest","class":"OC","type":"::","args":[]}],"File":"/var/www/html/apps/files_sharing/lib/Controller/ShareAPIController.php","Line":124,"message":"Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string","exception":{},"CustomMessage":"Cannot assign null to property OCA\\Files_Sharing\\Controller\\ShareAPIController::$currentUser of type string"}}

Could you guys please check if this is reproducible on your end?

Thanks,
Dan

[pbek]

@aleixq, can you reproduce this?
You can also spin up a Nextcloud instance with https://github.com/digital-blueprint/webapppassword/tree/main/docker.

[pbek]

#200 is even worse... 😬

[aleixq]

So it seems that parameters changed... again cat-and-mouse... the cost of living behind extends OCA\Files_Sharing\Controller\ShareAPIController .
adding:

string $expireDate = null,

in https://github.com/digital-blueprint/webapppassword/blob/v24.1.1/lib/Controller/ShareAPIController.php#L42

seems to solve this issue.

Also, I think that the way @pbek unpacks args will solve future changes.

Do you need a patch for this minor change or better bet for your workaround?

[pbek]

I released 8e619fe, so best test version 24.6.0 first.

[DanRiess]

Upgraded to 24.6.0 and also slowly upgraded Nextcloud versions from 26 to 29.0.2 locally. The app and sharing worked until v28, but sharing won't work with v29.0.2. Same error as before, somehow the 'currentuser' variable seems to be empty.

I think I was wrong about claiming that file sharing wasn't working on NC v28. My local NC v28 still used webapppassword 24.4.0 and it was fine. I suppose everything just broke with NC 29, like you mentioned in the other issue.

[aleixq]

I'll check it out ASAP. By now, Could you please check in browser console if is failing the preflight OPTIONS request, when trying to query or posting things to webappassword share api endpoint?

[DanRiess]

Yes, it's the OPTIONS part of the request that is failing. The error in the console is the standard CORS error.

The request itself is a POST to http://localhost:8080/index.php/apps/webapppassword/api/v1/shares with a shareType, path, expireDate and password in the body.

[pbek]

If it's about nextcloud/server@5dbb96f, those changes will also be backported to Nextcloud 28, but they are not released yet.

[aleixq]

I had inspected a little more, and I suspect that there are some issues there. Webapppassword Share api is not working in 29 too, with this error (in nextcloud logs):
1- The share api currentuser property type issues nextcloud/server#46081 . It could be workarounded redeclaring the property in our class and removing the type :
private $currentUser;

2 - After solving the first error there is this Reflector Internal error: Failed to retrieve the default value error:

{"file":"web/index.php","line":49,"function":"handleRequest","class":"OC","type":"::"}],"File":"web/lib/private/AppFramework/Utility/ControllerMethodReflector.php","Line":101,"message":"Internal error: Failed to retrieve the default value","exception":{},"CustomMessage":"Internal error: Failed to retrieve the default value"}}{
  "reqId": "qMEVVEzV1AgBLr6zKrFT",
  "level": 3,
  "time": "2024-06-24T19:53:10+00:00",
  "remoteAddr": "x.x.x.x",
  "user": "someuser",
  "app": "index",
  "method": "POST",
  "url": "/index.php/apps/webapppassword/api/v1/shares",
  "message": "Internal error: Failed to retrieve the default value",
  "userAgent": "Mozilla/5.0 (X11; Linux x86_64; rv:128.0) Gecko/20100101 Firefox/128.0",
  "version": "29.0.2.2",
  "exception": {
    "Exception": "ReflectionException",
    "Message": "Internal error: Failed to retrieve the default value",
    "Code": 0,
    "Trace": [
      {
        "file": "web/lib/private/AppFramework/Utility/ControllerMethodReflector.php",
        "line": 101,
        "function": "getDefaultValue",
        "class": "ReflectionParameter",
        "type": "->"
      },
      {
        "file": "web/lib/private/AppFramework/Http/Dispatcher.php",
        "line": 128,
        "function": "reflect",
        "class": "OC\\AppFramework\\Utility\\ControllerMethodReflector",
        "type": "->"
      },
      {
        "file": "web/lib/private/AppFramework/App.php",
        "line": 184,
        "function": "dispatch",
        "class": "OC\\AppFramework\\Http\\Dispatcher",
        "type": "->"
      },
      {
        "file": "web/lib/private/Route/Router.php",
        "line": 338,
        "function": "main",
        "class": "OC\\AppFramework\\App",
        "type": "::"
      },
      {
        "file": "web/lib/base.php",
        "line": 1050,
        "function": "match",
        "class": "OC\\Route\\Router",
        "type": "->"
      },
      {
        "file": "web/index.php",
        "line": 49,
        "function": "handleRequest",
        "class": "OC",
        "type": "::"
      }
    ],
    "File": "web/lib/private/AppFramework/Utility/ControllerMethodReflector.php",
    "Line": 101,
    "message": "Internal error: Failed to retrieve the default value",
    "exception": {},
    "CustomMessage": "Internal error: Failed to retrieve the default value"
  }
}

So second issue is about args unpacking, it is introduced with last @pbek commit... Maybe this could be solved more elegantly to avoid parameters changes in the future, but if I declare again the parameters and call the parent method with the same parameters it runs ok...

So finally if I patch with next changes it runs as it should:

diff --git a/lib/Controller/ShareAPIController.php b/lib/Controller/ShareAPIController.php
index 43b2863..e8d1572 100644
--- a/lib/Controller/ShareAPIController.php
+++ b/lib/Controller/ShareAPIController.php
@@ -11,32 +11,56 @@ use OCP\AppFramework\Http\DataResponse;
 
 class ShareAPIController extends FilesSharingShareAPIController
 {
-    use AccessControl;
-
-    /**
-     * @NoAdminRequired
-     *
-     * @param string $path
-     * @param int    $permissions
-     * @param string $shareWith
-     * @param string $sendPasswordByTalk
-     * @param string $attributes
-     *
-     * @throws NotFoundException
-     * @throws OCSBadRequestException
-     * @throws OCSException
-     * @throws OCSForbiddenException
-     * @throws OCSNotFoundException
-     * @throws InvalidPathException
-     *
-     * @suppress PhanUndeclaredClassMethod
-     */
-    public function createShare(...$args
-    ): DataResponse {
-        $response = parent::createShare(...$args);
-
-        return $this->checkOrigin($response);
-    }
+  use AccessControl;
+  private $currentUser;
+
+  /**
+   * @NoAdminRequired
+   *
+   * @param string $path
+   * @param int    $permissions
+   * @param string $shareWith
+   * @param string $sendPasswordByTalk
+   * @param string $attributes
+   *
+   * @throws NotFoundException
+   * @throws OCSBadRequestException
+   * @throws OCSException
+   * @throws OCSForbiddenException
+   * @throws OCSNotFoundException
+   * @throws InvalidPathException
+   *
+   * @suppress PhanUndeclaredClassMethod
+   */
+  public function createShare(
+    ?string $path = null,
+    ?int $permissions = null,
+    int $shareType = -1,
+    ?string $shareWith = null,
+    string $publicUpload = 'false',
+    string $password = '',
+    ?string $sendPasswordByTalk = null,
+    ?string $expireDate = null,
+    string $note = '',
+    string $label = '',
+    ?string $attributes = null
+  ): DataResponse {
+    $response = parent::createShare(
+      $path,
+      $permissions,
+      $shareType,
+      $shareWith,
+      $publicUpload,
+      $password,
+      $sendPasswordByTalk,
+      $expireDate,
+      $note,
+      $label,
+      $attributes
+    );
+
+    return $this->checkOrigin($response);
+  }
 
       /**
        * The getShares function.

[pbek]

You can create a PR, but it needs to work with both Nextcloud 29.0.1 and 29.0.2 (and the same for NC 28 and NC 27)!

[DanRiess]

Any news on this issue?

[aleixq]

Hi @DanRiess , I am a little bit confused about how to proceed. I could propose conditions to make it work with 27 28 and 29 , but sincerely It's not a future-proof...

I am waiting for an answer in upstream : nextcloud/server#46081 (or proposals here) to decide. I don't know if you understand all the context of this issue about public controllers, cors,... , so maybe it's difficult for you, but maybe you could ask about news there.

For the long-long-term solution, I am still convinced that nextcloud may support a flexible way to manage cors to allow public file sharing or public preview api, so please participate in nextcloud/server#37716 (and its PR in nextcloud/server#37896 ) to explain your use case. The more use cases that are detected and explained, the more it will be seen if it is useful to be incorporated into the core.

[DanRiess]

Hey @aleixq, excuse the late reply. I only have a high level understanding of Nextcloud internals. The way I understand is that Nextcloud's ShareAPIController requires a valid user (even though this variable is typed as string|undefined). Which makes sense I guess, because someone needs to share it. So in the current implementation, Nextcloud's ShareAPIController for some reason cannot extract the currentuser anymore, even though we are using a webapppassword bearer token, is that it?

If you have a monkey patch that works as a short term solution for this problem, I'd be happy if you could release that for now. Sharing functionality is quite important for my company's app. Maybe we could figure out a long term solution, if sharing breaks again in future NC releases ;D