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

Ban reasons are encoded when you enter them, but aren't unencoded when displayed. #341

Closed
popey456963 opened this Issue Aug 15, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@popey456963
Copy link

popey456963 commented Aug 15, 2017

Expected Behavior

  • Ban reasons are displayed as entered

Current Behavior

  • Ban reasons are displayed URL encoded

Possible Solution

  • URL unencode them?

Steps to Reproduce (for bugs)

  1. Set a custom ban reason to something like "RDM'ing"
  2. Ban someone for that reason.
  3. Observe how the reason is actually "RDM&&027;ing"

Context

  • Was trying to create a custom ban reason that had a special character in it
@rumblefrog

This comment has been minimized.

Copy link
Member

rumblefrog commented Aug 15, 2017

It's not URL encoding, it's htmlentities, and it's there to prevent XSS

@rumblefrog

This comment has been minimized.

Copy link
Member

rumblefrog commented Aug 15, 2017

<td height="16" class="listtable_1">{$ban.reason|escape:'html'}</td>

https://www.smarty.net/docsv2/en/language.modifier.escape

And it seems like it was escaping it improperly.

@popey456963

This comment has been minimized.

Copy link

popey456963 commented Aug 15, 2017

Oops, sorry for using the wrong terminology. I just had to solve an issue with MyBB that required using HTML entities in the URL which led my mind to be ever so confused.

My assumption is it's getting escaped twice, once here and another time some other place.

 - '
 - &027;
 - &amp; &027;

Then only getting decoded once:

 - &027;
@rumblefrog

This comment has been minimized.

Copy link
Member

rumblefrog commented Aug 15, 2017

You can try to remove the modifier, but it's best to leave it there for security sakes. A non-pretty ban reason is better than a defaced website :P

@popey456963

This comment has been minimized.

Copy link

popey456963 commented Aug 16, 2017

@rumblefrog An excellent point on the current version, I changed it to RDMing instead of RDM'ing. Been combing through the code for a bit, removed that statement on a debug version of SB++ & it still comes back to me as the correct (and safe) &#27; which displays as '.

My confusion is where it's getting encoded again, can't seem to spot it.

@rumblefrog

This comment has been minimized.

Copy link
Member

rumblefrog commented Aug 16, 2017

It escapes it when you input it, then escapes it again on fetch.

Here's my trace

include_once(INCLUDES_PATH . "/page-builder.php");

$page = INCLUDES_PATH . "/admin.php";

include TEMPLATES_PATH . "/admin.bans.php";

$theme->display('page_admin_bans_add.tpl');

{sb_button text="Add Ban" onclick="ProcessBan();" class="ok" id="aban" submit=false}

function ProcessBan()

xajax_AddBan($('nickname').value,

$xajax->registerFunction("AddBan");

function AddBan($nickname, $type, $steam, $ip, $length, $dfile, $dname, $reason, $fromsub)

$reason = RemoveCode($reason);

function RemoveCode($text)
{
return htmlspecialchars(strip_tags($text));
}

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