graphile / graphile-engine

Monorepo home of graphile-build, graphile-build-pg, graphile-utils, postgraphile-core and graphql-parse-resolve-info. Build a high-performance easily-extensible GraphQL schema by combining plugins!
https://www.graphile.org/
761 stars 129 forks source link

documentation/question: query escaping #154

Closed cdaringe closed 6 years ago

cdaringe commented 6 years ago

problem

discussion

i loaded the following into my query function:

{
  searchTopics(search: "abc; DROP FUNCTION search_topics;") {
    nodes {
      id
    }
  }
}

and my function remained, so i assume the input is escaped in node (maybe in the pg client?), but it's probably worth making explicit.

benjie commented 6 years ago

If I understand correctly you’re addressing the threat of SQL injection due to string concatenation within a SQL function; I suggest you read the functions section of the Postgres manual to understand how variables are used in SQL statements within SQL functions. I don’t think there’s any advantage duplicating that documentation into PostGraphile, and further I think it would be detrimental - giving the reader a false sense of security in their understanding. It would be better for them to doubt it and read about it in the PostgreSQL manual and get a solid understanding including the various caveats than for us to allay their fears with a sentence or two... and later they put their app in danger because they haven’t understood fully.

Aside: your attempt at SQL injection there wouldn’t do what you intended anyway because you don’t close the query (there was open parentheses) and your drop statement lacks parentheses so even if it were executed it would fail with an error. The best you’d hope for was a syntax error, which in itself is a useful tool for detecting SQLi vulnerabilities.

The string the function will receive is the one you type in GraphQL; it is possible to make your function vulnerable to SQLi and we do not (and will not, and probably can not) protect you from this. That said because the function does not EXECUTE any dynamic SQL the function we demonstrate is not vulnerable to SQLi no matter what string you pass to it (whether through a direct connection to postgres or via PostGraphile).

cdaringe commented 6 years ago

Thanks. I'm coming back to using PG for the first time in probably 2.5-3 years. Stuff is fuzzy. Thanks for the breakdown

benjie commented 6 years ago

No problem - it's definitely better to check things that you're not certain of!

You can ask questions like this in our Gitter too, if you like:

https://gitter.im/postgraphql/postgraphql