vendure-ecommerce / vendure

The commerce platform with customization in its DNA.
https://www.vendure.io
Other
5.56k stars 986 forks source link

Is there a reason why creating an account with an email already registered doesn't return an error? #746

Open mariomacedo opened 3 years ago

mariomacedo commented 3 years ago

Is there a reason why creating an account with an email already registered doesn't return an error?

Update - more information

Running the following mutation multiple times with the same credentials always returns success.

mutation Register($input: RegisterCustomerInput!) {
  registerCustomerAccount(input: $input) {
    __typename
    ... on Success {
      success
    }
    ...ErrorResult
  }
}

fragment ErrorResult on ErrorResult {
  errorCode
  message
}

Here are the two cases I could think of: 1) If the user registered, but has not yet verified his account. Any subsequent registration attempt with the same credentials returns success. Also, multiple verification emails are sent. If different passwords were specified during the registration attempts, the first one is the one recorded in the database. You could see how this might confuse customers.

2) If the account is verified, any subsequent registration attempt returns success.

Looking at the error codes currently used, it seems like it should return:

I'm now aware of commit b1ffa1e that makes the registration mutation silently fail to protect user data. You're trading off extra security for customer experience. As a user, I often forget if I have an account on a particular website - especially if I don't use it often. I'm curious to hear what you think.

Originally posted by @marwand in https://github.com/vendure-ecommerce/vendure/issues/33#issuecomment-713831098

michaelbromley commented 3 years ago

Thanks for resurrecting this detailed comment as an issue. I somehow missed it the first time and never replied.

This is well worth considering newly. Regarding the possibility of exposing user data if we return different results for existing email addresses, perhaps this can be better mitigated by e.g. rate-limiting certain mutations rather than just hiding the results.

alibeknow commented 2 years ago

Same problem how we can solve it ?

rolf-stargate commented 2 years ago

Isn't this a really big security problem? If I know the email address of a customer, I could change there password by registering again and would be able to receive there information like there orders, name and address, phone number. It would also be not so hard to automate this and simply scrape all shops I can find with a vendure backend for a list of known email addresses. I started to build a shop with vendure, please tell me that you fix this or that I'am wrong.

michaelbromley commented 2 years ago

Hi @ssex-dev. If you have authOptions.requireVerification set to true (which is the default), then the attacker would not be able to gain access to the account data without verifying the email address. To do this, he or she would need access to the victims email inbox. If they have such access, the victim has much, much bigger problems and is completely compromised anyway.

rolf-stargate commented 2 years ago

Thank you very much for the fast reply! Ok I understand, I freaked out for a moment, sorry.

usama8800 commented 2 years ago

Thanks for resurrecting this detailed comment as an issue. I somehow missed it the first time and never replied.

This is well worth considering newly. Regarding the possibility of exposing user data if we return different results for existing email addresses, perhaps this can be better mitigated by e.g. rate-limiting certain mutations rather than just hiding the results.

Can't I do the same thing (expose user data) during checkout? Try to order without logging in using the email and it gives me an email address conflict error?

Also, can you tell me why exposing an email which is registered is bad? I can't find a source where it's considered a bad practice and I tested a few other websites and all of them say something along the lines of "username is taken"

michaelbromley commented 1 year ago

@usama8800 this class of attack is known as an an enumeration attack - basically a way to extract data from the system which we don't want to expose, e.g. all the email addresses of our customers. Good write-up on it here: https://www.upguard.com/blog/what-is-an-enumeration-attack

My proposal is this:

I think long-term, this behaviour should be configurable via a strategy (defaulting to current behaviour), but a strategy-based approach would allow merchants to make a judgement call on their usability/security tradeoff and even implement e.g. a rate-limiting mechanism to mitigate the security risk.

TheLucDev commented 1 year ago

hey @michaelbromley , i just write my website base on Vendure , so i want to return an error if email already exist. How can i turn off this guard , i accept the security risk for my website . Thank you so much !

michaelbromley commented 1 year ago

@TheLucDev for now you could create your own custom mutation that implements this functionality the way you want it - see https://www.vendure.io/docs/plugins/plugin-examples/extending-graphql-api/#override-built-in-resolvers