Skip to content
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.

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

[5.x] Functions to access Request parameters with specific data types #17177

Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from

Conversation

jozefgrencik
Copy link
Contributor

These functions solve the problem with functions like Request->getQuery() that return a mixed type.

Many programmers overlook the possibility of receiving an unexpected array instead of a string, which can occur, for example, when bots are searching for security vulnerabilities. In certain cases, script execution may fail due to input validation occurring deeper in the code.

PHP is becoming more type-oriented, and tools like PhpStan encourage documenting mixed type through PhpDocs comments.

These functions prevent potentially dangerous typecasting of mixed type, so we enhance the overall security.

The development process becomes more efficient as programmers no longer need to focus on handling multiple input types. And no need to add comments/suppressions for Psalm and PhpStan.

Before:

$token = $this->request->getCookie($this->_config['cookie']);
if ($token !== null) {
    /** @psalm-suppress PossiblyInvalidCast */
    $token = (string)$token;
}

return $token;

After:

return $this->request->getCookieString($this->_config['cookie']);

The new functions cover the "get" operations from:

  • $request->cookies
  • $request->query
  • $request->params
  • $request->data

For types:

  • int
  • string
  • bool

Q: Is the $default parameter missing?
A: No, instead of $default, the ?? operator is now used, starting from PHP 7 :)

These functions solve the problem with functions like Request->getQuery() that return a mixed type.

Many programmers overlook the possibility of receiving an unexpected array instead of a string, which can occur, for example, when bots are searching for security vulnerabilities.
In certain cases, script execution may fail due to input validation occurring deeper in the code.

PHP is becoming more type-oriented, and tools like PhpStan encourage documenting mixed type through PhpDocs comments.

These functions prevent potentially dangerous typecasting of mixed type, so we enhance the overall security.

The development process becomes more efficient as programmers no longer need to focus on handling multiple input types. And no need to add comments/suppressions for Psalm and PhpStan.

Before:
```php
$token = $this->request->getCookie($this->_config['cookie']);
if ($token !== null) {
    /** @psalm-suppress PossiblyInvalidCast */
    $token = (string)$token;
}

return $token;
```

After:
```php
return $this->request->getCookieString($this->_config['cookie']);
```

The new functions cover the "get" operations from:
 - $request->cookies
 - $request->query
 - $request->params
 - $request->data

For types:
 - int
 - string
 - bool

Q: Is the `$default` parameter missing?
A: No, instead of `$default`, the `??` operator is now used, starting from PHP 7 :)
@othercorey othercorey added this to the 5.0 milestone Jul 4, 2023
@markstory
Copy link
Member

Thanks for putting this together. I like the idea of providing a type safe way to access request data. I'm a bit concerned with the number of methods we would be adding, and where we're adding them. While a trait makes it simple to compose this logic into components and controllers, I feel like we should be having this logic on the request itself. Perhaps we could use named parameters to help expand the existing getXyz methods to add casting.

$value = $this->request->getQuery('page', 1, as: 'int');

I think this interface could be more ergonomic, but is harder to have typing work out as PHP doesn't have a good way to narrow method return types. 🤔

@jozefgrencik
Copy link
Contributor Author

jozefgrencik commented Jul 5, 2023

@markstory Thanks for the good feedback ♥. We use that Trait in our work. Unfortunately, extending the ServerRequest itself outside of Cake is not possible, so we opted for using the Trait on Controllers instead. These methods are very popular :).

I have another idea. What do you think about it? (I wrote it quickly without any checking.)

/**
 * @method string|null getCookieString(string $key, string|null $default = null)
 * @method bool|null getCookieBool(string $key, bool|null $default = null)
 * @method int|null getCookieInt(string $key, int|null $default = null)
 * ...
 */
class ServerRequest implements ServerRequestInterface
{
    public function __call(string $name, array $params): mixed
    {
        if (str_starts_with($name, 'is')) {
            // ...
            return $this->is(...$params);
        } elseif (str_starts_with($name, 'getCookie')) {
            return $this->resolveAsType($name, $this->cookies, ...$params);
        } elseif (str_starts_with($name, 'getData')) {
            return $this->resolveAsType($name, $this->data ?? [], ...$params);
        } elseif (str_starts_with($name, 'getParam')) {
            return $this->resolveAsType($name, $this->params, ...$params);
        } elseif (str_starts_with($name, 'getQuery')) {
            return $this->resolveAsType($name, $this->query, ...$params);
        }
        throw new BadMethodCallException(sprintf('Method `%s()` does not exist.', $name));
    }

    private function resolveAsType(string $methodName, array|object $source, mixed ...$args): mixed
    {
        $asType = str_replace(['getCookie', 'getQuery', 'getParam', 'getData'], '', $methodName);
        $rawValue = Hash::get($source, $args[0], $args[1]);

        return match ($asType) {
            'String' => is_string($rawValue) ? $rawValue : null,
            'Int' => filter_var($rawValue, FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE),
            'Bool' => $this->filterBool($rawValue),
            default => throw new BadMethodCallException(sprintf('Method `%s()` does not exist.', $methodName)),
        };
    }
}

@markstory
Copy link
Member

@jozefgrencik That's an interesting approach. We had a related discussion in the core team and had another approach to consider as well:

namespace Cake\Utility;

function toInt(?mixed $value): ?int
{
}

Each type conversion would have a separate function that could be imported and used in both userland and framework code.

@jozefgrencik
Copy link
Contributor Author

@markstory Nice. I like it.

I will create a new class with the toInt()/toBool.. functions in a separate PR, and then we will see how we proceed with this PR. Okay?

@markstory
Copy link
Member

I will create a new class with the toInt()/toBool.. functions in a separate PR, and then we will see how we proceed with this PR. Okay?

Sounds great. Thank you 🙇

@ravage84 ravage84 changed the title Functions to access Request parameters with specific data types [5.x] Functions to access Request parameters with specific data types Aug 25, 2023
@markstory markstory modified the milestones: 5.0, 5.1.0 Sep 10, 2023
jozefgrencik added a commit to jozefgrencik/cakephp that referenced this pull request Sep 20, 2023
This PR introduces a set of methods for converting mixed values to string/int/bool data types, ensuring a type-safe approach to data manipulation.
This utility is useful for safely narrowing down the data types.

cakephp#17177 (comment)

https://en.wikipedia.org/wiki/IEEE_754
https://www.h-schmidt.net/FloatConverter/IEEE754.html
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER
@ajibarra
Copy link
Member

@markstory @ravage84 Hi guys, is there anything missing here from @jozefgrencik? or it can be merged.

@jozefgrencik
Copy link
Contributor Author

@markstory @ravage84 Hi guys, is there anything missing here from @jozefgrencik? or it can be merged.

Hi. First, we need to resolve #17295, then we'll be able to continue. I'll take a look at the PR today. Thx.

Copy link

This pull request is stale because it has been open 30 days with no activity. Remove the stale label or comment on this issue, or it will be closed in 15 days

@github-actions github-actions bot added the stale label Apr 14, 2024
@markstory markstory removed the stale label Apr 14, 2024
@jamisonbryant
Copy link
Contributor

Now that #17295 is merged I am happy to contribute to this branch to get it closer to done if @jozefgrencik doesn't mind.

@jamisonbryant jamisonbryant self-assigned this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants