fix: add invalid regex validation for commitConfig

[jescalada]

Fixes #1336.

Most of the changes are unit tests for the new code and /src/config/index.ts.

[netlify]

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name	Link
🔨 Latest commit	07da631
🔍 Latest deploy log	https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/69908b288ba7a7000830f9b5

[codecov]

Codecov Report

❌ Patch coverage is 94.56522% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.51%. Comparing base (588d7f3) to head (07da631).

Files with missing lines	Patch %	Lines
src/config/validators.ts	93.65%	4 Missing ⚠️
src/config/ConfigLoader.ts	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1355      +/-   ##
==========================================
+ Coverage   81.25%   81.51%   +0.26%     
==========================================
  Files          65       66       +1     
  Lines        4657     4713      +56     
  Branches      792      814      +22     
==========================================
+ Hits         3784     3842      +58     
+ Misses        858      856       -2     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kriswest]

Possibly a double call to loadFullConfiguration that needs looking at

[kriswest]

This generally LGTM.

However, I can't help thinking about the fact we have multiple types of validation in different places (validation via the schema and quicktype during load, then this later). A slightly deeper refactor could combine those steps, parsing individual configs from strings using quicktype then applying other validators - resulting in a n easier to follow and maintain codebase..

I'm ok with merging in the current form and leaving such a refactor for later if you want to - hence approving.

[kriswest]

This could be extracted to a helper function validateConfigRegex(config: GitProxyConfig, path:string) extracts the value (checking existence), checks if its an array, validates and logs. That would reduce repeated code.

[jescalada]

I tried doing this previously - had a bunch of failing tests so I gave up on it. Will take another look now with fresh eyes 🙂

[jescalada]

Managed to extract a few functions out of it - potentially useful for extending commitConfig checks or other regex validation down the line:

/**
 * Validates that a regular expression is valid.
 * @param pattern The regular expression to validate
 * @param context The context of the regular expression
 * @returns true if the regular expression is valid, false otherwise
 */
function isValidRegex(pattern: string, context: string): boolean {
  try {
    new RegExp(pattern);
    return true;
  } catch {
    console.error(`Invalid regular expression for ${context}: ${pattern}`);
    return false;
  }
}

/**
 * Validates that a value in the configuration is a valid regular expression.
 * @param config The configuration to validate
 * @param path The path to the value to validate
 * @returns true if the value is a valid regular expression, false otherwise
 */
function validateConfigRegex(config: GitProxyConfig, path: string): boolean {
  const getValueAtPath = (obj: unknown, path: string): unknown => {
    return path.split('.').reduce((current, key) => {
      if (current == null || typeof current !== 'object') {
        return undefined;
      }
      return (current as Record<string, unknown>)[key];
    }, obj);
  };

  const value = getValueAtPath(config, path);

  if (!value) return true;

  if (typeof value === 'string') {
    return isValidRegex(value, path);
  }

  if (Array.isArray(value)) {
    for (const pattern of value) {
      if (!isValidRegex(pattern, path)) return false;
    }
    return true;
  }

  if (typeof value === 'object') {
    return Object.values(value).every((pattern) => isValidRegex(pattern as string, path));
  }

  return true;
}

The main validator function turns into this:

function validateCommitConfig(config: GitProxyConfig): boolean {
  return (
    validateConfigRegex(config, 'commitConfig.author.email.local.block') &&
    validateConfigRegex(config, 'commitConfig.author.email.domain.allow') &&
    validateConfigRegex(config, 'commitConfig.message.block.patterns') &&
    validateConfigRegex(config, 'commitConfig.diff.block.patterns') &&
    validateConfigRegex(config, 'commitConfig.diff.block.providers')
  );
}

[jescalada]

@kriswest Did some refactoring here, although not quite a full-blown refactor for the QuickType stuff. At least, the actual QuickType Convert.toGitProxyConfig call is in a more appropriate location (src/config/validators.ts).

I'd consider further refactoring if there's any extra feature requests or bugs in the ConfigLoader.

A final check is much appreciated 😃