core icon indicating copy to clipboard operation
core copied to clipboard

Request's sanitizeInput() fails if testing an Action with a concrete Request that has URL params

Open rscarrasco opened this issue 1 year ago • 1 comments

Apiato Core Version

8.x

PHP Version

8.1.x

Database Driver & Version

No response

Description

Suppose we have an action that receives it's concrete Request as parameter at it's run() method, and that it uses the Requests's sanitizeInput() on an URL parameter. In this situation, the parameter returned will be null, and not the value given to it.

Steps To Reproduce

Assume the following code:

// SomeAction.php
class SomeAction extends Action
{
    // Notice that the argument is the SomeRequest concrete class, and not the Request parent
    public function run(SomeRequest $request)
    {
        $data = $request->sanitizeInput(['some-url-param']);
        return $data['some-url-param']); // will be null during unit testing
    }
}

// SomeRequest.php
class SomeRequest extends Request {
    protected $urlParameters = ['some-url-param'];
}

// SomeActionTest.php
class SomeActionTest extends UnitTestCase
{
    public function testShouldSanitizeUrlParam()
    {
        $request = SomeRequest::injectData()->withUrlParameters(['some-url-param' => 123]);

        $result = app(SomeAction::class)->run($request);

        $this->assertEquals(123, $result); // this will fail
    }
}

@Mohammad-Alavi and I follwed the trail, and we found that

  1. sanitizeInput() (Apiato\Core\Abstracts\Requests\Request) calls
  2. mergeUrlParametersWithRequestData() (same file), which in turn calls
  3. route() (Illuminate\Http\Request).

route() will return null whenever the Request's getRouteResolver() fail to find an route, which is the case here (SomeRequest was manually instantiated).

FakeRequest does not declare any URL parameters, and so sanitizeInput() will play along with it.

rscarrasco avatar Jun 10 '24 18:06 rscarrasco

@rscarrasco I'm using $data = $request->validated(); Why $request->validated() is Superior to $request->sanitize([])

  1. Security by Default
  • validated() only returns data that has passed validation rules - anything not defined in your rules is automatically rejected
  • sanitize([]) requires you to explicitly list every field, which is error-prone and easy to forget fields
  1. Mass Assignment Protection
  • validated() ensures only validated fields can be mass-assigned to models, preventing mass assignment vulnerabilities
  • sanitize([]) doesn't provide this protection - attackers could inject unexpected fields
  1. Laravel Standard
  • validated() is the Laravel framework standard, meaning:
    • Better IDE autocomplete and tooling support
    • Consistent with Laravel documentation and community practices
    • Future-proof as Laravel evolves
    • Easier onboarding for new developers
  1. Less Code, Fewer Bugs // With validated() - automatic, safe $data = $request->validated();

// With sanitize() - manual, error-prone $data = $request->sanitize([ 'field1', 'field2', 'field3', // Easy to forget a field or add one that shouldn't be there // ... must list everything ]);

  1. Single Source of Truth
  • With validated(), your validation rules ARE your whitelist
  • With sanitize(), you maintain two separate lists (validation rules + sanitize array), which can get out of sync
  1. Type Safety
  • Validation rules enforce data types, ensuring you get what you expect
  • sanitize() just filters - no type guarantees

Example of the Risk: // Dangerous - what if you forget to add 'is_admin' to sanitize? $data = $request->sanitize(['name', 'email']); // If 'is_admin' comes through, it's not sanitized but might still be used

// Safe - 'is_admin' won't be in the returned array unless validated $data = $request->validated();

Bottom Line: validated() follows the principle of "secure by default" - you explicitly define what's allowed, and everything else is rejected. sanitize() is "insecure by default" - you must remember to filter everything out.

Gigiart avatar Oct 06 '25 15:10 Gigiart