Skip to content

Fix #37740: Cast all Request parameter values to string #37755

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

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

rgeraads
Copy link

@rgeraads rgeraads commented Aug 6, 2020

Q A
Branch? 5.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #37740

This fix ensures that all parameter values in Browserkit\Request are received as strings on the receiving end.

@rgeraads rgeraads force-pushed the cast-parameter-values-to-string branch from 2caf49e to 7ef6a60 Compare August 6, 2020 13:25
@rgeraads rgeraads changed the base branch from master to 5.1 August 6, 2020 13:25
@stof
Copy link
Member

stof commented Aug 6, 2020

This is not the right fix for the issue. For instance, ->attributes is perfectly allowed to hold other types (for instance booleans) and that would be a BC break.

The right fix must be implemented in the conversion layer between BrowserKit and HttpFoundation, for the query and request parameters only.

@rgeraads
Copy link
Author

rgeraads commented Aug 6, 2020

Do you mean here?

$request = $parameters;
$query = [];
break;
default:
$request = [];
$query = $parameters;

I can put the casting right before the switch/case

@stof
Copy link
Member

stof commented Aug 6, 2020

No. This place is still in the HttpFoundation request object itself (which might add overhead the handling of actual web requests btw).

This should either be done when creating the BrowserKit request (might make sense as other BrowserKit implementations would also not be able to submit non-string values), or when converting from one to the other in

protected function filterRequest(DomRequest $request)
{
$httpRequest = Request::create($request->getUri(), $request->getMethod(), $request->getParameters(), $request->getCookies(), $request->getFiles(), $request->getServer(), $request->getContent());

@rgeraads
Copy link
Author

rgeraads commented Aug 6, 2020

It would make most sense to put it in BrowserKit\Request then.

@rgeraads rgeraads changed the title Fix #37740: Cast all ParameterBag values to string Fix #37740: Cast all parameters values to string Aug 6, 2020
@rgeraads rgeraads force-pushed the cast-parameter-values-to-string branch from 825b825 to 2592523 Compare August 6, 2020 20:26
@rgeraads rgeraads changed the title Fix #37740: Cast all parameters values to string Fix #37740: Cast all parameter values to string Aug 6, 2020
@rgeraads rgeraads changed the title Fix #37740: Cast all parameter values to string Fix #37740: Cast all Request parameter values to string Aug 6, 2020
@rgeraads rgeraads force-pushed the cast-parameter-values-to-string branch 2 times, most recently from 39a3c0d to aa891d8 Compare August 12, 2020 14:19
@rgeraads rgeraads force-pushed the cast-parameter-values-to-string branch from aa891d8 to eb6641f Compare August 18, 2020 10:34
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Should target master.

@@ -113,4 +113,13 @@ public function getContent()
{
return $this->content;
}

private static function convertAllValuesToString(array $parameters): array
Copy link
Member

Choose a reason for hiding this comment

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

Could be inline.

@rgeraads rgeraads force-pushed the cast-parameter-values-to-string branch from 81ada0b to eaba19f Compare August 18, 2020 11:42
@rgeraads rgeraads changed the base branch from 5.1 to master August 18, 2020 11:43
@rgeraads rgeraads requested a review from fabpot August 18, 2020 11:43
@xabbuh xabbuh added this to the next milestone Aug 18, 2020
@fabpot
Copy link
Member

fabpot commented Aug 18, 2020

Thank you @rgeraads.

@fabpot fabpot force-pushed the cast-parameter-values-to-string branch from d3aaf3e to d4e2cec Compare August 18, 2020 14:08
@fabpot fabpot merged commit 6539a0f into symfony:master Aug 18, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
@rgeraads rgeraads deleted the cast-parameter-values-to-string branch October 7, 2020 13:28
@kernio
Copy link

kernio commented Mar 13, 2021

Am I just wondering in addition to fix the "null" issue (that is good), why this modified behavior for parameters I passed as POST request?

Input request parameters
array:8 [
"profileId" => 1000
]

After
array_walk_recursive($parameters, static function (&$value) {
$value = (string) $value;
});

array:8 [
"profileId" => "1000"
]

Input validation for request
/**
* @Assert\Type(type="integer")
* @Assert\NotNull
*/

Response:
Error: profileId should be "int"

@kernio
Copy link

kernio commented Mar 13, 2021

Am I just wondering in addition to fix the "null" issue (that is good), why this modified behavior for parameters I passed as POST request?

And answer to my own question (for anyone who had the same issue) is not using "params" here that used for application/x-www-form-urlencoded, but using a body with JSON encoded parameters and a proper header.

Before:

        $this->client->request(
            'POST',
            $uri,
            $parameters,
            [],
            array_merge(['HTTP_AUTHORIZATION' => sprintf('Bearer %s', $this->jwtToken)], $headers)
        );

After:

        $this->client->request(
            'POST',
            $uri,
            [],
            [],
            array_merge(
                ['HTTP_AUTHORIZATION' => sprintf('Bearer %s', $this->jwtToken), 'CONTENT_TYPE' => 'application/json'],
                $headers
            ),
            json_encode($parameters, JSON_THROW_ON_ERROR)
        );

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.

It is possible to pass request parameters of any type to the Symfony web crawler
7 participants