howtographql / graphql-js

https://www.howtographql.com/graphql-js/1-getting-started/
363 stars 123 forks source link

Authorization check flaw: 'post' mutation does get invoked when no headers provided #84

Open steelcracker opened 3 years ago

steelcracker commented 3 years ago

On step 6-authentication there is an explanation that there will be an error indicating that the user was not authenticated: image This is only true if there is an authorization header present.

But if you post a mutation without any headers set, then getUserId() never gets invoked in the first place due to: https://github.com/howtographql/graphql-js/blob/e0a3a21dbf8e594226250b04fdc0a5776493e64d/src/index.js#L39-L42 so the code never gets to the line https://github.com/howtographql/graphql-js/blob/e0a3a21dbf8e594226250b04fdc0a5776493e64d/src/utils.js#L24 and the context.userId is just silently set to null. The code will proceed further and invoke the post mutation. To be honest, it won't succeed though, but due to another error

Invalid prisma.link.create() invocation. Unknown arg postedBy in data.postedBy for type LinkUncheckedCreateInput

which is a strange misleading Prisma's error due to the null userId value at https://github.com/howtographql/graphql-js/blob/e0a3a21dbf8e594226250b04fdc0a5776493e64d/src/resolvers/Mutation.js#L12 (by strange I mean we've set Prisma's schema to postedBy User? earlier in the tutorial, which suggests that User could actually be null, and by misleading - there is no such postedBy arg complaint when using valid userId, so ideally prisma's error should tell that it doesn't accept null inside such connect directive)

So to conclude, there is a potential flaw with such an authorization check.

dhyeythumar commented 3 years ago

Hi @steelcracker, so I think one extra check is required before calling context.prisma.link.create()

Such as:

const { userId } = context;
if (userId === null) throw new Error("Not Authenticated");

So now this won't give error.