pkp/pkp-lib#1660 Customizable Reviewer Recommendations

[touhidurabir]

for pkp/pkp-lib#1660

[asmecher]

This is an XSS flaw -- any HTML entered in the recommendation.title will be passed through to the browser. (Also the other keys that mix user-supplied data in here.)

[touhidurabir]

@asmecher good spot . I had this left out as v-strip-unsafe-html was not yet available for main as we introduced in 3.4.0 . But then forgot to update these as v-strip-unsafe-html was added just about a week ago in main branch .

One question , rather than applying this for replaceLocaleParams every time like

message: this.replaceLocaleParams(this.confirmDeleteMessage, {
    title: sanitizeHtml(this.localize(recommendation.title)),
})

should we add this sanitizeHtml into the implementation of replaceLocaleParams so that we don't need to make sure to add this every time ?

[jardakotesovec]

XSS is by default prevented by vue.js, and in cases where we use (and sometime overuse) v-html, its now replaced with v-strip-unsafe-html. Therefore this will go through that. Every component is responsible to do the XSS protection.

Therefore Dialog handles that when rendering message.

<div v-strip-unsafe-html="message" />

No point to sanitize twice.

[asmecher]

@jardakotesovec, I used a <i> tag to confirm that entered HTML was passed through, and I didn't check whether more malicious tags were filtered -- so it's possible that <i> gets through because it's "safe", but worse HTML would be filtered. However, it's still incorrectly escaping the content. The reviewer recommendation title is plaintext, not a rich text field, so any entered content should be fully escaped before getting glued into the locale string.

[jardakotesovec]

Thanks @touhidurabir. This is very nicely integrated with new code base. I am not sure whether this is aimed for 3.5, so I would understand if some of the 'composition api' updates would be left out for timeline purposes.

But if you decide to do the upgrade and struggle with something feel free to message me directly on mattermost. I should be able to point you to some good example or something.

[asmecher]

I am not sure whether this is aimed for 3.5

It's scheduled for 3.6, and I was considering pulling it forward to 3.5 -- but I'd rather spend a bit more time to get a clean implementation than rush it in. So let's leave it on the 3.6 milestone.

[jardakotesovec]

I guess that the reviewer recommendations are not supporting any html tags, right? So we basically just want to make sure that if it has any html tags it would not be interpreted?

We have couple places like tooltip or dialogs, where we introduced v-html because we had some message in translations which required that. To reduce attack surface area we introduced v-strip-unsafe-html, which at least ensures safe html.

I would like to improve the situation in 3.6 further.

I think for general components that because of some use case started offering v-html - we should have separate props for that. For Example having openDialog with options: message and messageHtml. So its decided when the dialog is created whether html is needed and whether its safe based on what I am passing there. For this improvement I will create separate ticket and introduce the html variants when necessary.

But your use case is bit more nuanced, because you have translation Are you sure you want to deactivate the recommendation <b>{$title}</b>. Therefore you want to render it as html, but escape the argument, which you are doing, but would be nice to also make the translation toolkit safe by default.

I will leave up to you whether you would add this improvement here, since you already touched on the escaping functions or whether you prefer me to add it in separate ticket, but I think having option in our t function will be best..

By default t should escape all the arguments being passed. But need to introduce option to allow html if needed.

So in your case it should look like (now putting aside that I will move the message to messageHtml in separate ticket):

message: item.status ? t('manager.reviewerRecommendations.confirmDeactivate', {title: localize(item.title)}) : ...

But there still would be option, to skip the escaping for specific fields:

t('manager.reviewerRecommendations.confirmDeactivate', {title: localize(item.title)}, {allowHtml: ['title']}) 

[touhidurabir]

I will and want to work on this but I think it will be better to handle this as a separate issue . Reason is, this would be a global impact level change and a very important one . Better to keep track of this as it's own issue so that we can in future get clear idea what plan and strategy we adapted for this one and what was the implementation. This issue is already got bit too big so better to separate this implementation from this issue .

[jardakotesovec]

Agree.. make sense to introduce these changes separately.

In that case I would suggest not to introduce the escapeHtml at this point. Because that will be default behaviour for the message of dialog. So if you remove it - it will be safe enough for now and after we work on the updates that I mentioned, it will also prevent sneaking in any html to affect rendering.

[touhidurabir]

Hi @jardakotesovec can you please take another look .

[jardakotesovec]

Regarding useForm, sorry I need to document this better. I did some testing and it seems to be working fine. Feel free to cherry-pick this commit - jardakotesovec@6e4ec1a

I also sneaked in spinner to the table to indicate when the data are being refetched for small connections.

So my suggestion would be to add this commit (seemed working fine from my testing, if you see issue ping me on mattermost).

Resolve other suggestions.

And ping me and I will do the last quick check.

Thank you!

[touhidurabir]

@jardakotesovec hopefully ready for the final look , please check .

[jardakotesovec]

Just couple small clean up items. Thanks!

[jardakotesovec]

Is this class needed?

[jardakotesovec]

No big deal, but I think you don't think you need this ref?

[jardakotesovec]

So could this be just

computed(() => recommendations.value?.items || [])

[jardakotesovec]

Minor, but isSuccess is not really needed.

[jardakotesovec]

minor, name is not used anymore

[jardakotesovec]

Alright.. :-).. this is tricky..

Automatic detection of the locale keys is happening in build time and therefore I have to rely on regex.

So it needs to find t('key') or tk('key') to pick it up.

With having condition like this inside, I don't think it would work.

[jardakotesovec]

This is very subjective suggestion about how to organise with composition API. Especially for more complex components trying to group things by feature. So since the beginning is focused on the fetching the recommendations I would do the initial trigger there.

		const {
			data: recommendations,
			fetch: fetchRecommendations,
			isLoading: isRecommendationsLoading,
		} = useFetch(apiUrl);
		// Initial data fetch
		fetchRecommendations();