search

laminas / laminas-mvc

Laminas's event-driven MVC layer, including MVC Applications, Controllers, and Plugins
https://docs.laminas.dev/laminas-mvc/
BSD 3-Clause "New" or "Revised" License
139 stars 51 forks source link

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

Open Koen1999 opened 3 years ago

Koen1999 commented 3 years ago

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 commented 1 year ago

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 commented 1 year ago

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.