147 scan on update via events

[unglaublicherdude]

No description provided.

[unglaublicherdude]

In my test, this catched both, the client upload and the ui upload

[unglaublicherdude]

Did a test with compose

---
services:
  nextcloud:
    image: nextcloud:30.0
    container_name: nextcloud
    restart: unless-stopped
    networks: 
      - cloud
    depends_on:
      - nextclouddb
      - redis
    ports:
      - 8081:80
    volumes:
      - ./custom_apps:/var/www/html/custom_apps
      - ./apps:/var/www/html/apps
      - ./html:/var/www/html
      - ./config:/var/www/html/config
      - ./data:/var/www/html/data
    environment:
      - PUID=1000
      - PGID=1000
      - TZ=America/Los_Angeles
      - MYSQL_DATABASE=nextcloud
      - MYSQL_USER=nextcloud
      - MYSQL_PASSWORD=dbpassword
      - MYSQL_HOST=nextclouddb
      - REDIS_HOST=redis

  nextclouddb:
    image: mariadb
    container_name: nextcloud-db
    restart: unless-stopped
    command: --transaction-isolation=READ-COMMITTED --binlog-format=ROW
    networks: 
      - cloud
    environment:
      - PUID=1000
      - PGID=1000
      - TZ=America/Los_Angeles
      - MYSQL_RANDOM_ROOT_PASSWORD=true
      - MYSQL_PASSWORD=dbpassword
      - MYSQL_DATABASE=nextcloud
      - MYSQL_USER=nextcloud
      
  collabora:
    image: collabora/code
    container_name: collabora
    restart: unless-stopped
    networks: 
      - cloud
    environment:
      - PUID=1000
      - PGID=1000
      - TZ=America/Los_Angeles
      - password=password
      - username=nextcloud
      - domain=example.com
      - extra_params=--o:ssl.enable=true
    ports:
      - 9980:9980

  redis:
    image: redis:alpine
    container_name: redis
    volumes:
      - ./redis:/data  
    networks: 
      - cloud
  
  nginx-proxy:
    image: 'jc21/nginx-proxy-manager:latest'
    container_name: nginx-proxy
    environment:
      - PUID=1000
      - PGID=1000
      - TZ=America/Los_Angeles
    restart: unless-stopped
    ports:
      - '80:80'
      - '81:81'
      - '443:443'
    volumes:
      - ./data:/data
      - ./letsencrypt:/etc/letsencrypt

networks:
  cloud:
    name: cloud
    driver: bridge

resulting in this when uploading

[unglaublicherdude]

The HintException is used because that one of two ecxeptions that are not catched by the hook/event-system. Instead they get rethrown and cause the upload to fail.

https://github.com/nextcloud/server/blob/428e7a32b984e67fe929a8cd9ece858bdb4d9902/lib/private/legacy/OC_Hook.php#L89

[unglaublicherdude]

It also blocks for sync-clients

[unglaublicherdude]

I will start implementing the virus scan tomorrow.

[unglaublicherdude]

The order of the relevant events is

1. BeforeNodeCreatedEvent/BeforeNodeUpdatedEent
2. BeforeNodeWrittenEvent

But the behavior is still a bit weird from the users perspective because the UI upload (and the client too) do retries. So when you throw the exception there it will not immediately stop the upload. In the UI it just looks, like the upload takes forever before it is actually declined.

This is basically the code-part where the events are emitted:
https://github.com/nextcloud/server/blob/d61d62b64f3cb1a22cc322fc747f2af3319280c3/apps/dav/lib/Connector/Sabre/File.php#L406

[unglaublicherdude]

It retries 4 times.

[unglaublicherdude]

This might be a better method. It hooks into the WebDav-Server. But will have to test if this also affects the inline editor/creator.

https://github.com/nextcloud/files_downloadlimit/blob/master/lib/AppInfo/Application.php#L34C1-L35C1

[unglaublicherdude]

The BeforeNodeCreateEvent will also not be emitted, when you update an existing file, like reupload the same file, so we have to listen on the BeforeNodeWrittenEvent.

[unglaublicherdude]

With the Sabre Plugin we can also return a proper error message.

[unglaublicherdude]

Also shown in sync clients.

[unglaublicherdude]

This solution also does not work for files created with the inline editor. But it does work for overwriting a file.

[unglaublicherdude]

Ok. Last update for today. You can basically do the same thing without a plugin within the eventlistener.

And with that you can also block editor files from beeing created. But without a specific error message, at least without any further investigation about that. I guess it would be better to not scan the file on beforeCreation beacuse its completely empty there.

@lennartdohmann I would like to work that out together with you on monday. On our dev-machine, there is already a version which deletes the Server-Plugin stuff and the AvirWrapper.

[lennartdohmann]

I expect this Closes #147 and Closes #85

[lennartdohmann]

@unglaublicherdude With the NodeWrittenEvent, scanning works both via the UI and via the client upload. Tagging, deletion in the malicious case and also the error message in the client work as expected with my pushed implementation. In addition, a file that is created via the UI is also scanned and tagged. Unfortunately, the file always ends up on the instance for a short moment before deletion. Also overwriting files triggers a rescan as we would expect.
However, I will test other events first and see how often the scanning is triggered on a production-ready instance. Also, the BATS tests fail because the error message is different now.

[lennartdohmann]

So still WIP

[unglaublicherdude]

The "add some more files to ignore" commit with ID 523a58e has broken the settings page but only the UI. Changing settings via CLI tools still works so the app also still functions and behaves normal. So if we had released it that way, the app would have been broken for the end customer. I tested it again on a production instance and noticed this. I was able to trace the error back to this point in the scoper.inc.php from the above mentioned commit:

iterator_to_array(
		Finder::create()->files()->in(__DIR__.'/templates'),
		false,
	),

Not ignoring the templates folder leads to an insertion of a namespace like this:

I will fix this with an auxiliary revert and try to keep as much as I can from the ignored files.

weird because I tested that. The code you mention there is the one that should lead to ignoring the templates (all of them) , because it basically generates an array with all the files of the templates folder. Better solution, than to ignore the templates file by file.

There are also examples in the scoper-repository, like this:
https://github.com/humbug/php-scoper/blob/79b2b4e0fbc1d1ef6ae99c4e078137b42bd43d19/fixtures/set027-laravel/scoper.inc.php#L9

[lennartdohmann]

The "add some more files to ignore" commit with ID 523a58e has broken the settings page but only the UI. Changing settings via CLI tools still works so the app also still functions and behaves normal. So if we had released it that way, the app would have been broken for the end customer. I tested it again on a production instance and noticed this. I was able to trace the error back to this point in the scoper.inc.php from the above mentioned commit:

iterator_to_array(
		Finder::create()->files()->in(__DIR__.'/templates'),
		false,
	),

Not ignoring the templates folder leads to an insertion of a namespace like this:

I will fix this with an auxiliary revert and try to keep as much as I can from the ignored files.

weird because I tested that. The code you mention there is the one that should lead to ignoring the templates (all of them) , because it basically generates an array with all the files of the templates folder. Better solution, than to ignore the templates file by file.

There are also examples in the scoper-repository, like this: https://github.com/humbug/php-scoper/blob/79b2b4e0fbc1d1ef6ae99c4e078137b42bd43d19/fixtures/set027-laravel/scoper.inc.php#L9

Yes I noticed that. But something did not work on that, as you can see in the screenshot, with this scoper.inc.php the files are not ignored and a namespace was inserted.

[unglaublicherdude]

[...]
weird because I tested that. The code you mention there is the one that should lead to ignoring the templates (all of them) , because it basically generates an array with all the files of the templates folder. Better solution, than to ignore the templates file by file.
There are also examples in the scoper-repository, like this: https://github.com/humbug/php-scoper/blob/79b2b4e0fbc1d1ef6ae99c4e078137b42bd43d19/fixtures/set027-laravel/scoper.inc.php#L9

Yes I noticed that. But something did not work on that, as you can see in the screenshot, with this scoper.inc.php the files are not ignored and a namespace was inserted.

I just commited a working version again.

[unglaublicherdude]

If this is not urgent I would still like to test the Cache-Deletion to avoid the files from being available in the ui.

[lennartdohmann]

If this is not urgent I would still like to test the Cache-Deletion to avoid the files from being available in the ui.

I would like to merge and backport this today, the linked issue is open for more than a month now. @unglaublicherdude can you/we try this on a new branch?

[unglaublicherdude]

If this is not urgent I would still like to test the Cache-Deletion to avoid the files from being available in the ui.

I would like to merge and backport this today, the linked issue is open for more than a month now. @unglaublicherdude can you/we try this on a new branch?

I am testing right now. So if possible I would like to ship it directly with this PR

[unglaublicherdude]

If this is not urgent I would still like to test the Cache-Deletion to avoid the files from being available in the ui.

I would like to merge and backport this today, the linked issue is open for more than a month now. @unglaublicherdude can you/we try this on a new branch?

I am testing right now. So if possible I would like to ship it directly with this PR

There is also allot of unused stuff in the EventListener. I will also throw that stuff out.

[unglaublicherdude]

If this is not urgent I would still like to test the Cache-Deletion to avoid the files from being available in the ui.

I would like to merge and backport this today, the linked issue is open for more than a month now. @unglaublicherdude can you/we try this on a new branch?

I am testing right now. So if possible I would like to ship it directly with this PR

There is also allot of unused stuff in the EventListener. I will also throw that stuff out.

dabf475

[unglaublicherdude]

So. The scanFileById does not work when the file is not in the cache. So any opinions?

The main reason why I tested that, is that if a scan does take its time, the file will be available in the UI and can be downloaded for that time. Removing the file from the cache would prevent that from happening because the file will only appear again in the UI, when the scan is done.

But for this PR I am OK with this behavior.