pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
304 stars 444 forks source link

Review html escaping strategy on vue.js codebase in stable-3_3_0 and stable-3_4_0 #9421

Closed jardakotesovec closed 8 months ago

jardakotesovec commented 1 year ago

Description Vue.js escape html by default, but to support html in translations and some metadata we use v-html on many places.

asmecher commented 10 months ago

(Several Vue components are using {{ submissionTitle }} to present submission titles -- this will escape HTML elements that are legitimately there. This will also need to be fixed when we have a good partial filter.)

jardakotesovec commented 8 months ago

As first 'catch all' round, replacing v-html with v-pkp-allowed-html, which sanitise the inputs with restricted set of elements and attributes - same as allowed_html in config.inc.php. This is significant improvement over using v-html. These changes should be safe to be applied also for 3.3 and 3.4, without causing regressions.

OJS: https://github.com/pkp/ojs/pull/4165 PKP: https://github.com/pkp/pkp-lib/pull/9673 UI-LIBRARY: https://github.com/pkp/ui-library/pull/320

It would be good to even reduce use of v-pkp-allowed-html to the minimum, which I will explore later for 3.5.

Goal is really not have any XSS opportunities in Vue.js code base. Should provide enough protection for cases like this - https://github.com/pkp/pkp-lib/issues/9408

jardakotesovec commented 8 months ago

For 3.3 and 3.4 just simple replacement of v-html with v-strip-unsafe-html. Thats using dompurify, which is performant & security oriented html sanitiser with default 'html' profile. Keeping things simple just to address security. Having allowed tags is something we can introduce in 3.5.

3.3 ojs - https://github.com/pkp/ojs/pull/4178 omp - https://github.com/pkp/omp/pull/1519 ops - https://github.com/pkp/ops/pull/634 ui-library - https://github.com/pkp/ui-library/pull/325 pkp-lib - https://github.com/pkp/pkp-lib/pull/9703

3.4 ojs - https://github.com/pkp/ojs/pull/4177 omp - https://github.com/pkp/omp/pull/1518 ops - https://github.com/pkp/ops/pull/633 ui-library - https://github.com/pkp/ui-library/pull/324 pkp-lib - https://github.com/pkp/pkp-lib/pull/9702

Hope its all correctly created and setup.. Lets see whether tests pass.

ping @asmecher @jonasraoni on code review. Thank you!

asmecher commented 8 months ago

Reviewed all and merged stable-3_3_0 PRs; I've restarted the OJS stable-3_4_0 build to see if there was a temporary hiccough during the validation stage, or whether something's actually broken there.

(Edit: it's a missing ui-library submodule update. comment)

asmecher commented 8 months ago

Thanks, @jardakotesovec! I've merged the stable-3_3_0 and stable-3_4_0 PRs.

I've moved this back to the 3.3.0-17 milestone (where this will first be released) and added a note to https://github.com/pkp/pkp-lib/issues/9717 referring back here, so when we revise our strategy on main we don't forget these aspects too. Therefore I'll close this issue as done.