Merge lp://staging/~widelands-dev/widelands-website/pybb_attachments into lp://staging/widelands-website

Code LGTM. Do we have a restriction on file types? We won't want people to upload malicious JavaScript ans stuff lie that, so we should have a list of legal file extensions at least.

Currently there is no restriction on file types. The problem is that a file may not contain what the extension says it should be. Everyone can rename 'image.js' into 'image.png'...

A list of allowed extension do not suffer. I'll try to implement some checks for validating a files type and probably an 'allowed extension list'.

What about restricting uploads to users who have written x posts prior? From my side this can be implemented.

> Everyone can rename 'image.js' into 'image.png'...

That's true. It would still be an advantage though if the browser then doesn't execute it.

Doing a full validation too is definitely a better idea.

> What about restricting uploads to users who have written x posts prior?

Also a good idea

I am a bit lost here...

Some checks are made now, but i am unsure if we need them all. What is implemented:

1. Link of Attachment:
If a user clicks on a link of an attachment, he will be ever prompted for a download (or to open the file with an associated program), even if it is a file which the browser may can display, e.g. an image (png, jpg) or text file. This is not optimal, but i think it is more secure.

2. Restricting upload permission
A user has only permission to upload files, if he has had written 5 posts prior. The number of 5 derives from MAX_HIDDEN_POST at the moment, but i think it's better to have an own value for this. What number do we want?

3. Allowed extensions:
Added a list:
ALLOWED_EXTENSIONS = ['wgf', 'jpg', 'jpeg', 'gif', 'png', 'ogg', 'lua', 'ods', 'zip', 'json', 'txt', 'csv']
I am unsure if this list will be ever complete. E.g. some may want to upload some other types of files (e.g. py, odg, doc, ...)

4. Extension handling:
- Files without extension aren't allowed (can be problematic e.g. with the campvis file)
- For 'wgf' files: Check if some base entries can be found (e.g. '/binary/' or '/minimap.png')
- For image files: Check if this is really an image using the Pillow library. This finds corrupted .png but not corrupted .jpg
Do we need a check for each allowed extension?

5. Comparing Mime-Types:
When uploading a file, a Mime-Type (content-type) is also submitted. The submitted mime-type is mostly derived from the extension of the file. E.g. if you rename a file from 'image.png' to 'image.txt' the OS (or browser) sends the Mime-type 'text/plain', which is obviously wrong. To prevent such mime-type mismatch, a check is made which compares the sent mime-type with the mime-type derived from python-magic. e.g. python magic will show for 'image.txt' the mime-type 'image/png'.

As said, i am a bit lost, not really knowing if we need all this checks, or if they are enough at all :-S Any opinions?

2. I have no opinion on this at this time. Maybe 10 or 20? We can easily change the value if it becomes a problem.

> Files without extension aren't allowed (can be problematic e.g. with the campvis file)

campvis will be replaced by campaigns.conf for the next version, so don't worry about it - we can allow .conf

Other extensions that we're using for Widelands & that might be interesting:

.wai - AI
.wmf - Map
.wrpl - Replay - useless with the accompanying .wgf though.

> Do we need a check for each allowed extension?

Since attachments are marked for download anyway, I'd say that checking for those that are easily checked will do for now.

> Other extensions that we're using for Widelands & that might be interesting:

> .wai - AI
Ok

> .wmf - Map

I forgot to mention: If the extension is wmf, an errormessage is shown: "This seems to be a widelands map file. Please upload it at our maps section."

> .wrpl - Replay - useless with the accompanying .wgf though.

I am planning to make a wikipage describing, beside other things, the usage of replays, e.g. they have to be zipped together before uploading and link that wikipage in the upload form. But i have to check for .wrpl.wgf.

I have to stop working on this, other wise i fear to change many other things :-D

Anyway i think this works. wai files need some special treatment though.

Please test on alpha: https://alpha.widelands.org/

stonerl, can you install and configure clamav on the server, please?

The current branch implemented a check with clamdscan, which needs a clamav daemon. We can also stick with clamscan, which can be run on demand, but this much slow: A file check last 55 sec. on my machine, whereas clamdscan usually needs only 0,1 sec.

The current branch is not running on alpha, yet. Since i am afk for some some days, i'll shut down alpha now.

Already installed. ;)

1 tiny nit.

I also wonder whether we should disallow map files - it might come in handy if somebody wants some feedback before publishing to the maps section.

> I also wonder whether we should disallow map files - it might come in handy if somebody wants some feedback before publishing to the maps section.

I just don't want to have map files in the forum. I 'fear' most (new) users will upload a map in the forum instead in the 'right' place. A Feedback can also be done in the map comments, or by linking a map in the forum and ask there for a feedback. Once there is a fix for bug 357914 'Allow submitters of maps to upload new versions' get implement, it should be fine to not allow maps in forum.

But of course we can remove that part of code and if we get in trouble we can add it again.

@Toni: Can we have the package 'clamdscan' installed? For some reason it is a recommended package for clamd and we do not install recommended packages by default.

Or is clamd configured to have on-access-scanning enabled? Didn't saw that it is configured like that.

How do you intend to access clamav? The current mode is via socket:

LocalSocket /var/run/clamav/clamd.ctl

If you'd like, I can add a local TCP daemon on port 3310.

Stupid me. :-) I installed clamdscan.

Thanks :-)

But the check for clamdscan doesn't work on the server... i know why, but i don't know the underlying cause. Will talk on IRC about that issue when i have some more time.

Just found that map uploading is broken with the changes in this branch. So the code for map uploading needs adapting too...

Finally i got it (hopefully)

Please check again on alpha... https://alpha.widelands.org/

I have tested map upload and attaching to forum.

Manx thanks :)

I wait only for janus to approve the changes regarding python3. He will look at it next weekend.

Finally this is productive now :-)