Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure we don't scan files that can not be accessed #1972

Merged
merged 2 commits into from
Nov 22, 2016

Conversation

nickvergessen
Copy link
Member

Steps

  1. create a folder on the file system:
mkdir "data/admin/files/te
st"
  1. Run files:scan admin
  2. Try to delete the folder after realizing your problem

With this fix the file is not added on scanning.

@icewind1991 @MorrisJobke @LukasReschke

Fix #1965

@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @DeepDiver1975 and @butonic to be potential reviewers.

@MorrisJobke
Copy link
Member

1) OCA\Files_Sharing\Tests\External\ScannerTest::testScan
Your test case is not allowed to access the database.
2) OCA\Files_Sharing\Tests\External\ScannerTest::testScanFile
Your test case is not allowed to access the database.

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 2, 2016
@nickvergessen
Copy link
Member Author

I will fix that, once @icewind1991 confirmed this is the way to go.

@@ -132,6 +132,24 @@ protected function getData($path) {
*/
public function scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData = null, $lock = true) {

if (!\OC::$server->getDatabaseConnection()->supports4ByteText()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we reuse the fragment from somewhere else (View?) and just call that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was exactly what I wondered aswell, but I think the cache is inside the view, so this looks shitty from dependency pov. But should we just move it to a separate helper or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@icewind1991 should we simply move all the checks from the View into IStorage::verifyPath and in the view then simply resolve the path to the storage and call the method?

@nickvergessen nickvergessen force-pushed the invalid-files-from-scanner branch from 7dcfb6f to 682307c Compare November 9, 2016 09:59
@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 9, 2016
@nickvergessen
Copy link
Member Author

@icewind1991 moved the validation from view to storage so it's in one place only

@nickvergessen nickvergessen force-pushed the invalid-files-from-scanner branch from 682307c to 00fbb5b Compare November 10, 2016 15:49
@MorrisJobke
Copy link
Member

Tested and works 👍

@rullzer @icewind1991 Please review :)

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@rullzer rullzer force-pushed the invalid-files-from-scanner branch from 00fbb5b to 558f169 Compare November 21, 2016 08:24
@rullzer
Copy link
Member

rullzer commented Nov 21, 2016

Fine by me 👍 But lets have our fs guru have the last vote. Summoning @icewind1991

@icewind1991
Copy link
Member

👍

@rullzer rullzer merged commit df21562 into master Nov 22, 2016
@rullzer rullzer deleted the invalid-files-from-scanner branch November 22, 2016 11:55
@MorrisJobke
Copy link
Member

@nickvergessen Could you open the backport PR? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can't delete directory
5 participants