Fix(Webserver) #6225 Added check in file creation path for _flows*

[coderkill]

What changes are being made and why?

Kestra allows the creation of files and directories with names starting with _flows*, despite the system's default restrictions intended to prevent such creations. Additionally, it fails to block the creation of folders starting with _flows*.

How the changes have been QAed?

I manually tested these examples:
Invalid Files and directories test cases:

Subfolder _flows* check:

Positive Test case

Note that this is not a replacement for unit tests but rather a way to demonstrate how the changes work in a real-life scenario, as the end-user would experience them.

closes #6225

[loicmathieu]

Hi,
Thanks for doing this.
I wonder if it would makes sense to only prevent files starting with _flows inside of containing. @brian-mulier-p WDYT?

It needs a test inside the existing NamespaceFileController test.

I also wonder if we should not do the same check in the UI to warn the user before he try to save the file. This is best practice, we usually duplicate this kind of checks in the UI and in the backend.

[coderkill]

Hi, Thanks for doing this. I wonder if it would makes sense to only prevent files starting with _flows inside of containing. @brian-mulier-p WDYT?

It needs a test inside the existing NamespaceFileController test.

I also wonder if we should not do the same check in the UI to warn the user before he try to save the file. This is best practice, we usually duplicate this kind of checks in the UI and in the backend.

Hi,

I would be happy to write the test case but I am not able to run them on my local so hence wasnt able to do it:

I am getting the following error when running NamespaceFileControllerTest on local inside Intellij:

Bean definition [io.kestra.webserver.services.BasicAuthService] could not be loaded: Failed to inject value for parameter [dslContext] of class: io.kestra.jdbc.JooqDSLContextWrapper

Message: Multiple possible bean candidates found: [DSLContext, DSLContext]
Path Taken: new BasicAuthService() --> BasicAuthService.settingRepository --> new H2SettingRepository([H2Repository<Setting T> repository],ApplicationContext applicationContext) --> new H2Repository(JdbcTableConfig jdbcTableConfig,QueueService queueService,[JooqDSLContextWrapper dslContextWrapper]) --> new JooqDSLContextWrapper([DSLContext dslContext],RetryUtils retryUtils)
io.micronaut.context.exceptions.BeanInstantiationException: Bean definition [io.kestra.webserver.services.BasicAuthService] could not be loaded: Failed to inject value for parameter [dslContext] of class: io.kestra.jdbc.JooqDSLContextWrapper

Message: Multiple possible bean candidates found: [DSLContext, DSLContext]
Path Taken: new BasicAuthService() --> BasicAuthService.settingRepository --> new H2SettingRepository([H2Repository<Setting T> repository],ApplicationContext applicationContext) --> new H2Repository(JdbcTableConfig jdbcTableConfig,QueueService queueService,[JooqDSLContextWrapper dslContextWrapper]) --> new JooqDSLContextWrapper([DSLContext dslContext],RetryUtils retryUtils)
	at io.micronaut.context.DefaultBeanContext.initializeContext(DefaultBeanContext.java:2000)
	at io.micronaut.context.DefaultApplicationContext.initializeContext(DefaultApplicationContext.java:314)
	at io.micronaut.context.DefaultBeanContext.configureAndStartContext(DefaultBeanContext.java:3318)
	at io.micronaut.context.DefaultBeanContext.start(DefaultBeanContext.java:345)
	at io.micronaut.context.DefaultApplicationContext.start(DefaultApplicationContext.java:216)
	at io.micronaut.test.extensions.AbstractMicronautExtension.startApplicationContext(AbstractMicronautExtension.java:507)
	at io.micronaut.test.extensions.AbstractMicronautExtension.beforeClass(AbstractMicronautExtension.java:346)
	at io.micronaut.test.extensions.junit5.MicronautJunit5Extension.beforeAll(MicronautJunit5Extension.java:84)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: io.micronaut.context.exceptions.DependencyInjectionException: Failed to inject value for parameter [dslContext] of class: io.kestra.jdbc.JooqDSLContextWrapper

Is there something I am doing wrong?

[loicmathieu]

No, I'm not sure why you have this issue, you can start a discussion on our Slack on the contributors channels so we can help troubleshoot this test run issue.

[coderkill]

I have tried posting the error on slack but to no avail, I have written the test cases but I am not able to run it due to the above error. It seems to be happening for all controller test classes.

[coderkill]

I have pushed my test case as requested

[loicmathieu]

LGTM however I want to discuss if we should not switch to startWith and duplicate the check in the UI internally before merging it.

[brian-mulier-p]

Hello ! Makes sense to use startsWith, just mind about the leading "/" of namespace files :)

[coderkill]

I have done some analysis, The difference in using startsWith from regex is that you will be able to create subfolders with the keyword _flows using startsWith. eg: /abc/_flows is a valid path. The same applies to files. eg /abc/_flows.txt is a valid file. The previous regex logic would stop it. I have made the changes on my local and tested them out.

[coderkill]

Proof of fix:

[coderkill]

The Junit tests have passed, something else has caused the build to fail

[coderkill]

Hi,

Any update on this?

[brian-mulier-p]

Please do the following changes and we're good

[coderkill]

@brian-mulier-p I have made the changes. Please review

[brian-mulier-p]

After further looks I'm not sure we want to do that. The previous implementation allowed to provide multiple forbidden patterns which may be a desirable thing, now you're hardcoding every forbidden paths logic.

Looking at the original issue, I'm not sure it's a true issue, as said in comment we just want to reserve the _flows folder. Folders like _flows{additional} are ok imo. Why do you consider this worth a change?

Sorry btw I didn't spot that you removed the whole multi-pattern check this morning

[coderkill]

The main issue was that it allowed the creation of files with name _flows which is reserved.

The previous logic (the regex logic) didn’t allow entry (can’t open) into _flows* directories but allowed the user to create the directories. So I assumed that logic was intentional and built it on that.

[brian-mulier-p]

Ideally, any _flows{somethingMore} directory is accessible, _flows is not.
For files yes _flows files should be forbidden so there may be a gap that must be filled 👍

[coderkill]

@brian-mulier-p I agree, the above logic is inline with what you have mentioned. It doesn’t allow the creation of file named _flows and it allows directories with name _flows*(including entering it). I will provide the screenshots as well after which we can confirm and merge.

[coderkill]

Valid scenarios:

[coderkill]

Do have a look at the above screenshots. I have outlined what the user sees when trying to create folders and files when using reserved keywords

[brian-mulier-p]

It looks correct except we should remove the error message part where there is "To import flow..." because it only makes sense for the import process. My bad again I didn't see it was wider than just the import so it can just be rolled back.

Still as said it would be great to keep the regex pattern thing because it will allow to extend the forbidden paths later on

[coderkill]

@brian-mulier-p I had previously kept the regex change which makes _flows* as forbidden path. (That means no folder or file will be able created with that regex and any folder with that regex will not be inaccessible like _flows). Do you want me to revert my change?

[brian-mulier-p]

Regex should be adapted to work for all the cases I described above

[brian-mulier-p]

I suggest putting all the tests I said above and then put back the regex and adapt it so that it covers everything

[coderkill]

@brian-mulier-p

private final List forbiddenPathPatterns = List.of(
Pattern.compile("/" + FLOWS_FOLDER + ".*")
);

This is the regex previously for forbidden path So I am a little confused as to what needs to be done. Can you please rehighlight what needs to be done so that I can work on it.

[brian-mulier-p]

The putNamespaceFiles should be left unchanged imo.
For the forbiddenPathsGuard should be left unchanged also but the pattern could be updated to:
Pattern.compile("/" + FLOWS_FOLDER + "(/.*)?$")

This should prevent any access to the flow folder itself or any file in it.
But please add some tests covering every cases we talked about above.

[coderkill]

I am assuming when you say "putNamespaceFiles should be left unchanged", do you mean of my latest changes?

When it comes to the forbiddenPathsGuard, My latest change will not allow opening of _flows directory, Which your regex is also doing, If you want me to use regex instead of startsWith then I can do that. but the behavior will be same as my latest code change.

[brian-mulier-p]

As said we should keep the regex implementation as it's a list of patterns instead of an hardcoded single if, I like the ability to extend in a simple way which paths we want to forbid in the future.
For the "putNamespaceFiles should be left unchanged" I mean literally keep what was there before

[coderkill]

As said we should keep the regex implementation as it's a list of patterns instead of an hardcoded single if, I like the ability to extend in a simple way which paths we want to forbid in the future. For the "putNamespaceFiles should be left unchanged" I mean literally keep what was there before

If we keep putNameSpaceFiles() as before then we will be able to create the file _flows which is what we want to prevent (if we keep it from the original code base, if you meant the changes I have done before please clarify).

The above shows _flows file. (Do ignore other files and folders as screenshot is old)
I understand the regex implementation for the forbiddenPathsGuard and will work on it.

[brian-mulier-p]

If we keep putNameSpaceFiles() as before then we will be able to create the file _flows which is what we want to prevent (if we keep it from the original code base, if you meant the changes I have done before please clarify).

You can just add something for that

[coderkill]

Noted, making changes

[coderkill]

Valid cases

File creation

Folder creation

Have added the junits as requested. Do let me know if I have missed any case

[coderkill]

@brian-mulier-p Any update? Do you have any other suggestions?

[MilosPaunovic]

Brian is going to be off for the next few weeks. @loicmathieu can do the review here, but first, you need to sort the conflict you're having in webserver/src/test/java/io/kestra/webserver/controllers/api/NamespaceFileControllerTest.java file.

[coderkill]

Brian is going to be off for the next few weeks. @loicmathieu can do the review here, but first, you need to sort the conflict you're having in webserver/src/test/java/io/kestra/webserver/controllers/api/NamespaceFileControllerTest.java file.

Done