rocket_auth icon indicating copy to clipboard operation
rocket_auth copied to clipboard

ValidationError leaks password if signup fails

Open ywegel opened this issue 2 years ago • 3 comments

When singing up a user with a password, that does not meet the requirements, the error message leaks the password that was used in the signup. I don't know if this is intended. I narrowed it down to the ValidationError::new(...) function. The error message is inserted into the code and the actual message is left empty. Then in the Display implementation for ValidationError there is a check if the error message is empty, which is always the case. Then it skips printing the actual error message, and instead prints the code (which contains the error message, because it is set wrongly) and only shows "Validation error: " with the params (which is the password). I am showing these errors to my api response directly. This way the password pops up on screen, even though it is supposed to be hidden in the signup page. i know this is actually an issue of the validator page, but i have an idea to avoid this. You could instead of using the display method to show the message, print the actual "code" field of the error. If you don't have enough time to fix this, i could create a pr

ywegel avatar Jun 29 '22 20:06 ywegel

Thanks for bringing this to my attention. This is definitely not intended. I don't think that modifying the Display impl of Error will be sufficient to fix this. Since the validator::ValidationErrors is still accessible from the error and it holds the password.
I don't know what the best aproach to fix this may be, but I will attempt this problem on the weekend.

tvallotton avatar Jun 30 '22 15:06 tvallotton

I created an issue in the 'validator' repository, but no one answered. Using the ValidatorError directly (not via the derive) it is not possible to set the error message. You can only set the code via the ValidatorError::new(...). I fixed "this" in my project by dirty matching the error type: ` #[derive(Error, Debug)] pub enum ResponseError { ... //all error types #[error("{0}")] RocketAuthError(#[from] rocket_auth::Error), ... }

impl ResponseError { pub fn message(&self) -> String { match self { ResponseError::RocketAuthError(e) => { match e { Error::FormValidationError(validation_error) => { format!("{}", validation_error.code) } Error::FormValidationErrors(validation_errors) => { let mut buffer = String::new(); let f = validation_errors .field_errors() .values() .map(|x| { x.iter().map(|y| { y.code.to_string() }).collect::<String>() }).collect(); f } ... //match rest } } } ` This is completly stupid, but it does the job. Maybe it can help you

ywegel avatar Jun 30 '22 15:06 ywegel

I think we'd have to consider to remove the dependency on validator and refactor Error, though that would be a masive breaking change.

tvallotton avatar Jun 30 '22 16:06 tvallotton

I can help you, if you want. Just notify me what to do/what you would find useful :)

ywegel avatar Jun 30 '22 16:06 ywegel