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

Return type of AbstractActionController->getRequest can be improved #77

Open
Koen1999 opened this issue Jul 14, 2021 · 2 comments
Open

Comments

@Koen1999
Copy link

Bug Report

Q A
Version(s) 3.2.0

Summary

PhpDoc of Laminas\Mvc\Controller\AbstractActionController (or its base class Laminas\Mvc\Controller\AbstractController actually) can be improved.
In reality when calling getRequest(), you may want to use methods like isPost, which is not implemented by the currently returned interface. A subclass like Laminas\Http\Request would be more desirable.
A similar issue applies to getResponse().
As far as I could tell from looking at the code, no other classes that implement this interface are ever returned. But I think that one of the contributors could better verify this.

Current behavior

Laminas\Mvc\Controller\AbstractActionController returns an Laminas\Stdlib\RequestInterface when calling getRequest()
Laminas\Mvc\Controller\AbstractActionController returns an Laminas\Stdlib\ResponseInterface when calling getResponse()

How to reproduce

Extend AbstractAction Controller and call getRequest() inside any action.
For a simple example, look at https://github.com/GEWIS/gewisweb/blob/1b16fa9a90043cd6d20e284e0bea0d8e18aededf/module/Frontpage/src/Controller/PollController.php#L77

Expected behavior

Laminas\Mvc\Controller\AbstractActionController returns an Laminas\Http\Request when calling getRequest()
Laminas\Mvc\Controller\AbstractActionController returns an Laminas\Http\Request when calling getResponse()

@amadis85
Copy link

In my case return types of AbstractActionController->getRequest and AbstractActionController->getResponse leads to the construction of an incorrect taint flow by PSALM SAST.

Functions getRequest() and getResponse() should return HttpRequest and HttpResponse objects, but return Request and Response types. It’s critical when we do taint analysis. Invalid types lead to missing a lot of vulnerability sources.

@Ocramius
Copy link
Member

The Laminas\Stdlib\* instances being returned are by design: that's because laminas/laminas-mvc was designed to also handle CLI actions inside controllers.

This can be adjusted with a BC break in ^4.

@Ocramius Ocramius added BC Break Enhancement and removed Bug Something isn't working labels Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants