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

<b-card>: "Tag" is displayed without escape in header, titlle and footer. #2245

Closed
howmuch515 opened this issue Dec 4, 2018 · 14 comments
Closed

Comments

@howmuch515
Copy link

howmuch515 commented Dec 4, 2018

Hello,

"Tag" is displayed without escape in b-card's header, titlle and footer.
"script-tag" is rejected. But "img-tag" can cause alert.
Is this correct specification?

OS: macOS Mojave 10.14.1
vue: 2.5.17
nuxt: 2.3.4
bootstrap-vue: vue-2.0.0-rc.11
<template>
    <b-card :header="text1"
            :title="text1"
            :footer="text1">
        <h1>{{ text1 }}</h1>
        <b-form-input v-model="text1"></b-form-input>
    </b-card>
</template>

<script>
export default {
    data: function() {
        return {
            text1: 'hoge'
        }
    }
}
</script>

b-vue_xss

@tmorehouse
Copy link
Member

tmorehouse commented Dec 4, 2018

The props for title, header, etc, support basic HTML for things like simple styling. It is up to you as the author of an app to determine what fields you will allow to contain untrusted user data.

@4dn-oss
Copy link

4dn-oss commented Dec 6, 2018

I think developers who need styling should use slot rather than property.

In the current specification, when using b-card in a reusable component, we must always escape the value to set to the property such as title etc. Otherwise the component has an XSS vulnerability.

Are there components other than b-card that have similar specifications? If so, I think that it is necessary to specify them in the document.

However, I think that it is desirable to treat all property values as plain text.

@4dn-oss
Copy link

4dn-oss commented Dec 6, 2018

HTML tags are available for the following properties:

component properties
b-card header, title, sub-title, footer
b-carousel caption, text
b-checkbox-group text of options array element
b-form-group label, description, feedback, valid-feedback, invalid-feedback
b-form-radio-group text of options array element
b-input-group prepend, append
b-navitem-dropdown text
b-pagination first-text, prev-text, ellipsis-text, next-text, last-text
b-pagination-nav first-text, prev-text, ellipsis-text, next-text, last-text, return value of page-gen's function
b-table caption, empty-text, empty-filtered-text, label of fields array element

HTML tags are not available for the following properties:

component properties
b-dropdown text
b-form-file placeholder
b-form-input placeholder
b-form-select text of options array element
b-form-textarea placeholder
b-jumbotron header, lead
b-modal title, ok-title, cancel-title
b-popover title, content
b-table key and value of items array element
b-tab title
b-tooltip title

I have examined all the way, but there may be some missing.

@howmuch515
Copy link
Author

Thank you for your research!

@tmorehouse
Copy link
Member

I would prefer styled html to be used in slots only, but we har many requests to be able to pass basic html to the props.

Removing this "feature" would be a breaking change for many users though.

@4dn-oss
Copy link

4dn-oss commented Dec 7, 2018

Compatibility is important, but if this goes on, it seems likely that this "feature" will cause XSS vulnerability on some site.

The problem is that the existence of this "feature" is not obvious from the document. I think that it is necessary to specify in the document whether this feature exists for each property.

The next problem is that there are properties with this feature and properties without it, and it can not predict the existence of this feature from the property name etc.

Therefore, even if a complete document is released, avoiding the pitfalls of this feature is not easy.

However, if we escape all the property values defensively (which I don't want to do...), undesirable texts will be displayed when we set values to properties without this feature.

Does it become as follows?

  • If the component has a property with a styling feature, be sure to prepare a property without a styling feature.
  • Property without a styling feature has a simple name (title etc).
  • Property with a styling feature has descriptive name (html-title etc).

Although this is a breaking change, but I think it is relatively easy to fix for users.

Also, code written by users without knowing that these properties have a styling feature works on the safe side.

@tmorehouse
Copy link
Member

b-form-group no longer accepts HTML for the props.

@tmorehouse
Copy link
Member

For things like b-form-select, b-form-radio-group and b-form-checkbox-group, the option to use HTML in the options array is important to many users. What we could do for those three components is default to rendering the option text as text, and then add an option to the component to enable rendering the text as html instead (i.e. an options-html boolean prop).

@jacobmllr95
Copy link
Member

jacobmllr95 commented Dec 18, 2018

What about using a prop like html or content in the option in the array to enable HTML and default to plain text for the text prop?

@tmorehouse
Copy link
Member

tmorehouse commented Dec 18, 2018

Yeah, the options mixin could be updated to accept an html property, and if present, is used as inner html, and the plain text can be passed direct to createElement as the content of no html

@tmorehouse
Copy link
Member

There are a few npm modules that can sanitize html strings, but they are rather large in size, and many still have loopholes.

@jacobmllr95
Copy link
Member

I think it is enough to give the user better control over this.

Default to the safe text prop and give the option to use the html with a notice in the docs that by using this the content need to be sanitized by the users themselves.

@tmorehouse
Copy link
Member

Yeah. And a few other components should have similar limitations as well for when inner html is used.

@4dn-oss
Copy link

4dn-oss commented Feb 6, 2019

3c6ba3e seems to fix this issue.

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

No branches or pull requests

4 participants