[Bug]: Dispatcher should parse and validate parameter types, fail if not as declared

[thlehmann-ionos]

⚠️ 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

Dispatcher should parse and validate parameter types, fail if not as declared

Expected

The dispatcher should fail calling the controller if the passed parameter type does not match the declared type.

Actual

The controller method is excuted with a wrong cast type.

Background

Coarse description of how requests are handled by controllers:

1. Router finds matching route, finds controller class
2. Dispatcher uses a reflector to inspect the target method arguments
3. Dispatcher converts request parameters
4. Dispatcher executes method on controller class

Analysis of executeController(Controller $controller, string $methodName)

The method extracts parameter values from the request and executes the looked up controller method with these parameters.

It uses a "Reflector" to determine types of the to-be-called method and tries to cast the types. It does not parse primitive values, but casts them.

Types can be specified by PHP comment annotation @type. Acceptable cast types are: int, integer, bool, boolean, float, double.

Type conversion is done via settype.

The problem

The problem with this is that wrong parameter values are silently converted to wrong values.

Passing a string notanumber to a controller method annotated with @int will result in a parameter 0 (type int) to be passed.

See also the example in the note below.

If zero is a valid value from the controller's point of view this might accidentally lead to a wrong code execution.

The proposed solution

The code shoud try to parse passed parameters and fail with an error response if parsing failed.

• filter_var() alone is not suitable because it can't handle hex numbers
• is_numeric() alone is not suitable because it can't handle hex numbers

I'm not familiar enough with PHP to come up with a thorough solution right now, but I think this involves testing the string using Regular Expressions, converting hex to decimal using hexdec() if applicable, combined with parsing them with filter_var().

Side note on settype()

The behavior of settype() is even unpredictable as it does not properly recognize (tempted to say "parse") the type. Here's an example with PHP 8.1.2:

php > $foo = "0x20";
php > settype($foo, "int");
php > echo $foo;
0
php > $foo = "20";
php > settype($foo, "int");
php > echo $foo
20
php > $bar = 0x20;
php > echo $bar;
32

Tried Github search terms:

• is:issue controller type cast
• is:issue controller type parameter

Tried Google searches:

• nextcloud controller type parameter validate OR validation OR cast site:github.com

Steps to reproduce

1. Find a controller that makes a method with @param int typed parameter callable via URL
2. Call that route and pass a string

Expected behavior

• The request fails
• The controller method is not executed

Installation method

Community Manual installation with Archive

Nextcloud Server version

29

Operating system

Debian/Ubuntu

PHP engine version

PHP 8.1

Web server

Apache (supported)

Database engine version

SQlite

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

Fresh Nextcloud Server install

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

No response

List of activated Apps

No response

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

[joshtrichards]

Reminds me a bit of some thinking I was having when I started on #46231 the other day.