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.7] Trim model class name when passing in middleware #25849

Merged
merged 1 commit into from
Oct 1, 2018
Merged

[5.7] Trim model class name when passing in middleware #25849

merged 1 commit into from
Oct 1, 2018

Conversation

ankurk91
Copy link
Contributor

@ankurk91 ankurk91 commented Oct 1, 2018

Hi,

When passing php class names in middleware, for example

// routes/web.php
Route::post('/', 'Job\DocumentController@store')->name('store')
            ->middleware('can:create, App\Models\JobDocument');
// Notice the whitespace after comma (,) in middleware()

Developer can accidentally add a whitespace before class name.
This whitespace will cause the linked policy not to be found and cause a 403 http response.

It took me hours to figure out why the policy class is not being loaded.

More insights here -
Laravel resolves for registered policy here

if (isset($this->policies[$class])) {

The registered policies array look like this

array:4 [▼
  "App\Models\Job" => "App\Policies\JobPolicy"
  "App\Models\Admin" => "App\Policies\AdminPolicy"
  "App\Models\UserDocument" => "App\Policies\UserDocumentPolicy"
  "App\Models\JobDocument" => "App\Policies\JobDocumentPolicy"
]

Having a space in model php class fails the isset($this->policies[$class]) condition.
The trim will ensure that isset call will not fail in this condition.
I don't see any side effects trimming the class name here.

Thanks.

When passing php class names in middleware, for example
```
->middleware('can:create, App\Models\JobDocument);
```

Developer can accidentally add a whitespace before class name.
This whitespace will cause the linked policy to not found and
cause a 403 http response.
@sisve
Copy link
Contributor

sisve commented Oct 1, 2018

I imagine that all comma-separated parameters sent to any middlewares should get this treatment. Does it make sense that only a specific parameter for a specific middleware supports comma+space?

If so, that would be done in Pipeline::parsePipeString(...) where we currently split on comma, but doesn't trim the resulting parts. Something as simple as a $parameters = array_map('trim', $parameters); may be enough.

protected function parsePipeString($pipe)
{
list($name, $parameters) = array_pad(explode(':', $pipe, 2), 2, []);
if (is_string($parameters)) {
$parameters = explode(',', $parameters);
}
return [$name, $parameters];
}

@ankurk91
Copy link
Contributor Author

ankurk91 commented Oct 1, 2018

@sisve
I agree with you; as long as it is non breaking.

@taylorotwell taylorotwell merged commit 54d3383 into laravel:5.7 Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants