Skip to content

chore_: use sensitive string in MarketDataProxyConfig #6592

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.

Sign up for GitHub

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

Open
qfrank wants to merge 3 commits into
base: develop
Choose a base branch
from
+52 −34
Open

chore_: use sensitive string in MarketDataProxyConfig #6592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6421acb
chore_: use sensitive string in MarketDataProxyConfig
qfrank May 20, 2025
9699ee8
chore_: add Equal method to SensitiveString for value comparison to a…
qfrank May 20, 2025
92e25bc
chore_: remove usage of Reveal in test
qfrank May 20, 2025
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
6 changes: 3 additions & 3 deletions api/default_networks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,16 @@ func TestBuildDefaultNetworks(t *testing.T) {

// check fallback options
if strings.Contains(n.RPCURL, "infura.io") {
require.True(t, strings.Contains(n.RPCURL, infuraToken.Reveal()))
require.True(t, infuraToken.ContainedIn(n.RPCURL))
}
if strings.Contains(n.FallbackURL, "grove.city") {
require.True(t, strings.Contains(n.FallbackURL, poktToken.Reveal()))
require.True(t, poktToken.ContainedIn(n.FallbackURL))
}

// Check proxy providers for stageName
for _, provider := range n.RpcProviders {
if provider.Type == params.EmbeddedProxyProviderType {
require.Contains(t, provider.URL.Reveal(), stageName, "Proxy provider URL should contain stageName")
require.True(t, provider.URL.Contains(stageName), "Proxy provider URL should contain stageName")
}
}

Expand Down
4 changes: 2 additions & 2 deletions api/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,10 @@ func buildWalletConfig(walletRequest *requests.WalletConfig, request *requests.W
if !request.StatusProxyMarketPassword.Empty() {
walletConfig.StatusProxyMarketPassword = request.StatusProxyMarketPassword
}
if request.MarketDataProxyUser != "" {
if !request.MarketDataProxyUser.Empty() {
walletConfig.MarketDataProxyConfig.User = request.MarketDataProxyUser
}
if request.MarketDataProxyPassword != "" {
if !request.MarketDataProxyPassword.Empty() {
walletConfig.MarketDataProxyConfig.Password = request.MarketDataProxyPassword
}
if request.MarketDataProxyUrl != "" {
Expand Down
16 changes: 16 additions & 0 deletions internal/security/sensitive_string.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ func (s SensitiveString) Contains(substr any) bool {
return strings.Contains(s.value, getValue(substr))
}

func (s SensitiveString) ContainedIn(str any) bool {
return strings.Contains(getValue(str), s.value)
}

Comment on lines +76 to +79
Copy link
Collaborator

@igor-sirotin igor-sirotin May 20, 2025

Choose a reason for hiding this comment

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

This is exactly why I was against adding TrimRight method. We will automatically start adding a ton of other methods, which are implemented in strings and other standard libraries.

It is absolutely fine to use strings.Contains("text", secret.Reveal()).

Think of single responsibility. SensitiveString should not implement each and every method for all use cases. It should remain as slim as possible, providing just 1 feature: keep strings secret until explicitly revealed.

Copy link
Collaborator

@igor-sirotin igor-sirotin May 20, 2025

Choose a reason for hiding this comment

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

Same about Equal method.
And especially if they're only used in tests!

func (s SensitiveString) Append(others ...any) SensitiveString {
result := s.value
for _, other := range others {
Expand Down Expand Up @@ -112,3 +116,15 @@ func (s *SensitiveString) Scan(value interface{}) error {
}
return nil
}

// Equal compares a SensitiveString with another value
func (s SensitiveString) Equal(other any) bool {
switch v := other.(type) {
case string:
return s.value == v
case SensitiveString:
return s.value == v.value
default:
return false
}
}
10 changes: 5 additions & 5 deletions params/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,11 +446,11 @@ type WalletConfig struct {
}

type MarketDataProxyConfig struct {
Url string `json:"Url"`
User string `json:"User"`
Password string `json:"Password"`
FullDataRefreshInterval int `json:"FullDataRefreshInterval"`
PriceRefreshInterval int `json:"PriceRefreshInterval"`
Url string `json:"Url"`
User security.SensitiveString `json:"User"`
Password security.SensitiveString `json:"Password"`
FullDataRefreshInterval int `json:"FullDataRefreshInterval"`
PriceRefreshInterval int `json:"PriceRefreshInterval"`
}

// MarshalJSON custom marshalling to avoid exposing sensitive data in log,
Expand Down
2 changes: 1 addition & 1 deletion params/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,5 +324,5 @@ func TestMarshalWalletConfigJSON(t *testing.T) {
walletConfig = params.WalletConfig{}
err = json.Unmarshal([]byte(`{"OpenseaAPIKey":"some-key"}`), &walletConfig)
require.NoError(t, err)
require.Equal(t, "some-key", walletConfig.OpenseaAPIKey.Reveal())
require.True(t, walletConfig.OpenseaAPIKey.Equal("some-key"))
}
4 changes: 2 additions & 2 deletions params/network_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ func TestRpcProvider_GetFullURL(t *testing.T) {
AuthToken: security.NewSensitiveString("mytoken"),
}
expectedFullURL := "https://api.example.com/mytoken"
assert.Equal(t, expectedFullURL, provider.GetFullURL().Reveal())
assert.True(t, provider.GetFullURL().Equal(expectedFullURL))

provider.AuthType = params.NoAuth
expectedFullURL = "https://api.example.com"
assert.Equal(t, expectedFullURL, provider.GetFullURL().Reveal())
assert.True(t, provider.GetFullURL().Equal(expectedFullURL))
}
6 changes: 3 additions & 3 deletions protocol/requests/create_account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,9 @@ type WalletSecretsConfig struct {
StatusProxyMarketUser security.SensitiveString `json:"statusProxyMarketUser"`
StatusProxyMarketPassword security.SensitiveString `json:"statusProxyMarketPassword"`

MarketDataProxyUrl string `json:"marketDataProxyUrl"`
MarketDataProxyUser string `json:"marketDataProxyUser"`
MarketDataProxyPassword string `json:"marketDataProxyPassword"`
MarketDataProxyUrl string `json:"marketDataProxyUrl"`
MarketDataProxyUser security.SensitiveString `json:"marketDataProxyUser"`
MarketDataProxyPassword security.SensitiveString `json:"marketDataProxyPassword"`
// FIXME: remove when EthRpcProxy* is integrated
StatusProxyBlockchainUser security.SensitiveString `json:"statusProxyBlockchainUser"`
StatusProxyBlockchainPassword security.SensitiveString `json:"statusProxyBlockchainPassword"`
Expand Down
6 changes: 4 additions & 2 deletions services/wallet/leaderboard/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package leaderboard
import (
"time"

"github.com/status-im/status-go/internal/security"

"github.com/status-im/status-go/params"
)

Expand All @@ -15,8 +17,8 @@ const (
type ServiceConfig struct {
// API connection settings
ProxyURL string
User string
Password string
User security.SensitiveString
Password security.SensitiveString

// Refresh intervals (in seconds)
FullDataInterval time.Duration
Expand Down
26 changes: 14 additions & 12 deletions services/wallet/leaderboard/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"testing"
"time"

"github.com/status-im/status-go/internal/security"

"github.com/stretchr/testify/require"

"github.com/status-im/status-go/params"
Expand All @@ -14,17 +16,17 @@ func TestServiceConfigValidate(t *testing.T) {
// Zero intervals
config := NewLeaderbordConfig(params.MarketDataProxyConfig{
Url: "https://example.com",
User: "user",
Password: "pass",
User: security.NewSensitiveString("user"),
Password: security.NewSensitiveString("pass"),
FullDataRefreshInterval: 0,
PriceRefreshInterval: 0,
})

require.Equal(t, defaultFullDataInterval, config.FullDataInterval)
require.Equal(t, defaultPriceUpdateInterval, config.PriceUpdateInterval)
require.Equal(t, "https://example.com", config.ProxyURL)
require.Equal(t, "user", config.User)
require.Equal(t, "pass", config.Password)
require.True(t, config.User.Equal("user"))
require.True(t, config.Password.Equal("pass"))
require.Equal(t, true, config.AllowGzip)
require.Equal(t, true, config.AllowETag)
}
Expand All @@ -33,17 +35,17 @@ func TestServiceConfigValidate(t *testing.T) {
// Negative intervals
config := NewLeaderbordConfig(params.MarketDataProxyConfig{
Url: "https://example.com",
User: "user",
Password: "pass",
User: security.NewSensitiveString("user"),
Password: security.NewSensitiveString("pass"),
FullDataRefreshInterval: -5,
PriceRefreshInterval: -5,
})

require.Equal(t, defaultFullDataInterval, config.FullDataInterval)
require.Equal(t, defaultPriceUpdateInterval, config.PriceUpdateInterval)
require.Equal(t, "https://example.com", config.ProxyURL)
require.Equal(t, "user", config.User)
require.Equal(t, "pass", config.Password)
require.True(t, config.User.Equal("user"))
require.True(t, config.Password.Equal("pass"))
require.Equal(t, true, config.AllowGzip)
require.Equal(t, true, config.AllowETag)
}
Expand All @@ -52,17 +54,17 @@ func TestServiceConfigValidate(t *testing.T) {
// Custom intervals
config := NewLeaderbordConfig(params.MarketDataProxyConfig{
Url: "https://example.com",
User: "user",
Password: "pass",
User: security.NewSensitiveString("user"),
Password: security.NewSensitiveString("pass"),
FullDataRefreshInterval: 50,
PriceRefreshInterval: 65,
})

require.Equal(t, 50*time.Second, config.FullDataInterval)
require.Equal(t, 65*time.Second, config.PriceUpdateInterval)
require.Equal(t, "https://example.com", config.ProxyURL)
require.Equal(t, "user", config.User)
require.Equal(t, "pass", config.Password)
require.True(t, config.User.Equal("user"))
require.True(t, config.Password.Equal("pass"))
require.Equal(t, true, config.AllowGzip)
require.Equal(t, true, config.AllowETag)
}
Expand Down
6 changes: 2 additions & 4 deletions services/wallet/leaderboard/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import (
"sync"
"time"

"github.com/status-im/status-go/internal/security"

"go.uber.org/zap"

"github.com/status-im/status-go/common"
Expand Down Expand Up @@ -217,8 +215,8 @@ func (f *ProxyFetcher) fetchData(ctx context.Context, endpoint string, etag stri
}

options = append(options, thirdparty.WithCredentials(&thirdparty.BasicCreds{
User: security.NewSensitiveString(f.config.User),
Password: security.NewSensitiveString(f.config.Password),
User: f.config.User,
Password: f.config.Password,
}))

body, newEtag, err := f.client.DoGetRequestWithEtag(ctx, url, nil, etag, options...)
Expand Down