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

Simplify seed creation/validation commands #1202

Merged
merged 5 commits into from
Oct 16, 2024
Merged

Conversation

goodov
Copy link
Member

@goodov goodov commented Aug 30, 2024

Removed unnecessary commands, reworked validation logic to always return errors as array of strings instead of throwing.

@goodov goodov force-pushed the check-and-create-seed branch from 2ca2fd8 to bdc8214 Compare October 8, 2024 13:36
@goodov goodov marked this pull request as ready for review October 8, 2024 13:41
@goodov goodov requested a review from a team as a code owner October 8, 2024 13:41
@goodov
Copy link
Member Author

goodov commented Oct 9, 2024

@brave/griffin-maintainers ptal

@goodov goodov force-pushed the check-and-create-seed branch from bdc8214 to 2040f36 Compare October 9, 2024 13:43
@atuchin-m
Copy link
Collaborator

BTW, we don't have a protection against duplicating some things:

      "channel": [
        "NIGHTLY",
        "BETA",
        "RELEASE",
         "NIGHTLY" // <= a possbile issue here
      ],

This is also considered only as a format error:

    "experiment": [
     {
       "name": "Enabled",
       "name": "AnotherEnabled",
       ...
       

src/seed_tools/commands/create_seed.ts Outdated Show resolved Hide resolved
src/seed_tools/commands/create_seed.ts Outdated Show resolved Hide resolved
src/seed_tools/commands/create_seed.ts Outdated Show resolved Hide resolved
src/seed_tools/utils/study_json_utils.ts Show resolved Hide resolved
src/seed_tools/utils/study_validation.test.ts Show resolved Hide resolved
src/seed_tools/commands/create_seed.ts Outdated Show resolved Hide resolved
@goodov goodov force-pushed the check-and-create-seed branch from 4ba7ac8 to 308ee06 Compare October 15, 2024 13:14
Copy link
Contributor

[puLL-Merge] - brave/brave-variations@1202

Description

This PR refactors and consolidates the seed tools, particularly focusing on the study validation and seed creation processes. It combines multiple commands into a single seed command with subcommands, improves error handling, and enhances the overall structure of the codebase.

Changes

Changes

  1. src/scripts/lint.ts:

    • Updated the lint command for studies to use the new seed lint subcommand.
  2. src/seed_tools/README.md:

    • Simplified the README, removing detailed command descriptions and adding a general help section.
  3. src/seed_tools/commands/:

    • Removed check_study.test.ts, check_study.ts, create_seed.test.ts, create_seed.ts, validate_seed.test.ts, and validate_seed.ts.
    • Added seed.test.ts and seed.ts, which consolidate the functionality of the removed files into a single seed command with subcommands.
    • Updated compare_seeds.ts and split_seed_json.ts to export functions that create commands instead of exporting commands directly.
  4. src/seed_tools/utils/:

    • Added file_utils.ts with a utility function to get file base names.
    • Updated seed_validation.ts, study_json_utils.ts, and study_validation.ts to improve error handling and consolidate functionality.
  5. src/seed_tools/seed_tools.ts:

    • Updated to use the new consolidated seed command and updated command creation functions.

Possible Issues

  1. The consolidation of commands may require updates to any scripts or documentation that reference the old command structure.
  2. The error handling changes might alter the format of error messages, potentially affecting any automated processes that parse these messages.

Security Hotspots

No significant security hotspots were identified in this change. The modifications primarily focus on code organization and error handling, which don't directly impact security. However, as with any significant refactoring, thorough testing should be performed to ensure that all functionality, including any security-related checks, continues to work as expected.

@goodov goodov force-pushed the check-and-create-seed branch from 308ee06 to 353b2b4 Compare October 15, 2024 13:46
const errors: string[] = [];

for (const file of files) {
const filePath = path.join(studiesDir, file);
Copy link
Contributor

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] Detected possible user input going into a path.join or path.resolve function. This could possibly lead to a path traversal vulnerability, where the attacker can access arbitrary files stored in the file system. Instead, be sure to sanitize or validate user input first.

Source: https://semgrep.dev/r/javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal


Cc @thypon @kdenhartog

Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing anyway that someone could produce a file traversal vulnerability with this. readdir returns an array of the current files in the directory and excludes the usage of '.' and '..'. This seems fine to me.

@goodov goodov added this pull request to the merge queue Oct 16, 2024
Merged via the queue into main with commit 4a7fbb2 Oct 16, 2024
6 checks passed
@goodov goodov deleted the check-and-create-seed branch October 16, 2024 05:13
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.

4 participants