tvallotton / rocket_auth

An implementation for an authentication API for Rocket applications.
https://docs.rs/rocket_auth/
Apache License 2.0
73 stars 35 forks source link

ValidationError leaks password if signup fails #48

Open ywegel opened 2 years ago

ywegel commented 2 years ago

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

tvallotton commented 2 years ago

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.

ywegel commented 2 years ago

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::() }).collect(); f } ... //match rest } } } ` This is completly stupid, but it does the job. Maybe it can help you

tvallotton commented 2 years ago

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