Fork me on GitHub
Subscribe 5

Ticket #681 (fixed bug)

Redirect and execution termination

  • Created: 2012-06-08 20:17:44
  • Reported by: yoorick
  • Assigned to: quy
  • Milestone: 1.5.1
  • Component: code
  • Priority: normal

In login.php, lines 231 and 115, there seems exit; statement to be missing after header('Location ...'); calls. It leads to rendering of "invisible" pages.

Also, I believe that in functions.php in redirect() function code on line 1300 should use exit; statement as well. If I am wrong and it is important to have rendered redirect page along with Location header sent (e.g. browser not supporting redirection), then maybe you should consider using redirect() function instead of header('Location ...'); in places like login.php or viewtopic.php or etc.?

History

Franz 2012-06-09 15:34:52

  • Milestone set to 1.5.1.

yoorick 2012-06-11 21:19:33

  • Uploaded patch functions.php.diff. (view)

One more notice: in redirect() function, lines 1362 and 1381 are not consistent. $destination_url needs to be escaped on every output.

I think it's not worth opening a new ticket.
A patch is attached to this message.

quy 2012-08-04 01:24:37

  • Owner set to quy.

quy 2012-08-04 02:51:11

Commit 44a23b7 to fluxbb fluxbb-1.5

#681: Added exit after redirect header. Reported by yoorick.

quy 2012-08-05 21:57:18

Based on these 2 calls and other calls to the redirect function, as seen the URL parameter is escaped in the call and not within the function.

redirect(htmlspecialchars($_POST['redirect_url']), $lang_login['Login redirect']);
redirect(htmlspecialchars($_POST['redirect_url']), $lang_misc['Email sent redirect']);

Therefore, this can be skipped:

<?php echo str_replace(array('<', '>', '"'), array('&lt;', '&gt;', '&quot;'), $destination_url) ?>

Please confirm to remove the above code.

Franz 2012-08-06 09:25:33

I agree, though maybe we should just move the escaping to the function instead?

Comment edited 1 times (Diff)

yoorick 2012-08-21 20:51:09

Escaping inside the redirect() function would be more convenient for new code because you do not have to explicitly escape it outside. But what about any existing code that is not in FluxBB's core (mods, plugins, etc.), that uses the function? It's like to change an API, I think.
(Actually, I don't know if it is used very often in plugins and mods or not. So it is just my thought on the topic.)

Franz 2012-08-21 22:00:41

The redirect() function is used quite often in mods etc.

quy 2012-10-17 16:10:43

Commit d7bcfa3 to fluxbb master

#681: Use pun_htmlspecialchars in redirect calls to set parameters in htmlspecialchars.

quy 2012-10-17 16:13:49

  • Status changed from open to fixed.

Franz 2012-10-17 16:29:32

As a general thing: it is never good to use htmlspecialchars() without any additional parameters. That allows bad things to happen, for example un-escaped single quotes. smile