copilot feedback

Author: StephenHodgson

src/logging.ts

@@ -243,6 +243,10 @@ export class Logger {
      * @param optionalParams Additional parameters to log.
      */
     public debugOptions(options: any, ...optionalParams: any[]): void {
+        // Avoid expensive scrubbing and stringification when debug logging is disabled.
+        if (this.logLevel !== LogLevel.DEBUG) {
+            return;
+        }
         const scrubbed = this.scrubSensitiveData(options);
         this.debug(JSON.stringify(scrubbed), ...optionalParams);
     }

src/unity-editor.ts

@@ -197,15 +197,18 @@ export class UnityEditor {
      * @param args The command-line arguments array.
      * @returns A new array with sensitive values redacted.
      */
-    private scrubSensitiveArgs(args: string[]): string[] {
+    public scrubSensitiveArgs(args: string[]): string[] {
         const sensitiveFlags = ['-username', '-password', '-cloudOrganization', '-serial'];
         const scrubbedArgs: string[] = [];
 
         for (let i = 0; i < args.length; i++) {
-            scrubbedArgs.push(args[i]);
+            const arg = args[i];
+            if (!arg) continue;
+            
+            scrubbedArgs.push(arg);
 
             // If this is a sensitive flag and the next item is its value
-            if (sensitiveFlags.includes(args[i]) && i + 1 < args.length) {
+            if (sensitiveFlags.includes(arg) && i + 1 < args.length) {
                 scrubbedArgs.push('[REDACTED]');
                 i++; // Skip the next item (the actual value) since we've already added [REDACTED]
             }

tests/unity-editor.test.ts

@@ -37,4 +37,122 @@ describe('UnityEditor', () => {
             expect(template).toBeDefined();
         }
     });
+
+        describe('scrubSensitiveArgs', () => {
+            // Create a simple UnityEditor instance for testing scrubSensitiveArgs
+            // We don't need a full editor setup since we're only testing a pure function
+            let testEditor: UnityEditor;
+
+            beforeAll(() => {
+                // Use the first available editor or create a mock-like one
+                if (editors.length > 0) {
+                    testEditor = editors[0];
+                }
+            });
+
+            it('should redact -username value', () => {
+                if (!testEditor) {
+                    // Skip if no editors available
+                    return;
+                }
+
+                const args = ['-batchmode', '-username', 'test@example.com', '-quit'];
+                const scrubbedArgs = testEditor.scrubSensitiveArgs(args);
+
+                expect(scrubbedArgs).toContain('-username');
+                expect(scrubbedArgs).toContain('[REDACTED]');
+                expect(scrubbedArgs).not.toContain('test@example.com');
+                expect(scrubbedArgs).toContain('-batchmode');
+                expect(scrubbedArgs).toContain('-quit');
+            });
+
+            it('should redact -password value', () => {
+                if (!testEditor) return;
+
+                const args = ['-batchmode', '-password', 'SuperSecret123', '-quit'];
+                const scrubbedArgs = testEditor.scrubSensitiveArgs(args);
+
+                expect(scrubbedArgs).toContain('-password');
+                expect(scrubbedArgs).toContain('[REDACTED]');
+                expect(scrubbedArgs).not.toContain('SuperSecret123');
+            });
+
+            it('should redact -cloudOrganization value', () => {
+                if (!testEditor) return;
+
+                const args = ['-batchmode', '-cloudOrganization', 'org-id-12345', '-quit'];
+                const scrubbedArgs = testEditor.scrubSensitiveArgs(args);
+
+                expect(scrubbedArgs).toContain('-cloudOrganization');
+                expect(scrubbedArgs).toContain('[REDACTED]');
+                expect(scrubbedArgs).not.toContain('org-id-12345');
+            });
+
+            it('should redact -serial value', () => {
+                if (!testEditor) return;
+
+                const args = ['-batchmode', '-serial', 'ABC-123-DEF-456', '-quit'];
+                const scrubbedArgs = testEditor.scrubSensitiveArgs(args);
+
+                expect(scrubbedArgs).toContain('-serial');
+                expect(scrubbedArgs).toContain('[REDACTED]');
+                expect(scrubbedArgs).not.toContain('ABC-123-DEF-456');
+            });
+
+            it('should redact multiple sensitive values', () => {
+                if (!testEditor) return;
+
+                const args = [
+                    '-batchmode',
+                    '-username', 'user@example.com',
+                    '-password', 'MyPassword123',
+                    '-cloudOrganization', 'org-xyz',
+                    '-quit'
+                ];
+                const scrubbedArgs = testEditor.scrubSensitiveArgs(args);
+
+                expect(scrubbedArgs).toContain('[REDACTED]');
+                expect(scrubbedArgs).not.toContain('user@example.com');
+                expect(scrubbedArgs).not.toContain('MyPassword123');
+                expect(scrubbedArgs).not.toContain('org-xyz');
+                expect(scrubbedArgs).toContain('-batchmode');
+                expect(scrubbedArgs).toContain('-quit');
+            });
+
+            it('should preserve non-sensitive arguments', () => {
+                if (!testEditor) return;
+
+                const args = [
+                    '-batchmode',
+                    '-projectPath', '/path/to/project',
+                    '-buildTarget', 'StandaloneWindows64',
+                    '-quit'
+                ];
+                const scrubbedArgs = testEditor.scrubSensitiveArgs(args);
+
+                expect(scrubbedArgs).toEqual(args);
+                expect(scrubbedArgs).toContain('/path/to/project');
+                expect(scrubbedArgs).toContain('StandaloneWindows64');
+            });
+
+            it('should handle empty array', () => {
+                if (!testEditor) return;
+
+                const args: string[] = [];
+                const scrubbedArgs = testEditor.scrubSensitiveArgs(args);
+
+                expect(scrubbedArgs).toEqual([]);
+            });
+
+            it('should handle sensitive flag at end of array without value', () => {
+                if (!testEditor) return;
+
+                const args = ['-batchmode', '-quit', '-username'];
+                const scrubbedArgs = testEditor.scrubSensitiveArgs(args);
+
+                // The flag is included but there's no value to redact
+                expect(scrubbedArgs).toContain('-username');
+                expect(scrubbedArgs).not.toContain('[REDACTED]');
+            });
+        });
 });