[Feature Request] report errors for `@@allow` policy rules that pass for anonymous users

[ymc9]

Today a user must ensure that policy rules cover the cases of anonymous users (where auth() evaluates to null). This can be not obvious when comparing auth() with other (nullable) fields. E.g.:

@@allow('all', owner == auth())

If owner field is nullable, anonymous users are effectively granted full access to the entity, which may not be the intention.

@sidharthv96 suggests that at compile time, we detect the cases where policy rules pass for anonymous users and report errors. Users can explicitly suppress the errors with some kind of special comments (similar to how you suppress lint errors).

[sidharthv96]

Instead of special comment, we could simply enforce a not null check. This way, the learning curve doesn't go up as people don't have to remember yet another suppression comment syntax.

model ThingWithNullableOwner {
owner   User?  @relation

// Error, nullable field `owner` is comapred to `auth()`, this policy will allow unauthenticated access. 
@@allow('all', owner == auth())
@@allow('all', auth() == owner)

// Pass cases
@@allow('all', auth() != null && owner == auth())
@@allow('all', owner != null && owner == auth())
@@allow('all', owner == auth() && owner != null)
@@allow('all', owner == auth() && auth() != null)
// and the rest of the combinations.
}

model ThingWithNonNullableOwner {
owner   User  @relation

// Pass
@@allow('all', owner == auth())
}

[ymc9]

I think we need to have a way to let it pass without fixing because one may intentionally want to grant such access. That's why I'm thinking of a way of suppressing it without changing the rule itself.

What do you think?

[sidharthv96]

If they need to allow unauthorized access, rules like this should be fine no?
It explicitly checks the null case and the == case.

@@allow('all', owner == null || owner == auth())
@@allow('all', auth() == null || owner == auth())
@@allow('all', auth() == null || owner == null || owner == auth())

The problem I've noticed with suppress comments is that they get abused a lot. People sometime just copy-paste it without knowing what the actual implications are. If it's an eslint suppression of any type, the risk is very low, compared to suppressing warnings that could lead to entire DBs being exposed openly.

Should this logic be enough to check all the possibilities?

flowchart-elk TD
    A[Get all `==` comparisons] -->|For each pair| B(Split LHS & RHS)
    B --> C{Is LHS/RHS a nullable field?}
    C -->|Only One Field is| D[Done]
    C -->|No| D
    C -->|Both are nullable| F{"Is there a `!= null` or `== null` check?"}
    F -->|Yes, for 1 field| D
    F -->|Yes, for both fields| D
    F -->|No| Error

Loading

[ymc9]

Great point! I agree suppressing with an explicit null check is cleaner (and safer) than with comments. This should be the way to go!

[justin-hackin]

For me this begs the question: what use is the @password attribute if it needs an enhanced prisma and the user is anonymous at creation time? I asked this as a part of my discussion here: #1264