Consider using an API design that prevents injection attacks

[not-an-aardvark]

Suppose someone is using this library to adding a star to a repository, where the repository ID comes from user input. If they don't know about GraphQL variables and they're being careless, they might implement it like this:

async function addStar(repoId) {
  const graphql = require('@octokit/graphql').defaults({
    headers: {
      authorization: `token secret123`
    }
  });

  const results = await graphql(`
    mutation {
      addStar(input: {clientMutationId: "x", starrableId: "${repoId}"}) {
        clientMutationId
      }
    }
  `);
}

This example code is vulnerable to a "GraphQL injection" attack, where someone could modify the structure of the query or perform additional actions by supplying malicious user input. (This works in a similar manner to a SQL injection attack.) For example, a malicious user could provide the following string in this case to maliciously add a topic to a repository:

some-repo-id"}) {
		clientMutationId
  }
  updateTopics(input: {clientMutationId: "y", topicNames:["evil-topic"], repositoryId: "some-other-repo-id

...which would result in the following malicious query getting executed after string concatenation happens:

mutation {
  addStar(input: {clientMutationId: "x", starrableId: "some-repo-id"}) {
		clientMutationId
  }
  updateTopics(input: {clientMutationId: "y", topicNames:["evil-topic"], repositoryId: "some-other-repo-id"}) {
		clientMutationId
  }
}

It's true that there's a way to rewrite this function to be safer (by using GraphQL variables), and similarly SQL engines usually allow for prepared statements. However, in SQL's case, developers still frequently shoot themselves in the foot and add injection vulnerabilities by using string concatenation, either by mistake or due to not knowing better. As a result, the broad consensus is that it's better if SQL libraries prevent string concatenation entirely (e.g. by not accepting strings as arguments). I think it would be a good idea if this library did a similar thing for GraphQL queries.

---

What would you think about the following alternative API?

// note: tagged template, not a regular function call
//                          ↓
const results = await graphql`
  mutation {
    addStar(input: {clientMutationId: "x", starrableId: ${repoId}}) {
      clientMutationId
    }
  }
`;

The library would expose a template tag function which either escapes embedded expressions or replaces them all embedded expressions with GraphQL variables, and adds them to the query appropriately. As far as I can tell, this would make it extremely difficult for someone to shoot themself in the foot and introduce an injection attack, because there would be no easy way to concatenate a string while still calling a template tag function.

If someone needed to use additional arguments (e.g. headers), they could use graphql.defaults:

const results = await graphql.defaults({ /* ... */ })`
  mutation { ... }
`

(With this design, you would probably want to guard against someone accidentally calling graphql(`foo`) or graphql([`foo`]) instead of graphql`foo` . This could be accomplished by making sure the first argument to the graphql function is an array with a raw property.)

Thanks for considering -- I don't mean to drop into the issue tracker and tell you how you should be designing your library. That said, I think a change like this would prevent a significant number of security bugs, and it would be easier to make earlier before compatibility is too big a concern.

[gr2m]

Thanks Teddy, I’ll look into it

[gr2m]

This is very interesting, thanks again for raising the issue! I haven’t worked with tagged templates yet, I’ll see how the escaping / replacement with variables could work. If I understand correctly we want

1. Prevent template literals outside of parameters by escaping them
2. Replace template literals used in parameters with variables

I like the graphql(query, options) api as it works similar to request(route, options), I’d like to keep that. But I could imagine to expose a graphql.query function which could be a template tag function and only allow the result of that to be passed as a query in the request.

Such a GraphQL query parsing function sounds like a good utility that would be useful in general, are you aware if it already exists? If not it would be cool to build it independent of octokit I think, wanna collaborate on that? I know https://www.npmjs.com/package/graphql-tag but we don’t want to work with ASTs

[gr2m]

Does that look good?

example 1

const repoId = 123
const result = query`
  mutation {
    addStar(input: {clientMutationId: "x", starrableId: "${repoId}"}) {
      clientMutationId
    }
  }
`

result would now be

{
  query: 'mutation {\n    addStar(input: {clientMutationId: "x", starrableId: "$starrableId"}) {\n      clientMutationId\n    }\n  }',
  variables: {
    starrableId: 123
  }
}

example 2

const fields = ['login', 'bio']
const result = query`
  {
    viewer {
      ${fields.join('\n')}
    }
  }
`

Throws an error

Error: literals not allowed outside of parameters

[gr2m]

people might still want to construct dynamic queries, we could provide some workaround that makes clear that what they do has the danger of query injections

[not-an-aardvark]

Sounds good to me.

[gr2m]

I've added a note to the README referencing this issue. I have decided not going to change the API. Thanks for making me aware of the potential injection attack and for bearing with me :)