Should change the JS api for "or" and some sql injection thoughts

[sroussey]

Supabaser should change the JS api for "or". It has the client add a raw postgrest query which is likely to not be cleaned for postgrest format leading to a kind of sql injection at the sql builder level. Particularly bad if done on a server with a service token with full access. Clearly a customer error, but a better api would prevent it.

Say someone is doing this:

// get somethings that are ownerless, plus the owner's somethings, but not others' somethings
const {data, error} = supabase.from('something').select().or(`account_id.is.null,account_id.eq.${account_id}`);

Now if account_id = '0,account_id.gte.1' then problems.

Maybe something like this:

const {data, error} = supabase.from('something').select().or(query=>query.is('account_id', null), query => query.eq('account_id',account_id`));

Also, you should check that someone is not accidentally passing a column with the name or as that can be lots of fun in the sql builder and open up all sorts of new fun stuff.

Also, assuming dot and comma and the like are encoded so they don't seem like sqlgrest stuff in all the other functions.

[steve-chavez]

const {data, error} = supabase.from('something').select().or(account_id.is.null,account_id.eq.${account_id});
Now if account_id = '0,account_id.gte.1' then problems.

Note that you can double quote inside id.eq."${account_id}", then an input like 0,account_id.gte.1 would be a value(which would give an according type error) and not an expression.

Not disagreeing otherwise with the idea, I also think or needs a better interface.

[steve-chavez]

I'm curious if the whitehat automated SQL injection tools also try the Postgrest SQL variety as well. They should or someone else will.

It's not SQL injection since you're not passing SQL, PostgREST's grammar is designed on purpose to be restrictive and not open full SQL to the world: https://postgrest.org/en/v8.0/api.html#custom-queries.

Yeah, the but you can just add quotes to account_id and continue the fun.

This is why Supabase enforces RLS. If a client passes an extra condition to an or it will just be including extra rows that the developer explicitly allowed the client to see, there's no leak there.

[sroussey]

This is why Supabase enforces RLS. If a client passes an extra condition to an or it will just be including extra rows that the developer explicitly allowed the client to see, there's no leak there.

Oh... it is always good to see a bias / context for a point of view....

What happens when you are using this on a server (or serverless function) and are using the service api key? You are bypassing the RLS.

BTW: this is why people that create tools need extra pairs of eyes for security issues -- when you or I build something, it often gets used in a way not intended. It might even get used that way more than the way intended.

[steve-chavez]

What happens when you are using this on a server (or serverless function) and are using the service api key? You are bypassing the RLS.

The service_role is only for backend use and not for frontend use(external clients). The backend is a "trusted" environment, if a malicious actor is manipulating REST calls there then that's a whole different issue.

[sroussey]

Yes, that was my third sentence in original post.

I think people will use the supabase api like an ORM. Both will generate SQL. If I passed something like account_id='dontcare" or 1=1"' to an ORM like const user = await connection.getRepository(User).createQueryBuilder("userdata").where("user.id is null or user.id = :account_id", { account_id }) I don't need to worry about SQL injection since it will write the SQL correctly.

But with supabase.from()... it can write the PostgRest query incorrectly (and so it is a Postgrest injection, if you want to call it that). And while the PostgRest language has a smaller surface area than SQL, it can still be written in a way to exfiltrate data. So if you are converting your API from TypeORM to supabase, this will surprise you. supabase.from('userdata').select().or(account_id.is.null,account_id.eq.${account_id}`) will get all users' data, not just yours. This is not like the ORM case, since hackers abused them and they are safe now, and they will not get all users data.

[marsupilamimon]

Hey! Is there any progress on this? The API still seems to be vulnerable to injection attacks.

Something like this would be safer:

supabase.from("userdata").select().or([["id", "gt", "1"], ["id", "lt", "5"]])

Just to be clear, I'm concerned about backend usage with the service key.

supabase.from("userdata").select().or(`id.gt.${request.get('attackerSuppliedString')}`)