tj-actions/eslint-changed-files

[BUG] Actions breaks if folders use parenthesis in the names

Closed this issue · 4 comments

jacob-8 commented

Is there an existing issue for this?

  • I have searched the existing issues

Does this issue exist in the latest version?

  • I'm using the latest release

Describe the bug?

SvelteKit has an advanced routing feature called layout groups: https://kit.svelte.dev/docs/advanced-routing#advanced-layouts-group - when a file within one of these folders is changed (e.g. src/routes/(app)/+page.svelte) then this action will incorrectly escape the parenthesis ending up with: INPUT_CHANGED_FILES: src/routes/\(app\)/+page.svelte and eslint will not error because it didn't find any files with that filename.

To Reproduce

In any of your examples, add parenthesis into a folder name and then commit to GitHub to see the action run and break.

What OS are you seeing the problem on?

ubuntu-latest or ubuntu-22.04

Expected behavior?

Action only runs lint on changed files and doesn't break.

Relevant log output

There is no log archive to download, but this section of the workflow run is applicable as you can see the correct filepath before the changed-files-patterns group runs but then it's wrong afterwards.

##[debug]Fetching target branch...
  /usr/bin/git fetch -q --no-tags --prune -u --progress --deepen=50 origin +refs/heads/main:refs/remotes/origin/main
  remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0        
  Completed fetching more history.
  ##[debug]Getting current SHA...
  /usr/bin/git rev-parse --verify defa9e3ed1dab59e06e66688997ed2532c36bdf2^{commit}
  defa9e3ed1dab59e06e66688997ed2532c36bdf2
  /usr/bin/git rev-parse --verify defa9e3ed1dab59e06e66688997ed2532c36bdf2^{commit}
  defa9e3ed1dab59e06e66688997ed2532c36bdf2
  ##[debug]Current SHA: defa9e3ed1dab59e06e66688997ed2532c36bdf2
  /usr/bin/git merge-base 18db55d30244cc161d9885c65a42ffc8b2d497f5 defa9e3ed1dab59e06e66688997ed2532c36bdf2
  18db55d30244cc161d9885c65a42ffc8b2d497f5
  /usr/bin/git log --format=%H 18db55d30244cc161d9885c65a42ffc8b2d497f5..defa9e3ed1dab59e06e66688997ed2532c36bdf2
  defa9e3ed1dab59e06e66688997ed2532c36bdf2
  /usr/bin/git merge-base 18db55d30244cc161d9885c65a42ffc8b2d497f5 defa9e3ed1dab59e06e66688997ed2532c36bdf2
  18db55d30244cc161d9885c65a42ffc8b2d497f5
  /usr/bin/git log --format=%H 18db55d30244cc161d9885c65a42ffc8b2d497f5..defa9e3ed1dab59e06e66688997ed2532c36bdf2
  defa9e3ed1dab59e06e66688997ed2532c36bdf2
  /usr/bin/git rev-parse --verify 18db55d30244cc161d9885c65a42ffc8b2d497f5^{commit}
  18db55d30244cc161d9885c65a42ffc8b2d497f5
  ##[debug]Previous SHA: 18db55d30244cc161d9885c65a42ffc8b2d497f5
  /usr/bin/git merge-base 18db55d30244cc161d9885c65a42ffc8b2d497f5 defa9e3ed1dab59e06e66688997ed2532c36bdf2
  18db55d30244cc161d9885c65a42ffc8b2d497f5
  /usr/bin/git log --format=%H 18db55d30244cc161d9885c65a42ffc8b2d497f5..defa9e3ed1dab59e06e66688997ed2532c36bdf2
  defa9e3ed1dab59e06e66688997ed2532c36bdf2
  Retrieving changes between 18db55d30244cc161d9885c65a42ffc8b2d497f5 (main) → defa9e3ed1dab59e06e66688997ed2532c36bdf2 (debug-lint)
  /usr/bin/git diff --name-status --ignore-submodules=all --diff-filter=ACDMRTUX 18db55d30244cc161d9885c65a42ffc8b2d497f5...defa9e3ed1dab59e06e66688997ed2532c36bdf2
  M	site/src/routes/[mother=locale]/[learning=locale]/(app)/account/+page.svelte
  ##[debug]All diff files: {"A":[],"C":[],"D":[],"M":["site/src/routes/[mother=locale]/[learning=locale]/(app)/account/+page.svelte"],"R":[],"T":[],"U":[],"X":[]}
  All Done!
  ::endgroup::
::group::changed-files-patterns
changed-files-patterns
##[debug]Node Action run completed with exit code 0
##[debug]Set output added_files = 
##[debug]Set output added_files_count = 0
##[debug]Set output copied_files = 
##[debug]Set output copied_files_count = 0
##[debug]Set output modified_files = site/src/routes/[mother=locale]/[learning=locale]/\(app\)/account/+page.svelte
##[debug]Set output modified_files_count = 1
##[debug]Set output renamed_files = 
##[debug]Set output renamed_files_count = 0
##[debug]Set output type_changed_files = 
##[debug]Set output type_changed_files_count = 0
##[debug]Set output unmerged_files = 
##[debug]Set output unmerged_files_count = 0
##[debug]Set output unknown_files = 
##[debug]Set output unknown_files_count = 0
##[debug]Set output all_changed_and_modified_files = site/src/routes/[mother=locale]/[learning=locale]/\(app\)/account/+page.svelte
##[debug]Set output all_changed_and_modified_files_count = 1
##[debug]Set output all_changed_files = site/src/routes/[mother=locale]/[learning=locale]/\(app\)/account/+page.svelte
##[debug]Set output all_changed_files_count = 1
##[debug]Set output any_changed = true
##[debug]Set output only_changed = true
##[debug]Set output other_changed_files = 
##[debug]Set output other_changed_files_count = 0
##[debug]Set output all_modified_files = site/src/routes/[mother=locale]/[learning=locale]/\(app\)/account/+page.svelte
##[debug]Set output all_modified_files_count = 1
##[debug]Set output any_modified = true
##[debug]Set output only_modified = true
##[debug]Set output other_modified_files = 
##[debug]Set output other_modified_files_count = 0
##[debug]Set output deleted_files = 
##[debug]Set output deleted_files_count = 0
##[debug]Set output any_deleted = false
##[debug]Set output only_deleted = false
##[debug]Set output other_deleted_files = 
##[debug]Set output other_deleted_files_count = 0
##[debug]Finished: run

Has all relevant logs been included?

  • I've included all relevant logs

Anything else?

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
jacob-8 commented

I guess the real issue is in https://github.com/tj-actions/changed-files - I'll try to add a failing test there and see if I can't submit a PR to fix it.

jacob-8 commented

This passes just fine so no issue here:

// Tests that getFilteredChangedFiles correctly handles parenthesis in folder names
it('should not escape parenthesis in folder names', async () => {
  const allDiffFiles = {
    [ChangeTypeEnum.Added]: [],
    [ChangeTypeEnum.Copied]: [],
    [ChangeTypeEnum.Deleted]: [],
    [ChangeTypeEnum.Modified]: [
      'src/routes/[learning=locale]/(app)/+page.svelte'
    ],
    [ChangeTypeEnum.Renamed]: [],
    [ChangeTypeEnum.TypeChanged]: [],
    [ChangeTypeEnum.Unmerged]: [],
    [ChangeTypeEnum.Unknown]: []
  }
  const filePatterns = ['**/*.svelte']
  const filteredFiles = await getFilteredChangedFiles({
    allDiffFiles,
    filePatterns
  })
  expect(filteredFiles[ChangeTypeEnum.Modified]).toEqual([
    'src/routes/[learning=locale]/(app)/+page.svelte'
  ])
})
jacob-8 commented

The issue happens in this block. So either in the inputs.escapeJson or the JSON.stringify or the getChangeTypeFiles function. That's about as far as I can debug. But yes, definitely this issue should be moved to https://github.com/tj-actions/changed-files as the issue is either in that package or me not knowing how to use the settings properly.

image

jackton1 commented

Hi @jacob-8 This issue has been resolved in the latest release.

You'll need to set the escape_paths input to false explicitly

      - name: Run ESLint on changed files
        uses: tj-actions/eslint-changed-files@v25
        with:
          ...
          escape_paths: false
          ...

For some context what you are referring to as a bug is an expected behaviour to prevent command injection via filenames. You can read more about it here

  • 👍1