[FEATURE] LocalFileExtractor block #478

OmarFourati · 2023-12-01T09:15:10Z

Related to #393

Create logic and a new extractor block that would take care of local files

Closes #393

github-actions · 2023-12-01T09:15:27Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

OmarFourati · 2023-12-01T09:18:38Z

I have read the CLA Document and I hereby sign the CLA

rhazn · 2023-12-04T13:43:17Z

For clarity, this is in Draft and you wanted to fix the high level things we spoke about first, right? Then I'll first remove myself as a reviewer and you please readd me when you think the PR is ready. @OmarFourati

OmarFourati · 2023-12-26T10:52:55Z

@rhazn didn't manage to repair the pipeline.
It fails because of file-util not being found but it follows the exaxt logic of other files and should definitely be found.
I need help there to be able to get review on this PR

rhazn

I think we are really close, well done!

We still have to:

Ensure that no path traversal up can happen using regex instead of .startsWith (and test for it)
Get the CI pipeline to work again

Regarding the CI pipeline I am honestly not sure what is happening there, the import looks identical to the HttpExtractor to me. But it has to work. Does it work locally for you?

libs/extensions/std/exec/src/local-file-extractor-executor.spec.ts

rhazn · 2023-12-27T08:51:41Z

libs/extensions/std/exec/src/local-file-extractor-executor.ts

+//
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import assert = require('assert');


We import import { strict as assert } from 'assert';, not the actual assert lib of node.

rhazn · 2023-12-27T08:54:59Z

libs/extensions/std/exec/src/local-file-extractor-executor.ts

+        rawData.buffer as ArrayBuffer,
+      );
+
+      assert(file instanceof BinaryFile);


Not sure this is a useful assert, it is created as binary file above so this should always be true?

rhazn · 2023-12-27T08:56:13Z

libs/extensions/std/exec/src/local-file-extractor-executor.ts

+    );
+
+    try {
+      const rawData = await fs.readFile(filePath);


Would be nice to add a context log message here like Successfully extraced file {path}, similar how it is done in the HttpExtractor.

rhazn · 2023-12-27T08:57:13Z

libs/extensions/std/exec/test/assets/local-file-extractor-executor/invalid-file-not-found.jv

+  }
+
+  block TestBlock oftype LocalFileExtractor {
+    filePath: './test.csv';


Better naming would be ./does-not-exist.csv which makes it obvious what the point is.

rhazn · 2023-12-27T08:58:27Z

libs/language-server/src/lib/validation/checks/blocktype-specific/property-assignment.ts

+  validationContext: ValidationContext,
+) {
+  if (propName === 'filePath') {
+    if (typeof propValue === 'string' && propValue.startsWith('../')) {


This is not a correct check, you need to ensure there are no .. anywhere in the path. The following path is valid for you but actually traverses up: ./../../something.csv.

You'll need to use regex here (check for the pattern .. to exist in the string I assume? Not sure, but I am sure that is something that is easy to google). This is security relevant so has to be fixed.

rhazn · 2023-12-27T09:00:34Z

libs/extensions/std/exec/src/local-file-extractor-executor.spec.ts

There are tests missing for invalid file path properties that should also include edge cases. I'd suggest ../path-traversal-up.csv and ./../../path-traversal-up.csv.

I sadly don't understand exactly what or how this should be implemented. With the regex implementation, I thought all the edge cases would through the error of the path traversal.
Need a hint there.
Thanks

You should add tests that verify that you check the property correctly. This is done already in the ZipArchiveInterpreter we spoke about before. The two paths I mentioned in my original comment would be good examples to test, because they are both invalid and should throw errors related to invalid paths (not missing file).

I do not know what you mean by "With the regex implementation, I thought all the edge cases would through the error of the path traversal.", the test cases are to verify that is true, independently of the implementation.

rhazn · 2023-12-27T09:01:53Z

libs/language-server/src/stdlib/builtin-blocktypes/LocalFileExtractor.jv

+// SPDX-License-Identifier: AGPL-3.0-only
+
+/**
+* Extracts a `File` from the local storage.


Confusing naming since localstorage exists in browsers. I'd speak of file system.

Suggested change

* Extracts a `File` from the local storage.

* Extracts a `File` from the local file system.

rhazn · 2023-12-27T09:03:21Z

libs/language-server/src/stdlib/builtin-blocktypes/LocalFileExtractor.jv

+	output default oftype File;
+
+	/**
+	* The PATH to the file in the local storage to extract.


Why PATH? There is a PATH env variable on basicaly any operating system so this is needlessly confusing. Just go with path. Same comment about localstorage as for the block description as well.

Suggested change

* The PATH to the file in the local storage to extract.

* The path to the file in the local file system to extract.

rhazn

I think we are really close, well done!

We still have to:

Ensure that no path traversal up can happen using regex instead of .startsWith (and test for it)
Get the CI pipeline to work again

Regarding the CI pipeline I am honestly not sure what is happening there, the import looks identical to the HttpExtractor to me. But it has to work. Does it work locally for you?

OmarFourati · 2023-12-27T18:02:24Z

I think we are really close, well done!

We still have to:

Ensure that no path traversal up can happen using regex instead of .startsWith (and test for it)

Get the CI pipeline to work again

Regarding the CI pipeline I am honestly not sure what is happening there, the import looks identical to the HttpExtractor to me. But it has to work. Does it work locally for you?

Yes it does. Locally all the tests and everything runs okey.
I don't understand neither. the util import doesn't show any error locally as well

rhazn · 2023-12-28T13:33:03Z

libs/language-server/src/lib/validation/checks/blocktype-specific/property-assignment.ts

@@ -264,7 +264,9 @@ function checkLocalFileExtractorProperty(
  validationContext: ValidationContext,
 ) {
  if (propName === 'filePath') {
-    if (typeof propValue === 'string' && propValue.startsWith('../')) {
+    const validPathRegex = /^(?!.*\.\.\/).*$/;


It would be much easier to test for an invalid path (which is anything that does include two dots I assume but please google a bit for if that makes sense). So I'd invert this check and check for invalid paths and if that is true, throw the error.

rhazn · 2023-12-28T13:35:22Z

libs/extensions/std/exec/src/local-file-extractor-executor.spec.ts

You should add tests that verify that you check the property correctly. This is done already in the ZipArchiveInterpreter we spoke about before. The two paths I mentioned in my original comment would be good examples to test, because they are both invalid and should throw errors related to invalid paths (not missing file).

I do not know what you mean by "With the regex implementation, I thought all the edge cases would through the error of the path traversal.", the test cases are to verify that is true, independently of the implementation.

Merge branch 'main' into feature/local-file-extractor

Moved changes from old PR that is now out of sync with main (#478) to a fresh PR, based on latest main commit. Co-authored-by: OmarFourati <[email protected]>

rhazn · 2024-02-06T15:50:05Z

Closed in favor of #519 so we can work in a cleaner branch.

OmarFourati marked this pull request as draft December 1, 2023 10:03

OmarFourati added the softwarecampus Issues related to the Softwarecampus grant label Dec 1, 2023

OmarFourati self-assigned this Dec 1, 2023

OmarFourati requested a review from rhazn December 1, 2023 13:32

rhazn removed their request for review December 4, 2023 13:42

OmarFourati marked this pull request as ready for review December 21, 2023 13:40

OmarFourati requested a review from rhazn December 23, 2023 15:48

OmarFourati force-pushed the feature/local-file-extractor branch from b792b9d to 18cb347 Compare December 26, 2023 10:08

rhazn requested changes Dec 27, 2023

View reviewed changes

OmarFourati requested a review from rhazn December 27, 2023 18:02

rhazn requested changes Dec 28, 2023

View reviewed changes

OmarFourati added 13 commits January 9, 2024 12:22

create first logic for local file extractor

820ef32

delete useless variables for block

bbcb067

deleted unused logic in block code

1ecf9d8

fix linting problems

dbe2568

refactor: update documentation for local file extractor block

1fe0503

refactor: move test files and block to assets

0874d79

add: tests on error for path traversal

d0810aa

refactor: update tests for local extractor

6ded048

fix: success of first test for local extractor

d105fad

fix: success on test for traversal path

3a5387e

fix checks and tests and delete useless files

6c0adc5

update licensing

e41208c

remove file extension logic

0f9374a

OmarFourati added 3 commits January 9, 2024 12:22

restore extension variables on local file extractor

3ba7f07

revew changes

02f8242

update property assignement for path traversal

6904ec0

OmarFourati force-pushed the feature/local-file-extractor branch from 6b16cb6 to 6904ec0 Compare January 9, 2024 11:22

OmarFourati added 2 commits January 12, 2024 14:09

refactor: create new test cases and files

37fbbbd

fix path for inferfileextension module

9e5ee3a

OmarFourati force-pushed the feature/local-file-extractor branch from f4b57ef to 9e5ee3a Compare January 12, 2024 15:25

OmarFourati and others added 9 commits January 20, 2024 12:15

fix: linting

53425c3

fix: fix tests local extractor

9b71d41

fix: add license file

3c13bf9

fix: fix naming license file

5543bea

bash: line 1: wq: command not found

32f705a

Merge branch 'main' into feature/local-file-extractor

fix tests for path traversal

da85116

Merge branch 'main' into feature/local-file-extractor

e10d229

merge main and sync fork

70d4f71

Merge remote-tracking branch 'origin/main' into pr/478

d26f6b7

rhazn added a commit that referenced this pull request Feb 6, 2024

Moved changes to new branch

e017940

Moved changes from old PR that is now out of sync with main (#478) to a fresh PR, based on latest main commit. Co-authored-by: OmarFourati <[email protected]>

rhazn mentioned this pull request Feb 6, 2024

LocalFileExtractor #519

Merged

rhazn closed this Feb 6, 2024

github-actions bot locked and limited conversation to collaborators Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] LocalFileExtractor block #478

[FEATURE] LocalFileExtractor block #478

OmarFourati commented Dec 1, 2023 •

edited

Loading

github-actions bot commented Dec 1, 2023 •

edited

Loading

OmarFourati commented Dec 1, 2023

rhazn commented Dec 4, 2023

OmarFourati commented Dec 26, 2023

rhazn left a comment

rhazn Dec 27, 2023

rhazn Dec 27, 2023

rhazn Dec 27, 2023

OmarFourati Dec 27, 2023

rhazn Dec 27, 2023

rhazn Dec 27, 2023

rhazn Dec 27, 2023

rhazn Dec 27, 2023

OmarFourati Dec 27, 2023

rhazn Dec 28, 2023

rhazn Dec 27, 2023

rhazn Dec 27, 2023

rhazn left a comment

OmarFourati commented Dec 27, 2023

rhazn Dec 28, 2023

rhazn Dec 28, 2023

rhazn commented Feb 6, 2024

	* Extracts a `File` from the local storage.
	* Extracts a `File` from the local file system.

	* The PATH to the file in the local storage to extract.
	* The path to the file in the local file system to extract.

[FEATURE] LocalFileExtractor block #478

[FEATURE] LocalFileExtractor block #478

Conversation

OmarFourati commented Dec 1, 2023 • edited Loading

github-actions bot commented Dec 1, 2023 • edited Loading

OmarFourati commented Dec 1, 2023

rhazn commented Dec 4, 2023

OmarFourati commented Dec 26, 2023

rhazn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhazn left a comment

Choose a reason for hiding this comment

OmarFourati commented Dec 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhazn commented Feb 6, 2024

OmarFourati commented Dec 1, 2023 •

edited

Loading

github-actions bot commented Dec 1, 2023 •

edited

Loading