SQLDataSource icon indicating copy to clipboard operation
SQLDataSource copied to clipboard

Using example will result in major security issues

Open marvin-kolja opened this issue 3 years ago • 2 comments

First of thanks for this nice data source.

The reason why I'm writing is your given example in the usage section. You're creating the MyDatabase instance outside of the context creation, thus, it will only get created once. Reusing this instance (data source) will result in context being overwritten by resolvers. A more detailed example:

  1. user 1 makes a request
    • database gets initialized with context that contains the user id
    • the resolver waits 5 seconds
    • then executes a database call
  2. user 2 makes a request after user 1 (first requests resolver still waits!)
    • database gets initialized with context (overwrites context) that contains the user id
    • the resolver waits 5 seconds
    • then executes a database call

Both request have different context, but the database instance context is being overwritten, meaning the first requests database call will have the context of request 2. Generally this won't happen since queries are fast, but when using a websocket server that will create the context only once on subscribe this becomes a major problem.

This is how I implemented it instead:

- const db = new MyDatabase(knexConfig);
+ const knexInstance = knex(knexConfig)

const server = new ApolloServer({
  typeDefs,
  resolvers,
  cache,
  context,
-  dataSources: () => ({ db })
+  dataSources: () => ({ db: new MyDatabase(knexInstance) })
});

Maybe I'm not understanding the example correctly. Anyway, I'd love to hear you feedback, thanks.

marvin-kolja avatar Sep 06 '22 14:09 marvin-kolja

I just realized that my new method introduces a new issue. It's discussed here: https://github.com/cvburgess/SQLDataSource/issues/81

If you don't have much time I would create a PR for this.

marvin-kolja avatar Sep 06 '22 15:09 marvin-kolja

@marvin-kolja - the example is not robust on purpose, its simply an example based loosely on Apollos docs from a few years back when this lib was created.

I personally don't use this anymore as I am several jobs removed from the team I was on that needed this.

If 2.1.0 doesnt resolve your issue, or if you have suggestions for improving the code or docs, please do open a PR!

cvburgess avatar Apr 24 '23 16:04 cvburgess