Remove local userpaths from error reporting #78
Conversation
src/lib/sanitize-for-logging.ts
Outdated
| // Matches expected user path on macOS | ||
| if ( typeof path === 'string' && /\/Users\/[^/]+\/Library/.test( path ) ) { | ||
| return path.replace( /\/Users\/[^/]+(\/Library)/, '~$1' ); | ||
| } |
There was a problem hiding this comment.
Currently, the userpath pattern matcher matches /Users/{username}/../.., which is the expected userpath on macOS (presently, the only OS that has been released).
When support for other operating systems is added, this userpath matcher can be expanded to cover the new OS userpath patterns. Also noting that this util will return the original string if not matching the pattern.
Open to feedback if we want to include other OS userpath matchers now, however (Windows, Linux).
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
The test plan succeeded for me. Thank you for address this. 🙇🏻
I note that logged server details include usernames in the projectPath and wpContentPath attributes. However, I was unable to find an instance of those values in Sentry. So, we may not need to scrub those. I'm not sure. WDYT?
Line 86 in c545108
src/lib/sanitize-for-logging.ts
Outdated
| export function sanitizeUserpath( path: string ): string { | ||
| const userPathRegex = /^\/Users\/([^/]+)\//; | ||
| if ( typeof path === 'string' && userPathRegex.test( path ) ) { | ||
| return path.replace( userPathRegex, '~/' ); |
There was a problem hiding this comment.
I think replacing with a tilde is sensible.
I wondered if we should instead rely upon the existing practice of inserting REDACTED instead, e.g. /Users/REDACTED/. That might more clearly communicate an action was taken on the string.
Also, when we support other OSs, I supposed need to update it to use the equivalent for macOS' tilde usage — e.g., my understanding is that Windows might use C:/.
Not necessarily requesting a change. Merely sharing thoughts.
src/lib/sanitize-for-logging.ts
Outdated
| // Returns the original path if it does not match the pattern or if it is not a string. | ||
| export function sanitizeUserpath( path: string ): string { | ||
| const userPathRegex = /^\/Users\/([^/]+)\//; | ||
| if ( typeof path === 'string' && userPathRegex.test( path ) ) { |
There was a problem hiding this comment.
Using both test and replace may be unnecessary. If replace doesn't find a pattern match, it would have no impact. Are we using test to guard against something in particular?
| if ( typeof path === 'string' && userPathRegex.test( path ) ) { | |
| if ( typeof path === 'string' ) { |
There was a problem hiding this comment.
Also, I suppose our usage of TypeScript should, in theory, negate a need to check with typeof, correct?
There was a problem hiding this comment.
Initially, I had this regex matcher as part of sanitizeForLogging function itself, but decided it was too convoluted to accept both key/value objects and strings. Using a dedicated function for sanitizeUserpath is more readable, and these extra typechecks are no longer needed. Updated below, and thanks for catching.
@dcalhoun Thanks for the feedback - I think these are good suggestions. Given that Windows support is under active development and release is intended to be pending soon-ish, I think it's a good idea to go ahead and accommodate Windows and Unix together in this PR. The test cases probably make the intent clearer, but I've made the following changes:
Let me know if you have further thoughts on this approach. |
I looked at these as well, and my understanding is that the filtering here is handled either by sanitizeForLogging, or by sanitizeUnstructuredData. Initially, I attempted to leverage either of these functions to also handle the regex matching for the userpath, but ultimately decided that |
katinthehatsite
left a comment
katinthehatsite
left a comment
There was a problem hiding this comment.
The testing steps succeeded for me. Here is what I did:
- First, I tested on trunk to confirm the issue - the userpath was not redacted there:
-
Next, I switched to the branch and observed that the userpath is not redacted either since I am running the path in the development mode.
-
I removed the following condition:
if ( process.env.NODE_ENV !== 'development' ) {
And then ran nvm use && npm install && npm start and the userpath was redacted as expected:
The changes seem to work on my end on macOs. Please note that I have not tested this on Windows so I am only confirming the functionality for macOS.
I don't have a very strong preference for this one. Actually, I like seeing the path when the app is in the dev mode in case I need to locate something quickly e.g. |
Resolves Automattic/dotcom-forge#6701
Proposed Changes
Prevents local userpaths from being logged to error reporting.
sanitizeUserpathto sanitize-for-logging utilitiesconsole.logevents to remove matching userpaths from logged eventsCurrently, the userpath pattern matcher matches
/Users/username/Library/..., which is the expected userpath on macOS. When release support for other operating systems is added, this userpath matcher can be expanded to cover the new OS userpath patterns.Testing Instructions
To test, observe that usernames are not included in userpaths when logging user-data functions:
Sentry
These functions can be observed on any Sentry session sending these events:
Local Development
Another more straightforward way to test the changes is to observe the same events when logged while running Studio locally:
Pre-merge Checklist