feat: switch to blocksync when catch up didn't work #2727
+748
−104
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Switch to blocksync | ||
| if err := conR.blocksyncR.SwitchToBlockSync(state); err != nil { | ||
| conR.Logger.Error("Failed to switch to blocksync", "err", err) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 27 days ago
The best way to address this issue is to redact (remove or obfuscate) any passwords or sensitive tokens from any remote URLs or connection strings before including them in error messages or logs. This should be done at the earliest point where such values are interpolated into errors, preferably where an error is about to be constructed using a remote URL that may contain credentials. Specifically, in functions like NewWithHTTPClient in rpc/jsonrpc/client/http_json_client.go, we should sanitize the remote address (strip the password from the URL) before including it in any error message (e.g., in fmt.Errorf("invalid remote %s: %s", remote, err)).
Steps:
- Define a small utility (function) to strip credentials (at least the password, optionally username as well) from URLs.
- Use this function to sanitize
remoteURLs (or any similar string that may embed secrets) before embedding them in error messages throughout the relevant call chains:- In
rpc/jsonrpc/client/http_json_client.go(inNewWithHTTPClient), - In
rpc/client/http/http.go(in any relevant error construction using the remote arg), - Anywhere else in the provided snippets where the potentially sensitive
remoteargument is used in error messages.
- In
- Make sure that only the sanitized/obfuscated versions are ever logged or formatted into errors.
Suggested changeset
2
rpc/jsonrpc/client/http_json_client.go
Outside changed files
| @@ -10,6 +10,7 @@ | ||
| "net/http" | ||
| "net/url" | ||
| "regexp" | ||
| "regexp" | ||
| "strings" | ||
|
|
||
| cmtsync "github.com/cometbft/cometbft/libs/sync" | ||
| @@ -165,6 +166,21 @@ | ||
| return NewWithHTTPClient(remote, httpClient) | ||
| } | ||
|
|
||
| // RedactURLCredentials redacts username and password from a URL string. | ||
| func RedactURLCredentials(rawURL string) string { | ||
| u, err := url.Parse(rawURL) | ||
| if err != nil { | ||
| return rawURL // fallback: don't mutate if can't parse | ||
| } | ||
| if u.User != nil { | ||
| // keep username but omit password | ||
| if _, isSet := u.User.Password(); isSet { | ||
| u.User = url.User(u.User.Username()) | ||
| } | ||
| } | ||
| return u.String() | ||
| } | ||
|
|
||
| // NewWithHTTPClient returns a Client pointed at the given | ||
| // address using a custom http client. An error is returned on invalid remote. | ||
| // The function panics when remote is nil. | ||
| @@ -175,7 +191,8 @@ | ||
|
|
||
| parsedURL, err := newParsedURL(remote) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid remote %s: %s", remote, err) | ||
| redacted := redactURLCredentials(remote) | ||
| return nil, fmt.Errorf("invalid remote %s: %s", redacted, err) | ||
| } | ||
|
|
||
| parsedURL.SetDefaultSchemeHTTP() |
rpc/client/http/http.go
Outside changed files
| @@ -113,7 +113,8 @@ | ||
| func New(remote, wsEndpoint string) (*HTTP, error) { | ||
| httpClient, err := jsonrpcclient.DefaultHTTPClient(remote) | ||
| if err != nil { | ||
| return nil, err | ||
| redacted := jsonrpcclient.RedactURLCredentials(remote) | ||
| return nil, fmt.Errorf("failed to create HTTP client for remote %s: %w", redacted, err) | ||
| } | ||
| return NewWithClient(remote, wsEndpoint, httpClient) | ||
| } | ||
| @@ -122,7 +123,8 @@ | ||
| func NewWithTimeout(remote, wsEndpoint string, timeout uint) (*HTTP, error) { | ||
| httpClient, err := jsonrpcclient.DefaultHTTPClient(remote) | ||
| if err != nil { | ||
| return nil, err | ||
| redacted := jsonrpcclient.RedactURLCredentials(remote) | ||
| return nil, fmt.Errorf("failed to create HTTP client for remote %s: %w", redacted, err) | ||
| } | ||
| httpClient.Timeout = time.Duration(timeout) * time.Second | ||
| return NewWithClient(remote, wsEndpoint, httpClient) |
Copilot is powered by AI and may make mistakes. Always verify output.
| conR.subscribeToBroadcastEvents() | ||
| } | ||
| if err := conR.conS.Start(); err != nil { | ||
| conR.Logger.Error("Failed to restart consensus after blocksync failure", "err", err) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 27 days ago
The overall fix is to prevent sensitive information (especially passwords) from being included in any logged error returned when failing to configure RPC clients or when processing remote URLs. This is done by stripping/redacting credentials (user:password) from any URL string used within error formatting and returned error values.
- In
rpc/jsonrpc/client/http_json_client.goand similar places, before including a remote in a returned error, always remove credentials from its string representation. - Define a helper function that takes a URL string, parses it, sets the User info to
nil, and returns the redacted string, falling back gracefully if parsing fails. - Use this helper function in all error returns in the path where the remote could be logged:
- In
NewWithHTTPClient(and any related functions in other clients), change error strings frominvalid remote %s: %sto useredactURL(remote)instead.
- In
- This ensures that, even if upstream errors are wrapped and finally logged in a consensus error, secrets are never included in the log output.
Files/Regions to change:
- Add and use a
redactURLfunction inrpc/jsonrpc/client/http_json_client.goand all relevant RPC client constructors. - Update error returns in
rpc/jsonrpc/client/http_json_client.go,rpc/client/http/http.go, and any other similar locations to redact credentials from displayed URLs. - It is not necessary to change logging within consensus/reactor.go if the error values have already been scrubbed upstream.
Suggested changeset
3
rpc/jsonrpc/client/http_json_client.go
Outside changed files
| @@ -175,7 +175,7 @@ | ||
|
|
||
| parsedURL, err := newParsedURL(remote) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid remote %s: %s", remote, err) | ||
| return nil, fmt.Errorf("invalid remote %s: %s", redactURL(remote), err) | ||
| } | ||
|
|
||
| parsedURL.SetDefaultSchemeHTTP() |
statesync/stateprovider.go
Outside changed files
| @@ -20,6 +20,7 @@ | ||
| sm "github.com/cometbft/cometbft/state" | ||
| "github.com/cometbft/cometbft/types" | ||
| "github.com/cometbft/cometbft/version" | ||
| "net/url" | ||
| ) | ||
|
|
||
| //go:generate ../scripts/mockery_generate.sh StateProvider | ||
| @@ -63,7 +64,7 @@ | ||
| for _, server := range servers { | ||
| client, err := rpcClient(server) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to set up RPC client: %w", err) | ||
| return nil, fmt.Errorf("failed to set up RPC client for server %s: %w", redactURL(server), err) | ||
| } | ||
| provider := lighthttp.NewWithClient(chainID, client) | ||
| providers = append(providers, provider) |
rpc/client/http/http.go
Outside changed files
| @@ -17,6 +17,7 @@ | ||
| ctypes "github.com/cometbft/cometbft/rpc/core/types" | ||
| jsonrpcclient "github.com/cometbft/cometbft/rpc/jsonrpc/client" | ||
| "github.com/cometbft/cometbft/types" | ||
| "net/url" | ||
| ) | ||
|
|
||
| /* | ||
| @@ -113,7 +114,7 @@ | ||
| func New(remote, wsEndpoint string) (*HTTP, error) { | ||
| httpClient, err := jsonrpcclient.DefaultHTTPClient(remote) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("failed to create default HTTP client for remote %s: %w", redactURL(remote), err) | ||
| } | ||
| return NewWithClient(remote, wsEndpoint, httpClient) | ||
| } | ||
| @@ -122,7 +123,7 @@ | ||
| func NewWithTimeout(remote, wsEndpoint string, timeout uint) (*HTTP, error) { | ||
| httpClient, err := jsonrpcclient.DefaultHTTPClient(remote) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("failed to create default HTTP client for remote %s: %w", redactURL(remote), err) | ||
| } | ||
| httpClient.Timeout = time.Duration(timeout) * time.Second | ||
| return NewWithClient(remote, wsEndpoint, httpClient) | ||
| @@ -137,12 +138,12 @@ | ||
|
|
||
| rc, err := jsonrpcclient.NewWithHTTPClient(remote, client) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("failed to create JSON-RPC client for remote %s: %w", redactURL(remote), err) | ||
| } | ||
|
|
||
| wsEvents, err := newWSEvents(remote, wsEndpoint) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("failed to create WebSocket events for remote %s: %w", redactURL(remote), err) | ||
| } | ||
|
|
||
| httpClient := &HTTP{ |
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Rough draft