fix: scrubNames() handle file exts better (aws#5332)

Problem:

The scrubNames() function removes PII from file paths
in a given string. The issue was that in certain cases where
the file path extension was composed of multiple extensions,
no file extension would be used.

So instead of 'x.ext1.ext2' -> 'x.ext2', we would just
get 'x'.

Solution:

Preserve the last file extension.

Also, this indirectly fixed some of the unit tests that were returning
unexpected results.

Signed-off-by: Nikolas Komonen <[email protected]>

Author: nkomonen-amazon

packages/core/src/shared/errors.ts

@@ -333,7 +333,7 @@ export function getTelemetryResult(error: unknown | undefined): Result {
  */
 export function scrubNames(s: string, username?: string) {
     let r = ''
-    const fileExtRe = /\.[^.]{1,4}$/
+    const fileExtRe = /\.[^.\/]{1,4}$/
     const slashdot = /^[~.]*[\/\\]*/
 
     /** Allowlisted filepath segments. */
@@ -375,7 +375,6 @@ export function scrubNames(s: string, username?: string) {
         let scrubbed = ''
         // Get the frontmatter ("/", "../", "~/", or "./").
         const start = word.trimStart().match(slashdot)?.[0] ?? ''
-        const fileExt = word.trimEnd().match(fileExtRe)?.[0] ?? ''
         pathSegments[0] = pathSegments[0].trimStart().replace(slashdot, '')
         for (const seg of pathSegments) {
             if (driveLetterRegex.test(seg)) {
@@ -391,6 +390,8 @@ export function scrubNames(s: string, username?: string) {
             }
         }
 
+        // includes leading '.', eg: '.json'
+        const fileExt = pathSegments[pathSegments.length - 1].match(fileExtRe) ?? ''
         r += ` ${start.replace(/\\/g, '/')}${scrubbed.replace(/^[\/\\]+/, '')}${fileExt}`
     }
 

packages/core/src/test/shared/errors.test.ts

@@ -477,10 +477,10 @@ describe('util', function () {
     it('scrubNames()', async function () {
         const fakeUser = 'jdoe123'
         assert.deepStrictEqual(scrubNames('', fakeUser), '')
-        assert.deepStrictEqual(scrubNames('a ./ b', fakeUser), 'a ././ b') // TODO: fix this
-        assert.deepStrictEqual(scrubNames('a ../ b', fakeUser), 'a .././ b') // TODO: fix this
-        assert.deepStrictEqual(scrubNames('a /.. b', fakeUser), 'a /.. b') // TODO: fix this
-        assert.deepStrictEqual(scrubNames('a //..// b', fakeUser), 'a //..//.// b') // TODO: fix this
+        assert.deepStrictEqual(scrubNames('a ./ b', fakeUser), 'a ./ b')
+        assert.deepStrictEqual(scrubNames('a ../ b', fakeUser), 'a ../ b')
+        assert.deepStrictEqual(scrubNames('a /.. b', fakeUser), 'a /.. b')
+        assert.deepStrictEqual(scrubNames('a //..// b', fakeUser), 'a //..// b')
         assert.deepStrictEqual(scrubNames('a / b', fakeUser), 'a / b')
         assert.deepStrictEqual(scrubNames('a ~/ b', fakeUser), 'a ~/ b')
         assert.deepStrictEqual(scrubNames('a //// b', fakeUser), 'a //// b')
@@ -501,6 +501,9 @@ describe('util', function () {
         assert.deepStrictEqual(scrubNames('/Users/user1/foo.jso (?)', fakeUser), '/Users/x/x.jso (?)')
         assert.deepStrictEqual(scrubNames('/Users/user1/foo.js (?)', fakeUser), '/Users/x/x.js (?)')
         assert.deepStrictEqual(scrubNames('/Users/user1/foo.longextension (?)', fakeUser), '/Users/x/x (?)')
+        assert.deepStrictEqual(scrubNames('/Users/user1/foo.ext1.ext2.ext3', fakeUser), '/Users/x/x.ext3')
+        assert.deepStrictEqual(scrubNames('/Users/user1/extMaxLength.1234', fakeUser), '/Users/x/x.1234')
+        assert.deepStrictEqual(scrubNames('/Users/user1/extExceedsMaxLength.12345', fakeUser), '/Users/x/x')
         assert.deepStrictEqual(scrubNames('c:\\fooß\\bar\\baz.txt', fakeUser), 'c:/xß/x/x.txt')
         assert.deepStrictEqual(
             scrubNames('uhh c:\\path with\\ spaces \\baz.. hmm...', fakeUser),