Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sensitive validator meta parameter #192

Closed
wants to merge 3 commits into from

Conversation

mdtusz
Copy link

@mdtusz mdtusz commented Feb 21, 2022

Often, struct fields contain sensitive data which should never be displayed once the struct has been initially constructed or user input has been parsed (e.g. passwords and credit card numbers). This is especially problematic when logging validation errors as the invalid values may be potentially leaked in plaintext to logs.

This change adds a boolean sensitive validator "meta parameter" similar to the message and code parameters. This marks a field as sensitive such that the value should not be included in validation errors.

Resolves #157

Often, struct fields contain sensitive data which should never be
displayed once the struct has been initially constructed or user input
has been parsed (e.g. passwords and credit card numbers). This is
especially problematic when logging validation errors as the invalid
values may be potentially leaked in plaintext to logs.

This change adds a boolean `sensitive` validator "meta parameter" similar
to the `message` and `code` parameters. This marks a field as sensitive
such that the value should not be included in validation errors.
@mdtusz
Copy link
Author

mdtusz commented Feb 21, 2022

On second thought, I think this may be better to have as a parameter equivalent to required which can just be in the top level of the macro usage. E.g.

#[validate(required, sensitive, length(min = 8))]

This will require some revisions, but should ultimately operate in a fairly similar way. Thoughts?

@Keats
Copy link
Owner

Keats commented Feb 23, 2022

+1 for having it at the top level

@mdtusz
Copy link
Author

mdtusz commented Feb 23, 2022

Apologies, my derive-macro-fu isn't very strong - any suggestions on how best to go about implementing it at the top level? The error output seems to be validator-specific and as far as I can grok in the macro quoting, there isn't really access to the "parent" macro scope without more significant refactoring.

@Keats
Copy link
Owner

Keats commented Feb 24, 2022

Honestly it's been years since I touched the proc macro part but it will require some refactoring most likely yes

@Keats Keats mentioned this pull request Apr 5, 2022
@IniterWorker
Copy link
Contributor

IniterWorker commented Apr 11, 2022

If we want to go with the current state of the art, this approach is good enough.

But, if we want to get in bed with a top-level approach, we should consider two things that I think we should debate in another issue.

  • Create a shared state in StructValidation at Struct's top level.
  • Create a shared state in FieldValidation at Field's top level.

From Validator's level, we have access to merged states.

Pros:

  • You will be able to access more information at the Validator's level.
  • You should be able to require all fields from StructValidation,
  • You should provide an easy way to implement and share new states/flags.

Cons:

  • The validator shares a common set of features required to be implemented upfront (required, sensitive...).
  • You increase the complexity.

@mdtusz
Copy link
Author

mdtusz commented Jun 17, 2022

Apologies for abandoning this for such a long time. I'll rebase and see what I can do to get a top level parameter working. I think it's definitely the superior approach and much more ergonomic - there's essentially no use cases where you would consider a property sensitive for some validation strategies, but not others.

This may balloon into more structural changes though as @IniterWorker pointed out, so we'll want to open a few new issues around getting those changes made first, then making the required changes for sensitive, required etc.

@Keats
Copy link
Owner

Keats commented Jun 27, 2022

Honestly I'm thinking that between #205 and this, it's probably easier to start from scratch. Seeing how many more features this crate now has compared to the PoC it was initially, it can probably be cleaned up quite a bit.

@IniterWorker
Copy link
Contributor

@Keats, I second #205 and the fact we must refactor the core to add a sensitive validator or any new validator.

@mdtusz
Copy link
Author

mdtusz commented Nov 6, 2024

Closing this as it is now incompatible with how the crate is implemented. I may be able to re-open with compatible changes in the next few weeks, but nobody hold their breath!

@mdtusz mdtusz closed this Nov 6, 2024
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.

errors returned by custom validators can't specify their own value parameter
3 participants