Add support for LGTM_INDEX_FILTERS #103

robertbrignull · 2020-07-07T14:31:29Z

This is only possible solution to double-globs in path filters. We make use of the LGTM_INDEX_FILTERS environment variable, as well as the two others that we already use, because LGTM_INDEX_FILTERS does support **.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.

robertbrignull · 2020-07-07T14:41:57Z

I made a test repo for this change at https://github.com/dsp-testing/robertbrignull-filter-test and you can see the changes that switches to this branch makes at https://github.com/dsp-testing/robertbrignull-filter-test/pull/1/checks?check_run_id=845738229

robertbrignull · 2020-07-08T14:08:00Z

@aibaars, would you be able to give this a review? Can also ask someone else if you'd rather not.

aibaars · 2020-07-08T15:18:25Z

src/analysis-paths.ts

+  // It does not understand double-globs because that would require it to
+  // traverse the entire file tree to determine which files are matched.
  if (config.paths.length !== 0) {
    core.exportVariable('LGTM_INDEX_INCLUDE', config.paths.join('\n'));


The LGTM_INDEX_INCLUDE and LGTM_INDEX_EXCLUDE properties don't support any form of wildcards. I think you should only put proper paths into these fields. The * symbol is not allowed in paths on Windows, so you might even get an exception.

I just remembered that there is some good documentation about these settings in the javascript autobuilder: https://github.com/github/codeql/blob/bfb734e1d74df1ae810feb7198cb271b767c4b9a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java#L80

Thanks. Makes sense to me. I'll filter those out of here.

I'd also forgotten that the JS extractor is public so thanks for that link.

aibaars · 2020-07-09T08:30:21Z

src/config-utils.ts

+
+// Checks that a paths of paths-ignore entry is valid, possibly modifying it
+// to make it valid, or if not possible then throws an error.
+export function validateAndSanitisePath(originalPath: string, propertyName: string, configFile: string): string {


I would also check for the special characters mentioned in https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet that are not supported by the underlying project layout implementation. Just report an error telling that these are not allowed, to avoid users getting confused why their perfectly sensible pattern string does not match any files.

aibaars · 2020-07-09T08:32:04Z

src/analysis-paths.ts

+
+// Builds an environment variable suitable for LGTM_INDEX_INCLUDE or LGTM_INDEX_EXCLUDE
+function buildIncludeExcludeEnvVar(paths: string[]): string {
+  return paths.filter(p => p.indexOf('**') === -1).join('\n');


This should filter out paths containing any kind of special matching character mentioned in https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet .

For windows I'd also paths containing any the forbidden characters mentioned in https://stackoverflow.com/a/31976060 . Otherwise things will crash at some point with a less friendly error message.

aibaars · 2020-07-09T08:37:28Z

This looks fine to me. Two more suggestions to add more checks to improve or avoid error messages.

aibaars · 2020-07-09T16:26:17Z

src/config-utils.ts

+  // Check for any characters that are illegal in path names
+  if (process.platform === 'win32' && path.match(illegalWindowsCharactersRegex)) {
+    core.warning('"' + path + '" contains an invalid character and will be ignored. ' +
+        'The characteres <, >, :, ", !, |, ?, * are forbidden to use in paths on windows.');


characteres -> characters

You are no longer filtering *, so best to adjust the error message.

I've moved this to analysis-paths.ts and kept it as not containing * because that is checked for separately on all platforms.

aibaars · 2020-07-09T16:32:36Z

src/config-utils.ts

+  }
+
+  // Check for any characters that are illegal in path names
+  if (process.platform === 'win32' && path.match(illegalWindowsCharactersRegex)) {


I meant for this check to be performed when adding a path to the exclude/include env vars. The paths in those env variables will be interpreted as path strings by the autobuilders and should therefore only contain valid path characters.

However, it should be fine here too as you no longer fail on *.

I've moved this check to when we construct the env vars. After some thought I agree it should be here and the right thing to do is to ignore it. Since the path can't legally exist anyway there should be no effect from not adding it to the include/exclude env vars.

I'd also prefer to not output any kind of error/warning about this because as explained above it shouldn't affect anything as the path legally can't exist anyway.

aibaars · 2020-07-09T16:34:50Z

src/config-utils.ts

+
+// Characters that are supported by filters in workflows, but not by us.
+// See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet
+const filterPatternCharactersRegex = /.*[\?\+\[\]!].*/;


you might also want to exclude \ . I think excluding windows-style path separators would be helpful to ensure patterns mean the right thing cross platform.

I'd prefer to not do any special handling for \ for now. We can add that in later. This PR is getting complicated enough as it is and I don't understand exactly what you're proposing. I recommend once this PR is in it would be easiest for you to make the change you're suggesting for \/

I suggest not allowing \ symbols at all. I don't know what the project layouts do with windows path separators and a windows user can just write /. I think it is better to be restrictive in what we accept now, and perhaps make things more liberal later if there is any use (which I doubt).

I see the point that windows users can use \ instead, and it's nice to be restrictive in the start. I do note that linux paths can legitimately contain \ characters (although it is rather unlikely) so we would be actively preventing people from using filters with files with those characters.

Ok, I'll just ban them. I don't anticipate many problems if any from file legitimately containing this character, so let's hope it's fine. As you say we can make it less restrictive in the future and that's easier than going the other way.

aibaars · 2020-07-09T16:38:11Z

src/analysis-paths.ts

+
+// Builds an environment variable suitable for LGTM_INDEX_INCLUDE or LGTM_INDEX_EXCLUDE
+function buildIncludeExcludeEnvVar(paths: string[]): string {
+  return paths.filter(p => p.indexOf('**') === -1).join('\n');


Suggested change

return paths.filter(p => p.indexOf('**') === -1).join('\n');

return paths.filter(p => p.indexOf('*') === -1).join('\n');

Paths with either * or ** make no sense in the include/exclude env variables.

Thanks. You're right and this now checks for any instances of *.

robertbrignull · 2020-07-10T10:17:54Z

Ok, I think this is finally ready. I've addressed all the comments from yesterday, I've just had another look through, and my tests over at https://github.com/dsp-testing/robertbrignull-filter-test are working as expected.

@aibaars, do you want to give this another sanity check? Otherwise I will merge today in a little bit.

aibaars · 2020-07-10T11:20:56Z

src/config-utils.ts

+  while (path.charAt(0) === '/') {
+    path = path.substring(1);
+  }
+


I suppose empty paths "" are fine and would be interpreted as ".", but might be good to check this and add a test case for both JS and Python analysis runs with such a path.

I've tested manually and including / seems to be fine.

aibaars · 2020-07-10T11:21:24Z

Looks good to me.

robertbrignull · 2020-07-10T13:48:42Z

I'm going to add some more filters to the integration. Can't believe I forgot to do that until now. Then I'll merge this.

robertbrignull · 2020-07-10T14:15:14Z

Seems while javascript is fine, the python autobuilder code borks if you include certain characters like *, ?, or < anywhere in one of the filters. I'm pretty sure this happens on the main branch too so this PR doesn't make things any worse.

robertbrignull · 2020-07-10T14:17:45Z

Also seems the python case throws an error if the INCLUDE env var includes paths to files, because it only accepts directories.

robertbrignull · 2020-07-10T14:34:43Z

Turns out adding / is fine on javascript but causes an error for python. So I take it back and let's ban that path. Also now turns out there's basically nothing useful I can add an integration tests as everything even slightly interesting causes errors for python.

Regardless, this PR is going to be merged as I've done enough testing now manually. In the future this situation needs to be sorted out to bring the two languages more in line because currently the behaviour is unacceptably different.

robertbrignull · 2020-07-10T14:47:11Z

I don't believe this PR will make any configs that were previous valid to be invalid, so from that point of view this is a safe change. There is a reasonable chance that we will find bugs or otherwise want to make changes to this area, but we should be able to response and address those problems when we find them.

Add support for LGTM_INDEX_FILTERS

fe3dbb7

give better error messages

dcebdd6

robertbrignull assigned aibaars Jul 8, 2020

robertbrignull added 2 commits July 8, 2020 15:11

clarify error slightly more

9fb69dd

trim leading slashes

abf6f23

aibaars reviewed Jul 8, 2020

View reviewed changes

filter ** paths

56417be

aibaars reviewed Jul 9, 2020

View reviewed changes

aibaars approved these changes Jul 9, 2020

View reviewed changes

robertbrignull added 2 commits July 9, 2020 10:30

Merge branch 'main' into lgtm_filters

d781c66

output better errors

bf5b437

aibaars approved these changes Jul 9, 2020

View reviewed changes

Make characters completely illegal on windows

70980b9

robertbrignull force-pushed the lgtm_filters branch from 9a357fd to 70980b9 Compare July 9, 2020 16:40

robertbrignull added 3 commits July 9, 2020 18:05

move checking to when env vars are constructed

24367a8

ban backslashes

60126bf

adjust comments

fb9f2af

aibaars reviewed Jul 10, 2020

View reviewed changes

aibaars approved these changes Jul 10, 2020

View reviewed changes

Merge branch 'main' into lgtm_filters

1a4c658

robertbrignull force-pushed the lgtm_filters branch 2 times, most recently from d5d447f to 64ca715 Compare July 10, 2020 14:13

robertbrignull force-pushed the lgtm_filters branch from 64ca715 to a1ba2b1 Compare July 10, 2020 14:17

ban / as a path

189b6ef

robertbrignull force-pushed the lgtm_filters branch from a1ba2b1 to 189b6ef Compare July 10, 2020 14:33

robertbrignull merged commit c3dcf26 into main Jul 10, 2020

robertbrignull deleted the lgtm_filters branch July 10, 2020 14:47

github-actions bot mentioned this pull request Jul 13, 2020

Merge main into v1 #108

Merged

aliscco mentioned this pull request Apr 24, 2023

[Snyk] Fix for 5 vulnerabilities aliscco/codeql-action#62

Open

snyk-bot mentioned this pull request Apr 24, 2023

[Snyk] Fix for 5 vulnerabilities aliscco/codeql-action#64

Open

This was referenced Apr 24, 2023

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#75

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#76

Open

This was referenced Apr 24, 2023

[Snyk] Fix for 3 vulnerabilities aliscco/codeql-action#83

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#84

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#86

Open

aliscco mentioned this pull request Apr 24, 2023

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#88

Open

This was referenced Apr 24, 2023

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#90

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#92

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#95

Open

	return paths.filter(p => p.indexOf('**') === -1).join('\n');
	return paths.filter(p => p.indexOf('*') === -1).join('\n');

Add support for LGTM_INDEX_FILTERS #103

Add support for LGTM_INDEX_FILTERS #103

Uh oh!

Conversation

robertbrignull commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge / deployment checklist

Uh oh!

robertbrignull commented Jul 7, 2020

Uh oh!

robertbrignull commented Jul 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aibaars commented Jul 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbrignull commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aibaars commented Jul 10, 2020

Uh oh!

robertbrignull commented Jul 10, 2020

Uh oh!

robertbrignull commented Jul 10, 2020

Uh oh!

robertbrignull commented Jul 10, 2020

Uh oh!

robertbrignull commented Jul 10, 2020

Uh oh!

robertbrignull commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robertbrignull commented Jul 7, 2020 •

edited

Loading