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

Fix redaction of options #75

Conversation

nick-schafhauser
Copy link
Contributor

@nick-schafhauser nick-schafhauser commented Nov 12, 2023

Currently on master there is a bug which hardcodes partial redaction for Options.
The exact line can be found here: https://github.com/primait/veil/pull/75/files#diff-db6d7ee4e2c406f36b3c82c694cbe07a593a6b7c6f4c7585a742c9990560810cL170

I have added a new test to veil-tests/src/redaction_tests.rs which demonstrates this bug if you run it without any of the other code introduced in this PR. That new test will pass with the code introduced here (really just a copy-paste from other code in the repo).

@nick-schafhauser nick-schafhauser requested a review from a team as a code owner November 12, 2023 23:06
Comment on lines -170 to +174
self.flags.redact_partial(fmt, inner)?;
if let RedactionLength::Partial = &self.flags.redact_length {
self.flags.redact_partial(fmt, inner)?;
} else {
self.flags.redact_full(fmt, inner)?;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the bug. If the value being redacted is of type Option then redact_partial is called no matter what, even if that option was assigned #[redact]

@@ -24,7 +24,7 @@ fn main() {
id: 1,
first_name: "John".to_string(),
last_name: "Doe".to_string(),
email: "johndoe@example.com".to_string(),
email: Some("johndoe@example.com".to_string()),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without my change in src/private.rs this will print the following:

Customer {
    id: 1,
    first_name: "****",
    last_name: "***",
    email: Some(
        "joh****@*******.com", <--- partial redaction even though I did not specify that 
    ),
    age: **,
}

@nick-schafhauser
Copy link
Contributor Author

How does opening a PR work in this repo? Are there tests I need to run and report status of? I see there is a workflow with three "expected" checks but I am not sure if anything is actionable by me at this point

@cpiemontese
Copy link
Contributor

Hello! Nice catch and thanks for this PR 😄

We'll review this as soon as we can, in the meantime I'll approve the workflow so that they can run

@cottinisimone
Copy link
Member

Hello @nick-schafhauser thank you for this PR! Can you please reformat the code?

@nick-schafhauser
Copy link
Contributor Author

Hello @nick-schafhauser thank you for this PR! Can you please reformat the code?

Thank you! I have pushed a new commit after running cargo fmt 👍

@MaeIsBad MaeIsBad force-pushed the partial_redaction_on_option_bug_fix branch from 984c8ac to f86a3e5 Compare November 15, 2023 12:43
Copy link
Member

@MaeIsBad MaeIsBad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've triggered the check_master_lock status check, you should be able to merge now.

We need to fix our status checks so we can actually merge external contributions, I'll take care of this later

@cpiemontese cpiemontese merged commit 91ee015 into primait:master Nov 15, 2023
7 checks passed
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.

4 participants