chore(eslint-plugin): rule against invalid paths

[kaizencc]

This is a follow up to #28658, #28772, and #28760. We had to fix multiple places where that file path extended beyond the package itself into other areas of the local repository (that would not be available after packaging). This caused myriad issues at synth time with file not found errors.

This PR introduces a linter rule with the following specifications:

• no inefficient paths, i.e. no going backwards multiple times. Ex. path.join(__dirname, '..', 'folder', '..', 'another-folder'). This should and can be easily simplified
• no paths that go backwards past a package.json file. This should catch the instances we faced next time.

The yarn lint command on aws-cdk-lib took 51.47s seconds without this new rule and 53.32s seconds with the rule enabled. The difference of ~2 seconds shouldn't be a hindrance in this case but I am happy to look for additional efficiencies in the rule I've written.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[paulhcsun]

Looks good, just one minor comment.

[rix0rrr]

First: is this check against oscillating worth the squeeze? (Or is this just to make the subsequent checks easier? If so, I can dig that. Might want to write that down.)

Second: can this not be simplified in the following way:

• .. may only occur in the path prefix.
• So any .. whose index is after the first non-.. is incorrect:

const components = confirmFirstArgumentIsDirnameAndRemove(node.arguments);

const firstDownDir = components.findIndex((p) => p !== '..');
if (components.some((p, i) => p === '..' && i > firstDownDir) {
  // ERROR: Oscillating paths
  return;
}

Then we can observe that package.json may only exist in the very last directory that we ended up with by doing ..: if it occurs anywhere earlier that's an error:

// Note the - 1 to exclude the last directory from the check
// Exclude the case where there are no '..' at all in the path -- those are never invalid
if (firstDownDir > 0) {
  for (let i = 0; i < firstDownDir - 1; i++) {
    const pjFile = path.join(...[path.dirname(currentFile), ...components.slice(0, i), 'package.json']);
    if (existsSync(pjFile)) {
      // ERROR: this path will end up going out of the package.json directory
      return;
    }
  }
}

[rix0rrr]

Take a moment to reason through this table to see how this works:

// components    | firstDownDir | closest invalid package.json
['x']              0              /* N/A */
['..', 'x']        1              'package.json'
['..', '..', 'x']  2              '../package.json'

[rix0rrr]

Oh you should also add a check that / doesn't occur anywhere inside any of the components, otherwise our checks are going to fail:

path.join(__dirname, '../../../..', 'banana')

Is bad style but works, and anything that accidentally works will occur in our code base 😅. We need to guard against it.

[rix0rrr]

Are we confirming anywhere that the first argument to path.join() is __dirname ? I can't find it

[kaizencc]

Yes, in isPathJoinFuncCall:

function isPathJoinFuncCall(node: any): boolean {
  return (
    node.callee?.property?.name === 'join' &&
    node.arguments.length > 1 &&
    node.arguments[0].name &&
    node.arguments[0].name === '__dirname'
  )
}

[kaizencc]

I assume that we don't have any rule that says path.join must have __dirname as the first argument, but rereading your comment suggests that that may be indeed what you are suggesting

[rix0rrr]

No, but just saying that we don't need to do any checks on path.joins where the first argument ISN'T __dirname.

[kaizencc]

I think this is the correct implementation of your algo @rix0rrr

[rix0rrr]

Yeah, you are right. Good catch!

I suppose we could also do i in [1..firstDownDir) as opposed to [0..firstDownDir - 1), and that would have avoided this i + 1.

[rix0rrr]

Conditionally approved! I'll leave you to decide what to do with the i + 1/i - 1 business

[rix0rrr]

Yeah, you are right. Good catch!

I suppose we could also do i in [1..firstDownDir) as opposed to [0..firstDownDir - 1), and that would have avoided this i + 1.

[mrgrain]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: a8e6610
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).