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

fix(cli): add js file validation #1288

Merged
merged 1 commit into from
May 12, 2023
Merged

fix(cli): add js file validation #1288

merged 1 commit into from
May 12, 2023

Conversation

ysfscream
Copy link
Member

@ysfscream ysfscream commented May 12, 2023

If you have any questions, you can refer to the Contributing Guide

What is the current behavior?

Please describe the current behavior and link to a relevant issue.

Issue Number

Example: #123

What is the new behavior?

image

Please describe the new behavior or provide screenshots.

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to review?

Other information

@ysfscream ysfscream added enhancement New feature or request CLI MQTTX CLI labels May 12, 2023
@ysfscream ysfscream self-assigned this May 12, 2023
if (!getScenarioFilePath(file)) {
signale.error(`Scenario file ${file} not found.`)
process.exit(1)
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review

  1. The code is making sure that if a name and/or file is provided, it exists in the scenario list or file path respectively.
  2. It also checks to make sure that the file provided ends with .js, to ensure that it is a JavaScript file.
  3. If neither of these conditions are met, an error message is printed and the process is exited.

Overall, the code looks good and is properly checking the input. However, it might be worth adding a check to see if the name provided is actually a valid name, as opposed to an empty string or undefined.

if (path.extname(filePath) !== '.js') {
throw new Error(`Invalid file type: ${filePath}. Only .js files are allowed.`)
}

const simulatorModule = require(filePath)

if (typeof simulatorModule.generator !== 'function') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review.

First, I will check the readability of the code. The code is easy to read and understand. The indentation is correct and the variables are named appropriately.

Next, I will check for any errors or bugs. The code looks good and there are no errors or bugs that I can see.

Finally, I will check for any security issues. The code looks secure and there is a check in place to ensure that only .js files are allowed. This will help protect against malicious code being uploaded.

Overall, the code looks good and should work as expected.

@Red-Asuka Red-Asuka merged commit 34f226e into main May 12, 2023
@Red-Asuka Red-Asuka deleted the ysf/dev branch May 12, 2023 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI MQTTX CLI enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants