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]

I see that you created some components with options api and some with composition api. This looks bit of mix of both. Mainly options API, but also using composables which are mainly intended for composition API.

Any chance we could have this consistently using composition api.

There is no need to use ajaxError, dialog, fetch mixins. We have all these covered in useModal and useFetch. We already have lots of usage examples and these are actually quite well documented and I am also able to respond on mattermost for any usage questions.

[touhidurabir]

Modified to use the composition API .

[jardakotesovec]

I am putting these new 'managing' components to the src/managers

We could name it something like ReviewerRecommendationsManager

[jardakotesovec]

You can find more details in storybook - https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/components-table--docs#general-guidelines

But long story short, aria-label is unnecessary. This table gets automatically linked to its label, which you did provide. So that fine. And even if would use aria-label somewhere it would have to be localised.

[jardakotesovec]

Styled checkboxes probably come with redesigned forms.

So yeah, we don't have that yet. It would be fine to use unstyled checkbox meanwhile. But not big deal either way.

[jardakotesovec]

for this use case I think recommendations.find would be better fit?

[jardakotesovec]

very minor thing - I am trying to follow convention of using one object as first arguments, even if its one thing, because in future there might be more. So it could be useDashboardConfigReviewActivity({recommendations})

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

[asmecher]

@jardakotesovec, would you be able to review the string escaping stuff here? I'm not sure it's matching what you've been working towards for new code.
@touhidurabir, there's a merge conflict here.

[jardakotesovec]

What behaviour are you aiming here exactly? Vue.js will escape html by default, so wondering how is this different to just use

<span>{{localize(item.title}}</span>

[jardakotesovec]

Just small convention that I aim to follow is to use object so its better flexibility to pass more data if needed, without dealing with the argument order.

So please just adjust it to :

const reviewerManagerConfig = extender.addFns(useReviewerManagerConfig({recommendations: props.recommendations}));

[jardakotesovec]

Since you already created store, which is great. Just put all the logic in there.

The motivations why its organised like this are explained in https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/guide-pinia-store--docs and https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/guide-plugins--docs

Long story short putting all logic to the store for easier extensibility. And it would be difficult to draw line what kind of logic to put directly to the component and which to the store. So if the component has its own store - lets just put everything there.

getActions is actually good example of the things that we might want to have extensible and having that exposed in the store, like we do in FileManager/GalleyManager/ParticipantManager/ReviewerManager... would make sense. But probably unncessary, because I think its very unlikely someone will need to extend this particular UI.

[jardakotesovec]

No need to pass translations as props anymore! https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/guide-translation--docs

Just use useLocalize and t('localisationKey') and it will just work. That will dramatically reduce number of props

[jardakotesovec]

Also no need for apiUrl - that is also now easy to achieve just on client side using useUrl - https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/composables-useurl--docs

[jardakotesovec]

How I think about the server updates is that ultimately what I am showing to the user should come from the server as that will be most accurate.
Making adjustments to the data client side is bit of additional work, which somewhat duplicates the edits that are happening on the server. The strategy sometime used is to adjust data client side (often called optimistic update) and on background reload the data from the server to ensure accuracy.

But in our use cases I am usually aiming for simplicity, as these API calls are quick and not often. So basically just send the update to the server and refetch recommendations will be perfectly sufficient.

Therefore you can simplify it to

await _actionFns.toggleStatus({...})
await fetchRecommendations();

And error handling also is already solved for you if you use useFetch to display that server returned error.

[jardakotesovec]

So there is useForm to abstract these manually updates to the form structure. It was created quickly to cover some use cases in 3.5.. it will mature more in 3.6, but I think this would be also good example where to use it:

https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/composables-useform--docs

Please try const {form, clearForm} = useForm(props.form);
clearForm()

Message me on mattermost if its not working for you, and I will improve it.

[jardakotesovec]

When passing form configuration to the modal - I would suggest just to pass the configuration and let the modal handle the state of it.

Than you can also use the useForm() again for managing state of that form -

ui-library/src/managers/ReviewerManager/modals/workflowLogResponseModalStore.js

Line 4 in d0d5dfb

} from '@/utils/defineComponentStore';

You can see that in this example Rob created own store for the modal, which is fine. But for simple modals, where we don't expect devs to extend/amend the behaviour, its fine to put that logic directly to the component.

Its intentional that the syntax in the component/store is exactly the same, so its easy to move things to the store if needed.

[jardakotesovec]

At this point I intend to handle extensibility just via stores, and avoid these emits, unless we find reason this to be necessary. So this can be removed.

[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']})