Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implemented all patches into appium code #19892

Closed
wants to merge 1 commit into from

Conversation

vavadikb
Copy link

Changes made:

  • implemented all patches into 1 appium repo

Copy link

CLA Not Signed

@oleksandr-vonahi
Copy link

@gooroodev can you review the changes?

@gooroodev
Copy link

gooroodev commented May 30, 2024

Thanks, @oleksandr-vonahi! Ready to assist!

Mistakes Typos Security Performance Best Practices Readability Others
1 1 2 2 3 2 1

Summary of Changes

  1. Added new utility functions to handle device status checks and file operations.
  2. Introduced custom logic to manage local application files and their integrity.
  3. Added new routes for checking the status of WDA and ADB.
  4. Enhanced error handling with specific exit codes based on error messages.
  5. Updated caching and file handling mechanisms to improve local application file management.

Found Issues

ID Type Details Severity Confidence
1 Best Practices Use of process.exit in multiple places without proper cleanup or logging. 🔴High 🔴High
2 Security Potential command injection via shell commands in executeShellWPromise. 🔴High 🔴High
3 Performance Repeatedly checking for file existence in a loop without a delay. 🟠Medium 🔴High
4 Readability Inconsistent use of single and double quotes. 🟡Low 🔴High
5 Best Practices Missing error handling in async functions. 🔴High 🔴High
6 Performance Using synchronous file operations in process.on('exit') handler. 🟠Medium 🔴High
7 Mistakes Incorrect import statement in getSharedFolderForAppUrl function. 🔴High 🔴High
8 Readability Inconsistent logging messages, making it hard to trace. 🟡Low 🔴High
9 Security Hardcoded sensitive information like sessionId and deviceId. 🔴High 🔴High
10 Others Use of @ts-nocheck without justification. 🟡Low 🔴High
11 Typos Typo in variable name downloadIsNeaded should be downloadIsNeeded. 🟡Low High

Fixes

1. Use of process.exit in multiple places without proper cleanup or logging

// Example fix in core.ts
async getStatusWDA(exitCode?: number | null) {
  // ... existing code ...
  if (exitCode != null) {
    this.log.error("Error for sending of WDA status http call. See appium logs for details")
    this.log.warn(`[MCLOUD] Killing Appium instance with "exit(${exitCode})"`);
    await cleanUpResources(); // Custom cleanup function
    process.exit(exitCode);
  }
  // ... existing code ...
}

// Define a cleanup function
async function cleanUpResources() {
  // Perform necessary cleanup operations
  logger.info("Cleaning up resources before exiting...");
}

2. Potential command injection via shell commands in executeShellWPromise

// Example fix in mcloud-utils.ts
async function executeShellWPromise(shellCommand) {
  const execPromisify = util.promisify(exec);
  if (typeof shellCommand !== 'string') {
    throw new Error('Invalid shell command');
  }
  return await execPromisify(shellCommand);
}

3. Repeatedly checking for file existence in a loop without a delay

// Example fix in helpers.js
while(!await fs.exists(localFile) && (attemptsCount++ < maxAttemptsCount)) {
  await new Promise((resolve) => {
    logger.info(`[MCLOUD] Attempt #${attemptsCount} for local app file to appear again`);
    setTimeout(resolve, waitingTime);
  });
}

4. Inconsistent use of single and double quotes

// Example fix in helpers.js
logger.info('[MCLOUD] Local version of app was found. Will check actuality of the file');

5. Missing error handling in async functions

// Example fix in mcloud-utils.ts
async function getLocalAppsFolder() {
  try {
    if (process.env.APPIUM_APPS_DIR === undefined || process.env.APPIUM_APPS_DIR === '') {
      return undefined;
    } else {
      return process.env.APPIUM_APPS_DIR;
    }
  } catch (error) {
    logger.error('Error getting local apps folder:', error);
    throw error;
  }
}

6. Using synchronous file operations in process.on('exit') handler

// Example fix in helpers.js
process.on('exit', async () => {
  for (const appPath of appPaths) {
    try {
      logger.info(`[MCLOUD] process.on('exit'). Deleting files by path: ${appPath}`);
      await fs.rimraf(appPath);
    } catch (e) {
      logger.warn(e.message);
    }
  }
});

7. Incorrect import statement in getSharedFolderForAppUrl function

// Example fix in mcloud-utils.ts
await fs.mkdir(targetPath, {recursive : true});

8. Inconsistent logging messages

// Example fix in helpers.js
logger.info(`[MCLOUD] Attempt #${attemptsCount} for local app file to appear again`);

9. Hardcoded sensitive information like sessionId and deviceId

// Example fix in helpers.js
var sessionId = process.env.sessionId || 'unknown session ID';
var deviceId = process.env.DEVICE_UDID || 'unknown device UUID';

10. Use of @ts-nocheck without justification

// Example fix in plugin.js
// Add a comment explaining why @ts-nocheck is necessary
// @ts-nocheck: This file uses dynamic imports which TypeScript cannot type check

11. Typo in variable name downloadIsNeaded should be downloadIsNeeded

// Example fix in helpers.js
let downloadIsNeeded = true;

General Review

The incoming changes introduce several useful functionalities and enhancements but also bring some issues that need addressing. The code quality is generally good, but there are areas where improvements can be made, particularly in error handling, security, and performance. Consistency in logging and coding style should be maintained for better readability and maintainability.

--
I only arrive when I am mentioned and asked to review the pull request.
React or reply to keep the discussion going!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants