Skip to content

Conversation

@provokateurin
Copy link
Member

Prevents error messages like this:

{"reqId":"ojXF2BwODOOk6tps9fW5","level":3,"time":"2025-08-20T11:32:03+00:00","remoteAddr":"::1","user":"admin","app":"no app in context","method":"DELETE","url":"/ocs/v2.php/cloud/users/admin/subadmins","message":"OCA\\Provisioning_API\\Controller\\UsersController::removeSubAdmin(): Argument #2 ($groupid) must be of type string, null given, called in /home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php on line 204 in file '/home/jld3103/src/github.com/nextcloud/server/apps/provisioning_api/lib/Controller/UsersController.php' line 1730","userAgent":"curl/8.15.0","version":"32.0.0.4","exception":{"Exception":"Exception","Message":"OCA\\Provisioning_API\\Controller\\UsersController::removeSubAdmin(): Argument #2 ($groupid) must be of type string, null given, called in /home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php on line 204 in file '/home/jld3103/src/github.com/nextcloud/server/apps/provisioning_api/lib/Controller/UsersController.php' line 1730","Code":0,"Trace":[{"file":"/home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/App.php","line":153,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/home/jld3103/src/github.com/nextcloud/server/lib/private/Route/Router.php","line":321,"function":"main","class":"OC\\AppFramework\\App","type":"::"},{"file":"/home/jld3103/src/github.com/nextcloud/server/ocs/v1.php","line":61,"function":"match","class":"OC\\Route\\Router","type":"->"},{"file":"/home/jld3103/src/github.com/nextcloud/server/ocs/v2.php","line":8,"args":["/home/jld3103/src/github.com/nextcloud/server/ocs/v1.php"],"function":"require_once"}],"File":"/home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php","Line":150,"Previous":{"Exception":"TypeError","Message":"OCA\\Provisioning_API\\Controller\\UsersController::removeSubAdmin(): Argument #2 ($groupid) must be of type string, null given, called in /home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php on line 204","Code":0,"Trace":[{"file":"/home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php","line":204,"function":"removeSubAdmin","class":"OCA\\Provisioning_API\\Controller\\UsersController","type":"->"},{"file":"/home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php","line":118,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/App.php","line":153,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/home/jld3103/src/github.com/nextcloud/server/lib/private/Route/Router.php","line":321,"function":"main","class":"OC\\AppFramework\\App","type":"::"},{"file":"/home/jld3103/src/github.com/nextcloud/server/ocs/v1.php","line":61,"function":"match","class":"OC\\Route\\Router","type":"->"},{"file":"/home/jld3103/src/github.com/nextcloud/server/ocs/v2.php","line":8,"args":["/home/jld3103/src/github.com/nextcloud/server/ocs/v1.php"],"function":"require_once"}],"File":"/home/jld3103/src/github.com/nextcloud/server/apps/provisioning_api/lib/Controller/UsersController.php","Line":1730},"message":"OCA\\Provisioning_API\\Controller\\UsersController::removeSubAdmin(): Argument #2 ($groupid) must be of type string, null given, called in /home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php on line 204 in file '/home/jld3103/src/github.com/nextcloud/server/apps/provisioning_api/lib/Controller/UsersController.php' line 1730","exception":{},"CustomMessage":"OCA\\Provisioning_API\\Controller\\UsersController::removeSubAdmin(): Argument #2 ($groupid) must be of type string, null given, called in /home/jld3103/src/github.com/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php on line 204 in file '/home/jld3103/src/github.com/nextcloud/server/apps/provisioning_api/lib/Controller/UsersController.php' line 1730"}}

The server is not doing anything wrong in this case and there is nothing the admin can do about it, because client is not sending a valid request to the endpoint.

@provokateurin provokateurin added this to the Nextcloud 32 milestone Aug 20, 2025
@provokateurin provokateurin requested a review from a team as a code owner August 20, 2025 11:37
@provokateurin provokateurin added bug 3. to review Waiting for reviews labels Aug 20, 2025
@provokateurin provokateurin requested review from ArtificialOwl, sorbaugh and yemkareems and removed request for a team August 20, 2025 11:37
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Is really every type error a bad request? This seems to be very broad as there could also just be application logic issues in the controller?

@provokateurin
Copy link
Member Author

Hm yes it could also be the fault of the code, but I think it is way more likely that it happens when the method is called and the request didn't provide valid parameters.

@susnux
Copy link
Contributor

susnux commented Aug 20, 2025

but I think it is way more likely that it happens when the method is called and the request didn't provide valid parameters.

how expensive would it be to use reflections and check this before?

@provokateurin
Copy link
Member Author

Probably not expensive, since we already use reflection before to cast the values, but it will be a lot of messy code that might be incomplete. I would not go this route, so either we merge this PR or close it.

@nickvergessen
Copy link
Member

This will be very painful for developers trying to figure out why something is not working during development 🙈

What I could see would be: when we have a TypeError, check if it's from the Controller::method call, if yes => debug, otherwise => error.
If that is too complicated, I vote for keeping everything error. It shouldn't happen regularly in prod anyway (when it does it also basically helps to notice BF attacks / scans going on)

@nextcloud-bot nextcloud-bot mentioned this pull request Aug 28, 2025
@provokateurin provokateurin force-pushed the fix/dispatcher/catch-type-errors-bad-request branch from 7cc04dd to 5937a87 Compare August 28, 2025 12:36
@provokateurin provokateurin requested a review from susnux August 28, 2025 12:36
@provokateurin
Copy link
Member Author

It was actually quite easy to check with reflection whether the TypeError happened when calling the controller method or if it happened later on.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🛑 Sounds okay to merge after branch off then? :P

@nickvergessen nickvergessen disabled auto-merge August 29, 2025 10:00
@susnux susnux enabled auto-merge September 4, 2025 15:20
@susnux susnux disabled auto-merge September 4, 2025 17:05
@susnux
Copy link
Contributor

susnux commented Sep 4, 2025

@provokateurin Cypress seems to be related

@provokateurin
Copy link
Member Author

Runner 8 with theming/admin-settings.cy.ts or something else?
If it's related, I really don't want to know how something depends on type errors 🙈

…onses

Signed-off-by: provokateurin <kate@provokateurin.de>
@susnux susnux force-pushed the fix/dispatcher/catch-type-errors-bad-request branch from 5937a87 to 9473f47 Compare September 4, 2025 22:49
@susnux susnux merged commit 6a0d4f3 into master Sep 4, 2025
199 checks passed
@susnux susnux deleted the fix/dispatcher/catch-type-errors-bad-request branch September 4, 2025 23:24
@susnux
Copy link
Contributor

susnux commented Sep 4, 2025

Hm I do not get it, it was broken with 8 retries and a rebase just fixed it...

@skjnldsv skjnldsv modified the milestones: Nextcloud 33, Nextcloud 32 Oct 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants