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

chore_: remove private information for error log #6334

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions abi-spec/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"go.uber.org/zap"

"github.com/ethereum/go-ethereum/common"
gocommon "github.com/status-im/status-go/common"
"github.com/status-im/status-go/eth-node/crypto"
"github.com/status-im/status-go/logutils"
)
Expand Down Expand Up @@ -48,7 +49,7 @@ func Sha3(str string) string {
if hexStrictPattern.MatchString(str) {
bytes, err = hex.DecodeString(str[2:])
if err != nil {
logutils.ZapLogger().Error("failed to decode hex string when sha3", zap.String("hex", str), zap.Error(err))
logutils.ZapLogger().Error("failed to decode hex string when sha3", zap.Error(err))
return ""
}
} else {
Expand Down Expand Up @@ -183,7 +184,7 @@ func ToChecksumAddress(address string) (string, error) {
return "", nil
}
if !addressBasicPattern.MatchString(address) {
return "", fmt.Errorf("given address '%s' is not a valid Ethereum address", address)
return "", fmt.Errorf("given address '%s' is not a valid Ethereum address", gocommon.TruncateWithDot(address))
}

address = strings.ToLower(address)
Expand Down
3 changes: 2 additions & 1 deletion account/accounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
gethcommon "github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/status-im/status-go/account/generator"
gocommon "github.com/status-im/status-go/common"
"github.com/status-im/status-go/eth-node/crypto"
"github.com/status-im/status-go/eth-node/keystore"
"github.com/status-im/status-go/eth-node/types"
Expand Down Expand Up @@ -230,7 +231,7 @@ func (m *DefaultManager) VerifyAccountPassword(keyStoreDir, address, password st

// avoid swap attack
if key.Address != addressObj {
return nil, fmt.Errorf("account mismatch: have %s, want %s", key.Address.Hex(), addressObj.Hex())
return nil, fmt.Errorf("account mismatch: have %s, want %s", gocommon.TruncateWithDot(key.Address.Hex()), gocommon.TruncateWithDot(addressObj.Hex()))
}

return key, nil
Expand Down
14 changes: 7 additions & 7 deletions api/geth_backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import (
"github.com/status-im/status-go/appdatabase"
"github.com/status-im/status-go/centralizedmetrics"
centralizedmetricscommon "github.com/status-im/status-go/centralizedmetrics/common"
d_common "github.com/status-im/status-go/common"
gocommon "github.com/status-im/status-go/common"
"github.com/status-im/status-go/common/dbsetup"
"github.com/status-im/status-go/connection"
"github.com/status-im/status-go/eth-node/crypto"
Expand Down Expand Up @@ -283,7 +283,7 @@ func (b *GethStatusBackend) getAccountByKeyUID(keyUID string) (*multiaccounts.Ac
return &acc, nil
}
}
return nil, fmt.Errorf("account with keyUID %s not found", keyUID)
return nil, fmt.Errorf("account with keyUID %s not found", gocommon.TruncateWithDot(keyUID))
Copy link
Contributor

Choose a reason for hiding this comment

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

is this necessary? since this would be a uuid that will be one of the 3-4 you have, I mean, it is ok to truncate but it is probably annoying if you get this error and you don't see the uuid that was not found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pls let me refer what @ilmotta said:

But to me, a the error level we shouldn't log any private data like the ones you mentioned

also we have <fist_4_chars>..<last_4_chars>

}

func (b *GethStatusBackend) SaveAccount(account multiaccounts.Account) error {
Expand Down Expand Up @@ -359,7 +359,7 @@ func (b *GethStatusBackend) DeleteImportedKey(address, password, keyStoreDir str
if strings.Contains(fileInfo.Name(), address) {
_, err := b.accountManager.VerifyAccountPassword(keyStoreDir, "0x"+address, password)
if err != nil {
b.logger.Error("failed to verify account", zap.String("account", address), zap.Error(err))
b.logger.Error("failed to verify account", zap.String("account", gocommon.TruncateWithDot(address)), zap.Error(err))
return err
}

Expand Down Expand Up @@ -586,7 +586,7 @@ func (b *GethStatusBackend) LoginAccount(request *requests.Login) error {
// and choose to upgrade a higher version instead, after upgrading, user first attempt to login will fail because the node config migration will fail.
// and second attempt to login will cause an empty node config saved in the db.
func (b *GethStatusBackend) workaroundToFixBadMigration(request *requests.Login) (err error) {
if !d_common.IsMobilePlatform() { // this issue only happens on mobile platform
if !gocommon.IsMobilePlatform() { // this issue only happens on mobile platform
return nil
}

Expand Down Expand Up @@ -2476,7 +2476,7 @@ func (b *GethStatusBackend) getVerifiedWalletAccount(address, password string) (
}
exists, err := db.AddressExists(types.HexToAddress(address))
if err != nil {
b.logger.Error("failed to query db for a given address", zap.String("address", address), zap.Error(err))
b.logger.Error("failed to query db for a given address", zap.String("address", gocommon.TruncateWithDot(address)), zap.Error(err))
return nil, err
}

Expand All @@ -2494,7 +2494,7 @@ func (b *GethStatusBackend) getVerifiedWalletAccount(address, password string) (
}

if err != nil {
b.logger.Error("failed to verify account", zap.String("account", address), zap.Error(err))
b.logger.Error("failed to verify account", zap.String("account", gocommon.TruncateWithDot(address)), zap.Error(err))
return nil, err
}

Expand All @@ -2508,7 +2508,7 @@ func (b *GethStatusBackend) generatePartialAccountKey(db *accounts.Database, add
dbPath, err := db.GetPath(types.HexToAddress(address))
path := "m/" + dbPath[strings.LastIndex(dbPath, "/")+1:]
if err != nil {
b.logger.Error("failed to get path for given account address", zap.String("account", address), zap.Error(err))
b.logger.Error("failed to get path for given account address", zap.String("account", gocommon.TruncateWithDot(address)), zap.Error(err))
return nil, err
}

Expand Down
4 changes: 2 additions & 2 deletions appdatabase/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func FixMissingKeyUIDForAccounts(sqlTx *sql.Tx) error {
}
pk, err := crypto.UnmarshalPubkey(pubkey)
if err != nil {
logutils.ZapLogger().Error("Migrating accounts: failed to unmarshal pubkey", zap.String("pubkey", string(pubkey)), zap.Error(err))
logutils.ZapLogger().Error("Migrating accounts: failed to unmarshal pubkey", zap.String("pubkey", d_common.TruncateWithDot(string(pubkey))), zap.Error(err))
return err
}
pkBytes := sha256.Sum256(crypto.FromECDSAPub(pk))
Expand Down Expand Up @@ -250,7 +250,7 @@ func migrateEnsUsernames(sqlTx *sql.Tx) error {

_, err = sqlTx.Exec(`INSERT INTO ens_usernames (username, chain_id) VALUES (?, ?)`, username, defaultChainID)
if err != nil {
logutils.ZapLogger().Error("Migrating ens usernames: failed to insert username into new database", zap.String("ensUsername", username), zap.Error(err))
logutils.ZapLogger().Error("Migrating ens usernames: failed to insert username into new database", zap.Error(err))
}
}

Expand Down
38 changes: 38 additions & 0 deletions common/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ var ErrInvalidDisplayNameNotAllowed = errors.New("name is not allowed")

var DISPLAY_NAME_EXT = []string{"_eth", ".eth", "-eth"}

const DefaultTruncateLength = 8

func RecoverKey(m *protobuf.ApplicationMetadataMessage) (*ecdsa.PublicKey, error) {
if m.Signature == nil {
return nil, nil
Expand Down Expand Up @@ -103,3 +105,39 @@ func LogOnPanic() {

panic(err)
}

// TruncateWithDotN truncates a string by keeping n characters (excluding "..") from start and end,
// replacing middle part with "..".
// The total length of returned string will be n + 2 (where 2 is the length of "..").
// For n characters total (excluding dots), characters are distributed evenly between start and end.
// If n is odd, start gets one extra character.
// Examples:
//
// TruncateWithDotN("0x123456789", 4) // returns "0x1..89" (total length 6)
// TruncateWithDotN("0x123456789", 3) // returns "0x..9" (total length 5)
// TruncateWithDotN("0x123456789", 5) // returns "0x1..89" (total length 7)
// TruncateWithDotN("acdkef099", 3) // returns "ac..9" (total length 5)
// TruncateWithDotN("ab", 4) // returns "ab" (no truncation needed as len <= n)
// TruncateWithDotN("test", 0) // returns "test" (no truncation for n <= 0)
func TruncateWithDotN(s string, n int) string {
if n <= 0 || len(s) <= n {
return s
}

if n < 2 { // need at least 2 chars (1 at start + 1 at end)
n = 2
}

const dots = ".."
// For n characters total (excluding dots), we want to keep:
// - ceil(n/2) characters from the start
// - floor(n/2) characters from the end
startChars := (n + 1) / 2 // ceil(n/2)
endChars := n / 2 // floor(n/2)

return s[:startChars] + dots + s[len(s)-endChars:]
}

func TruncateWithDot(s string) string {
return TruncateWithDotN(s, DefaultTruncateLength)
}
97 changes: 97 additions & 0 deletions common/utils_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package common

import (
"testing"
)

func TestTruncateWithDotN(t *testing.T) {
tests := []struct {
name string
input string
n int
expected string
}{
{
name: "empty string",
input: "",
n: 4,
expected: "",
},
{
name: "string shorter than n",
input: "123",
n: 4,
expected: "123",
},
{
name: "string equal to n",
input: "1234",
n: 4,
expected: "1234",
},
{
name: "hex string with n=4 (even split: 2+2)",
input: "0x123456789",
n: 4,
expected: "0x..89",
},
{
name: "hex string with n=3 (odd split: 2+1)",
input: "0x123456789",
n: 3,
expected: "0x..9",
},
{
name: "hex string with n=5 (odd split: 3+2)",
input: "0x123456789",
n: 5,
expected: "0x1..89",
},
{
name: "normal string with n=3 (odd split: 2+1)",
input: "acdkef099",
n: 3,
expected: "ac..9",
},
{
name: "hex address with n=6 (even split: 3+3)",
input: "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045",
n: 6,
expected: "0xd..045",
},
{
name: "very short n=2 (even split: 1+1)",
input: "abcdef",
n: 2,
expected: "a..f",
},
{
name: "n=1 adjusted to minimum 2 (even split: 1+1)",
input: "abcdef",
n: 1,
expected: "a..f",
},
{
name: "negative n",
input: "test",
n: -1,
expected: "test",
},
{
name: "zero n",
input: "test",
n: 0,
expected: "test",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := TruncateWithDotN(tt.input, tt.n)
if result != tt.expected {
t.Errorf("TruncateWithDotN(%q, %d) = %q, want %q",
tt.input, tt.n, result, tt.expected)
}
})
}
}
4 changes: 2 additions & 2 deletions eth-node/bridge/geth/ens/ens.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,13 @@ func (m *Verifier) verifyENSName(ensInfo enstypes.ENSDetails, ethclient *ethclie
// Resolve ensName
resolver, err := ens.NewResolver(ethclient, ensName)
if err != nil {
m.logger.Error("error while creating ENS name resolver", zap.String("ensName", ensName), zap.Error(err))
m.logger.Error("error while creating ENS name resolver", zap.Error(err))
response.Error = err
return response
}
x, y, err := resolver.PubKey()
if err != nil {
m.logger.Error("error while resolving public key from ENS name", zap.String("ensName", ensName), zap.Error(err))
m.logger.Error("error while resolving public key from ENS name", zap.Error(err))
response.Error = err
return response
}
Expand Down
Loading
Loading