thriftrw / thriftrw-go

A Thrift encoding code generator and library for Go
MIT License
101 stars 53 forks source link

Add: the ability to redact values from Stringer and Error interface. #592

Closed moisesvega closed 8 months ago

moisesvega commented 9 months ago

Currently, there's no feature to exclude sensitive information from outputs generated by the Stringer and Error interface implementations. However, we use the nolog annotation to exclude specific properties from being logged by the zap.objectMarshal method. This is particularly useful for omitting sensitive data in zap logger outputs. Despite this, there are scenarios where sensitive information might still be exposed. For instance, consider the following example:

exception DoesNotExistException {
   /** Key that was missing. */
   1: required string key
   2: optional string Error (go.name="Error2")
   3: optional string userName (go.nolog)
}

The code snippet above leads to the following implementation for the MarshalLogObject method:

// MarshalLogObject allows DoesNotExistException to be logged quickly by zap.
func (v *DoesNotExistException) MarshalLogObject(enc zapcore.ObjectEncoder) error {
   if v == nil {
      return nil
   }
   enc.AddString("key", v.Key)
   if v.Error2 != nil {
      enc.AddString("Error", *v.Error2)
   }
   return nil
}

However, our implementation of the Stringer and Error interfaces inadvertently exposes properties marked with nolog:

func (v *DoesNotExistException) Error() string {
   return v.String()
}

// String provides a human-readable representation of DoesNotExistException.
func (v *DoesNotExistException) String() string {
   if v == nil {
      return "<nil>"
   }

   var fields [3]string
   i := 0
   fields[i] = fmt.Sprintf("Key: %v", v.Key); i++
   if v.Error2 != nil {
      fields[i] = fmt.Sprintf("Error2: %v", *v.Error2); i++
   }
   if v.UserName != nil {
      fields[i] = fmt.Sprintf("UserName: %v", *v.UserName); i++
   }

   return fmt.Sprintf("DoesNotExistException{%s}", strings.Join(fields[:i], ", "))
}

This behavior can lead to unintended exposure of information annotated as nolog. For example, when this exception is utilized as an Error, or when logged using zap.Stringer("exception", e) or zap.Error(e) , the nolog-annotated property is inadvertently revealed:

playground link

func main() {
   name := "Jane Doe"
   e := &DoesNotExistException{
      Key:      "",
      Error2:   nil,
      UserName: &name,
   }
   logger, _ := zap.NewProduction()
   defer logger.Sync() // Ensures buffer is flushed
   logger.Info("hello",
      zap.Object("exception", e),
      zap.Error(e),
   )
     // output: {"level":"info","msg":"hello","exception":{"key":""},"error":"DoesNotExistException{Key: , UserName: Jane Doe"}
}

This PR introduces a new feature that enables the redaction of specified properties through a new Go annotation, go.redacted. When applied, this annotation ensures that the actual value of a property is replaced with in the outputs generated by our Stringer and Error interface implementations.

Output after this diff:

  // output: {"level":"info","msg":"hello","exception":{"key":""},"error":"DoesNotExistException{Key: , UserName: <redacted>}"}
codecov[bot] commented 9 months ago

Codecov Report

Attention: Patch coverage is 76.27119% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 69.03%. Comparing base (12ffd74) to head (6296da8).

Files Patch % Lines
gen/internal/tests/exceptions/exceptions.go 64.10% 7 Missing and 7 partials :warning:
gen/internal/tests/structs/structs.go 64.10% 7 Missing and 7 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #592 +/- ## ========================================== + Coverage 69.01% 69.03% +0.01% ========================================== Files 140 140 Lines 23523 23625 +102 ========================================== + Hits 16235 16309 +74 - Misses 4228 4242 +14 - Partials 3060 3074 +14 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.