From 9695ddb0275b10401be3937aab2cab79d73e6888 Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Wed, 7 Feb 2024 14:01:22 -0800 Subject: [PATCH 1/8] feat: Remove PII from Logs and Error Outputs Implemented privacy measures to strip all PII from zap log entries, and from the outputs of String() and Error() methods. This update minimizes privacy risks by sanitizing log entries and error information. --- gen/field.go | 9 +- gen/internal/tests/structs/structs.go | 189 ++++++++++++++++++++++- gen/internal/tests/thrift/structs.thrift | 3 + gen/zap.go | 27 +++- 4 files changed, 219 insertions(+), 9 deletions(-) diff --git a/gen/field.go b/gen/field.go index 93bfb40e..8300a1fa 100644 --- a/gen/field.go +++ b/gen/field.go @@ -694,9 +694,10 @@ func (f fieldGroupGenerator) String(g Generator) error { <$fields := newVar "fields"> <$i := newVar "i"> - var <$fields> []string + <$sanitizedFields := scrubPII .Fields > + var <$fields> []string <$i> := 0 - + <- $fname := goName . -> <- $f := printf "%s.%s" $v $fname -> @@ -717,7 +718,9 @@ func (f fieldGroupGenerator) String(g Generator) error { return <$fmt>.Sprintf("<.Name>{%v}", <$strings>.Join(<$fields>[:<$i>], ", ")) } - `, f) + `, f, + TemplateFunc("scrubPII", scrubPII), + ) } func (f fieldGroupGenerator) ErrorName(g Generator) error { diff --git a/gen/internal/tests/structs/structs.go b/gen/internal/tests/structs/structs.go index b1d8e938..919556cb 100644 --- a/gen/internal/tests/structs/structs.go +++ b/gen/internal/tests/structs/structs.go @@ -4751,7 +4751,10 @@ func (v *Omit) GetHidden() (o string) { } type PersonalInfo struct { - Age *int32 `json:"age,omitempty"` + Age *int32 `json:"age,omitempty"` + Race *string `json:"race,omitempty"` + Ssn *string `json:"ssn,omitempty"` + JobTitle *string `json:"jobTitle,omitempty"` } // ToWire translates a PersonalInfo struct into a Thrift-level intermediate @@ -4771,7 +4774,7 @@ type PersonalInfo struct { // } func (v *PersonalInfo) ToWire() (wire.Value, error) { var ( - fields [1]wire.Field + fields [4]wire.Field i int = 0 w wire.Value err error @@ -4785,6 +4788,30 @@ func (v *PersonalInfo) ToWire() (wire.Value, error) { fields[i] = wire.Field{ID: 1, Value: w} i++ } + if v.Race != nil { + w, err = wire.NewValueString(*(v.Race)), error(nil) + if err != nil { + return w, err + } + fields[i] = wire.Field{ID: 2, Value: w} + i++ + } + if v.Ssn != nil { + w, err = wire.NewValueString(*(v.Ssn)), error(nil) + if err != nil { + return w, err + } + fields[i] = wire.Field{ID: 3, Value: w} + i++ + } + if v.JobTitle != nil { + w, err = wire.NewValueString(*(v.JobTitle)), error(nil) + if err != nil { + return w, err + } + fields[i] = wire.Field{ID: 4, Value: w} + i++ + } return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil } @@ -4820,6 +4847,36 @@ func (v *PersonalInfo) FromWire(w wire.Value) error { return err } + } + case 2: + if field.Value.Type() == wire.TBinary { + var x string + x, err = field.Value.GetString(), error(nil) + v.Race = &x + if err != nil { + return err + } + + } + case 3: + if field.Value.Type() == wire.TBinary { + var x string + x, err = field.Value.GetString(), error(nil) + v.Ssn = &x + if err != nil { + return err + } + + } + case 4: + if field.Value.Type() == wire.TBinary { + var x string + x, err = field.Value.GetString(), error(nil) + v.JobTitle = &x + if err != nil { + return err + } + } } } @@ -4848,6 +4905,42 @@ func (v *PersonalInfo) Encode(sw stream.Writer) error { } } + if v.Race != nil { + if err := sw.WriteFieldBegin(stream.FieldHeader{ID: 2, Type: wire.TBinary}); err != nil { + return err + } + if err := sw.WriteString(*(v.Race)); err != nil { + return err + } + if err := sw.WriteFieldEnd(); err != nil { + return err + } + } + + if v.Ssn != nil { + if err := sw.WriteFieldBegin(stream.FieldHeader{ID: 3, Type: wire.TBinary}); err != nil { + return err + } + if err := sw.WriteString(*(v.Ssn)); err != nil { + return err + } + if err := sw.WriteFieldEnd(); err != nil { + return err + } + } + + if v.JobTitle != nil { + if err := sw.WriteFieldBegin(stream.FieldHeader{ID: 4, Type: wire.TBinary}); err != nil { + return err + } + if err := sw.WriteString(*(v.JobTitle)); err != nil { + return err + } + if err := sw.WriteFieldEnd(); err != nil { + return err + } + } + return sw.WriteStructEnd() } @@ -4877,6 +4970,30 @@ func (v *PersonalInfo) Decode(sr stream.Reader) error { return err } + case fh.ID == 2 && fh.Type == wire.TBinary: + var x string + x, err = sr.ReadString() + v.Race = &x + if err != nil { + return err + } + + case fh.ID == 3 && fh.Type == wire.TBinary: + var x string + x, err = sr.ReadString() + v.Ssn = &x + if err != nil { + return err + } + + case fh.ID == 4 && fh.Type == wire.TBinary: + var x string + x, err = sr.ReadString() + v.JobTitle = &x + if err != nil { + return err + } + default: if err := sr.Skip(fh.Type); err != nil { return err @@ -4906,12 +5023,16 @@ func (v *PersonalInfo) String() string { return "" } - var fields [1]string + var fields [2]string i := 0 if v.Age != nil { fields[i] = fmt.Sprintf("Age: %v", *(v.Age)) i++ } + if v.JobTitle != nil { + fields[i] = fmt.Sprintf("JobTitle: %v", *(v.JobTitle)) + i++ + } return fmt.Sprintf("PersonalInfo{%v}", strings.Join(fields[:i], ", ")) } @@ -4929,6 +5050,15 @@ func (v *PersonalInfo) Equals(rhs *PersonalInfo) bool { if !_I32_EqualsPtr(v.Age, rhs.Age) { return false } + if !_String_EqualsPtr(v.Race, rhs.Race) { + return false + } + if !_String_EqualsPtr(v.Ssn, rhs.Ssn) { + return false + } + if !_String_EqualsPtr(v.JobTitle, rhs.JobTitle) { + return false + } return true } @@ -4942,6 +5072,10 @@ func (v *PersonalInfo) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { if v.Age != nil { enc.AddInt32("age", *v.Age) } + + if v.JobTitle != nil { + enc.AddString("jobTitle", *v.JobTitle) + } return err } @@ -4960,6 +5094,51 @@ func (v *PersonalInfo) IsSetAge() bool { return v != nil && v.Age != nil } +// GetRace returns the value of Race if it is set or its +// zero value if it is unset. +func (v *PersonalInfo) GetRace() (o string) { + if v != nil && v.Race != nil { + return *v.Race + } + + return +} + +// IsSetRace returns true if Race is not nil. +func (v *PersonalInfo) IsSetRace() bool { + return v != nil && v.Race != nil +} + +// GetSsn returns the value of Ssn if it is set or its +// zero value if it is unset. +func (v *PersonalInfo) GetSsn() (o string) { + if v != nil && v.Ssn != nil { + return *v.Ssn + } + + return +} + +// IsSetSsn returns true if Ssn is not nil. +func (v *PersonalInfo) IsSetSsn() bool { + return v != nil && v.Ssn != nil +} + +// GetJobTitle returns the value of JobTitle if it is set or its +// zero value if it is unset. +func (v *PersonalInfo) GetJobTitle() (o string) { + if v != nil && v.JobTitle != nil { + return *v.JobTitle + } + + return +} + +// IsSetJobTitle returns true if JobTitle is not nil. +func (v *PersonalInfo) IsSetJobTitle() bool { + return v != nil && v.JobTitle != nil +} + // A point in 2D space. type Point struct { X float64 `json:"x,required"` @@ -8296,11 +8475,11 @@ var ThriftModule = &thriftreflect.ThriftModule{ Name: "structs", Package: "go.uber.org/thriftrw/gen/internal/tests/structs", FilePath: "structs.thrift", - SHA1: "2f43f38d30f0f9e3b089118d7ffc6b4dc532ada6", + SHA1: "11a54af4d2d1b524d5f01c5e695e74f6a47f3228", Includes: []*thriftreflect.ThriftModule{ enums.ThriftModule, }, Raw: rawIDL, } -const rawIDL = "include \"./enums.thrift\"\n\nstruct EmptyStruct {}\n\n//////////////////////////////////////////////////////////////////////////////\n// Structs with primitives\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are required.\n */\nstruct PrimitiveRequiredStruct {\n 1: required bool boolField\n 2: required byte byteField\n 3: required i16 int16Field\n 4: required i32 int32Field\n 5: required i64 int64Field\n 6: required double doubleField\n 7: required string stringField\n 8: required binary binaryField\n}\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are optional.\n */\nstruct PrimitiveOptionalStruct {\n 1: optional bool boolField\n 2: optional byte byteField\n 3: optional i16 int16Field\n 4: optional i32 int32Field\n 5: optional i64 int64Field\n 6: optional double doubleField\n 7: optional string stringField\n 8: optional binary binaryField\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Required)\n\n/**\n * A point in 2D space.\n */\nstruct Point {\n 1: required double x\n 2: required double y\n}\n\n/**\n * Size of something.\n */\nstruct Size {\n /**\n * Width in pixels.\n */\n 1: required double width\n /** Height in pixels. */\n 2: required double height\n}\n\nstruct Frame {\n 1: required Point topLeft\n 2: required Size size\n}\n\nstruct Edge {\n 1: required Point startPoint\n 2: required Point endPoint\n}\n\n/**\n * A graph is comprised of zero or more edges.\n */\nstruct Graph {\n /**\n * List of edges in the graph.\n *\n * May be empty.\n */\n 1: required list edges\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Optional)\n\nstruct ContactInfo {\n 1: required string emailAddress\n}\n\nstruct PersonalInfo {\n 1: optional i32 age\n}\n\nstruct User {\n 1: required string name\n 2: optional ContactInfo contact\n 3: optional PersonalInfo personal\n}\n\ntypedef map UserMap\n\n//////////////////////////////////////////////////////////////////////////////\n// self-referential struct\n\ntypedef Node List\n\n/**\n * Node is linked list of values.\n * All values are 32-bit integers.\n */\nstruct Node {\n 1: required i32 value\n 2: optional List tail\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// JSON tagged structs\n\nstruct Rename {\n 1: required string Default (go.tag = 'json:\"default\"')\n 2: required string camelCase (go.tag = 'json:\"snake_case\"')\n}\n\nstruct Omit {\n 1: required string serialized\n 2: required string hidden (go.tag = 'json:\"-\"')\n}\n\nstruct GoTags {\n 1: required string Foo (go.tag = 'json:\"-\" foo:\"bar\"')\n 2: optional string Bar (go.tag = 'bar:\"foo\"')\n 3: required string FooBar (go.tag = 'json:\"foobar,option1,option2\" bar:\"foo,option1\" foo:\"foobar\"')\n 4: required string FooBarWithSpace (go.tag = 'json:\"foobarWithSpace\" foo:\"foo bar foobar barfoo\"')\n 5: optional string FooBarWithOmitEmpty (go.tag = 'json:\"foobarWithOmitEmpty,omitempty\"')\n 6: required string FooBarWithRequired (go.tag = 'json:\"foobarWithRequired,required\"')\n}\n\nstruct NotOmitEmpty {\n 1: optional string NotOmitEmptyString (go.tag = 'json:\"notOmitEmptyString,!omitempty\"')\n 2: optional string NotOmitEmptyInt (go.tag = 'json:\"notOmitEmptyInt,!omitempty\"')\n 3: optional string NotOmitEmptyBool (go.tag = 'json:\"notOmitEmptyBool,!omitempty\"')\n 4: optional list NotOmitEmptyList (go.tag = 'json:\"notOmitEmptyList,!omitempty\"')\n 5: optional map NotOmitEmptyMap (go.tag = 'json:\"notOmitEmptyMap,!omitempty\"')\n 6: optional list NotOmitEmptyListMixedWithOmitEmpty (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmpty,!omitempty,omitempty\"')\n 7: optional list NotOmitEmptyListMixedWithOmitEmptyV2 (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmptyV2,omitempty,!omitempty\"')\n 8: optional string OmitEmptyString (go.tag = 'json:\"omitEmptyString,omitempty\"') // to test that there can be a mix of fields that do and don't have !omitempty\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Default values\n\nstruct DefaultsStruct {\n 1: required i32 requiredPrimitive = 100\n 2: optional i32 optionalPrimitive = 200\n\n 3: required enums.EnumDefault requiredEnum = enums.EnumDefault.Bar\n 4: optional enums.EnumDefault optionalEnum = 2\n\n 5: required list requiredList = [\"hello\", \"world\"]\n 6: optional list optionalList = [1, 2.0, 3]\n\n 7: required Frame requiredStruct = {\n \"topLeft\": {\"x\": 1, \"y\": 2},\n \"size\": {\"width\": 100, \"height\": 200},\n }\n 8: optional Edge optionalStruct = {\n \"startPoint\": {\"x\": 1, \"y\": 2},\n \"endPoint\": {\"x\": 3, \"y\": 4},\n }\n\n 9: required bool requiredBoolDefaultTrue = true\n 10: optional bool optionalBoolDefaultTrue = true\n\n 11: required bool requiredBoolDefaultFalse = false\n 12: optional bool optionalBoolDefaultFalse = false\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Opt-out of Zap\n\nstruct ZapOptOutStruct {\n 1: required string name\n 2: required string optout (go.nolog)\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Field jabels\n\nstruct StructLabels {\n // reserved keyword as label\n 1: optional bool isRequired (go.label = \"required\")\n\n // go.tag's JSON tag takes precedence over go.label\n 2: optional string foo (go.label = \"bar\", go.tag = 'json:\"not_bar\"')\n\n // Empty label\n 3: optional string qux (go.label = \"\")\n\n // All-caps label\n 4: optional string quux (go.label = \"QUUX\")\n}\n" +const rawIDL = "include \"./enums.thrift\"\n\nstruct EmptyStruct {}\n\n//////////////////////////////////////////////////////////////////////////////\n// Structs with primitives\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are required.\n */\nstruct PrimitiveRequiredStruct {\n 1: required bool boolField\n 2: required byte byteField\n 3: required i16 int16Field\n 4: required i32 int32Field\n 5: required i64 int64Field\n 6: required double doubleField\n 7: required string stringField\n 8: required binary binaryField\n}\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are optional.\n */\nstruct PrimitiveOptionalStruct {\n 1: optional bool boolField\n 2: optional byte byteField\n 3: optional i16 int16Field\n 4: optional i32 int32Field\n 5: optional i64 int64Field\n 6: optional double doubleField\n 7: optional string stringField\n 8: optional binary binaryField\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Required)\n\n/**\n * A point in 2D space.\n */\nstruct Point {\n 1: required double x\n 2: required double y\n}\n\n/**\n * Size of something.\n */\nstruct Size {\n /**\n * Width in pixels.\n */\n 1: required double width\n /** Height in pixels. */\n 2: required double height\n}\n\nstruct Frame {\n 1: required Point topLeft\n 2: required Size size\n}\n\nstruct Edge {\n 1: required Point startPoint\n 2: required Point endPoint\n}\n\n/**\n * A graph is comprised of zero or more edges.\n */\nstruct Graph {\n /**\n * List of edges in the graph.\n *\n * May be empty.\n */\n 1: required list edges\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Optional)\n\nstruct ContactInfo {\n 1: required string emailAddress\n}\n\nstruct PersonalInfo {\n 1: optional i32 age\n 2: optional string race (go.pii)\n 3: optional string ssn (go.pii)\n 4: optional string jobTitle\n}\n\nstruct User {\n 1: required string name\n 2: optional ContactInfo contact\n 3: optional PersonalInfo personal\n}\n\ntypedef map UserMap\n\n//////////////////////////////////////////////////////////////////////////////\n// self-referential struct\n\ntypedef Node List\n\n/**\n * Node is linked list of values.\n * All values are 32-bit integers.\n */\nstruct Node {\n 1: required i32 value\n 2: optional List tail\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// JSON tagged structs\n\nstruct Rename {\n 1: required string Default (go.tag = 'json:\"default\"')\n 2: required string camelCase (go.tag = 'json:\"snake_case\"')\n}\n\nstruct Omit {\n 1: required string serialized\n 2: required string hidden (go.tag = 'json:\"-\"')\n}\n\nstruct GoTags {\n 1: required string Foo (go.tag = 'json:\"-\" foo:\"bar\"')\n 2: optional string Bar (go.tag = 'bar:\"foo\"')\n 3: required string FooBar (go.tag = 'json:\"foobar,option1,option2\" bar:\"foo,option1\" foo:\"foobar\"')\n 4: required string FooBarWithSpace (go.tag = 'json:\"foobarWithSpace\" foo:\"foo bar foobar barfoo\"')\n 5: optional string FooBarWithOmitEmpty (go.tag = 'json:\"foobarWithOmitEmpty,omitempty\"')\n 6: required string FooBarWithRequired (go.tag = 'json:\"foobarWithRequired,required\"')\n}\n\nstruct NotOmitEmpty {\n 1: optional string NotOmitEmptyString (go.tag = 'json:\"notOmitEmptyString,!omitempty\"')\n 2: optional string NotOmitEmptyInt (go.tag = 'json:\"notOmitEmptyInt,!omitempty\"')\n 3: optional string NotOmitEmptyBool (go.tag = 'json:\"notOmitEmptyBool,!omitempty\"')\n 4: optional list NotOmitEmptyList (go.tag = 'json:\"notOmitEmptyList,!omitempty\"')\n 5: optional map NotOmitEmptyMap (go.tag = 'json:\"notOmitEmptyMap,!omitempty\"')\n 6: optional list NotOmitEmptyListMixedWithOmitEmpty (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmpty,!omitempty,omitempty\"')\n 7: optional list NotOmitEmptyListMixedWithOmitEmptyV2 (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmptyV2,omitempty,!omitempty\"')\n 8: optional string OmitEmptyString (go.tag = 'json:\"omitEmptyString,omitempty\"') // to test that there can be a mix of fields that do and don't have !omitempty\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Default values\n\nstruct DefaultsStruct {\n 1: required i32 requiredPrimitive = 100\n 2: optional i32 optionalPrimitive = 200\n\n 3: required enums.EnumDefault requiredEnum = enums.EnumDefault.Bar\n 4: optional enums.EnumDefault optionalEnum = 2\n\n 5: required list requiredList = [\"hello\", \"world\"]\n 6: optional list optionalList = [1, 2.0, 3]\n\n 7: required Frame requiredStruct = {\n \"topLeft\": {\"x\": 1, \"y\": 2},\n \"size\": {\"width\": 100, \"height\": 200},\n }\n 8: optional Edge optionalStruct = {\n \"startPoint\": {\"x\": 1, \"y\": 2},\n \"endPoint\": {\"x\": 3, \"y\": 4},\n }\n\n 9: required bool requiredBoolDefaultTrue = true\n 10: optional bool optionalBoolDefaultTrue = true\n\n 11: required bool requiredBoolDefaultFalse = false\n 12: optional bool optionalBoolDefaultFalse = false\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Opt-out of Zap\n\nstruct ZapOptOutStruct {\n 1: required string name\n 2: required string optout (go.nolog)\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Field jabels\n\nstruct StructLabels {\n // reserved keyword as label\n 1: optional bool isRequired (go.label = \"required\")\n\n // go.tag's JSON tag takes precedence over go.label\n 2: optional string foo (go.label = \"bar\", go.tag = 'json:\"not_bar\"')\n\n // Empty label\n 3: optional string qux (go.label = \"\")\n\n // All-caps label\n 4: optional string quux (go.label = \"QUUX\")\n}\n" diff --git a/gen/internal/tests/thrift/structs.thrift b/gen/internal/tests/thrift/structs.thrift index b1b0efef..ad9cb028 100644 --- a/gen/internal/tests/thrift/structs.thrift +++ b/gen/internal/tests/thrift/structs.thrift @@ -91,6 +91,9 @@ struct ContactInfo { struct PersonalInfo { 1: optional i32 age + 2: optional string race (go.pii) + 3: optional string ssn (go.pii) + 4: optional string jobTitle } struct User { diff --git a/gen/zap.go b/gen/zap.go index 91a43019..2149c08c 100644 --- a/gen/zap.go +++ b/gen/zap.go @@ -38,6 +38,15 @@ import ( // The above struct will be logged without the optout string. const NoZapLabel = "go.nolog" +// PIILabel provides a mechanism to excludes certain struct fields from +// Zap logs, and from the outputs of String() and Error() methods. +// +// struct Contact { +// 1: required string name +// 2: required string email (go.pii) // will be omitted from Zap logs, Stringer(), and Error() methods. +// } +const PIILabel = "go.pii" + type zapGenerator struct { mapG mapGenerator setG setGenerator @@ -46,7 +55,7 @@ type zapGenerator struct { // zapEncoder returns the Zap type name of the root spec, determining what type // the Zap marshaler needs to log it as (i.e. AddString, AppendObject, etc.) -func (z *zapGenerator) zapEncoder(g Generator, spec compile.TypeSpec) string { +func (z *zapGenerator) zapEncoder(_ Generator, spec compile.TypeSpec) string { root := compile.RootTypeSpec(spec) switch t := root.(type) { @@ -192,5 +201,21 @@ func (z *zapGenerator) zapEncodeEnd(spec compile.TypeSpec) string { func zapOptOut(spec *compile.FieldSpec) bool { _, ok := spec.Annotations[NoZapLabel] + return ok || isPII(spec) +} + +func isPII(spec *compile.FieldSpec) bool { + _, ok := spec.Annotations[PIILabel] return ok } + +func scrubPII(fields compile.FieldGroup) compile.FieldGroup { + out := make(compile.FieldGroup, 0) + for _, field := range fields { + if isPII(field) { + continue + } + out = append(out, field) + } + return out +} From 4798bbb7b84858cad578fcd2eda3f543000ca32f Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Wed, 7 Feb 2024 14:40:50 -0800 Subject: [PATCH 2/8] Add test for hasPIIAnnotation method --- gen/field.go | 25 +++++++++++++++ gen/field_test.go | 77 +++++++++++++++++++++++++++++++++++++++++++++++ gen/zap.go | 27 +---------------- 3 files changed, 103 insertions(+), 26 deletions(-) diff --git a/gen/field.go b/gen/field.go index 8300a1fa..f2166f6d 100644 --- a/gen/field.go +++ b/gen/field.go @@ -878,3 +878,28 @@ func verifyUniqueFieldLabels(fs compile.FieldGroup) error { } return nil } + +// PIILabel provides a mechanism to excludes certain struct fields from +// Zap logs, and from the outputs of String() and Error() methods. +// +// struct Contact { +// 1: required string name +// 2: required string email (go.pii) // will be omitted from Zap logs, Stringer(), and Error() methods. +// } +const PIILabel = "go.pii" + +func hasPIIAnnotation(spec *compile.FieldSpec) bool { + _, ok := spec.Annotations[PIILabel] + return ok +} + +func scrubPII(fields compile.FieldGroup) compile.FieldGroup { + out := make(compile.FieldGroup, 0) + for _, field := range fields { + if hasPIIAnnotation(field) { + continue + } + out = append(out, field) + } + return out +} diff --git a/gen/field_test.go b/gen/field_test.go index 561011e1..00956a84 100644 --- a/gen/field_test.go +++ b/gen/field_test.go @@ -137,3 +137,80 @@ func TestCompileJSONTag(t *testing.T) { }) } } + +func TestScrubPII(t *testing.T) { + foo := &compile.FieldSpec{ + Name: "foo", + Annotations: compile.Annotations{"go.label": "baz"}, + } + pii := &compile.FieldSpec{ + Name: "foo", + Annotations: compile.Annotations{"go.pii": ""}, + } + tests := []struct { + name string + spec compile.FieldGroup + want compile.FieldGroup + }{ + { + name: "without pii annotation", + spec: compile.FieldGroup{ + foo, + foo, + }, + want: compile.FieldGroup{ + foo, + foo, + }, + }, + { + name: "with pii annotation", + spec: compile.FieldGroup{ + foo, + pii, + foo, + }, + want: compile.FieldGroup{ + foo, + foo, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, scrubPII(tt.spec), "scrubPII(%v)", tt.spec) + }) + } +} + +func TestHasPIIAnnotation(t *testing.T) { + foo := &compile.FieldSpec{ + Name: "foo", + Annotations: compile.Annotations{"go.label": "baz"}, + } + pii := &compile.FieldSpec{ + Name: "foo", + Annotations: compile.Annotations{"go.pii": ""}, + } + tests := []struct { + name string + spec *compile.FieldSpec + want bool + }{ + { + name: "has pii annotation", + spec: pii, + want: true, + }, + { + name: "has not pii annotation", + spec: foo, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, hasPIIAnnotation(tt.spec), "hasPIIAnnotation(%v)", tt.spec) + }) + } +} diff --git a/gen/zap.go b/gen/zap.go index 2149c08c..3cad4f03 100644 --- a/gen/zap.go +++ b/gen/zap.go @@ -38,15 +38,6 @@ import ( // The above struct will be logged without the optout string. const NoZapLabel = "go.nolog" -// PIILabel provides a mechanism to excludes certain struct fields from -// Zap logs, and from the outputs of String() and Error() methods. -// -// struct Contact { -// 1: required string name -// 2: required string email (go.pii) // will be omitted from Zap logs, Stringer(), and Error() methods. -// } -const PIILabel = "go.pii" - type zapGenerator struct { mapG mapGenerator setG setGenerator @@ -201,21 +192,5 @@ func (z *zapGenerator) zapEncodeEnd(spec compile.TypeSpec) string { func zapOptOut(spec *compile.FieldSpec) bool { _, ok := spec.Annotations[NoZapLabel] - return ok || isPII(spec) -} - -func isPII(spec *compile.FieldSpec) bool { - _, ok := spec.Annotations[PIILabel] - return ok -} - -func scrubPII(fields compile.FieldGroup) compile.FieldGroup { - out := make(compile.FieldGroup, 0) - for _, field := range fields { - if isPII(field) { - continue - } - out = append(out, field) - } - return out + return ok || hasPIIAnnotation(spec) } From 89b5b22f41232d04b408464fa93da66c9ac1f349 Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Wed, 7 Feb 2024 14:53:28 -0800 Subject: [PATCH 3/8] Add more tests --- gen/field_test.go | 18 ++++++++---------- gen/zap_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/gen/field_test.go b/gen/field_test.go index 00956a84..0371c2e7 100644 --- a/gen/field_test.go +++ b/gen/field_test.go @@ -140,12 +140,11 @@ func TestCompileJSONTag(t *testing.T) { func TestScrubPII(t *testing.T) { foo := &compile.FieldSpec{ - Name: "foo", - Annotations: compile.Annotations{"go.label": "baz"}, + Name: "foo", } pii := &compile.FieldSpec{ - Name: "foo", - Annotations: compile.Annotations{"go.pii": ""}, + Name: "pii", + Annotations: compile.Annotations{PIILabel: ""}, } tests := []struct { name string @@ -185,12 +184,11 @@ func TestScrubPII(t *testing.T) { func TestHasPIIAnnotation(t *testing.T) { foo := &compile.FieldSpec{ - Name: "foo", - Annotations: compile.Annotations{"go.label": "baz"}, + Name: "foo", } pii := &compile.FieldSpec{ - Name: "foo", - Annotations: compile.Annotations{"go.pii": ""}, + Name: "pii", + Annotations: compile.Annotations{PIILabel: ""}, } tests := []struct { name string @@ -198,12 +196,12 @@ func TestHasPIIAnnotation(t *testing.T) { want bool }{ { - name: "has pii annotation", + name: "pii annotation", spec: pii, want: true, }, { - name: "has not pii annotation", + name: "no pii annotation", spec: foo, want: false, }, diff --git a/gen/zap_test.go b/gen/zap_test.go index f5025b57..ac95dd15 100644 --- a/gen/zap_test.go +++ b/gen/zap_test.go @@ -28,6 +28,7 @@ import ( gomock "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/thriftrw/compile" tc "go.uber.org/thriftrw/gen/internal/tests/containers" te "go.uber.org/thriftrw/gen/internal/tests/enums" tz "go.uber.org/thriftrw/gen/internal/tests/nozap" @@ -852,3 +853,44 @@ func TestLogNilStruct(t *testing.T) { require.NoError(t, x.MarshalLogObject(enc)) assert.Empty(t, enc.Fields) } + +func TestZapOptOut(t *testing.T) { + foo := &compile.FieldSpec{ + Name: "foo", + } + pii := &compile.FieldSpec{ + Name: "pii", + Annotations: compile.Annotations{PIILabel: ""}, + } + + nolog := &compile.FieldSpec{ + Name: "nolog", + Annotations: compile.Annotations{NoZapLabel: ""}, + } + tests := []struct { + name string + spec *compile.FieldSpec + want bool + }{ + { + name: "no annotation", + spec: foo, + want: false, + }, + { + name: "pii annotation", + spec: pii, + want: true, + }, + { + name: "nolog annotation", + spec: nolog, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, zapOptOut(tt.spec), "zapOptOut(%v)", tt.spec) + }) + } +} From 7418b4951f88d6def86d6b9b1123cfed3bed55f5 Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Wed, 7 Feb 2024 16:40:22 -0800 Subject: [PATCH 4/8] Add behavioral tests --- gen/field_test.go | 56 +++++++-- gen/internal/tests/exceptions/exceptions.go | 68 +++++++++- gen/internal/tests/structs/structs.go | 133 +------------------- gen/internal/tests/thrift/exceptions.thrift | 1 + gen/internal/tests/thrift/structs.thrift | 2 - gen/zap_test.go | 18 +-- 6 files changed, 117 insertions(+), 161 deletions(-) diff --git a/gen/field_test.go b/gen/field_test.go index 0371c2e7..0cba39f4 100644 --- a/gen/field_test.go +++ b/gen/field_test.go @@ -25,10 +25,12 @@ import ( "strings" "testing" - "go.uber.org/thriftrw/compile" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/thriftrw/compile" + "go.uber.org/thriftrw/gen/internal/tests/exceptions" + ts "go.uber.org/thriftrw/gen/internal/tests/structs" + "go.uber.org/zap/zapcore" ) func TestFieldLabelConflict(t *testing.T) { @@ -195,16 +197,8 @@ func TestHasPIIAnnotation(t *testing.T) { spec *compile.FieldSpec want bool }{ - { - name: "pii annotation", - spec: pii, - want: true, - }, - { - name: "no pii annotation", - spec: foo, - want: false, - }, + {name: "pii annotation", spec: pii, want: true}, + {name: "no pii annotation", spec: foo, want: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -212,3 +206,41 @@ func TestHasPIIAnnotation(t *testing.T) { }) } } + +func TestSanitizePII(t *testing.T) { + age := int32(21) + pi := ts.PersonalInfo{ + Age: toPtr(age), + Race: toPtr("kryptonian"), + } + piiException := exceptions.DoesNotExistException{ + Key: "s", + UserName: toPtr("superman"), + } + piEncoder := zapcore.NewMapObjectEncoder() + require.NoError(t, pi.MarshalLogObject(piEncoder)) + + piiExceptionEncoder := zapcore.NewMapObjectEncoder() + require.NoError(t, piiException.MarshalLogObject(piiExceptionEncoder)) + + tests := []struct { + name string + got any + want any + }{ + {name: "struct/zap", got: piEncoder.Fields, want: map[string]interface{}{"age": age}}, + {name: "struct/string", got: pi.String(), want: "PersonalInfo{Age: 21}"}, + {name: "exception/zap", got: piiExceptionEncoder.Fields, want: map[string]interface{}{"key": "s"}}, + {name: "exception/string", got: piiException.String(), want: "DoesNotExistException{Key: s}"}, + {name: "exception/error", got: piiException.Error(), want: "DoesNotExistException{Key: s}"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.got) + }) + } +} + +func toPtr[T any](input T) *T { + return &input +} diff --git a/gen/internal/tests/exceptions/exceptions.go b/gen/internal/tests/exceptions/exceptions.go index 89bd45bf..855d3131 100644 --- a/gen/internal/tests/exceptions/exceptions.go +++ b/gen/internal/tests/exceptions/exceptions.go @@ -16,8 +16,9 @@ import ( // Raised when something doesn't exist. type DoesNotExistException struct { // Key that was missing. - Key string `json:"key,required"` - Error2 *string `json:"Error,omitempty"` + Key string `json:"key,required"` + Error2 *string `json:"Error,omitempty"` + UserName *string `json:"userName,omitempty"` } // ToWire translates a DoesNotExistException struct into a Thrift-level intermediate @@ -37,7 +38,7 @@ type DoesNotExistException struct { // } func (v *DoesNotExistException) ToWire() (wire.Value, error) { var ( - fields [2]wire.Field + fields [3]wire.Field i int = 0 w wire.Value err error @@ -57,6 +58,14 @@ func (v *DoesNotExistException) ToWire() (wire.Value, error) { fields[i] = wire.Field{ID: 2, Value: w} i++ } + if v.UserName != nil { + w, err = wire.NewValueString(*(v.UserName)), error(nil) + if err != nil { + return w, err + } + fields[i] = wire.Field{ID: 3, Value: w} + i++ + } return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil } @@ -102,6 +111,16 @@ func (v *DoesNotExistException) FromWire(w wire.Value) error { return err } + } + case 3: + if field.Value.Type() == wire.TBinary { + var x string + x, err = field.Value.GetString(), error(nil) + v.UserName = &x + if err != nil { + return err + } + } } } @@ -144,6 +163,18 @@ func (v *DoesNotExistException) Encode(sw stream.Writer) error { } } + if v.UserName != nil { + if err := sw.WriteFieldBegin(stream.FieldHeader{ID: 3, Type: wire.TBinary}); err != nil { + return err + } + if err := sw.WriteString(*(v.UserName)); err != nil { + return err + } + if err := sw.WriteFieldEnd(); err != nil { + return err + } + } + return sw.WriteStructEnd() } @@ -181,6 +212,14 @@ func (v *DoesNotExistException) Decode(sr stream.Reader) error { return err } + case fh.ID == 3 && fh.Type == wire.TBinary: + var x string + x, err = sr.ReadString() + v.UserName = &x + if err != nil { + return err + } + default: if err := sr.Skip(fh.Type); err != nil { return err @@ -258,6 +297,9 @@ func (v *DoesNotExistException) Equals(rhs *DoesNotExistException) bool { if !_String_EqualsPtr(v.Error2, rhs.Error2) { return false } + if !_String_EqualsPtr(v.UserName, rhs.UserName) { + return false + } return true } @@ -272,6 +314,7 @@ func (v *DoesNotExistException) MarshalLogObject(enc zapcore.ObjectEncoder) (err if v.Error2 != nil { enc.AddString("Error", *v.Error2) } + return err } @@ -299,6 +342,21 @@ func (v *DoesNotExistException) IsSetError2() bool { return v != nil && v.Error2 != nil } +// GetUserName returns the value of UserName if it is set or its +// zero value if it is unset. +func (v *DoesNotExistException) GetUserName() (o string) { + if v != nil && v.UserName != nil { + return *v.UserName + } + + return +} + +// IsSetUserName returns true if UserName is not nil. +func (v *DoesNotExistException) IsSetUserName() bool { + return v != nil && v.UserName != nil +} + func (v *DoesNotExistException) Error() string { return v.String() } @@ -739,8 +797,8 @@ var ThriftModule = &thriftreflect.ThriftModule{ Name: "exceptions", Package: "go.uber.org/thriftrw/gen/internal/tests/exceptions", FilePath: "exceptions.thrift", - SHA1: "a31a29f9f7bca3221100e43e32b0d833ca9c774e", + SHA1: "79cea1006d14acc62a1e637626fd242b25ad7bd6", Raw: rawIDL, } -const rawIDL = "exception EmptyException {}\n\n/**\n * Raised when something doesn't exist.\n */\nexception DoesNotExistException {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n}\n\nexception Does_Not_Exist_Exception_Collision {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n} (go.name=\"DoesNotExistException2\")\n" +const rawIDL = "exception EmptyException {}\n\n/**\n * Raised when something doesn't exist.\n */\nexception DoesNotExistException {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n 3: optional string userName (go.pii)\n}\n\nexception Does_Not_Exist_Exception_Collision {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n} (go.name=\"DoesNotExistException2\")\n" diff --git a/gen/internal/tests/structs/structs.go b/gen/internal/tests/structs/structs.go index 919556cb..5aa4c7b4 100644 --- a/gen/internal/tests/structs/structs.go +++ b/gen/internal/tests/structs/structs.go @@ -4751,10 +4751,8 @@ func (v *Omit) GetHidden() (o string) { } type PersonalInfo struct { - Age *int32 `json:"age,omitempty"` - Race *string `json:"race,omitempty"` - Ssn *string `json:"ssn,omitempty"` - JobTitle *string `json:"jobTitle,omitempty"` + Age *int32 `json:"age,omitempty"` + Race *string `json:"race,omitempty"` } // ToWire translates a PersonalInfo struct into a Thrift-level intermediate @@ -4774,7 +4772,7 @@ type PersonalInfo struct { // } func (v *PersonalInfo) ToWire() (wire.Value, error) { var ( - fields [4]wire.Field + fields [2]wire.Field i int = 0 w wire.Value err error @@ -4796,22 +4794,6 @@ func (v *PersonalInfo) ToWire() (wire.Value, error) { fields[i] = wire.Field{ID: 2, Value: w} i++ } - if v.Ssn != nil { - w, err = wire.NewValueString(*(v.Ssn)), error(nil) - if err != nil { - return w, err - } - fields[i] = wire.Field{ID: 3, Value: w} - i++ - } - if v.JobTitle != nil { - w, err = wire.NewValueString(*(v.JobTitle)), error(nil) - if err != nil { - return w, err - } - fields[i] = wire.Field{ID: 4, Value: w} - i++ - } return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil } @@ -4857,26 +4839,6 @@ func (v *PersonalInfo) FromWire(w wire.Value) error { return err } - } - case 3: - if field.Value.Type() == wire.TBinary { - var x string - x, err = field.Value.GetString(), error(nil) - v.Ssn = &x - if err != nil { - return err - } - - } - case 4: - if field.Value.Type() == wire.TBinary { - var x string - x, err = field.Value.GetString(), error(nil) - v.JobTitle = &x - if err != nil { - return err - } - } } } @@ -4917,30 +4879,6 @@ func (v *PersonalInfo) Encode(sw stream.Writer) error { } } - if v.Ssn != nil { - if err := sw.WriteFieldBegin(stream.FieldHeader{ID: 3, Type: wire.TBinary}); err != nil { - return err - } - if err := sw.WriteString(*(v.Ssn)); err != nil { - return err - } - if err := sw.WriteFieldEnd(); err != nil { - return err - } - } - - if v.JobTitle != nil { - if err := sw.WriteFieldBegin(stream.FieldHeader{ID: 4, Type: wire.TBinary}); err != nil { - return err - } - if err := sw.WriteString(*(v.JobTitle)); err != nil { - return err - } - if err := sw.WriteFieldEnd(); err != nil { - return err - } - } - return sw.WriteStructEnd() } @@ -4978,22 +4916,6 @@ func (v *PersonalInfo) Decode(sr stream.Reader) error { return err } - case fh.ID == 3 && fh.Type == wire.TBinary: - var x string - x, err = sr.ReadString() - v.Ssn = &x - if err != nil { - return err - } - - case fh.ID == 4 && fh.Type == wire.TBinary: - var x string - x, err = sr.ReadString() - v.JobTitle = &x - if err != nil { - return err - } - default: if err := sr.Skip(fh.Type); err != nil { return err @@ -5023,16 +4945,12 @@ func (v *PersonalInfo) String() string { return "" } - var fields [2]string + var fields [1]string i := 0 if v.Age != nil { fields[i] = fmt.Sprintf("Age: %v", *(v.Age)) i++ } - if v.JobTitle != nil { - fields[i] = fmt.Sprintf("JobTitle: %v", *(v.JobTitle)) - i++ - } return fmt.Sprintf("PersonalInfo{%v}", strings.Join(fields[:i], ", ")) } @@ -5053,12 +4971,6 @@ func (v *PersonalInfo) Equals(rhs *PersonalInfo) bool { if !_String_EqualsPtr(v.Race, rhs.Race) { return false } - if !_String_EqualsPtr(v.Ssn, rhs.Ssn) { - return false - } - if !_String_EqualsPtr(v.JobTitle, rhs.JobTitle) { - return false - } return true } @@ -5073,9 +4985,6 @@ func (v *PersonalInfo) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { enc.AddInt32("age", *v.Age) } - if v.JobTitle != nil { - enc.AddString("jobTitle", *v.JobTitle) - } return err } @@ -5109,36 +5018,6 @@ func (v *PersonalInfo) IsSetRace() bool { return v != nil && v.Race != nil } -// GetSsn returns the value of Ssn if it is set or its -// zero value if it is unset. -func (v *PersonalInfo) GetSsn() (o string) { - if v != nil && v.Ssn != nil { - return *v.Ssn - } - - return -} - -// IsSetSsn returns true if Ssn is not nil. -func (v *PersonalInfo) IsSetSsn() bool { - return v != nil && v.Ssn != nil -} - -// GetJobTitle returns the value of JobTitle if it is set or its -// zero value if it is unset. -func (v *PersonalInfo) GetJobTitle() (o string) { - if v != nil && v.JobTitle != nil { - return *v.JobTitle - } - - return -} - -// IsSetJobTitle returns true if JobTitle is not nil. -func (v *PersonalInfo) IsSetJobTitle() bool { - return v != nil && v.JobTitle != nil -} - // A point in 2D space. type Point struct { X float64 `json:"x,required"` @@ -8475,11 +8354,11 @@ var ThriftModule = &thriftreflect.ThriftModule{ Name: "structs", Package: "go.uber.org/thriftrw/gen/internal/tests/structs", FilePath: "structs.thrift", - SHA1: "11a54af4d2d1b524d5f01c5e695e74f6a47f3228", + SHA1: "b19a941227d7418b94291c00368b54bc89d663ff", Includes: []*thriftreflect.ThriftModule{ enums.ThriftModule, }, Raw: rawIDL, } -const rawIDL = "include \"./enums.thrift\"\n\nstruct EmptyStruct {}\n\n//////////////////////////////////////////////////////////////////////////////\n// Structs with primitives\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are required.\n */\nstruct PrimitiveRequiredStruct {\n 1: required bool boolField\n 2: required byte byteField\n 3: required i16 int16Field\n 4: required i32 int32Field\n 5: required i64 int64Field\n 6: required double doubleField\n 7: required string stringField\n 8: required binary binaryField\n}\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are optional.\n */\nstruct PrimitiveOptionalStruct {\n 1: optional bool boolField\n 2: optional byte byteField\n 3: optional i16 int16Field\n 4: optional i32 int32Field\n 5: optional i64 int64Field\n 6: optional double doubleField\n 7: optional string stringField\n 8: optional binary binaryField\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Required)\n\n/**\n * A point in 2D space.\n */\nstruct Point {\n 1: required double x\n 2: required double y\n}\n\n/**\n * Size of something.\n */\nstruct Size {\n /**\n * Width in pixels.\n */\n 1: required double width\n /** Height in pixels. */\n 2: required double height\n}\n\nstruct Frame {\n 1: required Point topLeft\n 2: required Size size\n}\n\nstruct Edge {\n 1: required Point startPoint\n 2: required Point endPoint\n}\n\n/**\n * A graph is comprised of zero or more edges.\n */\nstruct Graph {\n /**\n * List of edges in the graph.\n *\n * May be empty.\n */\n 1: required list edges\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Optional)\n\nstruct ContactInfo {\n 1: required string emailAddress\n}\n\nstruct PersonalInfo {\n 1: optional i32 age\n 2: optional string race (go.pii)\n 3: optional string ssn (go.pii)\n 4: optional string jobTitle\n}\n\nstruct User {\n 1: required string name\n 2: optional ContactInfo contact\n 3: optional PersonalInfo personal\n}\n\ntypedef map UserMap\n\n//////////////////////////////////////////////////////////////////////////////\n// self-referential struct\n\ntypedef Node List\n\n/**\n * Node is linked list of values.\n * All values are 32-bit integers.\n */\nstruct Node {\n 1: required i32 value\n 2: optional List tail\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// JSON tagged structs\n\nstruct Rename {\n 1: required string Default (go.tag = 'json:\"default\"')\n 2: required string camelCase (go.tag = 'json:\"snake_case\"')\n}\n\nstruct Omit {\n 1: required string serialized\n 2: required string hidden (go.tag = 'json:\"-\"')\n}\n\nstruct GoTags {\n 1: required string Foo (go.tag = 'json:\"-\" foo:\"bar\"')\n 2: optional string Bar (go.tag = 'bar:\"foo\"')\n 3: required string FooBar (go.tag = 'json:\"foobar,option1,option2\" bar:\"foo,option1\" foo:\"foobar\"')\n 4: required string FooBarWithSpace (go.tag = 'json:\"foobarWithSpace\" foo:\"foo bar foobar barfoo\"')\n 5: optional string FooBarWithOmitEmpty (go.tag = 'json:\"foobarWithOmitEmpty,omitempty\"')\n 6: required string FooBarWithRequired (go.tag = 'json:\"foobarWithRequired,required\"')\n}\n\nstruct NotOmitEmpty {\n 1: optional string NotOmitEmptyString (go.tag = 'json:\"notOmitEmptyString,!omitempty\"')\n 2: optional string NotOmitEmptyInt (go.tag = 'json:\"notOmitEmptyInt,!omitempty\"')\n 3: optional string NotOmitEmptyBool (go.tag = 'json:\"notOmitEmptyBool,!omitempty\"')\n 4: optional list NotOmitEmptyList (go.tag = 'json:\"notOmitEmptyList,!omitempty\"')\n 5: optional map NotOmitEmptyMap (go.tag = 'json:\"notOmitEmptyMap,!omitempty\"')\n 6: optional list NotOmitEmptyListMixedWithOmitEmpty (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmpty,!omitempty,omitempty\"')\n 7: optional list NotOmitEmptyListMixedWithOmitEmptyV2 (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmptyV2,omitempty,!omitempty\"')\n 8: optional string OmitEmptyString (go.tag = 'json:\"omitEmptyString,omitempty\"') // to test that there can be a mix of fields that do and don't have !omitempty\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Default values\n\nstruct DefaultsStruct {\n 1: required i32 requiredPrimitive = 100\n 2: optional i32 optionalPrimitive = 200\n\n 3: required enums.EnumDefault requiredEnum = enums.EnumDefault.Bar\n 4: optional enums.EnumDefault optionalEnum = 2\n\n 5: required list requiredList = [\"hello\", \"world\"]\n 6: optional list optionalList = [1, 2.0, 3]\n\n 7: required Frame requiredStruct = {\n \"topLeft\": {\"x\": 1, \"y\": 2},\n \"size\": {\"width\": 100, \"height\": 200},\n }\n 8: optional Edge optionalStruct = {\n \"startPoint\": {\"x\": 1, \"y\": 2},\n \"endPoint\": {\"x\": 3, \"y\": 4},\n }\n\n 9: required bool requiredBoolDefaultTrue = true\n 10: optional bool optionalBoolDefaultTrue = true\n\n 11: required bool requiredBoolDefaultFalse = false\n 12: optional bool optionalBoolDefaultFalse = false\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Opt-out of Zap\n\nstruct ZapOptOutStruct {\n 1: required string name\n 2: required string optout (go.nolog)\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Field jabels\n\nstruct StructLabels {\n // reserved keyword as label\n 1: optional bool isRequired (go.label = \"required\")\n\n // go.tag's JSON tag takes precedence over go.label\n 2: optional string foo (go.label = \"bar\", go.tag = 'json:\"not_bar\"')\n\n // Empty label\n 3: optional string qux (go.label = \"\")\n\n // All-caps label\n 4: optional string quux (go.label = \"QUUX\")\n}\n" +const rawIDL = "include \"./enums.thrift\"\n\nstruct EmptyStruct {}\n\n//////////////////////////////////////////////////////////////////////////////\n// Structs with primitives\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are required.\n */\nstruct PrimitiveRequiredStruct {\n 1: required bool boolField\n 2: required byte byteField\n 3: required i16 int16Field\n 4: required i32 int32Field\n 5: required i64 int64Field\n 6: required double doubleField\n 7: required string stringField\n 8: required binary binaryField\n}\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are optional.\n */\nstruct PrimitiveOptionalStruct {\n 1: optional bool boolField\n 2: optional byte byteField\n 3: optional i16 int16Field\n 4: optional i32 int32Field\n 5: optional i64 int64Field\n 6: optional double doubleField\n 7: optional string stringField\n 8: optional binary binaryField\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Required)\n\n/**\n * A point in 2D space.\n */\nstruct Point {\n 1: required double x\n 2: required double y\n}\n\n/**\n * Size of something.\n */\nstruct Size {\n /**\n * Width in pixels.\n */\n 1: required double width\n /** Height in pixels. */\n 2: required double height\n}\n\nstruct Frame {\n 1: required Point topLeft\n 2: required Size size\n}\n\nstruct Edge {\n 1: required Point startPoint\n 2: required Point endPoint\n}\n\n/**\n * A graph is comprised of zero or more edges.\n */\nstruct Graph {\n /**\n * List of edges in the graph.\n *\n * May be empty.\n */\n 1: required list edges\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Optional)\n\nstruct ContactInfo {\n 1: required string emailAddress\n}\n\nstruct PersonalInfo {\n 1: optional i32 age\n 2: optional string race (go.pii)\n}\n\nstruct User {\n 1: required string name\n 2: optional ContactInfo contact\n 3: optional PersonalInfo personal\n}\n\ntypedef map UserMap\n\n//////////////////////////////////////////////////////////////////////////////\n// self-referential struct\n\ntypedef Node List\n\n/**\n * Node is linked list of values.\n * All values are 32-bit integers.\n */\nstruct Node {\n 1: required i32 value\n 2: optional List tail\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// JSON tagged structs\n\nstruct Rename {\n 1: required string Default (go.tag = 'json:\"default\"')\n 2: required string camelCase (go.tag = 'json:\"snake_case\"')\n}\n\nstruct Omit {\n 1: required string serialized\n 2: required string hidden (go.tag = 'json:\"-\"')\n}\n\nstruct GoTags {\n 1: required string Foo (go.tag = 'json:\"-\" foo:\"bar\"')\n 2: optional string Bar (go.tag = 'bar:\"foo\"')\n 3: required string FooBar (go.tag = 'json:\"foobar,option1,option2\" bar:\"foo,option1\" foo:\"foobar\"')\n 4: required string FooBarWithSpace (go.tag = 'json:\"foobarWithSpace\" foo:\"foo bar foobar barfoo\"')\n 5: optional string FooBarWithOmitEmpty (go.tag = 'json:\"foobarWithOmitEmpty,omitempty\"')\n 6: required string FooBarWithRequired (go.tag = 'json:\"foobarWithRequired,required\"')\n}\n\nstruct NotOmitEmpty {\n 1: optional string NotOmitEmptyString (go.tag = 'json:\"notOmitEmptyString,!omitempty\"')\n 2: optional string NotOmitEmptyInt (go.tag = 'json:\"notOmitEmptyInt,!omitempty\"')\n 3: optional string NotOmitEmptyBool (go.tag = 'json:\"notOmitEmptyBool,!omitempty\"')\n 4: optional list NotOmitEmptyList (go.tag = 'json:\"notOmitEmptyList,!omitempty\"')\n 5: optional map NotOmitEmptyMap (go.tag = 'json:\"notOmitEmptyMap,!omitempty\"')\n 6: optional list NotOmitEmptyListMixedWithOmitEmpty (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmpty,!omitempty,omitempty\"')\n 7: optional list NotOmitEmptyListMixedWithOmitEmptyV2 (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmptyV2,omitempty,!omitempty\"')\n 8: optional string OmitEmptyString (go.tag = 'json:\"omitEmptyString,omitempty\"') // to test that there can be a mix of fields that do and don't have !omitempty\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Default values\n\nstruct DefaultsStruct {\n 1: required i32 requiredPrimitive = 100\n 2: optional i32 optionalPrimitive = 200\n\n 3: required enums.EnumDefault requiredEnum = enums.EnumDefault.Bar\n 4: optional enums.EnumDefault optionalEnum = 2\n\n 5: required list requiredList = [\"hello\", \"world\"]\n 6: optional list optionalList = [1, 2.0, 3]\n\n 7: required Frame requiredStruct = {\n \"topLeft\": {\"x\": 1, \"y\": 2},\n \"size\": {\"width\": 100, \"height\": 200},\n }\n 8: optional Edge optionalStruct = {\n \"startPoint\": {\"x\": 1, \"y\": 2},\n \"endPoint\": {\"x\": 3, \"y\": 4},\n }\n\n 9: required bool requiredBoolDefaultTrue = true\n 10: optional bool optionalBoolDefaultTrue = true\n\n 11: required bool requiredBoolDefaultFalse = false\n 12: optional bool optionalBoolDefaultFalse = false\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Opt-out of Zap\n\nstruct ZapOptOutStruct {\n 1: required string name\n 2: required string optout (go.nolog)\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Field jabels\n\nstruct StructLabels {\n // reserved keyword as label\n 1: optional bool isRequired (go.label = \"required\")\n\n // go.tag's JSON tag takes precedence over go.label\n 2: optional string foo (go.label = \"bar\", go.tag = 'json:\"not_bar\"')\n\n // Empty label\n 3: optional string qux (go.label = \"\")\n\n // All-caps label\n 4: optional string quux (go.label = \"QUUX\")\n}\n" diff --git a/gen/internal/tests/thrift/exceptions.thrift b/gen/internal/tests/thrift/exceptions.thrift index 725b1695..8238e60e 100644 --- a/gen/internal/tests/thrift/exceptions.thrift +++ b/gen/internal/tests/thrift/exceptions.thrift @@ -7,6 +7,7 @@ exception DoesNotExistException { /** Key that was missing. */ 1: required string key 2: optional string Error (go.name="Error2") + 3: optional string userName (go.pii) } exception Does_Not_Exist_Exception_Collision { diff --git a/gen/internal/tests/thrift/structs.thrift b/gen/internal/tests/thrift/structs.thrift index ad9cb028..2afa6cab 100644 --- a/gen/internal/tests/thrift/structs.thrift +++ b/gen/internal/tests/thrift/structs.thrift @@ -92,8 +92,6 @@ struct ContactInfo { struct PersonalInfo { 1: optional i32 age 2: optional string race (go.pii) - 3: optional string ssn (go.pii) - 4: optional string jobTitle } struct User { diff --git a/gen/zap_test.go b/gen/zap_test.go index ac95dd15..14fb9f39 100644 --- a/gen/zap_test.go +++ b/gen/zap_test.go @@ -872,21 +872,9 @@ func TestZapOptOut(t *testing.T) { spec *compile.FieldSpec want bool }{ - { - name: "no annotation", - spec: foo, - want: false, - }, - { - name: "pii annotation", - spec: pii, - want: true, - }, - { - name: "nolog annotation", - spec: nolog, - want: true, - }, + {name: "no annotation", spec: foo, want: false}, + {name: "pii annotation", spec: pii, want: true}, + {name: "nolog annotation", spec: nolog, want: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 11e60c8b0c4ed6e0cf899a4bf9c4d79b90f6dc25 Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Thu, 14 Mar 2024 14:39:03 -0700 Subject: [PATCH 5/8] Update and redact values in Stringer method --- CHANGELOG.md | 4 +- gen/field.go | 53 +++++++------- gen/field_test.go | 80 +++++---------------- gen/internal/tests/exceptions/exceptions.go | 14 ++-- gen/internal/tests/structs/structs.go | 14 ++-- gen/internal/tests/thrift/exceptions.thrift | 2 +- gen/internal/tests/thrift/structs.thrift | 2 +- gen/zap.go | 2 +- gen/zap_test.go | 8 +-- 9 files changed, 76 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4dd2d512..fc8f8b0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## [Unreleased] -- No changes yet. +## Added +- Redacted annotation provides a mechanism to redact certain struct fields from + logs. ## [1.31.0] - 2023-06-09 ### Changed diff --git a/gen/field.go b/gen/field.go index f2166f6d..f2faec09 100644 --- a/gen/field.go +++ b/gen/field.go @@ -694,24 +694,33 @@ func (f fieldGroupGenerator) String(g Generator) error { <$fields := newVar "fields"> <$i := newVar "i"> - <$sanitizedFields := scrubPII .Fields > - var <$fields> []string + var <$fields> []string <$i> := 0 - + <- $fname := goName . -> <- $f := printf "%s.%s" $v $fname -> + <- $requiresRedaction := requiresRedaction . -> + <- $redactedContent := redactedContent -> <- if not .Required -> if <$f> != nil { - - <$fields>[<$i>] = <$fmt>.Sprintf("<$fname>: %v", *(<$f>)) - <- else -> - <$fields>[<$i>] = <$fmt>.Sprintf("<$fname>: %v", <$f>) + <- if $requiresRedaction -> + <$fields>[<$i>] = "<$fname>: <$redactedContent>" + <- else > + + <$fields>[<$i>] = <$fmt>.Sprintf("<$fname>: %v", *(<$f>)) + <- else -> + <$fields>[<$i>] = <$fmt>.Sprintf("<$fname>: %v", <$f>) + <- end> <- end> <$i>++ } <- else -> - <$fields>[<$i>] = <$fmt>.Sprintf("<$fname>: %v", <$f>) + <- if $requiresRedaction -> + <$fields>[<$i>] = "<$fname>: <$redactedContent>" + <- else -> + <$fields>[<$i>] = <$fmt>.Sprintf("<$fname>: %v", <$f>) + <- end> <$i>++ <- end> @@ -719,7 +728,8 @@ func (f fieldGroupGenerator) String(g Generator) error { return <$fmt>.Sprintf("<.Name>{%v}", <$strings>.Join(<$fields>[:<$i>], ", ")) } `, f, - TemplateFunc("scrubPII", scrubPII), + TemplateFunc("requiresRedaction", requiresRedaction), + TemplateFunc("redactedContent", redactedContent), ) } @@ -879,27 +889,22 @@ func verifyUniqueFieldLabels(fs compile.FieldGroup) error { return nil } -// PIILabel provides a mechanism to excludes certain struct fields from -// Zap logs, and from the outputs of String() and Error() methods. +// RedactedLabel provides a mechanism to redact certain struct fields from +// the outputs of String() and Error() methods. // // struct Contact { // 1: required string name -// 2: required string email (go.pii) // will be omitted from Zap logs, Stringer(), and Error() methods. +// 2: required string email (go.redacted) // } -const PIILabel = "go.pii" +const RedactedLabel = "go.redacted" -func hasPIIAnnotation(spec *compile.FieldSpec) bool { - _, ok := spec.Annotations[PIILabel] +func requiresRedaction(spec *compile.FieldSpec) bool { + _, ok := spec.Annotations[RedactedLabel] return ok } -func scrubPII(fields compile.FieldGroup) compile.FieldGroup { - out := make(compile.FieldGroup, 0) - for _, field := range fields { - if hasPIIAnnotation(field) { - continue - } - out = append(out, field) - } - return out +const _redactedContent = "" + +func redactedContent() string { + return _redactedContent } diff --git a/gen/field_test.go b/gen/field_test.go index 0cba39f4..90779c52 100644 --- a/gen/field_test.go +++ b/gen/field_test.go @@ -140,103 +140,57 @@ func TestCompileJSONTag(t *testing.T) { } } -func TestScrubPII(t *testing.T) { +func TestHasRedactedAnnotation(t *testing.T) { foo := &compile.FieldSpec{ Name: "foo", } - pii := &compile.FieldSpec{ - Name: "pii", - Annotations: compile.Annotations{PIILabel: ""}, - } - tests := []struct { - name string - spec compile.FieldGroup - want compile.FieldGroup - }{ - { - name: "without pii annotation", - spec: compile.FieldGroup{ - foo, - foo, - }, - want: compile.FieldGroup{ - foo, - foo, - }, - }, - { - name: "with pii annotation", - spec: compile.FieldGroup{ - foo, - pii, - foo, - }, - want: compile.FieldGroup{ - foo, - foo, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equalf(t, tt.want, scrubPII(tt.spec), "scrubPII(%v)", tt.spec) - }) - } -} - -func TestHasPIIAnnotation(t *testing.T) { - foo := &compile.FieldSpec{ - Name: "foo", - } - pii := &compile.FieldSpec{ - Name: "pii", - Annotations: compile.Annotations{PIILabel: ""}, + redacted := &compile.FieldSpec{ + Name: "redacted", + Annotations: compile.Annotations{RedactedLabel: ""}, } tests := []struct { name string spec *compile.FieldSpec want bool }{ - {name: "pii annotation", spec: pii, want: true}, - {name: "no pii annotation", spec: foo, want: false}, + {name: "redacted annotation", spec: redacted, want: true}, + {name: "no redacted annotation", spec: foo, want: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equalf(t, tt.want, hasPIIAnnotation(tt.spec), "hasPIIAnnotation(%v)", tt.spec) + assert.Equalf(t, tt.want, requiresRedaction(tt.spec), "requiresRedaction(%v)", tt.spec) }) } } -func TestSanitizePII(t *testing.T) { +func TestSanitizeRedacted(t *testing.T) { age := int32(21) pi := ts.PersonalInfo{ Age: toPtr(age), - Race: toPtr("kryptonian"), + Race: toPtr("martian"), } - piiException := exceptions.DoesNotExistException{ + redactedException := exceptions.DoesNotExistException{ Key: "s", - UserName: toPtr("superman"), + UserName: toPtr("john doe"), } piEncoder := zapcore.NewMapObjectEncoder() require.NoError(t, pi.MarshalLogObject(piEncoder)) - piiExceptionEncoder := zapcore.NewMapObjectEncoder() - require.NoError(t, piiException.MarshalLogObject(piiExceptionEncoder)) + redactedExceptionEncoder := zapcore.NewMapObjectEncoder() + require.NoError(t, redactedException.MarshalLogObject(redactedExceptionEncoder)) tests := []struct { name string got any want any }{ - {name: "struct/zap", got: piEncoder.Fields, want: map[string]interface{}{"age": age}}, - {name: "struct/string", got: pi.String(), want: "PersonalInfo{Age: 21}"}, - {name: "exception/zap", got: piiExceptionEncoder.Fields, want: map[string]interface{}{"key": "s"}}, - {name: "exception/string", got: piiException.String(), want: "DoesNotExistException{Key: s}"}, - {name: "exception/error", got: piiException.Error(), want: "DoesNotExistException{Key: s}"}, + {name: "struct/string", got: pi.String(), want: "PersonalInfo{Age: 21, Race: }"}, + {name: "exception/string", got: redactedException.String(), want: "DoesNotExistException{Key: s, UserName: }"}, + {name: "exception/error", got: redactedException.Error(), want: "DoesNotExistException{Key: s, UserName: }"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, tt.got) + assert.EqualValues(t, tt.want, tt.got) }) } } diff --git a/gen/internal/tests/exceptions/exceptions.go b/gen/internal/tests/exceptions/exceptions.go index 855d3131..e1596a45 100644 --- a/gen/internal/tests/exceptions/exceptions.go +++ b/gen/internal/tests/exceptions/exceptions.go @@ -253,7 +253,7 @@ func (v *DoesNotExistException) String() string { return "" } - var fields [2]string + var fields [3]string i := 0 fields[i] = fmt.Sprintf("Key: %v", v.Key) i++ @@ -261,6 +261,10 @@ func (v *DoesNotExistException) String() string { fields[i] = fmt.Sprintf("Error2: %v", *(v.Error2)) i++ } + if v.UserName != nil { + fields[i] = "UserName: " + i++ + } return fmt.Sprintf("DoesNotExistException{%v}", strings.Join(fields[:i], ", ")) } @@ -314,7 +318,9 @@ func (v *DoesNotExistException) MarshalLogObject(enc zapcore.ObjectEncoder) (err if v.Error2 != nil { enc.AddString("Error", *v.Error2) } - + if v.UserName != nil { + enc.AddString("userName", *v.UserName) + } return err } @@ -797,8 +803,8 @@ var ThriftModule = &thriftreflect.ThriftModule{ Name: "exceptions", Package: "go.uber.org/thriftrw/gen/internal/tests/exceptions", FilePath: "exceptions.thrift", - SHA1: "79cea1006d14acc62a1e637626fd242b25ad7bd6", + SHA1: "95b2e50ab244e43227e9bd23ee272a48ee5a30b9", Raw: rawIDL, } -const rawIDL = "exception EmptyException {}\n\n/**\n * Raised when something doesn't exist.\n */\nexception DoesNotExistException {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n 3: optional string userName (go.pii)\n}\n\nexception Does_Not_Exist_Exception_Collision {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n} (go.name=\"DoesNotExistException2\")\n" +const rawIDL = "exception EmptyException {}\n\n/**\n * Raised when something doesn't exist.\n */\nexception DoesNotExistException {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n 3: optional string userName (go.redacted)\n}\n\nexception Does_Not_Exist_Exception_Collision {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n} (go.name=\"DoesNotExistException2\")\n" diff --git a/gen/internal/tests/structs/structs.go b/gen/internal/tests/structs/structs.go index 5aa4c7b4..14166d84 100644 --- a/gen/internal/tests/structs/structs.go +++ b/gen/internal/tests/structs/structs.go @@ -4945,12 +4945,16 @@ func (v *PersonalInfo) String() string { return "" } - var fields [1]string + var fields [2]string i := 0 if v.Age != nil { fields[i] = fmt.Sprintf("Age: %v", *(v.Age)) i++ } + if v.Race != nil { + fields[i] = "Race: " + i++ + } return fmt.Sprintf("PersonalInfo{%v}", strings.Join(fields[:i], ", ")) } @@ -4984,7 +4988,9 @@ func (v *PersonalInfo) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { if v.Age != nil { enc.AddInt32("age", *v.Age) } - + if v.Race != nil { + enc.AddString("race", *v.Race) + } return err } @@ -8354,11 +8360,11 @@ var ThriftModule = &thriftreflect.ThriftModule{ Name: "structs", Package: "go.uber.org/thriftrw/gen/internal/tests/structs", FilePath: "structs.thrift", - SHA1: "b19a941227d7418b94291c00368b54bc89d663ff", + SHA1: "ee6ae8d5b450805a663f2de96a8198bd4abc0940", Includes: []*thriftreflect.ThriftModule{ enums.ThriftModule, }, Raw: rawIDL, } -const rawIDL = "include \"./enums.thrift\"\n\nstruct EmptyStruct {}\n\n//////////////////////////////////////////////////////////////////////////////\n// Structs with primitives\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are required.\n */\nstruct PrimitiveRequiredStruct {\n 1: required bool boolField\n 2: required byte byteField\n 3: required i16 int16Field\n 4: required i32 int32Field\n 5: required i64 int64Field\n 6: required double doubleField\n 7: required string stringField\n 8: required binary binaryField\n}\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are optional.\n */\nstruct PrimitiveOptionalStruct {\n 1: optional bool boolField\n 2: optional byte byteField\n 3: optional i16 int16Field\n 4: optional i32 int32Field\n 5: optional i64 int64Field\n 6: optional double doubleField\n 7: optional string stringField\n 8: optional binary binaryField\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Required)\n\n/**\n * A point in 2D space.\n */\nstruct Point {\n 1: required double x\n 2: required double y\n}\n\n/**\n * Size of something.\n */\nstruct Size {\n /**\n * Width in pixels.\n */\n 1: required double width\n /** Height in pixels. */\n 2: required double height\n}\n\nstruct Frame {\n 1: required Point topLeft\n 2: required Size size\n}\n\nstruct Edge {\n 1: required Point startPoint\n 2: required Point endPoint\n}\n\n/**\n * A graph is comprised of zero or more edges.\n */\nstruct Graph {\n /**\n * List of edges in the graph.\n *\n * May be empty.\n */\n 1: required list edges\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Optional)\n\nstruct ContactInfo {\n 1: required string emailAddress\n}\n\nstruct PersonalInfo {\n 1: optional i32 age\n 2: optional string race (go.pii)\n}\n\nstruct User {\n 1: required string name\n 2: optional ContactInfo contact\n 3: optional PersonalInfo personal\n}\n\ntypedef map UserMap\n\n//////////////////////////////////////////////////////////////////////////////\n// self-referential struct\n\ntypedef Node List\n\n/**\n * Node is linked list of values.\n * All values are 32-bit integers.\n */\nstruct Node {\n 1: required i32 value\n 2: optional List tail\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// JSON tagged structs\n\nstruct Rename {\n 1: required string Default (go.tag = 'json:\"default\"')\n 2: required string camelCase (go.tag = 'json:\"snake_case\"')\n}\n\nstruct Omit {\n 1: required string serialized\n 2: required string hidden (go.tag = 'json:\"-\"')\n}\n\nstruct GoTags {\n 1: required string Foo (go.tag = 'json:\"-\" foo:\"bar\"')\n 2: optional string Bar (go.tag = 'bar:\"foo\"')\n 3: required string FooBar (go.tag = 'json:\"foobar,option1,option2\" bar:\"foo,option1\" foo:\"foobar\"')\n 4: required string FooBarWithSpace (go.tag = 'json:\"foobarWithSpace\" foo:\"foo bar foobar barfoo\"')\n 5: optional string FooBarWithOmitEmpty (go.tag = 'json:\"foobarWithOmitEmpty,omitempty\"')\n 6: required string FooBarWithRequired (go.tag = 'json:\"foobarWithRequired,required\"')\n}\n\nstruct NotOmitEmpty {\n 1: optional string NotOmitEmptyString (go.tag = 'json:\"notOmitEmptyString,!omitempty\"')\n 2: optional string NotOmitEmptyInt (go.tag = 'json:\"notOmitEmptyInt,!omitempty\"')\n 3: optional string NotOmitEmptyBool (go.tag = 'json:\"notOmitEmptyBool,!omitempty\"')\n 4: optional list NotOmitEmptyList (go.tag = 'json:\"notOmitEmptyList,!omitempty\"')\n 5: optional map NotOmitEmptyMap (go.tag = 'json:\"notOmitEmptyMap,!omitempty\"')\n 6: optional list NotOmitEmptyListMixedWithOmitEmpty (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmpty,!omitempty,omitempty\"')\n 7: optional list NotOmitEmptyListMixedWithOmitEmptyV2 (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmptyV2,omitempty,!omitempty\"')\n 8: optional string OmitEmptyString (go.tag = 'json:\"omitEmptyString,omitempty\"') // to test that there can be a mix of fields that do and don't have !omitempty\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Default values\n\nstruct DefaultsStruct {\n 1: required i32 requiredPrimitive = 100\n 2: optional i32 optionalPrimitive = 200\n\n 3: required enums.EnumDefault requiredEnum = enums.EnumDefault.Bar\n 4: optional enums.EnumDefault optionalEnum = 2\n\n 5: required list requiredList = [\"hello\", \"world\"]\n 6: optional list optionalList = [1, 2.0, 3]\n\n 7: required Frame requiredStruct = {\n \"topLeft\": {\"x\": 1, \"y\": 2},\n \"size\": {\"width\": 100, \"height\": 200},\n }\n 8: optional Edge optionalStruct = {\n \"startPoint\": {\"x\": 1, \"y\": 2},\n \"endPoint\": {\"x\": 3, \"y\": 4},\n }\n\n 9: required bool requiredBoolDefaultTrue = true\n 10: optional bool optionalBoolDefaultTrue = true\n\n 11: required bool requiredBoolDefaultFalse = false\n 12: optional bool optionalBoolDefaultFalse = false\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Opt-out of Zap\n\nstruct ZapOptOutStruct {\n 1: required string name\n 2: required string optout (go.nolog)\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Field jabels\n\nstruct StructLabels {\n // reserved keyword as label\n 1: optional bool isRequired (go.label = \"required\")\n\n // go.tag's JSON tag takes precedence over go.label\n 2: optional string foo (go.label = \"bar\", go.tag = 'json:\"not_bar\"')\n\n // Empty label\n 3: optional string qux (go.label = \"\")\n\n // All-caps label\n 4: optional string quux (go.label = \"QUUX\")\n}\n" +const rawIDL = "include \"./enums.thrift\"\n\nstruct EmptyStruct {}\n\n//////////////////////////////////////////////////////////////////////////////\n// Structs with primitives\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are required.\n */\nstruct PrimitiveRequiredStruct {\n 1: required bool boolField\n 2: required byte byteField\n 3: required i16 int16Field\n 4: required i32 int32Field\n 5: required i64 int64Field\n 6: required double doubleField\n 7: required string stringField\n 8: required binary binaryField\n}\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are optional.\n */\nstruct PrimitiveOptionalStruct {\n 1: optional bool boolField\n 2: optional byte byteField\n 3: optional i16 int16Field\n 4: optional i32 int32Field\n 5: optional i64 int64Field\n 6: optional double doubleField\n 7: optional string stringField\n 8: optional binary binaryField\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Required)\n\n/**\n * A point in 2D space.\n */\nstruct Point {\n 1: required double x\n 2: required double y\n}\n\n/**\n * Size of something.\n */\nstruct Size {\n /**\n * Width in pixels.\n */\n 1: required double width\n /** Height in pixels. */\n 2: required double height\n}\n\nstruct Frame {\n 1: required Point topLeft\n 2: required Size size\n}\n\nstruct Edge {\n 1: required Point startPoint\n 2: required Point endPoint\n}\n\n/**\n * A graph is comprised of zero or more edges.\n */\nstruct Graph {\n /**\n * List of edges in the graph.\n *\n * May be empty.\n */\n 1: required list edges\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Optional)\n\nstruct ContactInfo {\n 1: required string emailAddress\n}\n\nstruct PersonalInfo {\n 1: optional i32 age\n 2: optional string race (go.redacted)\n}\n\nstruct User {\n 1: required string name\n 2: optional ContactInfo contact\n 3: optional PersonalInfo personal\n}\n\ntypedef map UserMap\n\n//////////////////////////////////////////////////////////////////////////////\n// self-referential struct\n\ntypedef Node List\n\n/**\n * Node is linked list of values.\n * All values are 32-bit integers.\n */\nstruct Node {\n 1: required i32 value\n 2: optional List tail\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// JSON tagged structs\n\nstruct Rename {\n 1: required string Default (go.tag = 'json:\"default\"')\n 2: required string camelCase (go.tag = 'json:\"snake_case\"')\n}\n\nstruct Omit {\n 1: required string serialized\n 2: required string hidden (go.tag = 'json:\"-\"')\n}\n\nstruct GoTags {\n 1: required string Foo (go.tag = 'json:\"-\" foo:\"bar\"')\n 2: optional string Bar (go.tag = 'bar:\"foo\"')\n 3: required string FooBar (go.tag = 'json:\"foobar,option1,option2\" bar:\"foo,option1\" foo:\"foobar\"')\n 4: required string FooBarWithSpace (go.tag = 'json:\"foobarWithSpace\" foo:\"foo bar foobar barfoo\"')\n 5: optional string FooBarWithOmitEmpty (go.tag = 'json:\"foobarWithOmitEmpty,omitempty\"')\n 6: required string FooBarWithRequired (go.tag = 'json:\"foobarWithRequired,required\"')\n}\n\nstruct NotOmitEmpty {\n 1: optional string NotOmitEmptyString (go.tag = 'json:\"notOmitEmptyString,!omitempty\"')\n 2: optional string NotOmitEmptyInt (go.tag = 'json:\"notOmitEmptyInt,!omitempty\"')\n 3: optional string NotOmitEmptyBool (go.tag = 'json:\"notOmitEmptyBool,!omitempty\"')\n 4: optional list NotOmitEmptyList (go.tag = 'json:\"notOmitEmptyList,!omitempty\"')\n 5: optional map NotOmitEmptyMap (go.tag = 'json:\"notOmitEmptyMap,!omitempty\"')\n 6: optional list NotOmitEmptyListMixedWithOmitEmpty (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmpty,!omitempty,omitempty\"')\n 7: optional list NotOmitEmptyListMixedWithOmitEmptyV2 (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmptyV2,omitempty,!omitempty\"')\n 8: optional string OmitEmptyString (go.tag = 'json:\"omitEmptyString,omitempty\"') // to test that there can be a mix of fields that do and don't have !omitempty\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Default values\n\nstruct DefaultsStruct {\n 1: required i32 requiredPrimitive = 100\n 2: optional i32 optionalPrimitive = 200\n\n 3: required enums.EnumDefault requiredEnum = enums.EnumDefault.Bar\n 4: optional enums.EnumDefault optionalEnum = 2\n\n 5: required list requiredList = [\"hello\", \"world\"]\n 6: optional list optionalList = [1, 2.0, 3]\n\n 7: required Frame requiredStruct = {\n \"topLeft\": {\"x\": 1, \"y\": 2},\n \"size\": {\"width\": 100, \"height\": 200},\n }\n 8: optional Edge optionalStruct = {\n \"startPoint\": {\"x\": 1, \"y\": 2},\n \"endPoint\": {\"x\": 3, \"y\": 4},\n }\n\n 9: required bool requiredBoolDefaultTrue = true\n 10: optional bool optionalBoolDefaultTrue = true\n\n 11: required bool requiredBoolDefaultFalse = false\n 12: optional bool optionalBoolDefaultFalse = false\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Opt-out of Zap\n\nstruct ZapOptOutStruct {\n 1: required string name\n 2: required string optout (go.nolog)\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Field jabels\n\nstruct StructLabels {\n // reserved keyword as label\n 1: optional bool isRequired (go.label = \"required\")\n\n // go.tag's JSON tag takes precedence over go.label\n 2: optional string foo (go.label = \"bar\", go.tag = 'json:\"not_bar\"')\n\n // Empty label\n 3: optional string qux (go.label = \"\")\n\n // All-caps label\n 4: optional string quux (go.label = \"QUUX\")\n}\n" diff --git a/gen/internal/tests/thrift/exceptions.thrift b/gen/internal/tests/thrift/exceptions.thrift index 8238e60e..3494a0d5 100644 --- a/gen/internal/tests/thrift/exceptions.thrift +++ b/gen/internal/tests/thrift/exceptions.thrift @@ -7,7 +7,7 @@ exception DoesNotExistException { /** Key that was missing. */ 1: required string key 2: optional string Error (go.name="Error2") - 3: optional string userName (go.pii) + 3: optional string userName (go.redacted) } exception Does_Not_Exist_Exception_Collision { diff --git a/gen/internal/tests/thrift/structs.thrift b/gen/internal/tests/thrift/structs.thrift index 2afa6cab..53fa546a 100644 --- a/gen/internal/tests/thrift/structs.thrift +++ b/gen/internal/tests/thrift/structs.thrift @@ -91,7 +91,7 @@ struct ContactInfo { struct PersonalInfo { 1: optional i32 age - 2: optional string race (go.pii) + 2: optional string race (go.redacted) } struct User { diff --git a/gen/zap.go b/gen/zap.go index 3cad4f03..f00406f5 100644 --- a/gen/zap.go +++ b/gen/zap.go @@ -192,5 +192,5 @@ func (z *zapGenerator) zapEncodeEnd(spec compile.TypeSpec) string { func zapOptOut(spec *compile.FieldSpec) bool { _, ok := spec.Annotations[NoZapLabel] - return ok || hasPIIAnnotation(spec) + return ok } diff --git a/gen/zap_test.go b/gen/zap_test.go index 14fb9f39..007b8925 100644 --- a/gen/zap_test.go +++ b/gen/zap_test.go @@ -858,9 +858,9 @@ func TestZapOptOut(t *testing.T) { foo := &compile.FieldSpec{ Name: "foo", } - pii := &compile.FieldSpec{ - Name: "pii", - Annotations: compile.Annotations{PIILabel: ""}, + redacted := &compile.FieldSpec{ + Name: "redacted", + Annotations: compile.Annotations{RedactedLabel: ""}, } nolog := &compile.FieldSpec{ @@ -873,7 +873,7 @@ func TestZapOptOut(t *testing.T) { want bool }{ {name: "no annotation", spec: foo, want: false}, - {name: "pii annotation", spec: pii, want: true}, + {name: "redacted annotation", spec: redacted, want: false}, {name: "nolog annotation", spec: nolog, want: true}, } for _, tt := range tests { From 899cabfde231246ee680a8473b74e40ea5aaaa18 Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Fri, 15 Mar 2024 15:58:58 -0700 Subject: [PATCH 6/8] Add redacted values in marshalobject --- gen/field.go | 26 +++++++++++++++------ gen/field_test.go | 18 ++++++++++---- gen/internal/tests/exceptions/exceptions.go | 2 +- gen/internal/tests/structs/structs.go | 2 +- gen/zap_test.go | 6 ----- 5 files changed, 34 insertions(+), 20 deletions(-) diff --git a/gen/field.go b/gen/field.go index f2faec09..ce6629c4 100644 --- a/gen/field.go +++ b/gen/field.go @@ -792,18 +792,28 @@ func (f fieldGroupGenerator) Zap(g Generator) error { if <$v> == nil { return nil } + < $redactedContent := redactedContent -> + <- $requiresRedaction := requiresRedaction . -> <- if not (zapOptOut .) -> <- $fval := printf "%s.%s" $v (goName .) -> <- if .Required -> - - <$enc>.Add("", ) - <- zapEncodeEnd .Type> + <- if $requiresRedaction -> + <$enc>.AddString("", "<$redactedContent>") + <- else -> + <- zapEncodeBegin .Type -> + <$enc>.Add("", ) + <- zapEncodeEnd .Type -> + <- end -> <- else -> if <$fval> != nil { - - <$enc>.Add("", ) - <- zapEncodeEnd .Type> + <- if $requiresRedaction -> + <$enc>.AddString("", "<$redactedContent>") + <- else -> + <- zapEncodeBegin .Type -> + <$enc>.Add("", ) + <- zapEncodeEnd .Type -> + <- end > } <- end> <- end> @@ -813,6 +823,8 @@ func (f fieldGroupGenerator) Zap(g Generator) error { `, f, TemplateFunc("zapOptOut", zapOptOut), TemplateFunc("fieldLabel", entityLabel), + TemplateFunc("requiresRedaction", requiresRedaction), + TemplateFunc("redactedContent", redactedContent), ) } @@ -890,7 +902,7 @@ func verifyUniqueFieldLabels(fs compile.FieldGroup) error { } // RedactedLabel provides a mechanism to redact certain struct fields from -// the outputs of String() and Error() methods. +// the outputs of String(), Error() and MarshalLogObject() methods. // // struct Contact { // 1: required string name diff --git a/gen/field_test.go b/gen/field_test.go index 90779c52..837e7702 100644 --- a/gen/field_test.go +++ b/gen/field_test.go @@ -163,7 +163,7 @@ func TestHasRedactedAnnotation(t *testing.T) { } } -func TestSanitizeRedacted(t *testing.T) { +func TestRedactedAnnotation(t *testing.T) { age := int32(21) pi := ts.PersonalInfo{ Age: toPtr(age), @@ -173,11 +173,17 @@ func TestSanitizeRedacted(t *testing.T) { Key: "s", UserName: toPtr("john doe"), } - piEncoder := zapcore.NewMapObjectEncoder() - require.NoError(t, pi.MarshalLogObject(piEncoder)) + enc := zapcore.NewMapObjectEncoder() + require.NoError(t, pi.MarshalLogObject(enc)) + require.Len(t, enc.Fields, 2) + _, ok := enc.Fields["race"] + require.True(t, ok) - redactedExceptionEncoder := zapcore.NewMapObjectEncoder() - require.NoError(t, redactedException.MarshalLogObject(redactedExceptionEncoder)) + eEncoder := zapcore.NewMapObjectEncoder() + require.NoError(t, redactedException.MarshalLogObject(eEncoder)) + require.Len(t, eEncoder.Fields, 2) + _, ok = eEncoder.Fields["userName"] + require.True(t, ok) tests := []struct { name string @@ -185,8 +191,10 @@ func TestSanitizeRedacted(t *testing.T) { want any }{ {name: "struct/string", got: pi.String(), want: "PersonalInfo{Age: 21, Race: }"}, + {name: "struct/MarshalLogObject", got: enc.Fields["race"], want: _redactedContent}, {name: "exception/string", got: redactedException.String(), want: "DoesNotExistException{Key: s, UserName: }"}, {name: "exception/error", got: redactedException.Error(), want: "DoesNotExistException{Key: s, UserName: }"}, + {name: "exception/MarshalLogObject", got: eEncoder.Fields["userName"], want: _redactedContent}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/gen/internal/tests/exceptions/exceptions.go b/gen/internal/tests/exceptions/exceptions.go index e1596a45..bfe5beeb 100644 --- a/gen/internal/tests/exceptions/exceptions.go +++ b/gen/internal/tests/exceptions/exceptions.go @@ -319,7 +319,7 @@ func (v *DoesNotExistException) MarshalLogObject(enc zapcore.ObjectEncoder) (err enc.AddString("Error", *v.Error2) } if v.UserName != nil { - enc.AddString("userName", *v.UserName) + enc.AddString("userName", "") } return err } diff --git a/gen/internal/tests/structs/structs.go b/gen/internal/tests/structs/structs.go index 14166d84..1c22b6a9 100644 --- a/gen/internal/tests/structs/structs.go +++ b/gen/internal/tests/structs/structs.go @@ -4989,7 +4989,7 @@ func (v *PersonalInfo) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { enc.AddInt32("age", *v.Age) } if v.Race != nil { - enc.AddString("race", *v.Race) + enc.AddString("race", "") } return err } diff --git a/gen/zap_test.go b/gen/zap_test.go index 007b8925..a412aed7 100644 --- a/gen/zap_test.go +++ b/gen/zap_test.go @@ -858,11 +858,6 @@ func TestZapOptOut(t *testing.T) { foo := &compile.FieldSpec{ Name: "foo", } - redacted := &compile.FieldSpec{ - Name: "redacted", - Annotations: compile.Annotations{RedactedLabel: ""}, - } - nolog := &compile.FieldSpec{ Name: "nolog", Annotations: compile.Annotations{NoZapLabel: ""}, @@ -873,7 +868,6 @@ func TestZapOptOut(t *testing.T) { want bool }{ {name: "no annotation", spec: foo, want: false}, - {name: "redacted annotation", spec: redacted, want: false}, {name: "nolog annotation", spec: nolog, want: true}, } for _, tt := range tests { From 5e2e80c40dd7686eff5f2697a18963a4eba7efd7 Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Wed, 20 Mar 2024 10:47:38 -0700 Subject: [PATCH 7/8] Rename redact and add test in ZapOut --- CHANGELOG.md | 4 +-- gen/field.go | 30 ++++++++++----------- gen/field_test.go | 24 ++++++++--------- gen/internal/tests/exceptions/exceptions.go | 4 +-- gen/internal/tests/structs/structs.go | 4 +-- gen/internal/tests/thrift/exceptions.thrift | 2 +- gen/internal/tests/thrift/structs.thrift | 2 +- 7 files changed, 35 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc8f8b0a..9cb3d9b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] ## Added -- Redacted annotation provides a mechanism to redact certain struct fields from - logs. +- Redacted annotation provides a mechanism to redact certain struct fields from +errors messages and log objects. ## [1.31.0] - 2023-06-09 ### Changed diff --git a/gen/field.go b/gen/field.go index ce6629c4..2ef1c669 100644 --- a/gen/field.go +++ b/gen/field.go @@ -699,12 +699,12 @@ func (f fieldGroupGenerator) String(g Generator) error { <- $fname := goName . -> <- $f := printf "%s.%s" $v $fname -> - <- $requiresRedaction := requiresRedaction . -> + <- $shouldRedact := shouldRedact . -> <- $redactedContent := redactedContent -> <- if not .Required -> if <$f> != nil { - <- if $requiresRedaction -> + <- if $shouldRedact -> <$fields>[<$i>] = "<$fname>: <$redactedContent>" <- else > @@ -716,7 +716,7 @@ func (f fieldGroupGenerator) String(g Generator) error { <$i>++ } <- else -> - <- if $requiresRedaction -> + <- if $shouldRedact -> <$fields>[<$i>] = "<$fname>: <$redactedContent>" <- else -> <$fields>[<$i>] = <$fmt>.Sprintf("<$fname>: %v", <$f>) @@ -728,7 +728,7 @@ func (f fieldGroupGenerator) String(g Generator) error { return <$fmt>.Sprintf("<.Name>{%v}", <$strings>.Join(<$fields>[:<$i>], ", ")) } `, f, - TemplateFunc("requiresRedaction", requiresRedaction), + TemplateFunc("shouldRedact", shouldRedact), TemplateFunc("redactedContent", redactedContent), ) } @@ -794,11 +794,11 @@ func (f fieldGroupGenerator) Zap(g Generator) error { } < $redactedContent := redactedContent -> - <- $requiresRedaction := requiresRedaction . -> + <- $shouldRedact := shouldRedact . -> <- if not (zapOptOut .) -> <- $fval := printf "%s.%s" $v (goName .) -> <- if .Required -> - <- if $requiresRedaction -> + <- if $shouldRedact -> <$enc>.AddString("", "<$redactedContent>") <- else -> <- zapEncodeBegin .Type -> @@ -807,7 +807,7 @@ func (f fieldGroupGenerator) Zap(g Generator) error { <- end -> <- else -> if <$fval> != nil { - <- if $requiresRedaction -> + <- if $shouldRedact -> <$enc>.AddString("", "<$redactedContent>") <- else -> <- zapEncodeBegin .Type -> @@ -823,7 +823,7 @@ func (f fieldGroupGenerator) Zap(g Generator) error { `, f, TemplateFunc("zapOptOut", zapOptOut), TemplateFunc("fieldLabel", entityLabel), - TemplateFunc("requiresRedaction", requiresRedaction), + TemplateFunc("shouldRedact", shouldRedact), TemplateFunc("redactedContent", redactedContent), ) } @@ -901,22 +901,22 @@ func verifyUniqueFieldLabels(fs compile.FieldGroup) error { return nil } -// RedactedLabel provides a mechanism to redact certain struct fields from +// RedactLabel provides a mechanism to redact certain struct fields from // the outputs of String(), Error() and MarshalLogObject() methods. // // struct Contact { // 1: required string name -// 2: required string email (go.redacted) +// 2: required string email (go.redact) // } -const RedactedLabel = "go.redacted" +const RedactLabel = "go.redact" -func requiresRedaction(spec *compile.FieldSpec) bool { - _, ok := spec.Annotations[RedactedLabel] +func shouldRedact(spec *compile.FieldSpec) bool { + _, ok := spec.Annotations[RedactLabel] return ok } -const _redactedContent = "" +const _redactContent = "" func redactedContent() string { - return _redactedContent + return _redactContent } diff --git a/gen/field_test.go b/gen/field_test.go index 837e7702..7299b413 100644 --- a/gen/field_test.go +++ b/gen/field_test.go @@ -144,21 +144,21 @@ func TestHasRedactedAnnotation(t *testing.T) { foo := &compile.FieldSpec{ Name: "foo", } - redacted := &compile.FieldSpec{ - Name: "redacted", - Annotations: compile.Annotations{RedactedLabel: ""}, + redact := &compile.FieldSpec{ + Name: "redact", + Annotations: compile.Annotations{RedactLabel: ""}, } tests := []struct { name string spec *compile.FieldSpec want bool }{ - {name: "redacted annotation", spec: redacted, want: true}, - {name: "no redacted annotation", spec: foo, want: false}, + {name: "redact annotation", spec: redact, want: true}, + {name: "no redact annotation", spec: foo, want: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equalf(t, tt.want, requiresRedaction(tt.spec), "requiresRedaction(%v)", tt.spec) + assert.Equalf(t, tt.want, shouldRedact(tt.spec), "shouldRedact(%v)", tt.spec) }) } } @@ -169,7 +169,7 @@ func TestRedactedAnnotation(t *testing.T) { Age: toPtr(age), Race: toPtr("martian"), } - redactedException := exceptions.DoesNotExistException{ + redactException := exceptions.DoesNotExistException{ Key: "s", UserName: toPtr("john doe"), } @@ -180,7 +180,7 @@ func TestRedactedAnnotation(t *testing.T) { require.True(t, ok) eEncoder := zapcore.NewMapObjectEncoder() - require.NoError(t, redactedException.MarshalLogObject(eEncoder)) + require.NoError(t, redactException.MarshalLogObject(eEncoder)) require.Len(t, eEncoder.Fields, 2) _, ok = eEncoder.Fields["userName"] require.True(t, ok) @@ -191,10 +191,10 @@ func TestRedactedAnnotation(t *testing.T) { want any }{ {name: "struct/string", got: pi.String(), want: "PersonalInfo{Age: 21, Race: }"}, - {name: "struct/MarshalLogObject", got: enc.Fields["race"], want: _redactedContent}, - {name: "exception/string", got: redactedException.String(), want: "DoesNotExistException{Key: s, UserName: }"}, - {name: "exception/error", got: redactedException.Error(), want: "DoesNotExistException{Key: s, UserName: }"}, - {name: "exception/MarshalLogObject", got: eEncoder.Fields["userName"], want: _redactedContent}, + {name: "struct/MarshalLogObject", got: enc.Fields["race"], want: _redactContent}, + {name: "exception/string", got: redactException.String(), want: "DoesNotExistException{Key: s, UserName: }"}, + {name: "exception/error", got: redactException.Error(), want: "DoesNotExistException{Key: s, UserName: }"}, + {name: "exception/MarshalLogObject", got: eEncoder.Fields["userName"], want: _redactContent}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/gen/internal/tests/exceptions/exceptions.go b/gen/internal/tests/exceptions/exceptions.go index bfe5beeb..67b6b208 100644 --- a/gen/internal/tests/exceptions/exceptions.go +++ b/gen/internal/tests/exceptions/exceptions.go @@ -803,8 +803,8 @@ var ThriftModule = &thriftreflect.ThriftModule{ Name: "exceptions", Package: "go.uber.org/thriftrw/gen/internal/tests/exceptions", FilePath: "exceptions.thrift", - SHA1: "95b2e50ab244e43227e9bd23ee272a48ee5a30b9", + SHA1: "671449b355e9a5f64483f157e93dc762fe3d1944", Raw: rawIDL, } -const rawIDL = "exception EmptyException {}\n\n/**\n * Raised when something doesn't exist.\n */\nexception DoesNotExistException {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n 3: optional string userName (go.redacted)\n}\n\nexception Does_Not_Exist_Exception_Collision {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n} (go.name=\"DoesNotExistException2\")\n" +const rawIDL = "exception EmptyException {}\n\n/**\n * Raised when something doesn't exist.\n */\nexception DoesNotExistException {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n 3: optional string userName (go.redact)\n}\n\nexception Does_Not_Exist_Exception_Collision {\n /** Key that was missing. */\n 1: required string key\n 2: optional string Error (go.name=\"Error2\")\n} (go.name=\"DoesNotExistException2\")\n" diff --git a/gen/internal/tests/structs/structs.go b/gen/internal/tests/structs/structs.go index 1c22b6a9..6ca9c416 100644 --- a/gen/internal/tests/structs/structs.go +++ b/gen/internal/tests/structs/structs.go @@ -8360,11 +8360,11 @@ var ThriftModule = &thriftreflect.ThriftModule{ Name: "structs", Package: "go.uber.org/thriftrw/gen/internal/tests/structs", FilePath: "structs.thrift", - SHA1: "ee6ae8d5b450805a663f2de96a8198bd4abc0940", + SHA1: "29ff87a73c860add2d19995adbd0351cf24423c3", Includes: []*thriftreflect.ThriftModule{ enums.ThriftModule, }, Raw: rawIDL, } -const rawIDL = "include \"./enums.thrift\"\n\nstruct EmptyStruct {}\n\n//////////////////////////////////////////////////////////////////////////////\n// Structs with primitives\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are required.\n */\nstruct PrimitiveRequiredStruct {\n 1: required bool boolField\n 2: required byte byteField\n 3: required i16 int16Field\n 4: required i32 int32Field\n 5: required i64 int64Field\n 6: required double doubleField\n 7: required string stringField\n 8: required binary binaryField\n}\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are optional.\n */\nstruct PrimitiveOptionalStruct {\n 1: optional bool boolField\n 2: optional byte byteField\n 3: optional i16 int16Field\n 4: optional i32 int32Field\n 5: optional i64 int64Field\n 6: optional double doubleField\n 7: optional string stringField\n 8: optional binary binaryField\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Required)\n\n/**\n * A point in 2D space.\n */\nstruct Point {\n 1: required double x\n 2: required double y\n}\n\n/**\n * Size of something.\n */\nstruct Size {\n /**\n * Width in pixels.\n */\n 1: required double width\n /** Height in pixels. */\n 2: required double height\n}\n\nstruct Frame {\n 1: required Point topLeft\n 2: required Size size\n}\n\nstruct Edge {\n 1: required Point startPoint\n 2: required Point endPoint\n}\n\n/**\n * A graph is comprised of zero or more edges.\n */\nstruct Graph {\n /**\n * List of edges in the graph.\n *\n * May be empty.\n */\n 1: required list edges\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Optional)\n\nstruct ContactInfo {\n 1: required string emailAddress\n}\n\nstruct PersonalInfo {\n 1: optional i32 age\n 2: optional string race (go.redacted)\n}\n\nstruct User {\n 1: required string name\n 2: optional ContactInfo contact\n 3: optional PersonalInfo personal\n}\n\ntypedef map UserMap\n\n//////////////////////////////////////////////////////////////////////////////\n// self-referential struct\n\ntypedef Node List\n\n/**\n * Node is linked list of values.\n * All values are 32-bit integers.\n */\nstruct Node {\n 1: required i32 value\n 2: optional List tail\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// JSON tagged structs\n\nstruct Rename {\n 1: required string Default (go.tag = 'json:\"default\"')\n 2: required string camelCase (go.tag = 'json:\"snake_case\"')\n}\n\nstruct Omit {\n 1: required string serialized\n 2: required string hidden (go.tag = 'json:\"-\"')\n}\n\nstruct GoTags {\n 1: required string Foo (go.tag = 'json:\"-\" foo:\"bar\"')\n 2: optional string Bar (go.tag = 'bar:\"foo\"')\n 3: required string FooBar (go.tag = 'json:\"foobar,option1,option2\" bar:\"foo,option1\" foo:\"foobar\"')\n 4: required string FooBarWithSpace (go.tag = 'json:\"foobarWithSpace\" foo:\"foo bar foobar barfoo\"')\n 5: optional string FooBarWithOmitEmpty (go.tag = 'json:\"foobarWithOmitEmpty,omitempty\"')\n 6: required string FooBarWithRequired (go.tag = 'json:\"foobarWithRequired,required\"')\n}\n\nstruct NotOmitEmpty {\n 1: optional string NotOmitEmptyString (go.tag = 'json:\"notOmitEmptyString,!omitempty\"')\n 2: optional string NotOmitEmptyInt (go.tag = 'json:\"notOmitEmptyInt,!omitempty\"')\n 3: optional string NotOmitEmptyBool (go.tag = 'json:\"notOmitEmptyBool,!omitempty\"')\n 4: optional list NotOmitEmptyList (go.tag = 'json:\"notOmitEmptyList,!omitempty\"')\n 5: optional map NotOmitEmptyMap (go.tag = 'json:\"notOmitEmptyMap,!omitempty\"')\n 6: optional list NotOmitEmptyListMixedWithOmitEmpty (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmpty,!omitempty,omitempty\"')\n 7: optional list NotOmitEmptyListMixedWithOmitEmptyV2 (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmptyV2,omitempty,!omitempty\"')\n 8: optional string OmitEmptyString (go.tag = 'json:\"omitEmptyString,omitempty\"') // to test that there can be a mix of fields that do and don't have !omitempty\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Default values\n\nstruct DefaultsStruct {\n 1: required i32 requiredPrimitive = 100\n 2: optional i32 optionalPrimitive = 200\n\n 3: required enums.EnumDefault requiredEnum = enums.EnumDefault.Bar\n 4: optional enums.EnumDefault optionalEnum = 2\n\n 5: required list requiredList = [\"hello\", \"world\"]\n 6: optional list optionalList = [1, 2.0, 3]\n\n 7: required Frame requiredStruct = {\n \"topLeft\": {\"x\": 1, \"y\": 2},\n \"size\": {\"width\": 100, \"height\": 200},\n }\n 8: optional Edge optionalStruct = {\n \"startPoint\": {\"x\": 1, \"y\": 2},\n \"endPoint\": {\"x\": 3, \"y\": 4},\n }\n\n 9: required bool requiredBoolDefaultTrue = true\n 10: optional bool optionalBoolDefaultTrue = true\n\n 11: required bool requiredBoolDefaultFalse = false\n 12: optional bool optionalBoolDefaultFalse = false\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Opt-out of Zap\n\nstruct ZapOptOutStruct {\n 1: required string name\n 2: required string optout (go.nolog)\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Field jabels\n\nstruct StructLabels {\n // reserved keyword as label\n 1: optional bool isRequired (go.label = \"required\")\n\n // go.tag's JSON tag takes precedence over go.label\n 2: optional string foo (go.label = \"bar\", go.tag = 'json:\"not_bar\"')\n\n // Empty label\n 3: optional string qux (go.label = \"\")\n\n // All-caps label\n 4: optional string quux (go.label = \"QUUX\")\n}\n" +const rawIDL = "include \"./enums.thrift\"\n\nstruct EmptyStruct {}\n\n//////////////////////////////////////////////////////////////////////////////\n// Structs with primitives\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are required.\n */\nstruct PrimitiveRequiredStruct {\n 1: required bool boolField\n 2: required byte byteField\n 3: required i16 int16Field\n 4: required i32 int32Field\n 5: required i64 int64Field\n 6: required double doubleField\n 7: required string stringField\n 8: required binary binaryField\n}\n\n/**\n * A struct that contains primitive fields exclusively.\n *\n * All fields are optional.\n */\nstruct PrimitiveOptionalStruct {\n 1: optional bool boolField\n 2: optional byte byteField\n 3: optional i16 int16Field\n 4: optional i32 int32Field\n 5: optional i64 int64Field\n 6: optional double doubleField\n 7: optional string stringField\n 8: optional binary binaryField\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Required)\n\n/**\n * A point in 2D space.\n */\nstruct Point {\n 1: required double x\n 2: required double y\n}\n\n/**\n * Size of something.\n */\nstruct Size {\n /**\n * Width in pixels.\n */\n 1: required double width\n /** Height in pixels. */\n 2: required double height\n}\n\nstruct Frame {\n 1: required Point topLeft\n 2: required Size size\n}\n\nstruct Edge {\n 1: required Point startPoint\n 2: required Point endPoint\n}\n\n/**\n * A graph is comprised of zero or more edges.\n */\nstruct Graph {\n /**\n * List of edges in the graph.\n *\n * May be empty.\n */\n 1: required list edges\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Nested structs (Optional)\n\nstruct ContactInfo {\n 1: required string emailAddress\n}\n\nstruct PersonalInfo {\n 1: optional i32 age\n 2: optional string race (go.redact)\n}\n\nstruct User {\n 1: required string name\n 2: optional ContactInfo contact\n 3: optional PersonalInfo personal\n}\n\ntypedef map UserMap\n\n//////////////////////////////////////////////////////////////////////////////\n// self-referential struct\n\ntypedef Node List\n\n/**\n * Node is linked list of values.\n * All values are 32-bit integers.\n */\nstruct Node {\n 1: required i32 value\n 2: optional List tail\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// JSON tagged structs\n\nstruct Rename {\n 1: required string Default (go.tag = 'json:\"default\"')\n 2: required string camelCase (go.tag = 'json:\"snake_case\"')\n}\n\nstruct Omit {\n 1: required string serialized\n 2: required string hidden (go.tag = 'json:\"-\"')\n}\n\nstruct GoTags {\n 1: required string Foo (go.tag = 'json:\"-\" foo:\"bar\"')\n 2: optional string Bar (go.tag = 'bar:\"foo\"')\n 3: required string FooBar (go.tag = 'json:\"foobar,option1,option2\" bar:\"foo,option1\" foo:\"foobar\"')\n 4: required string FooBarWithSpace (go.tag = 'json:\"foobarWithSpace\" foo:\"foo bar foobar barfoo\"')\n 5: optional string FooBarWithOmitEmpty (go.tag = 'json:\"foobarWithOmitEmpty,omitempty\"')\n 6: required string FooBarWithRequired (go.tag = 'json:\"foobarWithRequired,required\"')\n}\n\nstruct NotOmitEmpty {\n 1: optional string NotOmitEmptyString (go.tag = 'json:\"notOmitEmptyString,!omitempty\"')\n 2: optional string NotOmitEmptyInt (go.tag = 'json:\"notOmitEmptyInt,!omitempty\"')\n 3: optional string NotOmitEmptyBool (go.tag = 'json:\"notOmitEmptyBool,!omitempty\"')\n 4: optional list NotOmitEmptyList (go.tag = 'json:\"notOmitEmptyList,!omitempty\"')\n 5: optional map NotOmitEmptyMap (go.tag = 'json:\"notOmitEmptyMap,!omitempty\"')\n 6: optional list NotOmitEmptyListMixedWithOmitEmpty (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmpty,!omitempty,omitempty\"')\n 7: optional list NotOmitEmptyListMixedWithOmitEmptyV2 (go.tag = 'json:\"notOmitEmptyListMixedWithOmitEmptyV2,omitempty,!omitempty\"')\n 8: optional string OmitEmptyString (go.tag = 'json:\"omitEmptyString,omitempty\"') // to test that there can be a mix of fields that do and don't have !omitempty\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Default values\n\nstruct DefaultsStruct {\n 1: required i32 requiredPrimitive = 100\n 2: optional i32 optionalPrimitive = 200\n\n 3: required enums.EnumDefault requiredEnum = enums.EnumDefault.Bar\n 4: optional enums.EnumDefault optionalEnum = 2\n\n 5: required list requiredList = [\"hello\", \"world\"]\n 6: optional list optionalList = [1, 2.0, 3]\n\n 7: required Frame requiredStruct = {\n \"topLeft\": {\"x\": 1, \"y\": 2},\n \"size\": {\"width\": 100, \"height\": 200},\n }\n 8: optional Edge optionalStruct = {\n \"startPoint\": {\"x\": 1, \"y\": 2},\n \"endPoint\": {\"x\": 3, \"y\": 4},\n }\n\n 9: required bool requiredBoolDefaultTrue = true\n 10: optional bool optionalBoolDefaultTrue = true\n\n 11: required bool requiredBoolDefaultFalse = false\n 12: optional bool optionalBoolDefaultFalse = false\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Opt-out of Zap\n\nstruct ZapOptOutStruct {\n 1: required string name\n 2: required string optout (go.nolog)\n}\n\n//////////////////////////////////////////////////////////////////////////////\n// Field jabels\n\nstruct StructLabels {\n // reserved keyword as label\n 1: optional bool isRequired (go.label = \"required\")\n\n // go.tag's JSON tag takes precedence over go.label\n 2: optional string foo (go.label = \"bar\", go.tag = 'json:\"not_bar\"')\n\n // Empty label\n 3: optional string qux (go.label = \"\")\n\n // All-caps label\n 4: optional string quux (go.label = \"QUUX\")\n}\n" diff --git a/gen/internal/tests/thrift/exceptions.thrift b/gen/internal/tests/thrift/exceptions.thrift index 3494a0d5..23ae55b8 100644 --- a/gen/internal/tests/thrift/exceptions.thrift +++ b/gen/internal/tests/thrift/exceptions.thrift @@ -7,7 +7,7 @@ exception DoesNotExistException { /** Key that was missing. */ 1: required string key 2: optional string Error (go.name="Error2") - 3: optional string userName (go.redacted) + 3: optional string userName (go.redact) } exception Does_Not_Exist_Exception_Collision { diff --git a/gen/internal/tests/thrift/structs.thrift b/gen/internal/tests/thrift/structs.thrift index 53fa546a..ef08a3f1 100644 --- a/gen/internal/tests/thrift/structs.thrift +++ b/gen/internal/tests/thrift/structs.thrift @@ -91,7 +91,7 @@ struct ContactInfo { struct PersonalInfo { 1: optional i32 age - 2: optional string race (go.redacted) + 2: optional string race (go.redact) } struct User { From 6296da84d8773e893304560172506f3f28948a6f Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Wed, 20 Mar 2024 10:48:03 -0700 Subject: [PATCH 8/8] Add test case in ZapOut --- gen/zap_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gen/zap_test.go b/gen/zap_test.go index a412aed7..1d96ecc2 100644 --- a/gen/zap_test.go +++ b/gen/zap_test.go @@ -858,6 +858,10 @@ func TestZapOptOut(t *testing.T) { foo := &compile.FieldSpec{ Name: "foo", } + redact := &compile.FieldSpec{ + Name: "redact", + Annotations: compile.Annotations{RedactLabel: ""}, + } nolog := &compile.FieldSpec{ Name: "nolog", Annotations: compile.Annotations{NoZapLabel: ""}, @@ -868,6 +872,7 @@ func TestZapOptOut(t *testing.T) { want bool }{ {name: "no annotation", spec: foo, want: false}, + {name: "redact annotation", spec: redact, want: false}, {name: "nolog annotation", spec: nolog, want: true}, } for _, tt := range tests {