Skip to content

Commit

Permalink
refactor: remove redacted message (#11960)
Browse files Browse the repository at this point in the history
## Description

Closes: #11539

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit bcf2088)

# Conflicts:
#	CHANGELOG.md
#	errors/abci.go
  • Loading branch information
technicallyty authored and mergify[bot] committed May 19, 2022
1 parent cf17549 commit 8ab98df
Show file tree
Hide file tree
Showing 3 changed files with 162 additions and 76 deletions.
35 changes: 35 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,42 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

<<<<<<< HEAD
* (x/feegrant) [\#11813](https://github.com/cosmos/cosmos-sdk/pull/11813) Fix pagination total count in `AllowancesByGranter` query.
=======
* [\#11696](https://github.com/cosmos/cosmos-sdk/pull/11696) Rename `helpers.GenTx` to `GenSignedMockTx` to avoid confusion with genutil's `GenTxCmd`.
* (x/auth/vesting) [\#11652](https://github.com/cosmos/cosmos-sdk/pull/11652) Add util functions for `Period(s)`
* [\#11630](https://github.com/cosmos/cosmos-sdk/pull/11630) Add SafeSub method to sdk.Coin.
* [\#11511](https://github.com/cosmos/cosmos-sdk/pull/11511) Add api server flags to start command.
* [\#11484](https://github.com/cosmos/cosmos-sdk/pull/11484) Implement getter for keyring backend option.
* [\#11449](https://github.com/cosmos/cosmos-sdk/pull/11449) Improved error messages when node isn't synced.
* [\#11349](https://github.com/cosmos/cosmos-sdk/pull/11349) Add `RegisterAminoMsg` function that checks that a msg name is <40 chars (else this would break ledger nano signing) then registers the concrete msg type with amino, it should be used for registering `sdk.Msg`s with amino instead of `cdc.RegisterConcrete`.
* [\#11089](https://github.com/cosmos/cosmos-sdk/pull/11089]) Now cosmos-sdk consumers can upgrade gRPC to its newest versions.
* [\#10439](https://github.com/cosmos/cosmos-sdk/pull/10439) Check error for `RegisterQueryHandlerClient` in all modules `RegisterGRPCGatewayRoutes`.
* [\#9780](https://github.com/cosmos/cosmos-sdk/pull/9780) Remove gogoproto `moretags` YAML annotations and add `sigs.k8s.io/yaml` for YAML marshalling.
* (x/bank) [\#10134](https://github.com/cosmos/cosmos-sdk/pull/10134) Add `HasDenomMetadata` function to bank `Keeper` to check if a client coin denom metadata exists in state.
* (x/bank) [\#10022](https://github.com/cosmos/cosmos-sdk/pull/10022) `BankKeeper.SendCoins` now takes less execution time.
* (deps) [\#9987](https://github.com/cosmos/cosmos-sdk/pull/9987) Bump Go version minimum requirement to `1.17`
* (cli) [\#9856](https://github.com/cosmos/cosmos-sdk/pull/9856) Overwrite `--sequence` and `--account-number` flags with default flag values when used with `offline=false` in `sign-batch` command.
* (rosetta) [\#10001](https://github.com/cosmos/cosmos-sdk/issues/10001) Add documentation for rosetta-cli dockerfile and rename folder for the rosetta-ci dockerfile
* [\#9699](https://github.com/cosmos/cosmos-sdk/pull/9699) Add `:`, `.`, `-`, and `_` as allowed characters in the default denom regular expression.
* (genesis) [\#9697](https://github.com/cosmos/cosmos-sdk/pull/9697) Ensure `InitGenesis` returns with non-empty validator set.
* [\#10341](https://github.com/cosmos/cosmos-sdk/pull/10341) Move from `io/ioutil` to `io` and `os` packages.
* [\#10468](https://github.com/cosmos/cosmos-sdk/pull/10468) Allow futureOps to queue additional operations in simulations
* [\#10625](https://github.com/cosmos/cosmos-sdk/pull/10625) Add `--fee-payer` CLI flag
* (cli) [\#10683](https://github.com/cosmos/cosmos-sdk/pull/10683) In CLI, allow 1 SIGN_MODE_DIRECT signer in transactions with multiple signers.
* (deps) [\#10210](https://github.com/cosmos/cosmos-sdk/pull/10210) Bump Tendermint to [v0.35.0](https://github.com/tendermint/tendermint/releases/tag/v0.35.0).
* (deps) [\#10706](https://github.com/cosmos/cosmos-sdk/issues/10706) Bump rosetta-sdk-go to v0.7.2 and rosetta-cli to v0.7.3
* (types/errors) [\#10779](https://github.com/cosmos/cosmos-sdk/pull/10779) Move most functionality in `types/errors` to a standalone `errors` go module, except the `RootCodespace` errors and ABCI response helpers. All functions and types that used to live in `types/errors` are now aliased so this is not a breaking change.
* (gov) [\#10854](https://github.com/cosmos/cosmos-sdk/pull/10854) v1beta2's vote doesn't include the deprecate `option VoteOption` anymore. Instead, it only uses `WeightedVoteOption`.
* (types) [\#11004](https://github.com/cosmos/cosmos-sdk/pull/11004) Added mutable versions of many of the sdk.Dec types operations. This improves performance when used by avoiding reallocating a new bigint for each operation.
* (x/auth) [\#10880](https://github.com/cosmos/cosmos-sdk/pull/10880) Added a new query to the tx query service that returns a block with transactions fully decoded.
* (types) [\#11200](https://github.com/cosmos/cosmos-sdk/pull/11200) Added `Min()` and `Max()` operations on sdk.Coins.
* (gov) [\#11287](https://github.com/cosmos/cosmos-sdk/pull/11287) Fix error message when no flags are provided while executing `submit-legacy-proposal` transaction.
* (x/auth) [\#11482](https://github.com/cosmos/cosmos-sdk/pull/11482) Improve panic message when attempting to register a method handler for a message that does not implement sdk.Msg
* (x/staking) [\#11596](https://github.com/cosmos/cosmos-sdk/pull/11596) Add (re)delegation getters
* (errors) [\#11960](https://github.com/cosmos/cosmos-sdk/pull/11960) Removed 'redacted' error message from defaultErrEncoder
>>>>>>> bcf208892 (refactor: remove redacted message (#11960))
### Bug Fixes

Expand Down
112 changes: 112 additions & 0 deletions errors/abci.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package errors

import (
"fmt"
"reflect"
)

const (
// SuccessABCICode declares an ABCI response use 0 to signal that the
// processing was successful and no error is returned.
SuccessABCICode uint32 = 0

// All unclassified errors that do not provide an ABCI code are clubbed
// under an internal error code and a generic message instead of
// detailed error string.
internalABCICodespace = UndefinedCodespace
internalABCICode uint32 = 1
)

// ABCIInfo returns the ABCI error information as consumed by the tendermint
// client. Returned codespace, code, and log message should be used as a ABCI response.
// Any error that does not provide ABCICode information is categorized as error
// with code 1, codespace UndefinedCodespace
// When not running in a debug mode all messages of errors that do not provide
// ABCICode information are replaced with generic "internal error". Errors
// without an ABCICode information as considered internal.
func ABCIInfo(err error, debug bool) (codespace string, code uint32, log string) {
if errIsNil(err) {
return "", SuccessABCICode, ""
}

encode := defaultErrEncoder
if debug {
encode = debugErrEncoder
}

return abciCodespace(err), abciCode(err), encode(err)
}

// The debugErrEncoder encodes the error with a stacktrace.
func debugErrEncoder(err error) string {
return fmt.Sprintf("%+v", err)
}

func defaultErrEncoder(err error) string {
return err.Error()
}

type coder interface {
ABCICode() uint32
}

// abciCode tests if given error contains an ABCI code and returns the value of
// it if available. This function is testing for the causer interface as well
// and unwraps the error.
func abciCode(err error) uint32 {
if errIsNil(err) {
return SuccessABCICode
}

for {
if c, ok := err.(coder); ok {
return c.ABCICode()
}

if c, ok := err.(causer); ok {
err = c.Cause()
} else {
return internalABCICode
}
}
}

type codespacer interface {
Codespace() string
}

// abciCodespace tests if given error contains a codespace and returns the value of
// it if available. This function is testing for the causer interface as well
// and unwraps the error.
func abciCodespace(err error) string {
if errIsNil(err) {
return ""
}

for {
if c, ok := err.(codespacer); ok {
return c.Codespace()
}

if c, ok := err.(causer); ok {
err = c.Cause()
} else {
return internalABCICodespace
}
}
}

// errIsNil returns true if value represented by the given error is nil.
//
// Most of the time a simple == check is enough. There is a very narrowed
// spectrum of cases (mostly in tests) where a more sophisticated check is
// required.
func errIsNil(err error) bool {
if err == nil {
return true
}
if val := reflect.ValueOf(err); val.Kind() == reflect.Ptr {
return val.IsNil()
}
return false
}
91 changes: 15 additions & 76 deletions types/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 = "github.com/cosmos/cosmos-sdk/types/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

0 comments on commit 8ab98df

Please sign in to comment.