login link at top of forum index doesn't go back to forum [#40751]

Comment	File	Size	Author
#23	forum_url_1.patch	605 bytes	chx

#22	forum_url_0.patch	1.31 KB	chx

#21	forum_url.patch	1.31 KB	chx

#12	forum_issue40751.patch	2.05 KB	puregin

#5	loginlinkredirect_2.patch	751 bytes	torgeirb

#2	40751.patch	640 bytes	Wesley Tanaka

	loginlinkredirect.patch	637 bytes	Wesley Tanaka

Comment #1

Dries CreditAttribution: Dries commented 11 December 2005 at 12:33

Status:

Needs review

» Needs work

Incorrect use of url(). Does $tid need to be escaped?

Log in or register to post comments

Comment #2

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 11 December 2005 at 14:03

Status:

Needs work

» Needs review

File	Size
40751.patch	640 bytes

$tid is not escaped elsewhere in the function either because it is expected to be an integer.

Log in or register to post comments

Comment #3

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 13 December 2005 at 09:57

Title:

login link doesn't go back to forum

» login link at top of forum index doesn't go back to forum

Log in or register to post comments

Comment #4

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 17 December 2005 at 12:49

patch still applies against 4.7.0-test2

Log in or register to post comments

Comment #5

torgeirb CreditAttribution: torgeirb commented 17 December 2005 at 19:13

File	Size
loginlinkredirect_2.patch	751 bytes

If user does not actually have access to post in the forum, they are treated to a big fat 'access denied'. I'd rather they be redirected to the page they came from.

Alternatively, the login prompt on the forum needs to be removed if authenticated users are not allowed to create new forum topics.

Log in or register to post comments

Comment #6

moshe weitzman CreditAttribution: moshe weitzman commented 17 December 2005 at 19:29

that $tid looks dangerous to me. url() does not sanitize input. has the tid been sanitized already? otherwise, the intent of the patch is nice.

Log in or register to post comments

Comment #7

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 17 December 2005 at 19:39

See comment #2:

$tid is not escaped elsewhere in the function either because it is expected to be an integer.

Log in or register to post comments

Comment #8

moshe weitzman CreditAttribution: moshe weitzman commented 17 December 2005 at 20:02

Status:

Needs review

» Needs work

that comment says nothing. "it is expected" does not cut it. we have to be sure that this input has been sanitized. this patch won't be accepted without proving that.

Log in or register to post comments

Comment #9

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 17 December 2005 at 20:05

well, then the patch would need to be expanded to include all other times in the function that $tid is printed.

Log in or register to post comments

Comment #10

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 17 December 2005 at 20:06

because the pre-existing code already assumes that $tid has been sanitized

Log in or register to post comments

Comment #11

puregin CreditAttribution: puregin commented 18 December 2005 at 00:05

Added code to ensure $tid is a numeric integer in this function; 'Login' link is generated according to torgeirb's patch.

Log in or register to post comments

Comment #12

puregin CreditAttribution: puregin commented 18 December 2005 at 00:10

File	Size
forum_issue40751.patch	2.05 KB

Oops, here's the patch

Log in or register to post comments

Comment #13

moshe weitzman CreditAttribution: moshe weitzman commented 18 December 2005 at 02:35

i think a simple check_plain() solves the sanitizing need ... there is a line of debug code in there.

Log in or register to post comments

Comment #14

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 18 December 2005 at 07:21

also the last change where you get rid of the check on $tid looks weird.

Log in or register to post comments

Comment #15

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 18 December 2005 at 07:49

settype($tid, 'integer') would also work

Log in or register to post comments

Comment #16

puregin CreditAttribution: puregin commented 18 December 2005 at 21:18

I find typecasting and settype() have behaviours which don't seem appropriate here.

It looks like there are other places in the code where ensuring a given parameter is an integer is important, e.g.

http://drupal.org/node/41369

Is it best to clean up the parameters, or ignore them and just check_plain() the generated links?

Log in or register to post comments

Comment #17

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 19 December 2005 at 03:57

settype would actually do a reasonable thing -- if the user created a url with a $tid that was completely not integer, settype would cast it to 0 which would give you the forum listing.

anyway, like i said back in comment #2, the original non-checking patch doesn't do any worse than the current code.

i don't care how it's done, and if it gives a better feeling of consensus, do check_plain like suggested in comment #13

Log in or register to post comments

Comment #18

Zen CreditAttribution: Zen commented 3 February 2006 at 17:48

Priority:

Critical

» Normal

Downgrading priority. Can someone provide a patch and get this rolling again?

Cheers
-K

Log in or register to post comments

Comment #19

Wesley Tanaka CreditAttribution: Wesley Tanaka commented 4 February 2006 at 08:56

Status:

Needs work

» Needs review

there are several patches that haven't been reviewed yet.

Log in or register to post comments

Comment #20

Dries CreditAttribution: Dries commented 4 February 2006 at 21:55

There is debug information in the last patch.

+  // insure $tid is an integer
+  if (isset($tid) && is_numeric($tid)) {
+      // could still be a numeric string; make it numeric
+      $tid = $tid + 0;
+      // if $tid is not an int, force it to zero
+      if (!is_int($tid)) {
+          $tid = 0;
+      }
+  }
+  else {
+    $tid = 0;
+  }
+ echo "theme_forum_display::tid = $tid<br/>\n";

How about using intval() instead?