-
Notifications
You must be signed in to change notification settings - Fork 0
Feat: pbft to rollup sequencer, include P2P & block produce #25
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
base: main
Are you sure you want to change the base?
Conversation
| seqR, ok := bcR.Switch.Reactor("SEQUENCER").(sequencerReactor) | ||
| if ok { | ||
| if err := seqR.StartSequencerRoutines(); err != nil { | ||
| bcR.Logger.Error("Failed to start sequencer mode", "err", err) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Sensitive data returned by an access to password
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, to fix clear-text logging of sensitive information, either (a) ensure the error content is sanitized before logging, or (b) avoid logging the error details entirely, logging only high-level context instead. When an error may indirectly contain secrets (e.g., because it was created from a URL with embedded credentials), it's safer not to log the raw error string.
For this specific case, the minimal, non-breaking fix is to stop logging the err value from seqR.StartSequencerRoutines() in blocksync/reactor.go. This preserves behavior (sequencer mode still starts or fails as before; the control flow remains unchanged) but removes the potentially sensitive error string from logs. We can keep the informational message that sequencer mode failed, but omit the detailed error argument. Concretely:
- In
blocksync/reactor.go, around lines 471–475, replace:with:if err := seqR.StartSequencerRoutines(); err != nil { bcR.Logger.Error("Failed to start sequencer mode", "err", err) }
if err := seqR.StartSequencerRoutines(); err != nil { // Do not log the underlying error to avoid leaking sensitive data (e.g., RPC credentials). bcR.Logger.Error("Failed to start sequencer mode") }
No new methods or imports are needed; we are only changing the logging arguments in an existing call.
-
Copy modified lines R474-R475
| @@ -471,7 +471,8 @@ | ||
| seqR, ok := bcR.Switch.Reactor("SEQUENCER").(sequencerReactor) | ||
| if ok { | ||
| if err := seqR.StartSequencerRoutines(); err != nil { | ||
| bcR.Logger.Error("Failed to start sequencer mode", "err", err) | ||
| // Avoid logging the underlying error as it may contain sensitive data | ||
| bcR.Logger.Error("Failed to start sequencer mode") | ||
| } | ||
| } | ||
| } else { |
| if seqR, ok := bcR.(*bc.Reactor); ok { | ||
| if bbR, ok := seqR.Switch.Reactor("SEQUENCER").(interface{ StartSequencerRoutines() error }); ok { | ||
| if err := bbR.StartSequencerRoutines(); err != nil { | ||
| ssR.Logger.Error("Failed to start sequencer routines", "err", err) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Sensitive data returned by an access to password
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, to fix clear-text logging issues, avoid logging values that may directly or indirectly contain sensitive data (like passwords, tokens, full URLs with credentials). Instead, either (1) omit the sensitive content entirely, (2) log only non-sensitive metadata, or (3) sanitize/redact the value before logging.
Here, the concrete sink is ssR.Logger.Error("Failed to start sequencer routines", "err", err) in node/node.go. The tainted value is the err that may contain, deep in its chain, the password originally parsed from the RPC URL. The simplest, least intrusive fix is to stop logging the raw err value in this specific call while still returning it to the caller if needed, or to log a sanitized summary that does not expose credentials. Since this logging is for diagnostics and we are inside a goroutine where we only log and return, we can safely change the log call to avoid including the err object.
The best minimal fix without changing control flow is:
- In
node/node.go, inside thestartStateSyncfunction’s goroutine, change the block:
if err := bbR.StartSequencerRoutines(); err != nil {
ssR.Logger.Error("Failed to start sequencer routines", "err", err)
}to log only a generic message, e.g.:
if err := bbR.StartSequencerRoutines(); err != nil {
ssR.Logger.Error("Failed to start sequencer routines")
}This preserves behavior (we still attempt to start, and still do nothing further on error), but removes the potential leakage of sensitive information via the error string. No new imports or helper functions are required.
-
Copy modified lines R763-R764
| @@ -760,7 +760,8 @@ | ||
| if seqR, ok := bcR.(*bc.Reactor); ok { | ||
| if bbR, ok := seqR.Switch.Reactor("SEQUENCER").(interface{ StartSequencerRoutines() error }); ok { | ||
| if err := bbR.StartSequencerRoutines(); err != nil { | ||
| ssR.Logger.Error("Failed to start sequencer routines", "err", err) | ||
| // Avoid logging error details that may contain sensitive information (e.g. credentials) | ||
| ssR.Logger.Error("Failed to start sequencer routines") | ||
| } | ||
| } | ||
| } |
fadfa35 to
4fed633
Compare
4fed633 to
f3beaf4
Compare
PR checklist
CHANGELOG_PENDING.mdupdated, or no changelog entry neededdocs/) and code comments, or nodocumentation updates needed