Skip to content

Conversation

@raimund-schluessler
Copy link
Member

Fixes #1123. Related to nextcloud/server#20976.

This will only work with Nextcloud 18.0.5 nextcloud/server#21127 and 19.0.1 nextcloud/server#21126 onwards.

Before merging we need to check that this doesn't introduce any XSS vulnerabilities.

@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #1124 into master will not change coverage.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master    #1124   +/-   ##
=======================================
  Coverage   27.82%   27.82%           
=======================================
  Files          48       48           
  Lines        2609     2609           
  Branches      494      494           
=======================================
  Hits          726      726           
  Misses       1743     1743           
  Partials      140      140           

@raimund-schluessler
Copy link
Member Author

Seems to be fine XSS wise, but another opinion from @nextcloud/javascript and @ChristophWurst (since you introduced the fix in server) would be great.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make not to use v-html with this

@raimund-schluessler
Copy link
Member Author

Make not to use v-html with this

Thanks for the feedback. We don't use v-html anywhere in Tasks (I also think one should use a component with a render function instead of v-html, but that's a different story I guess.)

@raimund-schluessler
Copy link
Member Author

I just realized there are more occurrences where we have to fix it:

  • Creating a calendar with an already used name
  • Deleting a calendar
  • Removing a calendar share
  • Batch-deleting tasks from a calendar

@raimund-schluessler raimund-schluessler added this to the 0.13.3 milestone Jul 15, 2020
Signed-off-by: Raimund Schlüßler <[email protected]>
@raimund-schluessler
Copy link
Member Author

raimund-schluessler commented Jul 15, 2020

I think I found all occurrences now. If anything else shows up, we can fix it in a later PR.

escaping

@raimund-schluessler raimund-schluessler merged commit a6c3ea0 into master Jul 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/1123/escaping branch July 15, 2020 19:35
@gitaarik
Copy link

Just a question, what is this.$t? It looks like vue-i18n but I don't recognize all the options. Also I'm not very familiar with Vue so I don't know where to look to find out where this.$t is defined.

@gary-kim
Copy link
Member

It's Nextcloud's l10n system. You will find it bound to window.t on all Nextcloud pages or you can use this npm package. You find where it gets set here:

tasks/src/main.js

Lines 61 to 66 in e56ed81

Vue.prototype.$t = function() {
return t.apply(null, arguments).toString()
}
Vue.prototype.$n = function() {
return n.apply(null, arguments).toString()
}

@gitaarik
Copy link

gitaarik commented Jul 16, 2020 via email

@raimund-schluessler
Copy link
Member Author

So the translations of nextcloud apps are all managed by a centralized system?

Yes, they are handled by Transifex. The translateable strings get extracted from the source automatically, get uploaded to transifex, translated there, and the translated strings are pushed to the repo by @nextcloud-bot.

Also, don't you mean it's bound to the Vue object instead of the window object?

Both, actually. We bind the window.t to the Vue object here:

return t.apply(null, arguments).toString()

@gitaarik
Copy link

gitaarik commented Jul 18, 2020 via email

@raimund-schluessler
Copy link
Member Author

Aha cool, so Nextcloud does not enforce apps to be written in Vue?

You can write your apps using React or Angluar or plain JavaScript as well. However, I would highly recommend Vue, since the Nextcloud interface library https://github.com/nextcloud/nextcloud-vue is written in Vue and Vue is the standard framework used all over Nextcloud.

Let's discuss the other questions in the respective issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Special characters show html-escaped in "Add task" input field placeholder value

5 participants