Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using example will result in major security issues #83

Open
marvin-kolja opened this issue Sep 6, 2022 · 2 comments
Open

Using example will result in major security issues #83

marvin-kolja opened this issue Sep 6, 2022 · 2 comments

Comments

@marvin-kolja
Copy link

marvin-kolja commented Sep 6, 2022

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 marvin-kolja changed the title Example will result in major security issues Using example will result in major security issues Sep 6, 2022
@marvin-kolja
Copy link
Author

I just realized that my new method introduces a new issue. It's discussed here: #81

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

daniel-keller added a commit to daniel-keller/SQLDataSource that referenced this issue Sep 28, 2022
…cache')"

Corrected bug where passing in an existing instance of Knex connection results in failure to extend QueryBuilder with cache method.
`ERROR: Can't extend QueryBuilder with existing method ('cache')`

Fixes cvburgess#81 and I think cvburgess#83 but I would like @marvin-kolja thoughts on that.
See explanation here cvburgess#81 (comment)
@cvburgess
Copy link
Owner

@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 cvburgess reopened this Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants