feat(support): Move SecureValuesPreprocessor to @appum/logger (#20228)

The SecureValuesPreprocessor has been moved directly to the @appum/logger module and set as a private logger instance property.
The loadSecureValuesPreprocessingRules method is now public instance method of the Logger class. Unit tests were ported to the @appium/logger module as well.

package-lock.json

@@ -22459,6 +22459,7 @@
       "license": "ISC",
       "dependencies": {
         "console-control-strings": "1.1.0",
+        "lodash": "4.17.21",
         "set-blocking": "2.0.0"
       },
       "engines": {
@@ -23431,6 +23432,7 @@
       "version": "file:packages/logger",
       "requires": {
         "console-control-strings": "1.1.0",
+        "lodash": "4.17.21",
         "set-blocking": "2.0.0"
       }
     },

packages/appium/lib/main.js

@@ -8,7 +8,7 @@ import {
   server as baseServer,
   normalizeBasePath,
 } from '@appium/base-driver';
-import {logger as logFactory, util, env} from '@appium/support';
+import {util, env} from '@appium/support';
 import {asyncify} from 'asyncbox';
 import _ from 'lodash';
 import {AppiumDriver} from './appium';
@@ -246,7 +246,7 @@ async function init(args) {
     await logsinkInit(serverArgs);
 
     if (serverArgs.logFilters) {
-      const {issues, rules} = await logFactory.loadSecureValuesPreprocessingRules(
+      const {issues, rules} = await logger.unwrap().loadSecureValuesPreprocessingRules(
         serverArgs.logFilters,
       );
       if (!_.isEmpty(issues)) {

packages/logger/lib/log.ts

@@ -1,13 +1,22 @@
+import _ from 'lodash';
 import {EventEmitter} from 'node:events';
 // @ts-ignore This module does not provide type definitons
 import setBlocking from 'set-blocking';
 // @ts-ignore This module does not provide type definitons
 import consoleControl from 'console-control-strings';
 import * as util from 'node:util';
-import type {MessageObject, StyleObject, Logger, LogLevel} from './types';
+import type {
+  MessageObject,
+  StyleObject,
+  Logger,
+  LogLevel,
+  PreprocessingRulesLoadResult,
+  LogFiltersConfig
+} from './types';
 import type {Writable} from 'node:stream';
 import {AsyncLocalStorage} from 'node:async_hooks';
 import { unleakString } from './utils';
+import { SecureValuesPreprocessor } from './secure-values-preprocessor';
 
 const DEFAULT_LOG_LEVELS: any[][] = [
   ['silly', -Infinity, {inverse: true}, 'sill'],
@@ -41,6 +50,7 @@ export class Log extends EventEmitter implements Logger {
   _disp: Record<LogLevel | string, number | string>;
   _id: number;
   _paused: boolean;
+  _secureValuesPreprocessor: SecureValuesPreprocessor;
 
   constructor() {
     super();
@@ -56,6 +66,7 @@ export class Log extends EventEmitter implements Logger {
     this._id = 0;
     this._paused = false;
     this._asyncStorage = new AsyncLocalStorage();
+    this._secureValuesPreprocessor = new SecureValuesPreprocessor();
 
     this._style = {};
     this._levels = {};
@@ -186,7 +197,7 @@ export class Log extends EventEmitter implements Logger {
     for (const formatArg of [message, ...args]) {
       messageArguments.push(formatArg);
       // resolve stack traces to a plain string.
-      if (typeof formatArg === 'object' && formatArg instanceof Error && formatArg.stack) {
+      if (_.isError(formatArg) && formatArg.stack) {
         Object.defineProperty(formatArg, 'stack', {
           value: (stack = formatArg.stack + ''),
           enumerable: true,
@@ -203,8 +214,8 @@ export class Log extends EventEmitter implements Logger {
       id: this._id++,
       timestamp: Date.now(),
       level,
-      prefix: unleakString(prefix || ''),
-      message: unleakString(formattedMessage),
+      prefix: this._secureValuesPreprocessor.preprocess(unleakString(prefix || '')),
+      message: this._secureValuesPreprocessor.preprocess(unleakString(formattedMessage)),
     };
 
     this.emit('log', m);
@@ -222,6 +233,28 @@ export class Log extends EventEmitter implements Logger {
     this.emitLog(m);
   }
 
+  /**
+   * Loads the JSON file containing secure values replacement rules.
+   * This might be necessary to hide sensitive values that may possibly
+   * appear in Appium logs.
+   * Each call to this method replaces the previously loaded rules if any existed.
+   *
+   * @param {string|string[]|LogFiltersConfig} rulesJsonPath The full path to the JSON file containing
+   * the replacement rules. Each rule could either be a string to be replaced
+   * or an object with predefined properties.
+   * @throws {Error} If the given file cannot be loaded
+   * @returns {Promise<PreprocessingRulesLoadResult>}
+   */
+  async loadSecureValuesPreprocessingRules(
+    rulesJsonPath: string | string[] | LogFiltersConfig
+  ): Promise<PreprocessingRulesLoadResult> {
+    const issues = await this._secureValuesPreprocessor.loadRules(rulesJsonPath);
+    return {
+      issues,
+      rules: _.cloneDeep(this._secureValuesPreprocessor.rules),
+    };
+  }
+
   private emitLog(m: MessageObject): void {
     if (this._paused) {
       this._buffer.push(m);

packages/logger/lib/secure-values-preprocessor.ts

@@ -1,17 +1,25 @@
 import _ from 'lodash';
+import type {
+  SecureValuePreprocessingRule,
+  LogFilterRegex,
+  LogFiltersConfig,
+  LogFilter,
+} from './types';
 
 const DEFAULT_REPLACER = '**SECURE**';
 
 /**
  * Type guard for log filter type
  * @param {object} value
- * @returns {value is import('@appium/types').LogFilterRegex}
+ * @returns {value is LogFilterRegex}
  */
-function isLogFilterRegex(value) {
+function isLogFilterRegex(value: object): value is LogFilterRegex {
   return 'pattern' in value;
 }
 
-class SecureValuesPreprocessor {
+export class SecureValuesPreprocessor {
+  _rules: SecureValuePreprocessingRule[];
+
   constructor() {
     this._rules = [];
   }
@@ -20,20 +28,20 @@ class SecureValuesPreprocessor {
    * @returns {Array<SecureValuePreprocessingRule>} The list of successfully
    * parsed preprocessing rules
    */
-  get rules() {
+  get rules(): Array<SecureValuePreprocessingRule> {
     return this._rules;
   }
 
   /**
    * Parses single rule from the given JSON file
    *
-   * @param {string|import('@appium/types').LogFilter} rule The rule might
+   * @param {string|LogFilter} rule The rule might
    * either be represented as a single string or a configuration object
    * @throws {Error} If there was an error while parsing the rule
    * @returns {SecureValuePreprocessingRule} The parsed rule
    */
-  parseRule(rule) {
-    let pattern;
+  parseRule(rule: string | LogFilter): SecureValuePreprocessingRule {
+    let pattern: string | undefined;
     let replacer = DEFAULT_REPLACER;
     let flags = ['g'];
     if (_.isString(rule)) {
@@ -89,23 +97,21 @@ class SecureValuesPreprocessor {
   /**
    * Loads rules from the given JSON file
    *
-   * @param {string|string[]|import('@appium/types').LogFiltersConfig} filters
+   * @param {string|string[]|LogFiltersConfig} filters
    * One or more log parsing rules
    * @throws {Error} If the format of the source file is invalid or
    * it does not exist
    * @returns {Promise<string[]>} The list of issues found while parsing each rule.
    * An empty list is returned if no rule parsing issues were found
    */
-  async loadRules(filters) {
-    /** @type {string[]} */
-    const issues = [];
-    /** @type {(import('@appium/types').LogFilter|string)[]} */
-    const rawRules = [];
+  async loadRules(filters: string | string[] | LogFiltersConfig): Promise<string[]> {
+    const issues: string[] = [];
+    const rawRules: (LogFilter | string)[] = [];
     for (const source of (_.isArray(filters) ? filters : [filters])) {
       if (_.isPlainObject(source)) {
-        rawRules.push(/** @type {import('@appium/types').LogFilter} */ (source));
+        rawRules.push(source as LogFilter);
       } else if (_.isString(source)) {
-        rawRules.push(source);
+        rawRules.push(String(source));
       } else {
         issues.push(`'${source}' must be a valid log filtering rule`);
       }
@@ -115,7 +121,7 @@ class SecureValuesPreprocessor {
       try {
         this._rules.push(this.parseRule(rawRule));
       } catch (e) {
-        issues.push(e.message);
+        issues.push((e as Error).message);
       }
     }
     return issues;
@@ -129,27 +135,15 @@ class SecureValuesPreprocessor {
    * @param {string} str The string to make replacements in
    * @returns {string} The string with replacements made
    */
-  preprocess(str) {
-    if (this._rules.length === 0 || !_.isString(str)) {
+  preprocess(str: string): string {
+    if (this._rules.length === 0 || !str || !_.isString(str)) {
       return str;
     }
 
     let result = str;
     for (const rule of this._rules) {
-      result = result.replace(rule.pattern, rule.replacer);
+      result = result.replace(rule.pattern, rule.replacer ?? DEFAULT_REPLACER);
     }
     return result;
   }
 }
-
-const SECURE_VALUES_PREPROCESSOR = new SecureValuesPreprocessor();
-
-export {SECURE_VALUES_PREPROCESSOR, SecureValuesPreprocessor};
-export default SECURE_VALUES_PREPROCESSOR;
-
-/**
- * @typedef SecureValuePreprocessingRule
- * @property {RegExp} pattern The parsed pattern which is going to be used for replacement
- * @property {string} [replacer] The replacer value to use. By default
- * equals to `DEFAULT_SECURE_REPLACER`
- */

packages/logger/lib/types.ts

@@ -35,6 +35,10 @@ export interface Logger extends EventEmitter {
   error(prefix: string, message: any, ...args: any[]): void;
   silent(prefix: string, message: any, ...args: any[]): void;
 
+  loadSecureValuesPreprocessingRules(
+    rulesJsonPath: string | string[] | LogFiltersConfig
+  ): Promise<PreprocessingRulesLoadResult>;
+
   enableColor(): void;
   disableColor(): void;
 
@@ -86,3 +90,57 @@ export interface MessageObject {
   prefix: string;
   message: string;
 }
+
+export interface SecureValuePreprocessingRule {
+  /** The parsed pattern which is going to be used for replacement */
+  pattern: RegExp;
+  /** The replacer value to use. By default equals to `DEFAULT_SECURE_REPLACER` */
+  replacer?: string;
+}
+
+export interface PreprocessingRulesLoadResult {
+  /**
+   * The list of rule parsing issues (one item per rule).
+   * Rules with issues are skipped. An empty list is returned if no parsing issues exist.
+   */
+  issues: string[];
+  /**
+   * The list of successfully loaded
+   * replacement rules. The list could be empty if no rules were loaded.
+   */
+  rules: SecureValuePreprocessingRule[];
+}
+
+export type LogFilter = {
+  /**
+   * Replacement string for matched text
+   */
+  replacer?: string;
+  /**
+   * Matching flags; see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#advanced_searching_with_flags
+   */
+  flags?: string;
+  [k: string]: unknown;
+} & (LogFilterText | LogFilterRegex);
+/**
+ * One or more log filtering rules
+ */
+export type LogFiltersConfig = LogFilter[];
+
+export interface LogFilterText {
+  /**
+   * Text to match
+   */
+  text: string;
+  [k: string]: unknown;
+}
+/**
+ * Log filter with regular expression
+ */
+export interface LogFilterRegex {
+  /**
+   * Regex pattern to match
+   */
+  pattern: string;
+  [k: string]: unknown;
+}

packages/logger/package.json

@@ -28,6 +28,7 @@
   },
   "dependencies": {
     "console-control-strings": "1.1.0",
+    "lodash": "4.17.21",
     "set-blocking": "2.0.0"
   },
   "license": "ISC",

packages/logger/test/unit/preprocessor-specs.js

@@ -1,7 +1,7 @@
-import {SecureValuesPreprocessor} from '../../lib/log-internal';
+import {SecureValuesPreprocessor} from '../../lib/secure-values-preprocessor';
 
 describe('Log Internals', function () {
-  /** @type {import('../../lib/log-internal').SecureValuesPreprocessor} */
+  /** @type {import('../../lib/secure-values-prepreocessor').SecureValuesPreprocessor} */
   let preprocessor;
 
   beforeEach(function () {

packages/support/README.md

@@ -51,7 +51,6 @@ All utility functions are split into a bunch of different categories. Each categ
 |env|Several helpers needed by the server to cope with internal dependencies and manifests|
 |fs|Most of the functions here are just thin wrappers over utility functions available in [Promises API](https://nodejs.org/api/fs.html#promises-api)|
 |image-util|Utilities to work with images. Use [sharp](https://github.com/lovell/sharp) under the hood.<br>:bangbang: Node >=18.17 is required to use these utilities|
-|log-internal|Utilities needed for internal Appium log config assistance|
 |logging|See [the logging section below](#logging)|
 |mjpeg|Helpers needed to implement [MJPEG streaming](https://en.wikipedia.org/wiki/Motion_JPEG#Video_streaming)|
 |net|Helpers needed for network interactions, for example, upload and download of files|

packages/support/lib/index.ts

@@ -80,4 +80,3 @@ export type {
   ZipCompressionOptions,
   ZipSourceOptions,
 } from './zip';
-export type {SecureValuePreprocessingRule} from './log-internal';

packages/support/lib/logging.js

@@ -3,7 +3,6 @@
 import globalLog from '@appium/logger';
 import _ from 'lodash';
 import moment from 'moment';
-import SECURE_VALUES_PREPROCESSOR from './log-internal';
 
 /** @type {import('@appium/types').AppiumLoggerLevel[]} */
 export const LEVELS = ['silly', 'verbose', 'debug', 'info', 'http', 'warn', 'error'];
@@ -11,11 +10,19 @@ const MAX_LOG_RECORDS_COUNT = 3000;
 
 const PREFIX_TIMESTAMP_FORMAT = 'HH-mm-ss:SSS';
 
-// mock log object used in testing mode
-let mockLog = {};
-for (let level of LEVELS) {
-  mockLog[level] = () => {};
-}
+// mock log object used in testing mode to silence the output
+const MOCK_LOG = {
+  unwrap: () => ({
+    loadSecureValuesPreprocessingRules: () => ({
+      issues: [],
+      rules: [],
+    }),
+    level: '',
+    prefix: '',
+    log: _.noop,
+  }),
+  ...(_.fromPairs(LEVELS.map((l) => [l, _.noop]))),
+};
 
 /**
  *
@@ -32,7 +39,7 @@ function _getLogger() {
   let logger;
   if (testingMode && !forceLogMode) {
     // in testing mode, use a mock logger object that we can query
-    logger = mockLog;
+    logger = MOCK_LOG;
   } else {
     // otherwise, either use the global, or a new `npmlog` object
     logger = global._global_npmlog || globalLog;
@@ -85,12 +92,8 @@ function getLogger(prefix = null) {
   for (const level of LEVELS) {
     wrappedLogger[level] = /** @param {...any} args */ function (...args) {
       const actualPrefix = getActualPrefix(this.prefix, logTimestamp);
-      for (const arg of args) {
-        const out = _.isError(arg) && arg.stack ? arg.stack : `${arg}`;
-        for (const line of out.split('\n')) {
-          logger[level](actualPrefix, SECURE_VALUES_PREPROCESSOR.preprocess(line));
-        }
-      }
+      // @ts-ignore This is OK
+      logger[level](actualPrefix, ...args);
     };
   }
   wrappedLogger.errorWithException = function (/** @type {any[]} */ ...args) {
@@ -113,38 +116,10 @@ function getLogger(prefix = null) {
   return /** @type {AppiumLogger} */ (wrappedLogger);
 }
 
-/**
- * @typedef LoadResult
- * @property {string[]} issues The list of rule parsing issues (one item per rule).
- * Rules with issues are skipped. An empty list is returned if no parsing issues exist.
- * @property {import('./log-internal').SecureValuePreprocessingRule[]} rules The list of successfully loaded
- * replacement rules. The list could be empty if no rules were loaded.
- */
-
-/**
- * Loads the JSON file containing secure values replacement rules.
- * This might be necessary to hide sensitive values that may possibly
- * appear in Appium logs.
- * Each call to this method replaces the previously loaded rules if any existed.
- *
- * @param {string|string[]|import('@appium/types').LogFiltersConfig} rulesJsonPath The full path to the JSON file containing
- * the replacement rules. Each rule could either be a string to be replaced
- * or an object with predefined properties.
- * @throws {Error} If the given file cannot be loaded
- * @returns {Promise<LoadResult>}
- */
-async function loadSecureValuesPreprocessingRules(rulesJsonPath) {
-  const issues = await SECURE_VALUES_PREPROCESSOR.loadRules(rulesJsonPath);
-  return {
-    issues,
-    rules: _.cloneDeep(SECURE_VALUES_PREPROCESSOR.rules),
-  };
-}
-
 // export a default logger with no prefix
 const log = getLogger();
 
-export {log, getLogger, loadSecureValuesPreprocessingRules};
+export {log, getLogger};
 export default log;
 
 /**