Skip to content

Public graphql directive - #592

Closed
iagoaraujo wants to merge 2 commits into
masterfrom
public-graphql-directive
Closed

Public graphql directive#592
iagoaraujo wants to merge 2 commits into
masterfrom
public-graphql-directive

Conversation

@iagoaraujo

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

Create a new graphql directive in order to allow define a query or mutation as public.

What problem is this solving?

This directive was created to solve a security vulnerability caused by we don't have a way to sinalize that the route is public. So basically if there is no Auth directive, the default is consider the field as public.

How should this be manually tested?

  • Link node-vtex-api with yarn
  • Link node-vtex-api into a graphql app
  • Try to use the public directive

Screenshots or example usage

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Requires change to documentation, which has been updated accordingly.

@silvadenisaraujo silvadenisaraujo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember that @arturpimentel has provided an interesting feedback to try to scope the authorization in the directive, like public, user and admin. I see that the @public directive is a great starting point, but we could take advantage to think about other authorization methods that we can offer here not only the current @auth or @public. What do you think?

@iagoaraujo

Copy link
Copy Markdown
Contributor Author

@silvadenisaraujo I think we should focus on the security vuln and start with baby steps. We talk very little about store and admin scope in the context of graphql, I think we first need to define if this is in fact a problem, what types of apps have this problem, success metrics... it's another initiative.

But now I'm thinking... maybe we could just have the @auth directive, with a Scope field that's an enum. One of the enum's values is Public. In the future, we could have Store, Admin, etc., if needed. Ex:

type Query {
  listaProdutos: [Produto]!
    @auth(scope: PUBLIC)

  meusPedidos: [Pedido]!
    @auth(scope: STORE, resource: ..., product: ...)

  pedidosLoja: [Pedido]!
    @auth(scope: ADMIN, resource: ..., product: ...)
}

Advantage of using Public as a directive: semantic clarity is very high. Much easier to read and simpler to maintain.

Advantage of using Public as a value of auth directive: more concise and we can maintain only one directive. Flexible for possible evolution.

About a new major of graphql builder

Since we don't need to change anything in GraphQL-Server to resolve the vulnerability, I'm now thinking we can implement both solutions in GraphQL 1.x. We start with a developer alert, set a deadline for everyone to start using it, and make it mandatory to use it within that deadline. The most likely issue we'll face is a developer complaining that they can't build the app. But what's running in production today remains the same; we haven't broken any apps that are currently running.

Disadvantage: If our goal is to map apps that still expose queries and mutations without any auth directives, this becomes more difficult without releasing a major 2.x release. But I believe that's not the goal (or is it?). I understand the goal is to eliminate "public as default" in future app builds.

cc @arturpimentel

@iagoaraujo iagoaraujo closed this Jul 17, 2025
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

Successfully merging this pull request may close these issues.

2 participants