nextcloud / tables

🍱 Nextcloud tables app
https://apps.nextcloud.com/apps/tables
GNU Affero General Public License v3.0
148 stars 23 forks source link

[Bug]: TypeError on unauthenticated calls against OCS routes, success error code #1192

Open blizzz opened 4 months ago

blizzz commented 4 months ago

Steps to reproduce

  1. Fire an unauthenticated GET request against e.g. ocs/v2.php/apps/tables/api/2/contexts

Expected behavior

The server responds with 401.

No (unnecessary) exceptions/errors are thrown or logged in nextcloud.log

Actual behavior

The ContextController and the AOCSController expect the $userId in their constructor as string. In an unauthenticated request it is null, and so the Controller class cannot be instantiated in lib/private/AppFramework/App::main() and a TypeError is logged like:

{
  "reqId": "5nXYwv9Cf4SX3EegzQSK",
  "level": 3,
  "time": "2024-07-12T10:21:03+00:00",
  "remoteAddr": "127.0.0.1",
  "user": "--",
  "app": "PHP",
  "method": "GET",
  "url": "/ocs/v2.php/apps/tables/api/2/contexts",
  "message": "TypeError: OCA\\Tables\\Controller\\ContextController::__construct(): Argument #4 ($userId) must be of type string, null given at /path/to/nextcloud/apps-repos/tables/lib/Controller/ContextController.php#31",
  "userAgent": "curl/8.8.0",
  "version": "30.0.0.1",
  "data": {
    "app": "PHP"
  }
}

However the server responds with 200:

# curl
$ ╚> curl -i -X GET -H 'OCS-ApiRequest: true'  https://cloud.example.com/ocs/v2.php/apps/tables/api/2/contexts
HTTP/1.1 200 OK
…

# access.log
127.0.0.1 - - [12/Jul/2024:12:21:03 +0200] "GET /ocs/v2.php/apps/tables/api/2/contexts HTTP/1.1" 200 3569 "-" "curl/8.8.0" 66900

Tables app version

main

Additional info

Both ContextController and AOCSController (and maybe other Controllers that extend AOCSController) need to lax the $userId parameter and accept null.

Authenticated checks are implemented in the Middleware, and authorized usage is the default. Logic should be checked to not have wrong assumption on $userId, i.e. might need to have a check for not being null for static analysis.