Model relations don't work in global scopes of eager loaded models

[wizzymore]

Laravel Version

12.28.1

PHP Version

8.4.12

Database Driver & Version

Sqlite and postgres

Description

Doing queries in the constructor of a Global Scope that is applied to a model that is eager loaded will not apply the foreign key conditions for the relations loaded in scope for queries unrelated to the scope itself. You can see more details about this below
To note: I tested and it doesn't affect the result if the scope is added by an attribute or directly in booted

Steps To Reproduce

1. Run git clone https://github.com/wizzymore/laravel-bug-query
2. Run the migrations ( ignore the dumps in the console )
3. Open the application on the web
   The dumps will show the query that was run and the models that were fetched.
   The problem is that the query runs without the where condition
   The second dump is the model that was fetched ( wrong )
   The third dump is the model that should have been fetched

The source of the bug and dumps are in \App\Models\Scopes\TestScope.
TestScope is used on the Testing model, the bug appears when we run: Testing2::with('testing')->first()
Testing2 is a model that lazy loads testing ( which has the previously mentioned global scope ), when we lazy load the scoped model all queries ran inside the scope don't use foreign keys.

You can also access the /2 path which shows that if we don't eager load the scoped model and just construct the scope directly the query will work.

[wizzymore]

Well, after digging through the laravel code i kind of found out why(i think), when eager loading relationships the Relation::noContraints() is called so there will be no constraints in all the scopes either, neither in construct nor apply. Is there anyway this behaviour could be done so it is consistent between uses? Or atleast change when the constructor for scopes is created so that the constructor doesn't suffer from the noConstraints of the eager load?

[wizzymore]

For me this is a big issue because let's say we have the following:

a Post model with a column 'tenant_id'
User that has a relationship current_tenant() something like hasOne(Tenant::class)->where('active', true) as an example
If you want to globally scope the Post model with 'tenant_id' as a column with a global scope that get's the current user's tenant_id like so: $query->where('tenant_id', Auth::user()->current_tenant->id) this will only work if you don't eager load the query, if you eager load the Post anywhere this will result in unexpected behaviour, because if it was not eager loaded it would have worked, if it was a local scope and eager loaded it would have worked, currently it is like so:

• Global scope + eager loading - bonked
• Global scope + lazy loading - works
• Global scope + direct model query ( not as relationship ) - works
• Global scope registered in ->with like so: ->with(['relation_with_global_scope' => fn ($q) => $q->withGlobalScope(Scope::class, new Scope)]) - works
• Local scope + eager loading - works
• Local scope + lazy loading - works
• Local scope + direct model query ( not as relationship ) - works

[macropay-solutions]

This is an ancient bug(vulnerability) that we fixed in our crud lib (also in the free one) and it was not fixed in the framework because it is considered an edge case.

You can either try a pull request with the fix (see the link) in laravel, either use our free crud lib.

[wizzymore]

Relation::noContraints() is called so there will be no constraints in all the scopes either, neither in construct nor apply

Apparently i tested this wrong, doing the query in the apply() function actually fixes the issue. I will keep this open for visibility ( as i still consider it a bug because the noConstraints is put too early )

[macropay-solutions]

@wizzymore what is your opinion on this eager load bug. Is it just a corner case (that can lead to data leaks) or it should be fixed?

[wizzymore]

@wizzymore what is your opinion on this eager load bug. Is it just a corner case (that can lead to data leaks) or it should be fixed?

I consider this a data vulnerability, not a bug, it almost lead to a data leak in the application I'm working on but fortunately we already have tests that check if we return data that doesn't belong to a user, but this will not be the case for someone else.
Having a condition dismissed from a query is a huge issue, the big reason people use global scopes is for exactly what this bug does to not happen.

[macropay-solutions]

That is what we thought also.
Recently we needed to implement a global scope that uses the fix. Gladly we saw it and worked on it proactively before we needed it.

We were and still are stunned that Laravel community does not fix this.
The fix is documented in comment and also as a reference with the final version in the original issue. We tested it. Works.