Improve SignHash error message to clarify intentional security design #2141
Conversation
Update the UnsupportedSignerOperation error display to explicitly mention that blind signing is intentionally disabled as a security best practice, rather than appearing as a technical limitation. This provides clearer guidance to users encountering this error.
neithanmo
requested review from
DaniPopes,
gakonst,
mattsse,
onbjerg and
klkvr
as code owners
| @@ -79,7 +79,12 @@ pub enum UnsupportedSignerOperation { | |||
|
|
|||
| impl fmt::Display for UnsupportedSignerOperation { | |||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
| self.as_str().fmt(f) | |||
| match self { | |||
There was a problem hiding this comment.
you want to change this above in enum Error, but i think it should be another variant
There was a problem hiding this comment.
do you mean convert UnsupportedSignerOperation into an error via thiserror?
I think we should do this because otherwise this new message will mess with the error message here:
alloy/crates/signer/src/error.rs
Lines 13 to 14 in 750004c
There was a problem hiding this comment.
I think we should keep things as is and just add another variant with a message that it is intentionally unimplemented
There was a problem hiding this comment.
right. Your suggestion is better by making a distinction between what is unsupported and what is opted out due to security reasons. maybe a new variant to the Error definition:
pub enum Error {
// ...
/// The operation is deliberately disabled for security reasons
#[error("Operation {0} is restricted due to security restrictions")]
SecurityRestriction(String),
// ...
}
| @@ -79,7 +79,12 @@ pub enum UnsupportedSignerOperation { | |||
|
|
|||
| impl fmt::Display for UnsupportedSignerOperation { | |||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
| self.as_str().fmt(f) | |||
| match self { | |||
There was a problem hiding this comment.
do you mean convert UnsupportedSignerOperation into an error via thiserror?
I think we should do this because otherwise this new message will mess with the error message here:
alloy/crates/signer/src/error.rs
Lines 13 to 14 in 750004c
Motivation
see #2114
Solution
Update the
UnsupportedSignerOperationerror display to explicitly mention that blind signing is intentionally disabled as a security best practicePR Checklist