Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jardakotesovec opened this issue Oct 16, 2023 · 5 comments
Assignees
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. UI/UX Issues affecting the user interface/user experience
Milestone

Comments

@jardakotesovec
Copy link
Contributor

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

  • Look into whether there is opportunity to reduce use of v-html
  • Look into whether replacing v-html by html-sanitizer which allows subset of html tags would had meaningful security impact
@jardakotesovec jardakotesovec added the Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. label Oct 16, 2023
@jardakotesovec jardakotesovec added this to the 3.5.0 LTS milestone Oct 16, 2023
@jardakotesovec jardakotesovec self-assigned this Oct 16, 2023
@asmecher
Copy link
Member

asmecher commented Dec 6, 2023

(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 added a commit to jardakotesovec/pkp-lib that referenced this issue Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jan 31, 2024
@jardakotesovec
Copy link
Contributor Author

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: pkp/ojs#4165
PKP: #9673
UI-LIBRARY: pkp/ui-library#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 - #9408

jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Jan 31, 2024
…apiBaseUrl to use same pkp.serverContext object
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jan 31, 2024
@jonasraoni jonasraoni added the UI/UX Issues affecting the user interface/user experience label Jan 31, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/ops that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/ops that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Feb 6, 2024
@jardakotesovec
Copy link
Contributor Author

jardakotesovec commented Feb 6, 2024

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 - pkp/ojs#4178
omp - pkp/omp#1519
ops - pkp/ops#634
ui-library - pkp/ui-library#325
pkp-lib - #9703

3.4
ojs - pkp/ojs#4177
omp - pkp/omp#1518
ops - pkp/ops#633
ui-library - pkp/ui-library#324
pkp-lib - #9702

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

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

@asmecher
Copy link
Member

asmecher commented Feb 7, 2024

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)

jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Feb 8, 2024
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Feb 8, 2024
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Feb 8, 2024
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Feb 8, 2024
asmecher pushed a commit to pkp/ui-library that referenced this issue Feb 9, 2024
* pkp/pkp-lib#9421 replace v-html with v-strip-unsave-html

* pkp/pkp-lib#9421 doc tool might require v-html
@asmecher asmecher changed the title Review html escaping strategy on vue.js codebase Review html escaping strategy on vue.js codebase in stable-3_3_0 and stable-3_4_0 Feb 9, 2024
@asmecher asmecher modified the milestones: 3.5.0 LTS, 3.3.0-17 Feb 9, 2024
@asmecher
Copy link
Member

asmecher commented Feb 9, 2024

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

@asmecher asmecher closed this as completed Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. UI/UX Issues affecting the user interface/user experience
Projects
None yet
Development

No branches or pull requests

3 participants