Download attachments and whitelist/blacklist strategy

[surli]

Hi everyone,

we got recently a bug report (https://jira.xwiki.org/browse/XWIKI-17754) about using the attachment.download.blacklist configuration to force downloading an attachment, in case of an attachment added by a PR user. Following this issue, I proposed a first PR with a new behaviour but after discussing it with @tmortagne (discussion started there: https://matrix.to/#/!ikPtGZaGWtyblizzlR:matrix.xwiki.com/$16012960212711ZcDTy:matrix.xwiki.com?via=matrix.xwiki.com&via=matrix.org&via=t2bot.io) it appeared that:

1. the reported bug is right now an expected behaviour
2. we need to decide if it’s a behaviour we still want
3. we need to decide how to improve it to fix the “bug”

So, right now the behaviour of downloading attachment is the following:

1. if the attachment has been added by a user with PR (Programming Rights), the attachment is always displayed inline.
2. if the attachment has been added by someone without PR, and a configuration is given for attachment.download.blacklist and no configuration is given for attachment.download.whitelist then the mimetype of the attachment is checked against the mimetype list of this blacklist to define if it should be downloaded or displayed inline (if present in blacklist, it won’t be displayed inline)
3. in other cases, the mimetype of the attachment is checked against the mimetype list defined in attachment.download.whitelist: this one might be the default whitelist or a custom one.

Note that this behaviour is currently partially explained in https://www.xwiki.org/xwiki/bin/view/FAQ/How%20to%20open%20PDF%20attachments%20from%20the%20wiki%20directly%20in%20the%20browser (partially because it doesn’t explain in which condition the blacklist should be used).
Note 2: It’s always possible to use a force-download request parameter to override the mentioned behaviour and force the download, I won’t mention it anymore here since this proposal is about the default behaviour without specific parameters.

So the problem with the current behaviour, and the bug reported in XWIKI-17754 is that right now the attachments can never be forced to be downloaded by default if they have been added by a PR user. My first proposal to overcome this issue was to change the way we resolve this, by checking first the blacklist, before checking if the attachment was added with PR or not.
As indicated in my first PR, the resolution was then the following:

The resolution order is now the following:

1. If a force-download argument is given, the attachment is downloaded
2. If the mimetype of the attachment is present in the blacklist, the attachment is downloaded
3. If the user has PR the attachment is displayed inline
4. if the attachment mimetype is not present in the whitelist, the attachment is downloaded

The problem with that solution is that it could break compatibility of some configurations: we couldn’t handle usecase where a wiki is configured to allow only PR users to display images attachments inline. If you have blacklisted some mimetypes thinking “it’s only for normal users” you’ll be hit by the change.
A way to overcome this, would be to introduce a new boolean configuration, to basically say “use the old mechanism”.

Now @tmortagne proposed another solution, which would be to introduce a new list of mimetypes, for attachments “forced to be downloaded”: this list would be taken into account before the actual mechanism and would allow to force downloading attachments based on mimetype, even if added by PR users.

AFAICS there is no scenario (based on mimetype) that we couldn’t support. The only problem I can think of with this solution is that it makes the configuration for mimetypes of attachment a bit more difficult to understand with 3 different lists. Now Thomas mentioned that we could move the existing configuration lists to a “Security” section since those are about protecting against XSS, and put the new configuration in a “Attachment” section, since it’s only about usability.

wdyt?

[tmortagne]

@surli

Now @tmortagne proposed another solution, which would be to introduce a new list of mimetypes, for attachments “forced to be downloaded”: this list would be taken into account before the actual mechanism and would allow to force downloading attachments based on mimetype, even if added by PR users.

I vote for this one

It’s a lot safer in term of retro compatibility (especially since we are talking about security here) and once you moved the security stuff in another section I find it much easier to use when all you want is to indicate some types that should always be downloaded instead of opened (the default when supported by the browser).

[vmassol]

@surli

if the attachment has been added by a user with PR (Programming Rights), the attachment is always displayed inline.

FTR, I don’t really understand this logic. I don’t see why someone with PR would not be able to get a download instead of inline where it makes sense.

EDIT: I also agree that changing that would break backward compat. Just don’t know how important it is.

[vmassol]

I’ll trust you guys. Just seems like we’ll have a lot of config params and thus it may be complex to use them. Maybe it’s worth legacifying some to keep it simple.

[surli]

@vmassol

Just seems like we’ll have a lot of config params and thus it may be complex to use them. Maybe it’s worth legacifying some to keep it simple.

Well the latest proposal involve to add one new configuration parameter for it, but it’s doesn’t replace even partially the two other existing ones: the new one is about usability where the 2 others are about security. So we cannot really legacify any of them.

[vmassol]

@surli

Well the latest proposal involve to add one new configuration parameter for it, but it’s doesn’t replace even partially the two other existing ones: the new one is about usability where the 2 others are about security. So we cannot really legacify any of them.

It seems to me there would be 3 configs:

• attachment.download.blacklist
• attachment.download.whitelist
• “forced to be downloaded”

And you said:

about using the attachment.download.blacklist configuration to force downloading an attachment

So attachment.download.blacklist is also about forcing to download an attachment, same as “forced to be downloaded”, hence the confusion for users.

[vmassol]

What’s the proposed new config name?

[surli]

@vmassol

What’s the proposed new config name?

TBD

@vmassol

It seems to me there would be 3 configs:

Yes, 3 total, but only 1 new.

@vmassol

So attachment.download.blacklist is also about forcing to download an attachment, same as “forced to be downloaded”, hence the confusion for users.

Here’s what I wrote about it:

@surli

The only problem I can think of with this solution is that it makes the configuration for mimetypes of attachment a bit more difficult to understand with 3 different lists. Now Thomas mentioned that we could move the existing configuration lists to a “Security” section since those are about protecting against XSS, and put the new configuration in a “Attachment” section, since it’s only about usability.

There is a bit of an overlap, but that can be documented. Basically the blacklist would be for all attachments not added by a PR user (same for whitelist), and it’s the current behaviour: those are security properties for attachment pushed by people without PR rights.
Now the new property will be about the attachments types that should be always download and not displayed inline, not matter what is the security configuration: it’s not about security, just about a behaviour we want in the UI.

[vmassol]

I still don’t get the use case for PR…

[vmassol]

@surli

The only problem I can think of with this solution is that it makes the configuration for mimetypes of attachment a bit more difficult to understand with 3 different lists. Now Thomas mentioned that we could move the existing configuration lists to a “Security” section since those are about protecting against XSS, and put the new configuration in a “Attachment” section, since it’s only about usability.

So you mean deprecating the 2 existing properties and changing the config names to be prefixed with “security” and moved under https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm#L654 ?

[surli]

@vmassol

I still don’t get the use case for PR…

Imagine a wiki configured to blacklist images mimetypes: right now it still allow to display attached images in wiki pages. If you change the behaviour of DownloadAction it won’t be the case anymore.
Knowing that any attachment we put in XWiki Standard is made with superadmin which by definition has PR rights, it could break all distribution to change it now.

[surli]

@vmassol

So you mean deprecating the 2 existing properties and changing the config names to be prefixed with “security” and moved under https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm#L654 ?

Not sure we’d need to deprecate their name here since we wouldn’t change their behaviour. We could but I don’t think it’s mandatory. Now yes I think we should move them in the security section.

[vmassol]

@surli

. Now yes I think we should move them in the security section.

I’m really -0 and maybe even -1 for that. We shouldn’t use invalid config names for sections, that’s starting a bad trend and is even more confusing. There are other configs that are in extensions or other modules that are related to security, yet they should remain under these other prefixes.

[vmassol]

We’d also need to decide if if makes sense to be security.* or if it better to be attachment.security.*.

[vmassol]

@surli

Imagine a wiki configured to blacklist images mimetypes

I must be missing some knowledge. What does blacklisting the image mimetypes mean? For me it means forcing to download these image attachments when you click on them vs displaying them inline.

What does this have to do with PR? What is the security need?

Thanks

[surli]

@vmassol

I’m really -0 and maybe even -1 for that. We shouldn’t use invalid config names for sections, that’s starting a bad trend and is even more confusing. There are other configs that are in extensions or other modules that are related to security, yet they should remain under these other prefixes.

Ok so let’s keep them where they are now then. They are definitely related to attachment, we can provide more documentation to properly distinguish them for the new one to add.

@vmassol

We’d also need to decide if if makes sense to be security.* or if it better to be attachment.security.* .

I really wouldn’t change their name since those are already used configurations, and the behaviour wouldn’t change.

@vmassol

For me it means forcing to download these image attachments when you click on them vs displaying them inline.

Yes

@vmassol

What does this have to do with PR? What is the security need?

I was just taking an example with images because we have in XS attached images with superadmin who has PR by definition. I was trying to illustrate that changing the behaviour of the existing configurations would change the behaviour of interacting with those images with DownloadAction, which could be potentially a breaking change.
Let’s be a bit more explicit: right now we are checking first if the user who attach the files has PR or not, and then only we check for blacklist/whitelist.
So in a configuration where the images are blacklisted, if they are attached by superadmin, they are always displayed inline when using DownloadAction without even checking the blacklist. Now if we change this behaviour by not checking anymore if the user has PR, but by only relying on the blacklist, then those images would be always forced to be downloaded, that is the potentially breaking change I was talking about.

[surli]

@surli

Ok so let’s keep them where they are now then. They are definitely related to attachment, we can provide more documentation to properly distinguish them for the new one to add.

So without further opinion I will proceed on what I proposed: a new dedicated configuration for allowing to force download some mimetype. I’m gonna update my draft PR in that way.

[surli]

Change performed and documented in https://www.xwiki.org/xwiki/bin/view/Documentation/AdminGuide/Attachments#HAttachmentdisplayordownload so it will be part of 12.10RC1 release.