Enforce alternatives to %v and %+v #1639
Conversation
theunrepentantgeek
requested review from
babbageclunk,
davefellows,
matthchr and
Porges
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1639 +/- ##
==========================================
- Coverage 67.08% 67.04% -0.05%
==========================================
Files 205 205
Lines 14840 14864 +24
==========================================
+ Hits 9956 9966 +10
- Misses 4126 4141 +15
+ Partials 758 757 -1
Continue to review full report at Codecov.
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
api/v1alpha2/mysqlserver_types.go
Outdated
| @@ -33,7 +33,7 @@ type MySQLServerSpec struct { | |||
| // If AdminSecret is specified but a secret with the given name is not found in the same namespace | |||
| // as the MySQLServer, then reconciliation will block until the secret is created. | |||
| // If this is not specified, a username and password will be automatically generated. | |||
| AdminSecret string `json:"adminSecret,omitempty"` | |||
| AdminSecret string `json:"adminSecret,omitempty"` | |||
There was a problem hiding this comment.
The fact that the old ASO build lets me merge this angers me... maybe will make a fix to that.
| return v | ||
| default: | ||
| klog.Error(fmt.Sprintf("unexpected value for kubebuilder comment - %s", value.Kind())) | ||
| return "%%UNKNOWN%%" |
There was a problem hiding this comment.
I think it's appropriate to panic here rather than emit incorrect code
| @@ -16,7 +16,7 @@ func TestRetryTimeout(t *testing.T) { | |||
| }) | |||
| stop := time.Now().Sub(start) | |||
| if stop < 5*time.Second { | |||
| t.Errorf("retry ended too soon: %v", stop) | |||
| t.Errorf("retry ended too soon: %s", stop) | |||
There was a problem hiding this comment.
Not that there's any issue with this change, but wondering if we want to just exclude _test files from the check?
There was a problem hiding this comment.
If we have a %v in a test that dumps, say, the API key that we use for testing, wouldn't that be an issue? The whole PR is about being a bit paranoid about disclosures, so I'm happier checking the _test files too.
| @@ -11,6 +11,7 @@ import ( | |||
|
|
|||
| azuresql "github.com/Azure/azure-sdk-for-go/services/preview/sql/mgmt/v3.0/sql" | |||
| _ "github.com/denisenkom/go-mssqldb" | |||
| "github.com/pkg/errors" | |||
There was a problem hiding this comment.
without an amendment to the makefile (that builds old ASO), these can regress. I don't see that in this PR.
There was a problem hiding this comment.
The Taskfile targets don't run for these packages because they're not in hack
There was a problem hiding this comment.
I'll make a TODO for following up with changes to the Makefile to match.
Or do we want to migrate ASO to the same Task approach?
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
What this PR does / why we need it:
The
%vand%+vformat specifiers blindly dump all of the contents of any struct into the string - which can result in disclosure of secrets and other PII into log files where it is uncontrolled; this is a potential security and GDPR issue we need to prevent.This PR adds a check to enforce that the format specifiers
%vand%+vare not used.Closes #1585
How does this PR make you feel:
