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

Bug - @auth GraphQL @hasInverse Security Hole #8193

Open
jdgamble555 opened this issue Aug 30, 2022 · 3 comments
Open

Bug - @auth GraphQL @hasInverse Security Hole #8193

jdgamble555 opened this issue Aug 30, 2022 · 3 comments
Labels
community Issue or PR created by the community. kind/bug Something is broken.

Comments

@jdgamble555
Copy link

jdgamble555 commented Aug 30, 2022

https://discuss.dgraph.io/t/cant-update-node-to-create-edge-due-to-auth-rules/13061
https://discuss.dgraph.io/t/bug-auth-rules-of-parent-not-respected-when-child-with-hasinverse-is-added/12955/5

Even if you have an @auth rule on a node, the inverse node can still be created:

schema.graphql

type Foo
  @auth(
    add: { rule: "{$ROLE: { eq: \"ADMIN\" }}" }
    delete: { rule: "{$ROLE: { eq: \"ADMIN\" }}" }
    update: { rule: "{$ROLE: { eq: \"ADMIN\" }}" }
  ) {
  id: String! @id
  blas: [Bla!]! @hasInverse(field: foo)
}

type Bla {
  foo: Foo
}

# Dgraph.Authorization {"VerificationKey":"totallysecret","Header":"Auth","Namespace":"lol","Algo":"HS256"}

mutation

mutation {
  addFoo(input: [
    {
      id: "1234"
      blas: []
    }
  ]) {
    numUids
  }
}

query - Node was indeed created!

query {
  queryFoo {
    blas {
      __typename
    }
  }
}

Update-Rules of parent should be respected when an inverse connection exists.

This is a HUGE security risk. However, as @amaster507 said, this may should not be fixed until the field-level-auth feature is implemented in order to protect the individual fields.

J

@amaster507
Copy link
Contributor

Let me try to help explain this. I think you missed one of the explanations from the discuss post linked so I was really confused. The mutation you posted above of addFoo does rightfully respect the @auth rules.

What wouldn't respect the @auth rules is the addBla mutation or an updateBla mutation that is creating or deleting an edge to an existing Foo.

Let's try to explain it with a better example.

# GraphQL Schema
type User @auth({
  update: { rule: "query { queryUser(filter:{not: {archived:true}}){ id } }" }
}) {
  id: ID!
  username: String! @id
  accounts: [Account]
  archived: Boolean! @seach
}
type Account {
  number: String! @id
  for: User! @hasInverse(field: "accounts")
}

Logically we don't want to be able to update any Users that are archived. We would think that this would include ALL predicates AND edges.

So in theory we would not be able to add the equivalent of any triple (through the GraphQL API) that has the subject of a User that has a predicate value of User.archived equal to true.

Which is partially true. We cannot directly run any updateUser mutations to add accounts to archived Users.

But, what we can do [which is the bug] is create or update an account and link/unlink it to/from an existing User that is archived, which will create/delete the inverse edge having the subject of the restricted User.

For some situations this may be wanted. For instance You might have a rule that prevents someone from creating/updating Users, but you want to allow them to create accounts for users.

So in order to make a node and all of it's outbound edges 100% immutable, you have to either choose to not use the @hasInverse directive, or create rules on all of the other types to not allow add/update/delete on them if the inverse node is restricted. But even then that may create an undesirable affect, because you might want to allow updating a predicate for a child of a immutable parent node without being able to detach it or add it to other immutable nodes.

More or less this is another reason that shows the need for field level auth. And to make sure that field level auth is not just for predicate values, but also edges.

@MichelDiz MichelDiz added kind/bug Something is broken. and removed bug labels Sep 9, 2022
@MichelDiz MichelDiz added the community Issue or PR created by the community. label Oct 24, 2022
Copy link

This issue has been stale for 60 days and will be closed automatically in 7 days. Comment to keep it open.

@github-actions github-actions bot added the Stale label Jul 31, 2024
@amaster507
Copy link
Contributor

Really important to at least know and document if not fix.

@github-actions github-actions bot removed the Stale label Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Issue or PR created by the community. kind/bug Something is broken.
Development

No branches or pull requests

3 participants