Make users aware of risks when implementing autoComplete

[needlag]

Hello guys, thank you for your contribution, however I suggest you make the users aware that they have to work on protecting their search form from XFS & XSS attacks... e.g.

Goto your own example form https://tarekraafat.github.io/autoComplete.js/demo/ and search literally for:

<a href="javascript:alert('Hi')">hi</a>

then search literally for:

<iframe src="https://tarekraafat.github.io/autoComplete.js/#/" height="200" width="300" title="Iframe Example"></iframe>

Malicious users may find autoComplete a source of vulnerable clients or victims for attacking others or right on them.

My suggestion is to make users aware of such risks before they go into production. To check their code for Cross-Frame Scripting and Cross-Site Scripting vulnerabilities...

I am not saying you are super wrong, no, your contribution is beautiful, yet there are less experienced users out there probably implementing your code.

Cheers.

[folknor]

Just for reference (and in case anyone has better tips on how I should be doing it), here's how I currently sanitize the query for use in the HTML:

import DOMPurify from "dompurify"

function init() {
    DOMPurify.setConfig({ ALLOWED_TAGS: [] }) // No allowed tags
}

var s = dt => String(DOMPurify.sanitize(String(dt))).trim()

new autoComplete({
    resultsList: {
        element: (element, data) => {
            const { query, matches } = data
            if (
                matches.length === 0
            ) {
                const result = document.createElement("p")
                result.innerHTML = `<span class="fa fa-exclamation-triangle"></span>Found no results for "${s(query)}".`
                element.append(result)
            }
        },
    },
})

https://github.com/cure53/DOMPurify

[needlag]

Just for reference (and in case anyone has better tips on how I should be doing it), here's how I currently sanitize the query for use in the HTML:

import DOMPurify from "dompurify"

function init() {
    DOMPurify.setConfig({ ALLOWED_TAGS: [] }) // No allowed tags
}

var s = dt => String(DOMPurify.sanitize(String(dt))).trim()

new autoComplete({
  resultsList: {
      element: (element, data) => {
          const { query, matches } = data
          if (
              matches.length === 0
          ) {
              const result = document.createElement("p")
              result.innerHTML = `<span class="fa fa-exclamation-triangle"></span>Found no results for "${s(query)}".`
              element.append(result)
          }
      },
  },
})

https://github.com/cure53/DOMPurify

Anyway I would avoid any output that may contain the query, e.g. -Found no results for "${s(query)}". Found no results.

Best practices consider (but not limited to) filtering input on arrival, just the way you are doing it, then encoding the output returned, implementing a content security policy (CSP), and prevent other dangling markup attacks. I suggest you dig much deeper into it.

Good luck!

[TarekRaafat]

Hello @needlag,

I totally agree with you on your point of spreading awareness of the possible risks.

That's why I have added to the library's docs under the Usage section a security alert on this specific risk along with some recommendations, and please feel free to check it out and share your feedback on it if you'd like to.

Thank you for raising such a critical point, and have a nice day! :)

[TarekRaafat]

Hey @folknor,

Why don't you use the query API instead of the resultsList?

[folknor]

Why don't you use the query API instead of the resultsList?

Do you mean like this?

var s = dt => String(DOMPurify.sanitize(String(dt))).trim()
...
query: q => {
    return s(q)
},

I was investigating it now because of your comment and I think I found a bug in start.js:

export default async function (ctx, q) {
  // Get "input" query value
  let queryVal = q || getQuery(ctx.input);
  queryVal = ctx.query ? query(queryVal) : queryVal;

Last line there should be queryVal = ctx.query ? ctx.query(queryVal) : queryVal;. The bug was introduced now in 10.2.2: https://github.com/TarekRaafat/autoComplete.js/commit/485fd7d398207d0c0da41dbc3e3cb39057f62a1e#diff-d51bf424857227bf919f175f26f72008021098c66d58de2682b0787f8fe5f4d7L16

EDIT: Also, the query method invocation in start.js can be moved inside the trigger conditional?

[folknor]

Could be worth mentioning https://github.com/sindresorhus/escape-goat in the docs as well.

[TarekRaafat]

@folknor, good catch as usual!

I've fixed it and publishing it now, and I've added escape-goat to the references as well.

Thanks man, and have a nice day! :)