Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

#127 Username XSS #130

Merged
merged 5 commits into from
May 19, 2015
Merged

#127 Username XSS #130

merged 5 commits into from
May 19, 2015

Conversation

JN-Jones
Copy link
Contributor

@JN-Jones JN-Jones commented May 4, 2015

#127

The XSS is in the powertip plugin we're using which allows the usage of HTML but unfortunately doesn't have a setting to display it. We need to subscribe to the render event and reescape the strings there (afaik we don't have a tooltip which has HTML). As I don't have NodeJS installed on my Laptop I need to take a look at that later.

@JN-Jones
Copy link
Contributor Author

JN-Jones commented May 4, 2015

Events didn't work so I needed to overwrite one of the internal functions. I've created an issue so hopefully an option will be added but till then this seems to be the best way.

@Destroy666x
Copy link
Contributor

I don't think that escaping it globally in Powertip is a good solution, we may want to use HTML in a tooltip at a later stage. Why wouldn't escaping the username variable with Twig work instead?

@JN-Jones
Copy link
Contributor Author

JN-Jones commented May 6, 2015

Twig is configured to escape everything automatically. I've also tried to escape it again but that didn't work too. As mentioned I'm hoping that a proper option is added to make it a lot easier.

@Destroy666x
Copy link
Contributor

Twig is configured to escape everything automatically

Yes, but the "strategy" can be changed to make it work 2 times: http://twig.sensiolabs.org/doc/filters/escape.html

{% set strategy = 'html' %}

{% autoescape 'html' %}
    {{ var|escape('html') }}   {# won't be double-escaped #}
    {{ var|escape(strategy) }} {# will be double-escaped #}
{% endautoescape %}

@JN-Jones
Copy link
Contributor Author

JN-Jones commented May 7, 2015

I'll try to take a look at that later but I don't think that'll work too. Also I think that the global (or option wise) escaping is better, instead of multiple escaping. Depending on how the option is implemented to the library I'd add a flag attribute to the powertip elements:
<a href="(...)" title="{{ var }}">(...)</a> would be escaped (default) and <a href="(...)" title="{{ var }}" data-powertip-escape="false">(...)</a> wouldn't be escaped. But as said, I need the option first for that.

@JN-Jones
Copy link
Contributor Author

@euantorano @wpillar

@JN-Jones JN-Jones changed the title WIP #127 Username XSS #127 Username XSS May 18, 2015
Conflicts:
	public/assets/js/main.js.min.map
	public/assets/js/main.min.js
@euantorano
Copy link
Member

Looks like this PR is now broken ;)

@JN-Jones
Copy link
Contributor Author

As always xD will fix it when I'm home (haven't installed gulp on my laptop)

Conflicts:
	public/assets/js/main.js
	public/assets/js/main.js.min.map
	public/assets/js/main.min.js
	public/js/other.js
@JN-Jones
Copy link
Contributor Author

This has been fixed @euantorano

euantorano added a commit that referenced this pull request May 19, 2015
@euantorano euantorano merged commit 3d0da84 into mybb:master May 19, 2015
@euantorano euantorano deleted the fix-127-username-xss branch May 19, 2015 17:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants