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

refactor: remove panic redacted message #11960

Merged
merged 9 commits into from
May 19, 2022
19 changes: 1 addition & 18 deletions errors/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,8 @@ func debugErrEncoder(err error) string {
return fmt.Sprintf("%+v", err)
}

// The defaultErrEncoder applies Redact on the error before encoding it with its internal error message.
func defaultErrEncoder(err error) string {
return Redact(err).Error()
return err.Error()
yihuang marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add a changelog entry too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added 👍🏻

}

type coder interface {
Expand Down Expand Up @@ -111,19 +110,3 @@ func errIsNil(err error) bool {
}
return false
}

var errPanicWithMsg = Wrapf(ErrPanic, "error message redacted to hide potential sensitive info. Use the '--trace' flag if you are running a node to see the full stack trace error")
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved

// Redact replaces an error that is not initialized as an ABCI Error with a
// generic internal error instance. If the error is an ABCI Error, that error is
// simply returned.
func Redact(err error) error {
if ErrPanic.Is(err) {
return errPanicWithMsg
}
if abciCode(err) == internalABCICode {
return errInternal
}

return err
}
91 changes: 15 additions & 76 deletions errors/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,27 +57,13 @@ func (s *abciTestSuite) TestABCInfo() {
wantCode: 0,
wantSpace: "",
},
"stdlib is generic message": {
err: io.EOF,
debug: false,
wantLog: "internal",
wantCode: 1,
wantSpace: UndefinedCodespace,
},
"stdlib returns error message in debug mode": {
err: io.EOF,
debug: true,
wantLog: "EOF",
wantCode: 1,
wantSpace: UndefinedCodespace,
},
"wrapped stdlib is only a generic message": {
err: Wrap(io.EOF, "cannot read file"),
debug: false,
wantLog: "internal",
wantCode: 1,
wantSpace: UndefinedCodespace,
},
// This is hard to test because of attached stacktrace. This
// case is tested in an another test.
//"wrapped stdlib is a full message in debug mode": {
Expand All @@ -103,10 +89,12 @@ func (s *abciTestSuite) TestABCInfo() {
}

for testName, tc := range cases {
space, code, log := ABCIInfo(tc.err, tc.debug)
s.Require().Equal(tc.wantSpace, space, testName)
s.Require().Equal(tc.wantCode, code, testName)
s.Require().Equal(tc.wantLog, log, testName)
s.T().Run(testName, func(t *testing.T) {
space, code, log := ABCIInfo(tc.err, tc.debug)
s.Require().Equal(tc.wantSpace, space, testName)
s.Require().Equal(tc.wantCode, code, testName)
s.Require().Equal(tc.wantLog, log, testName)
})
}
}

Expand Down Expand Up @@ -135,25 +123,20 @@ func (s *abciTestSuite) TestABCIInfoStacktrace() {
wantStacktrace: true,
wantErrMsg: "wrapped: stdlib",
},
"wrapped stdlib error in non-debug mode does not have stacktrace": {
err: Wrap(fmt.Errorf("stdlib"), "wrapped"),
debug: false,
wantStacktrace: false,
wantErrMsg: "internal",
},
}

const thisTestSrc = "cosmossdk.io/errors.(*abciTestSuite).TestABCIInfoStacktrace"

for testName, tc := range cases {
_, _, log := ABCIInfo(tc.err, tc.debug)
if !tc.wantStacktrace {
s.Require().Equal(tc.wantErrMsg, log, testName)
continue
}

s.Require().True(strings.Contains(log, thisTestSrc), testName)
s.Require().True(strings.Contains(log, tc.wantErrMsg), testName)
s.T().Run(testName, func(t *testing.T) {
_, _, log := ABCIInfo(tc.err, tc.debug)
if !tc.wantStacktrace {
s.Require().Equal(tc.wantErrMsg, log, testName)
} else {
s.Require().True(strings.Contains(log, thisTestSrc), testName)
s.Require().True(strings.Contains(log, tc.wantErrMsg), testName)
}
})
}
}

Expand All @@ -163,46 +146,6 @@ func (s *abciTestSuite) TestABCIInfoHidesStacktrace() {
s.Require().Equal("wrapped: unauthorized", log)
}

func (s *abciTestSuite) TestRedact() {
cases := map[string]struct {
err error
untouched bool // if true we expect the same error after redact
changed error // if untouched == false, expect this error
}{
"panic looses message": {
err: Wrap(ErrPanic, "some secret stack trace"),
changed: errPanicWithMsg,
},
"sdk errors untouched": {
err: Wrap(ErrUnauthorized, "cannot drop db"),
untouched: true,
},
"pass though custom errors with ABCI code": {
err: customErr{},
untouched: true,
},
"redact stdlib error": {
err: fmt.Errorf("stdlib error"),
changed: errInternal,
},
}

for name, tc := range cases {
spec := tc
redacted := Redact(spec.err)
if spec.untouched {
s.Require().Equal(spec.err, redacted, name)
continue
}

// see if we got the expected redact
s.Require().Equal(spec.changed, redacted, name)
// make sure the ABCI code did not change
s.Require().Equal(abciCode(spec.err), abciCode(redacted), name)

}
}

func (s *abciTestSuite) TestABCIInfoSerializeErr() {
var (
// Create errors with stacktrace for equal comparison.
Expand Down Expand Up @@ -231,10 +174,6 @@ func (s *abciTestSuite) TestABCIInfoSerializeErr() {
debug: true,
exp: fmt.Sprintf("%+v", myErrDecode),
},
"redact in default encoder": {
src: myPanic,
exp: "error message redacted to hide potential sensitive info. Use the '--trace' flag if you are running a node to see the full stack trace error: panic",
},
"do not redact in debug encoder": {
src: myPanic,
debug: true,
Expand Down