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

unsafeContent doesn't show unless message is set to false #220

Open
petervukovic opened this issue Apr 14, 2017 · 3 comments
Open

unsafeContent doesn't show unless message is set to false #220

petervukovic opened this issue Apr 14, 2017 · 3 comments

Comments

@petervukovic
Copy link

petervukovic commented Apr 14, 2017

Per documentation, unsafe content should be sent via unsafeContent option.

However it seems that the actual option is unsafeMessage:

  1. Expecting "Thank You", getting an empty dialog

vex.dialog.open({ unsafeContent: '<h2>Thank you</h2>', });

  1. Expecting "Thank you", getting "Thank you":

vex.dialog.open({ unsafeMessage: '<h2>Thank you</h2>', });

@nicoknoll
Copy link

I think the main issue is rather this line:
var form = options.unsafeContent = buildDialogForm(options) why is it overwriting unsafeContent without checking if it is set...

@craigweston
Copy link

craigweston commented May 19, 2017

@nicoknoll it seems to be setting it with the output of buildDialogForm to wrap the content in the form html before calling var dialogInstance = vex.open(options), since vex.open expects unsafeContent/content options only:

HubSpot/vex/blob/master/src/vex.js#L186

    // `content` is unsafe internally, so translate
    // safe default: HTML-escape the content before passing it through
    if (opts.unsafeContent && !opts.content) {
      opts.content = opts.unsafeContent
    } else if (opts.content) {
      opts.content = escapeHtml(opts.content)
    }

The problem seems to be that the vex-dialog doesn't take into consideration content/unsafeContent at all (being that buildDialogForm only uses the message option internally and this never gets set to unsafeContent/content). Therefore this ends up just writing the form html without any inner content to unsafeMessage:

vex-dialog/blob/master/src/vex.dialog.js#L11

    open: function open (opts) {
      var options = Object.assign({}, this.defaultOptions, opts)

      // `message` is unsafe internally, so translate
      // safe default: HTML-escape the message before passing it through
      if (options.unsafeMessage && !options.message) {
        options.message = options.unsafeMessage
      } else if (options.message) {
        options.message = vex._escapeHtml(options.message)
      }

      // Build the form from the options
      var form = options.unsafeContent = buildDialogForm(options)

...


// Build DOM elements for the structure of the dialog
var buildDialogForm = function buildDialogForm (options) {
  var form = document.createElement('form')
  form.classList.add('vex-dialog-form')

  var message = document.createElement('div')
  message.classList.add('vex-dialog-message')
  message.appendChild(options.message instanceof window.Node ? options.message : domify(options.message))

Therefore a possible fix/hack would be to set the message option to unsafeContent/content in vex-dialog's open before calling buildDialogForm:

    // Open
    open: function open (opts) {
      var options = Object.assign({}, this.defaultOptions, opts)

      // `message` is unsafe internally, so translate
      // safe default: HTML-escape the message before passing it through
      if (options.unsafeMessage && !options.message) {
          options.message = options.unsafeMessage
      } else if (options.message) {
          options.message = vex._escapeHtml(options.message)
      } else {
         // override options.message with content/unsafeContent
          if (options.unsafeContent && !options.content) {
              options.message = options.unsafeContent
          } else if (options.content) {
              options.message = vex._escapeHtml(options.content)
          }
      }

@bbatliner
Copy link
Contributor

I think this is a documentation problem, rather than a code problem.

Also per the documentation:

These all call the vex.open method with different combinations of options. All of the options that vex.open supports are also supported here.

vex-dialog provides safe by default behavior by treating the message you provide as a regular string, not raw HTML.

If you need to pass through HTML to your dialog, use the unsafeMessage option. The unsafeMessage option is safe to use as long as you provide either static HTML or HTML-escape (html-escape, _.escape, etc.) any untrusted content passed through, such as user-supplied content.

You'll see that when using vex-dialog, the argument is message/unsafeMessage, and when using a regular vex.open, the argument is content/unsafeContent. When vex-dialog sets unsafeContent without checking if it exists first, that's because it expects the message for the dialog to be passed by a different argument. This is not well documented.

Do you agree this is a documentation problem or should we find a better code solution?

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

No branches or pull requests

4 participants