anti-work / shortest

Have AI write and update tests for you
https://shortest.com
MIT License
187 stars 21 forks source link

security: prevent db queries from being callable on the client. #79

Closed bjornpagen closed 3 weeks ago

bjornpagen commented 3 weeks ago

This PR changes queries.ts from "use-server" to "server-only", ensuring that DB query functions are reserved for server-side use.

Exported functions in a "use-server" file are exposed to the frontend. This meant users could call functions like getUserByClerkId() or updateUserSubscription() with an arbitrary clerkId.

https://nextjs.org/blog/security-nextjs-server-components-actions#write

Impact is low, since clerkId is a random value.

vercel[bot] commented 3 weeks ago

@bjornpagen is attempting to deploy a commit to the Antiwork Team on Vercel.

A member of the Team first needs to authorize it.

bjornpagen commented 3 weeks ago

Maybe shortest should do some code auditing as well in addition to just writing tests?

slavingia commented 3 weeks ago

You're referencing a year-old blog post. Don't think this is encouraged. "use server" should work fine.

slavingia commented 3 weeks ago

If truly a vulnerability, could you replicate?

bjornpagen commented 3 weeks ago

Yep! I pasted this in my JS console to replicate in local dev mode on my machine. Hopefully it works for you but the 'Next-Action' hash could be different for you:

fetch('/dashboard', {
  method: 'POST',
  headers: {
    'Accept': 'text/x-component',
    'Content-Type': 'text/plain;charset=UTF-8',
    'Next-Action': '517fcff9babb9d9cc7156a7efd147563b7b68787',
    'Next-Router-State-Tree': '%5B%22%22%2C%7B%22children%22%3A%5B%22(dashboard)%22%2C%7B%22children%22%3A%5B%22dashboard%22%2C%7B%22children%22%3A%5B%22__PAGE__%22%2C%7B%7D%2C%22%2Fdashboard%22%2C%22refresh%22%5D%7D%5D%7D%5D%7D%2Cnull%2Cnull%2Ctrue%5D'
  },
  body: '["payload"]',
  credentials: 'include'
})
.then(res => res.text())
.then(console.log)
.catch(console.error)

This code snippet will call the endpoint defined by getUserByClerkId() with the string "payload".

Here's the response:

0:{"a":"$@1","f":"","b":"development"}
1:E{"digest":"1264022799","message":"relation \"users\" does not exist","stack":[["ErrorResponse","/Users/bjorn/Code/shortest/.next/server/chunks/ssr/d6103_postgres_src_a87039._.js",1256,224],["handle","/Users/bjorn/Code/shortest/.next/server/chunks/ssr/d6103_postgres_src_a87039._.js",1056,728],["TLSSocket.data","/Users/bjorn/Code/shortest/.next/server/chunks/ssr/d6103_postgres_src_a87039._.js",944,17]],"env":"Server"}

Of course, this requires the user to figure out first the 'Next-Action' hash already for the function in question. If the frontend has called this function before, the user can inspect DevTools to find out the hash, and call the function again with arbitrary parameters.

You're right that this isn't a strong vulnerability, especially since these functions are not used anywhere client side at the moment. But it might be better practice to not expose these functions at all if possible, since they appear to be for server use.

At the very least, we can remove the "use server" directive from the top of the file, without adding import "server-only". That fixes the problem as well. server-only is just a package that prevents you from accidentally importing the function client side.