Skip to content

Commit 85490b7

Browse files
knzknz
committed
Make New() and Wrap() consider their string as PII-free
**THIS IS A BREAKING CHANGE.** The `errors.New()`, `errors.NewWithDepth()`, `errors.Wrap()` and `errors.WrapWithDepth()` now consider their string argument as PII-free, and the string is now also included in Sentry reports. This is a breaking change: in previous versions, only the format argument to `errors.Newf`, `errors.Wrapf` etc was considered to be PII-free. This also means that **care must be taken when using this `errors` library as drop-in replacement for other errors libraries**: callers that use `New()` and `Wrap()` directly must be audited to ensure that they indeed pass strings that are safe for reporting to Sentry. This can be enforced e.g. via linters that assert that the argument is a literal (constant) string. This is the approach taken e.g. in CockroachDB.
1 parent 0c18419 commit 85490b7

File tree

3 files changed

+45
-17
lines changed

3 files changed

+45
-17
lines changed

‎README.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ older version of the package.
7575

7676
| Error detail | `Error()` and format `%s`/`%q`/`%v` | format `%+v` | `GetSafeDetails()` | Sentry report via `ReportError()` |
7777
|-----------------------------------------------------------------|-------------------------------------|--------------|-------------------------------|-----------------------------------|
78-
| main message, eg `New()` | visible | visible | redacted | redacted |
79-
| wrap prefix, eg `WithMessage()` | visible (as prefix) | visible | redacted | redacted |
78+
| main message, eg `New()` | visible | visible | yes (CHANGED IN v1.6) | full (CHANGED IN v1.6) |
79+
| wrap prefix, eg `WithMessage()` | visible (as prefix) | visible | yes (CHANGED IN v1.6) | full (CHANGED IN v1.6) |
8080
| stack trace, eg `WithStack()` | not visible | simplified | yes | full |
8181
| hint , eg `WithHint()` | not visible | visible | no | type only |
8282
| detail, eg `WithDetail()` | not visible | visible | no | type only |
@@ -98,7 +98,7 @@ method.
9898
- `New(string) error`, `Newf(string, ...interface{}) error`, `Errorf(string, ...interface{}) error`: leaf errors with message
9999
- **when to use: common error cases.**
100100
- what it does: also captures the stack trace at point of call and redacts the provided message for safe reporting.
101-
- how to access the detail: `Error()`, regular Go formatting. Details redacted in Sentry report.
101+
- how to access the detail: `Error()`, regular Go formatting. **Details in Sentry report.**
102102
- see also: Section [Error composition](#Error-composition-summary) below. `errors.NewWithDepth()` variants to customize at which call depth the stack trace is captured.
103103

104104
- `AssertionFailedf(string, ...interface{}) error`, `NewAssertionFailureWithWrappedErrf(error, string, ...interface{}) error`: signals an assertion failure / programming error.
@@ -141,7 +141,7 @@ return errors.Wrap(foo(), "foo")
141141
- `Wrap(error, string) error`, `Wrapf(error, string, ...interface{}) error`:
142142
- **when to use: on error return paths.**
143143
- what it does: combines `WithMessage()`, `WithStack()`, `WithSafeDetails()`.
144-
- how to access the details: `Error()`, regular Go formatting. Details redacted in Sentry report.
144+
- how to access the details: `Error()`, regular Go formatting. **Details in Sentry report.**
145145
- see also: Section [Error composition](#Error-composition-summary) below. `WrapWithDepth()` variants to customize at which depth the stack trace is captured.
146146

147147
- `WithSecondaryError(error, error) error`: annotate an error with a secondary error.

‎errutil/fmt_wrap_test.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,17 +52,19 @@ func TestErrorWrap(t *testing.T) {
5252
| github.com/cockroachdb/errors/errutil_test.TestErrorWrap
5353
| <tab><path>
5454
| [...repeated from below...]
55-
Wraps: (2) woo
56-
Wraps: (3) hello
57-
Wraps: (4) attached stack trace
55+
Wraps: (2) 1 safe detail enclosed
56+
Wraps: (3) woo
57+
Wraps: (4) hello
58+
Wraps: (5) attached stack trace
5859
| github.com/cockroachdb/errors/errutil_test.TestErrorWrap
5960
| <tab><path>
6061
| testing.tRunner
6162
| <tab><path>
6263
| runtime.goexit
6364
| <tab><path>
64-
Wraps: (5) world
65-
Error types: (1) *withstack.withStack (2) *errutil.withMessage (3) *fmt.wrapError (4) *withstack.withStack (5) *errors.errorString`},
65+
Wraps: (6) 1 safe detail enclosed
66+
Wraps: (7) world
67+
Error types: (1) *withstack.withStack (2) *safedetails.withSafeDetails (3) *errutil.withMessage (4) *fmt.wrapError (5) *withstack.withStack (6) *safedetails.withSafeDetails (7) *errors.errorString`},
6668

6769
{"new wrap err",
6870
errutil.Newf("hello: %w", baseErr),
@@ -81,8 +83,9 @@ Wraps: (4) attached stack trace
8183
| <tab><path>
8284
| runtime.goexit
8385
| <tab><path>
84-
Wraps: (5) world
85-
Error types: (1) *withstack.withStack (2) *safedetails.withSafeDetails (3) *fmt.wrapError (4) *withstack.withStack (5) *errors.errorString`},
86+
Wraps: (5) 1 safe detail enclosed
87+
Wraps: (6) world
88+
Error types: (1) *withstack.withStack (2) *safedetails.withSafeDetails (3) *fmt.wrapError (4) *withstack.withStack (5) *safedetails.withSafeDetails (6) *errors.errorString`},
8689
}
8790

8891
for _, test := range testCases {

‎errutil/utilities.go‎

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,16 @@ import (
2525
// New creates an error with a simple error message.
2626
// A stack trace is retained.
2727
//
28+
// Note: the message string is assumed to not contain
29+
// PII and is included in Sentry reports.
30+
// Use errors.Newf("%s", <unsafestring>) for errors
31+
// strings that may contain PII information.
32+
//
2833
// Detail output:
2934
// - message via `Error()` and formatting using `%v`/`%s`/`%q`.
3035
// - everything when formatting with `%+v`.
31-
// - stack trace (not message) via `errors.GetSafeDetails()`.
32-
// - stack trace (not message) in Sentry reports.
36+
// - stack trace and message via `errors.GetSafeDetails()`.
37+
// - stack trace and message in Sentry reports.
3338
func New(msg string) error {
3439
return NewWithDepth(1, msg)
3540
}
@@ -39,12 +44,21 @@ func New(msg string) error {
3944
// See the doc of `New()` for more details.
4045
func NewWithDepth(depth int, msg string) error {
4146
err := goErr.New(msg)
47+
if len(msg) > 0 {
48+
err = safedetails.WithSafeDetails(err, msg)
49+
}
4250
err = withstack.WithStackDepth(err, 1+depth)
4351
return err
4452
}
4553

4654
// Newf creates an error with a formatted error message.
4755
// A stack trace is retained.
56+
//
57+
// Note: the format string is assumed to not contain
58+
// PII and is included in Sentry reports.
59+
// Use errors.Newf("%s", <unsafestring>) for errors
60+
// strings that may contain PII information.
61+
//
4862
// See the doc of `New()` for more details.
4963
func Newf(format string, args ...interface{}) error {
5064
return NewWithDepthf(1, format, args...)
@@ -65,11 +79,16 @@ func NewWithDepthf(depth int, format string, args ...interface{}) error {
6579
// Wrap wraps an error with a message prefix.
6680
// A stack trace is retained.
6781
//
82+
// Note: the prefix string is assumed to not contain
83+
// PII and is included in Sentry reports.
84+
// Use errors.Wrapf(err, "%s", <unsafestring>) for errors
85+
// strings that may contain PII information.
86+
//
6887
// Detail output:
6988
// - original error message + prefix via `Error()` and formatting using `%v`/`%s`/`%q`.
7089
// - everything when formatting with `%+v`.
71-
// - stack trace (not message) via `errors.GetSafeDetails()`.
72-
// - stack trace (not message) in Sentry reports.
90+
// - stack trace and message via `errors.GetSafeDetails()`.
91+
// - stack trace and message in Sentry reports.
7392
func Wrap(err error, msg string) error {
7493
return WrapWithDepth(1, err, msg)
7594
}
@@ -80,6 +99,7 @@ func Wrap(err error, msg string) error {
8099
func WrapWithDepth(depth int, err error, msg string) error {
81100
if msg != "" {
82101
err = WithMessage(err, msg)
102+
err = safedetails.WithSafeDetails(err, msg)
83103
}
84104
err = withstack.WithStackDepth(err, depth+1)
85105
return err
@@ -89,11 +109,16 @@ func WrapWithDepth(depth int, err error, msg string) error {
89109
// trace is also retained. If the format is empty, no prefix is added,
90110
// but the extra arguments are still processed for reportable strings.
91111
//
112+
// Note: the format string is assumed to not contain
113+
// PII and is included in Sentry reports.
114+
// Use errors.Wrapf(err, "%s", <unsafestring>) for errors
115+
// strings that may contain PII information.
116+
//
92117
// Detail output:
93118
// - original error message + prefix via `Error()` and formatting using `%v`/`%s`/`%q`.
94119
// - everything when formatting with `%+v`.
95-
// - stack trace (not message) and redacted details via `errors.GetSafeDetails()`.
96-
// - stack trace (not message) and redacted details in Sentry reports.
120+
// - stack trace, format, and redacted details via `errors.GetSafeDetails()`.
121+
// - stack trace, format, and redacted details in Sentry reports.
97122
func Wrapf(err error, format string, args ...interface{}) error {
98123
return WrapWithDepthf(1, err, format, args...)
99124
}

0 commit comments

Comments
 (0)