fix(forge): sensitive broadcast logs #4892
Conversation
devanoneth
force-pushed
the
fix/sensitive-broadcast
branch
from
5f9946b
to
15d208f
Compare
cli/src/cmd/forge/script/sequence.rs
Outdated
| @@ -45,6 +48,29 @@ pub struct ScriptSequence { | |||
| pub commit: Option<String>, | |||
| } | |||
|
|
|||
| #[derive(Deserialize, Serialize, Clone, Default)] | |||
| pub struct TransactionWithMetadataSensitive { | |||
There was a problem hiding this comment.
Could we add some docs about what this struct is?
Also, I think we could have a better name e.g SensitiveTxMetadata—We only really have the sensitive logged info here, rather than being a full blown tx that has sensitive info.
cli/src/cmd/forge/script/sequence.rs
Outdated
|
|
||
| /// Sensitive info from the script sequence which is saved into the cache folder | ||
| #[derive(Deserialize, Serialize, Clone, Default)] | ||
| pub struct ScriptSequenceSensitive { |
There was a problem hiding this comment.
ditto on above comment—let's try and look for a more intuitive/accurate name
There was a problem hiding this comment.
Would SensitiveScriptSequence be better? I wanted to keep the name close to the ScriptSequence struct as that's where these values come from.
That way you have your ScriptSequence which can be committed and SensitiveScriptSequence which shouldn't be.
There was a problem hiding this comment.
Cool, that's already in my last commit :)
| )?), | ||
| &script_sequence_sensitive, | ||
| )?; | ||
|
|
||
| shell::println(format!("\nTransactions saved to: {path}\n"))?; |
There was a problem hiding this comment.
maybe we should print an extra message indicating sensitive info has been written to a separate path?
|
hey hey, will TAL tomorrow and hopefully merge this! looks solid, just haven't fully tested yet |
|
No prob @Evalir, you're smashing PRs out lately! <3 |
|
@mattsse thanks for reviewing! I'm not sure why this would need changes in the default |
|
I see, so the senstive logs are always ignored? |
|
@mattsse Yeah, the sensitive logs go into directories under the cache directory. See the |
Motivation
Remove sensitive values from broadcast logs. Namely
rpcandpath. Fixes #4730.Solution
As proposed in #4730, saves sensitive values into a file with the same name / folder path but instead of the
broadcastdir, we use thecachedir. It introduces aScriptSequenceSensitivestruct which can be used to write sensitive values and later load them back into theScriptSequencestruct.Also fixes and extends the
check_broadcast_logtest so the changes can be tested.