Announcement title not properly escaped on delete

[asmecher]

Describe the bug
Announcement titles containing HTML special characters are not properly escaped in the delete confirmation modal.

[asmecher]

Draft PRs:

• stable-3_3_0
• TODO: stable-3_4_0
• TODO: main

[jardakotesovec]

Added some comments to the PR.

Also found same problem when deleting contributor..

And Highlights seems to have same implementation as Announcements (was not able to test it as main is broken for me now)

I also attempted to review all uses of browser side param replacement. And did not find anything that would look like it needs html for params. I think that mostly should be title right? Using any of other richtext fields as param is highly unlikely and did not see anything suggesting that. Therefore I would be inclined just to escape all params. If you think its risky for 3.3/3.4.

Than the other cases are ContributorsListPanel.vue

message: this.replaceLocaleParams(this.i18nConfirmDelete, {
	name: contributor.fullName,
}),

And HighlightsListPanel.vue

  message: this.replaceLocaleParams(this.i18nConfirmDelete, {
      title: this.localize(highlight.title),
  }),

Even though I agree this should be correctly handled by frontend, bit surprised that we don't attempt to filter these out during validation step, when data are submitted.

[jardakotesovec]

Created frontend specific issue for me to circle back to it later on - #9421 .

@asmecher Not sure if you would want to also review validation on server side to not let such content in first place?

[asmecher]

Deferring; this is a privileged action anyway, so the risk is low.

[asmecher]

@jardakotesovec, I've revisited my PR above after your comments -- thanks. I think you're looking for cases in stable-3_4_0, when I only grepped through stable-3_3_0 -- for example, both highlights and contributor deletion don't appear until 3.4.0. This is easy enough to grep for -- there aren't that many use cases for replaceLocaleParams. Do you mind if I go ahead with this expedient fix for stable-3_3_0 and stable-3_4_0?

[jardakotesovec]

@asmecher I found strong candidate for replacing v-html, intend to test that tomorrow. If thats successful and we would be able to do the sanitisation on vue.js level (using the allowed_html whitelist). Would that be good enough to cover this case in your opinion? It still would be possible to sneak in things like image via announcements to the dialog. Which is not ideal, but not security issue.

I still would like to explore whether there are better patterns to minimise even use of any 'v-html-restricted', but this could be good catch all approach across all versions, with minimal risk of regressing something.

[asmecher]

Sounds good; I'll leave this open against 3.3.0-17 and let you come back to it.

[jardakotesovec]

Testing locally seems promising.. now running tests on travis -#9421

[asmecher]

@jardakotesovec, just to confirm, this is resolved in stable-3_3_0 and stable-3_4_0 by #9421, correct?

[jardakotesovec]

@asmecher Yes, from security point of view. It still can show some 'safe' html in that dialog. This is one of the cases when it would be useful to 'label' translation to allow html or not.

[asmecher]

Great, let's work through that in #9683 for main. Thanks!