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

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

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

It also blocks for sync-clients

I will start implementing the virus scan tomorrow.

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

It retries 4 times.

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

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.

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

Also shown in sync clients.

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

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.

I expect this Closes #147 and Closes #85

@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.

So still WIP

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.

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.