fix!: replace custom glob with minimatch for improved pattern matching

NOTICE.md

@@ -850,7 +850,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 - isexe, 2.0.0, ISC,
 - json-stringify-safe, 5.0.1, ISC,
 - lru-cache, 6.0.0, ISC,
-- minimatch, 3.0.4, ISC,
+- minimatch, 10.0.1, ISC,
 - octokit-auth-probot, 1.2.3, ISC,
 - once, 1.4.0, ISC,
 - probot, 11.0.6, ISC,

README.md

@@ -64,16 +64,40 @@ The App listens to the following webhook events:
 If you rename a `<repo.yml>` that corresponds to a repo, safe-settings will rename the repo to the new name. This behavior will take effect whether the env variable `BLOCK_REPO_RENAME_BY_HUMAN` is set or not.
 
 ### Restricting `safe-settings` to specific repos
-`safe-settings` can be turned on only to a subset of repos by specifying them in the runtime settings file, `deployment-settings.yml`. If no file is specified, then the following repositories -  `'admin', '.github', 'safe-settings'` are exempted by default.
-A sample of `deployment-settings` file is found [here](docs/sample-settings/sample-deployment-settings.yml).
-
-To apply `safe-settings` __only__ to a specific list of repos, add them to the `restrictedRepos` section as `include` array.
 
-To ignore `safe-settings` for a specific list of repos, add them to the `restrictedRepos` section as `exclude` array.
+To restrict which repositories `safe-settings` can manage, create a `deployment-settings.yml` file. This file controls the app's scope throught the `restrictedRepos` configuration:
+
+```yml
+# Using include/exclude
+restrictedRepos:
+  include:
+    - api
+    - core-*    # Matches `core-api`, `core-service`, etc.
+  exclude:
+    - admin
+    - .github
+    - safe-settings
+    - test-*    # Matches `test-repo`, etc.
+
+# Or using simple array syntax for includes
+restrictedRepos: 
+  - admin
+  - .github
+  # ...
+```
 
 > [!NOTE]
-> The `include` and `exclude` attributes support as well regular expressions.
-> By default they look for regex, Example include: ['SQL'] will look apply to repos with SQL and SQL_ and SQL- etc if you want only SQL repo then use include:['^SQL$']
+> Pattern matching uses glob expressions, e.g use * for wildcards.
+
+When using `include` and `exclude`:
+
+- If `include` is specified, will **only** run on repositories that match pattern(s)
+- If `exlcude` is specified, will run on all repositories **expect** those matching pattern(s)
+- If both are specified, will run only on included repositories that are'nt excluded
+
+By default, if no configuration file is provided, `safe-settings` will excludes these repos: `admin`, `.github` and `safe-settings`.
+
+See our [deployment-settings.yml sample](docs/sample-settings/sample-deployment-settings.yml).
 
 ### Custom rules
 
@@ -329,24 +353,28 @@ The following can be configured:
 - `Rulesets`
 - `Environments` - wait timer, required reviewers, prevent self review, protected branches deployment branch policy, custom deployment branch policy, variables, deployment protection rules
 
-> [!important]
-> It is possible to provide an `include` or `exclude` settings to restrict the `collaborators`, `teams`, `labels` to a list of repos or exclude a set of repos for a collaborator. 
-> The include/exclude pattern can also be for glob. For e.g.:
-```
-teams:
-  - name: Myteam-admins
-    permission: admin
-  - name: Myteam-developers
-    permission: push
-  - name: Other-team
-    permission: push
-    include:
-      - '*-config'
-```
-> Will only add `Other-team` to only `*-config` repos
-
 See [`docs/sample-settings/settings.yml`](docs/sample-settings/settings.yml) for a sample settings file.
 
+> [!note]
+> When using `collaborators`, `teams` or `labels`, you can control which repositories they apply to using `include` and `exclude`:
+>
+> - If `include` is specified, settings will **only** apply to repositories that match those patterns
+> - If `exclude` is specified, settings will apply to all repositories **except** those matching the patterns  
+> - If both are specified, `exclude` takes precedence over `include` but `include` patterns will still be respected
+>
+> Pattern matching uses glob expressions, e.g use * for wildcards. For example:
+>
+> ```yml
+> teams:
+>   - name: Myteam-admins
+>     permission: admin
+>   - name: Myteam-developers
+>     permission: push
+>   - name: Other-team
+>     permission: push
+>     include:
+>       - '*-config'
+>  ```
 
 ### Additional values
 

docs/sample-settings/sample-deployment-settings.yml

@@ -1,16 +1,27 @@
+# This is a sample deployment settings file
+# See the documentation for more details on how to use this file
+
+# If no file is specified, the following repositories are excluded by default
+# restrictedRepos: ['admin', '.github', 'safe-settings']
+
 restrictedRepos:
-  # You can exclude certain repos from safe-settings processing
-  #  If no file is specified, then the following repositories - 'admin', '.github', 'safe-settings' are exempted by default
-  exclude: ['^admin$', '^\.github$', '^safe-settings$', '.*-test']
-  # Alternatively you can only include certain repos
-  include: ['^test$']
+  exclude: 
+    - admin
+    - .github
+    - safe-settings
+    - admin-*
+  include:
+    - docs
+    - core-*
+
 configvalidators:
   - plugin: collaborators
     error: |
       `Admin cannot be assigned to collaborators`
     script: |
       console.log(`baseConfig ${JSON.stringify(baseconfig)}`)
       return baseconfig.permission != 'admin'
+
 overridevalidators:
   - plugin: branches
     error: |

index.js

@@ -134,75 +134,60 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
   }
 
   function getAllChangedSubOrgConfigs (payload) {
-    const settingPattern = Settings.SUB_ORG_PATTERN
-    // Changes will be an array of files that were added
-    const added = payload.commits.map(c => {
-      return (c.added.filter(s => {
-        robot.log.debug(JSON.stringify(s))
-        return (s.search(settingPattern) >= 0)
-      }))
-    }).flat(2)
-    const modified = payload.commits.map(c => {
-      return (c.modified.filter(s => {
-        robot.log.debug(JSON.stringify(s))
-        return (s.search(settingPattern) >= 0)
-      }))
-    }).flat(2)
-    const changes = added.concat(modified)
-    const configs = changes.map(file => {
-      const matches = file.match(settingPattern)
-      robot.log.debug(`${JSON.stringify(file)} \n ${matches[1]}`)
-      return { name: matches[1] + '.yml', path: file }
-    })
-    return configs
+    const pattern = Settings.SUB_ORG_PATTERN
+
+    const getMatchingFiles = (commits, type) =>
+      commits.flatMap((c) => c[type].filter((file) => pattern.test(file)))
+
+    const changes = [
+      ...getMatchingFiles(payload.commits, 'added'),
+      ...getMatchingFiles(payload.commits, 'modified')
+    ]
+
+    return changes.map((file) => ({
+      name: file.match(/([^/]+)\.yml$/)[1],
+      path: file
+    }))
   }
 
   function getAllChangedRepoConfigs (payload, owner) {
-    const settingPattern = Settings.REPO_PATTERN
-    // Changes will be an array of files that were added
-    const added = payload.commits.map(c => {
-      return (c.added.filter(s => {
-        robot.log.debug(JSON.stringify(s))
-        return (s.search(settingPattern) >= 0)
-      }))
-    }).flat(2)
-    const modified = payload.commits.map(c => {
-      return (c.modified.filter(s => {
-        robot.log.debug(JSON.stringify(s))
-        return (s.search(settingPattern) >= 0)
-      }))
-    }).flat(2)
-    const changes = added.concat(modified)
-    const configs = changes.map(file => {
-      robot.log.debug(`${JSON.stringify(file)}`)
-      return { repo: file.match(settingPattern)[1], owner }
-    })
-    return configs
-  }
+    const pattern = Settings.REPO_PATTERN
 
-  function getChangedRepoConfigName (glob, files, owner) {
-    const modifiedFiles = files.filter(s => {
-      robot.log.debug(JSON.stringify(s))
-      return (s.search(glob) >= 0)
-    })
+    const getMatchingFiles = (commits, type) =>
+      commits.flatMap((c) => c[type].filter((file) => pattern.test(file)))
 
-    return modifiedFiles.map(modifiedFile => {
-      return { repo: modifiedFile.match(glob)[1], owner }
-    })
+    const changes = [
+      ...getMatchingFiles(payload.commits, 'added'),
+      ...getMatchingFiles(payload.commits, 'modified')
+    ]
+
+    return changes.map((file) => ({
+      name: file.match(/([^/]+)\.yml$/)[1],
+      owner
+    }))
   }
 
-  function getChangedSubOrgConfigName (glob, files) {
-    const modifiedFiles = files.filter(s => {
-      robot.log.debug(JSON.stringify(s))
-      return (s.search(glob) >= 0)
-    })
+  function getChangedRepoConfigName (files, owner) {
+    const pattern = Settings.REPO_PATTERN
 
-    return modifiedFiles.map(modifiedFile => {
-      robot.log.debug(`${JSON.stringify(modifiedFile)}`)
-      return { name: modifiedFile.match(glob)[1] + '.yml', path: modifiedFile }
-    })
+    const modifiedFiles = files.filter((s) => pattern.test(s))
+
+    return modifiedFiles.map((modifiedFile) => ({
+      repo: modifiedFile.match(/([^/]+)\.yml$/)[1],
+      owner
+    }))
   }
 
+  function getChangedSubOrgConfigName (files) {
+    const pattern = Settings.SUB_ORG_PATTERN
+
+    const modifiedFiles = files.filter((s) => pattern.test(s))
+
+    return modifiedFiles.map((modifiedFile) => ({
+      name: modifiedFile.match(/([^/]+)\.yml$/)[1],
+      path: modifiedFile
+    }))
+  }
   async function createCheckRun (context, pull_request, head_sha, head_branch) {
     const { payload } = context
     // robot.log.debug(`Check suite was requested! for ${context.repo()} ${pull_request.number} ${head_sha} ${head_branch}`)
@@ -604,14 +589,14 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
       return syncAllSettings(true, context, context.repo(), pull_request.head.ref)
     }
 
-    const repoChanges = getChangedRepoConfigName(Settings.REPO_PATTERN, files, context.repo().owner)
+    const repoChanges = getChangedRepoConfigName(files, context.repo().owner)
     if (repoChanges.length > 0) {
       return Promise.all(repoChanges.map(repo => {
         return syncSettings(true, context, repo, pull_request.head.ref)
       }))
     }
 
-    const subOrgChanges = getChangedSubOrgConfigName(Settings.SUB_ORG_PATTERN, files, context.repo().owner)
+    const subOrgChanges = getChangedSubOrgConfigName(files)
     if (subOrgChanges.length) {
       return Promise.all(subOrgChanges.map(suborg => {
         return syncSubOrgSettings(true, context, suborg, context.repo(), pull_request.head.ref)

lib/glob.js

@@ -1,44 +1,14 @@
-class Glob {
-  constructor (glob) {
-    this.glob = glob
-
-    // If not a glob pattern then just match the string.
-    if (!this.glob.includes('*')) {
-      this.regexp = new RegExp(`.*${this.glob}.*`, 'u')
-      return
-    }
-    this.regexptText = this.globize(this.glob)
-    this.regexp = new RegExp(`^${this.regexptText}$`, 'u')
-  }
-
-  globize (glob) {
-    return glob
-      .replace(/\\/g, '\\\\') // escape backslashes
-      .replace(/\//g, '\\/') // escape forward slashes
-      .replace(/\./g, '\\.') // escape periods
-      .replace(/\?/g, '([^\\/])') // match any single character except /
-      .replace(/\*\*/g, '.+') // match any character except /, including /
-      .replace(/\*/g, '([^\\/]*)') // match any character except /
-  }
-
-  toString () {
-    return this.glob
-  }
+const { minimatch } = require('minimatch')
 
-  [Symbol.search] (s) {
-    return s.search(this.regexp)
-  }
-
-  [Symbol.match] (s) {
-    return s.match(this.regexp)
-  }
-
-  [Symbol.replace] (s, replacement) {
-    return s.replace(this.regexp, replacement)
+class Glob {
+  constructor (pattern, options = {}) {
+    this.pattern = pattern
+    this.options = options
   }
 
-  [Symbol.replaceAll] (s, replacement) {
-    return s.replaceAll(this.regexp, replacement)
+  test (input) {
+    return minimatch(input, this.pattern, this.options)
   }
 }
+
 module.exports = Glob

lib/plugins/diffable.js

@@ -37,41 +37,22 @@ module.exports = class Diffable extends ErrorStash {
   filterEntries () {
     let filteredEntries = Array.from(this.entries)
 
-    // this.log.debug(` entries ${JSON.stringify(filteredEntries)}`)
     filteredEntries = filteredEntries.filter(attrs => {
-      if (Array.isArray(attrs.exclude)) {
-        const excludeGlobs = attrs.exclude.map(exc => new Glob(exc));
-        const excludeGlobsMatch = excludeGlobs.some(glob => !!this.repo.repo.match(glob));
+      if (!Array.isArray(attrs.exclude)) return true
 
-        if (!attrs.exclude.includes(this.repo.repo) && !excludeGlobsMatch) {
-          // this.log.debug(`returning not excluded entry = ${JSON.stringify(attrs)} for repo ${this.repo.repo}`)
-          return true
-        } else {
-          // this.log.debug(`skipping excluded entry = ${JSON.stringify(attrs)} for repo ${this.repo.repo}`)
-          return false
-        }
-      } else {
-        // this.log.debug(`No excludes. Returning unfiltered entries = ${JSON.stringify(attrs)} for repo ${this.repo.repo}`)
-        return true
-      }
+      const excludeGlobs = attrs.exclude.map(exc => new Glob(exc))
+      const isExcluded = excludeGlobs.some(glob => glob.test(this.repo.repo))
+      return !isExcluded
     })
+
     filteredEntries = filteredEntries.filter(attrs => {
-      if (Array.isArray(attrs.include)) {
-        const includeGlobs = attrs.include.map(inc => new Glob(inc));
-        const includeGlobsMatch = includeGlobs.some(glob => !!this.repo.repo.match(glob));
+      if (!Array.isArray(attrs.include)) return true
 
-        if (attrs.include.includes(this.repo.repo) || includeGlobsMatch) {
-          // this.log.debug(`returning included entry  = ${JSON.stringify(attrs)} for repo ${this.repo.repo}`)
-          return true
-        } else {
-          // this.log.debug(`skipping not included entry = ${JSON.stringify(attrs)} for repo ${this.repo.repo}`)
-          return false
-        }
-      } else {
-        // this.log.debug(`No includes. Returning unfiltered entries = ${JSON.stringify(attrs)} for repo ${this.repo.repo}`)
-        return true
-      }
+      const includeGlobs = attrs.include.map(exc => new Glob(exc))
+      const isIncluded = includeGlobs.some(glob => glob.test(this.repo.repo))
+      return isIncluded
     })
+
     filteredEntries = filteredEntries.map(e => {
       const { exclude, include, ...o } = e
       return o