Most data leaks are caused by quark in ->orWhere()

[joelharkes]

For some context: we are building a multi tenant application in Laravel using a single database because we need to do operations over all our clients this seemed for us the best and simplest model. We deploy regularly (now once a week). In the last 2 months we had (almost) 2 data leaks because of a single 'bug/quark' in Laravel:

$organization->users()
    ->where('role', 'x')
    ->orWhere('age', 10);

As you might think: this code does not get all users of organization with either role x or age 10.
No, this wil get 'users from $organization with role X or ANY user of all organizations with age 10.
As you can see data can easily be leaked this way.

To 'fix' this code a Laravel developer must write it so:

$organization->users()
    ->where(fn($query) => $query
      ->where('role', 'x')
      ->orWhere('age', 10)
    );

We are at a loss:

• we put it at the top of our security checklists and
• we are even looking into building a custom larastan analyzer to make sure orWhere() are never happening at the root level of a query.

My question is: are no other companies using the Laravel stack dealing with this exact same problem?

And secondly to plead: can we please fix this quark in the next major Laravel release. My suggestion would by that everything after the relationship: $organization->users() part is al done automatically in a sub-where statement so that:

$organization->users()
    ->where(fn($query) => $query
      ->where('role', 'x')
      ->orWhere('age', 10)
    );

Translates to:

SELECT *
FROM Users
WHERE organization_id = :id
AND (
   role = x
   OR age = 10
)

instead of now:

SELECT *
FROM Users
WHERE organization_id = :id
AND role = x
OR age = 10

[joelharkes]

To explain: do not allow any orWhere() at root level of query builder.

Laravel could disable or throw exception whenever orWhere is used in root level of query so that the following code would fail:

User::query()->where()->orWhere()

And would have be rewritten as:

User::query()->where(fn() => 
    ->where()
    ->orWhere()
);

Although i think this is very drastic because it will reduce usability/simplicity. It will make it very explicit when using OR's in queries it is always done with consious thinking

[rojtjo]

Blocking root level orWheres on relationships would actually break a use case I have.

Imagine a Product with a relatedProducts relation. In my current setup the product_id is nullable which makes the related product generic. In order to retrieve all related products I perform a query like this:

$relatedProducts = $product
    ->relatedProducts()
    ->orWhereNull('product_id')
    ->get();

I think your use case is best solved by just creating a test and/or a custom phpstan rule that blocks it.

[joelharkes]

This you could must better solve by not starting from the relationship but directly from the eloquent model.

To be honest i think most people would have to reread this code 3 times before understanding what you want to achieve.

Ideally you still want to put your both product_id where's in a nested where to avoid any further filters/where rules not working correctly. (Images also having to filter on active products and your query already breaks)

[macropay-solutions]

@joelharkes thanks for this heads up. A better solution would be to not allow orWhere method on the Relation.

\Illuminate\Database\Eloquent\Relations\Relation::__call

      /**
       * Handle dynamic method calls to the relationship.
       *
       * @param  string  $method
       * @param  array  $parameters
       * @return mixed
       * @throws \Exception
       */
      public function __call($method, $parameters)
      {
          if (static::hasMacro($method)) {
              return $this->macroCall($method, $parameters);
          }

          if (strpos($method, 'orWhere') === 0) {
               throw new \Exception('orWhere methods on a relation are forbidden.'); // to include all orWhere methods from QueriesRelationships, eloquent builder and from query builder
          }
  
          return $this->forwardDecoratedCallTo($this->query, $method, $parameters);
      }

or using a macro on Relation named orWhere that throws Exception (valid only for one function at a time)

      Relation::macro(
          'orWhere',
          /**
           * @throws \Throwable
           */
          function (...$arguments): \Illuminate\Database\Eloquent\Builder {
              throw new \Exception('orWhere method on a relation is forbidden.');
          }
      );

but all orWhere functions are calling
public function where($column, $operator = null, $value = null, $boolean = 'and') so the above is not a bullet proof solution....

[henzeb]

I think at root level no longer orWhere. But in that case I would add a way to disable that, because not every developer is a changeling named Odo.

[macropay-solutions]

But there will still be where method with $boolean param as 'or' so... better to train the developers to NOT use OR without parenthesis/callback.

[henzeb]

The method I had in mind should log this as a deprecation warning. In the major version that follows you can take away that switch.

[macropay-solutions]

There are tens of functions that need tweaking (in these 3 classes: QueriesRelationships, Eloquent builder and Query builder) and just for the Relation instance not for other instances. I think this will never be blocked. I changed my vote to no.