avoid fread on directories and unencrypted files

[jknockaert]

Reworking the logic in order to first check the filecache and only then reading the fileheader.
This in order to solve #21578.
Signed-off-by: Jasper Knockaert jasper@knockaert.nl

[jknockaert]

@nextcloud/encryption Pls review. Also check the discussion of the issue I posted in #21578. That said, I didn't yet find out why this function is evaluated very often for keyfiles.

[jknockaert]

Signed-off-by: Jasper Knockaert jasper@knockaert.nl

[jknockaert]

Can anyone help with the testing? It is my understanding that in the tests the call to the cache was never evaluated, so I guess that should somewhere be caught now that it comes first.

[jknockaert]

OK, with all checks passed now I think this one is ready for review. As for the getHeader function I'm pretty sure I fully understand the code and I think my implementation is fine; but still then I don't have a full overview of all use cases of this function so I may miss out on a corner case. As for the tests I somehow got it working; not sure though if I fully understand what I did here. I couldn't do any meaningful benchmarking so I can't say much on the impact of the changes introduced. I wouldn't expect it to matter much.

[ghost]

@jknockaert I applied the below fix and im not getting the errors anymore

[RedKage]

I dont know what this is doing, nor am I a nextcloud maintainer.
However I think that IF can be simplified and be made more readable with

    if (!isset($result[Util::HEADER_ENCRYPTION_MODULE_KEY] && (!empty($result) || $exists)) {
        $result[Util::HEADER_ENCRYPTION_MODULE_KEY] = 'OC_DEFAULT_MODULE';
    }

[jknockaert]

@RedKage Thanks; fixed.

[jknockaert]

@ChristophWurst Any suggestion who to invite for a review?

[jknockaert]

@nextcloud/encryption Can anyone review? This is a fairly minor patch that fixes errors with php7.4. It would be good to have it included in the next point release of nextcloud.

[moverduinsecov]

Have applied changes manually, and so far everything looks fine.

[jknockaert]

Can anyone review? This is a minor (and straightforward) patch that fixes errors with php7.4. It would be good to have it included in the next point release of nextcloud.

[vitormattos]

@icewind1991

I believe that this should be fine, but I haven't looked into the encryption logic for a long time

If the test passes, what's wrong? :-)

[vitormattos]

It seems to be ok, there seems to be no need to create new test scenarios.

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[jknockaert]

Thanks @vitormattos and @icewind1991 for the reviews.

[vitormattos]

It is in the same class but does not seem to be related to this pull request.

Looking at the log quickly, without a thorough analysis, the method changed in this pull request is not in the callstack of the log you posted.

[vitormattos]

I suggest sending a new pull request solving this problem :)

The solution is already described in the log, it is necessary to implement, check the impacts, review the unit tests to ensure they are ok and send the pull request.

[ghost]

I just did! i tagged you in it so maybe we can have someone look at it quicker :)

[vitormattos]

You have opened an issue. Submit the pull request with the solution. The correction is simple, but it is necessary to check the impacts well.

[ghost]

@vitormattos i would not know the solutions i don't code. maybe @jknockaert knows it but i dont

[jknockaert]

@axheli Pls tag me. I may have time to have a look at it over the weekend.
That said, I think the filesystem code is in some need for a more thorough review and possibly rewrite.

[vitormattos]

I just did! i tagged you in it so maybe we can have someone look at it quicker :)

@jknockaert #26464

[ghost]

@jknockaert Thank you so much :)

[ghost]

Still an issue on Nextcloud 21.0.2 @LukasReschke can we please add this fix ?

[solracsf]

/backport to stable21

[solracsf]

/backport to stable20

[solracsf]

/backport to stable19

[ghost]

Hello everyone is anybody willing to add this fix to the Next release 21.0.4 ?