silverstripe / silverstripe-redirectedurls

Silverstripe module to let users to configure arbitrary redirections in the CMS
BSD 3-Clause "New" or "Revised" License
32 stars 48 forks source link

BUGFIX: Fixed warning if the variable passed to RedirectedURLHandler::arrayToLowercase() is not an array #36

Closed UndefinedOffset closed 7 years ago

UndefinedOffset commented 7 years ago

In some cases RedirectedURLHandler::arrayToLowercase() can get passed an argument that is not an array resulting in a warning "[Warning] Invalid argument supplied for foreach() in RedirectedURLHandler.php line 24". This pull request adds a check to ensure that the $vars argument is actually an array before attempting to iterate over the $vars argument.

I'm not exactly sure how malformed the url has to be to actually produce it I wasn't able to line up the error timestamp with the access logs. However this fix should resolve the problem by simply checking to see if the $vars argument is actually an array or not before attempting to iterate over it.

dhensby commented 7 years ago

Which cases cause a non-array to be passed? I think it's better to fix those cases (the root issue) than just allow the method to fail silently

UndefinedOffset commented 7 years ago

Ya I tried finding it in the logs I just keep seeing it every once and a while appear in New Relic which by default does not capture the url parameters. I can keep an eye open for the next time it occurs, but I doubt it's normal traffic causing it but traffic attempting to do things it shouldn't ;).

dhensby commented 7 years ago

Bots that don't build the URL's properly because the ignore the <base>?

If you could find some more info about what's causing the problem I'd rather take that approach than changing the behaviour if this method as this PR does.

UndefinedOffset commented 7 years ago

@dhensby finally got one that produced the error, good old Baiduspider :P

http://example.com/plus/download.php?open=1&arrs1[]=99&arrs1[]=102&arrs1[]=103&arrs1[]=95&arrs1[]=100&arrs1[]=98&arrs1[]=112&arrs1[]=114&arrs1[]=101&arrs1[]=102&arrs1[]=105&arrs1[]=120&arrs2[]=109&arrs2[]=121&arrs2[]=116&arrs2[]=97&arrs2[]=103&arrs2[]=96&arrs2[]=32&arrs2[]=40&arrs2[]=97&arrs2[]=105&arrs2[]=100&arrs2[]=44&arrs2[]=101&arrs2[]=120&arrs2[]=112&arrs2[]=98&arrs2[]=111&arrs2[]=100&arrs2[]=121&arrs2[]=44&arrs2[]=110&arrs2[]=111&arrs2[]=114&arrs2[]=109&arrs2[]=98&arrs2[]=111&arrs2[]=100&arrs2[]=121&arrs2[]=41&arrs2[]=32&arrs2[]=86&arrs2[]=65&arrs2[]=76&arrs2[]=85&arrs2[]=69&arrs2[]=83&arrs2[]=40&arrs2[]=57&arrs2[]=48&arrs2[]=49&arrs2[]=51&arrs2[]=44&arrs2[]=64&arrs2[]=96&arrs2[]=92&arrs2[]=39&arrs2[]=96&arrs2[]=44&arrs2[]=39&arrs2[]=123&arrs2[]=100&arrs2[]=101&arrs2[]=100&arrs2[]=101&arrs2[]=58&arrs2[]=112&arrs2[]=104&arrs2[]=112&arrs2[]=125&arrs2[]=102&arrs2[]=105&arrs2[]=108&arrs2[]=101& arrs2[]= 95&arrs2[]=112&arrs2[]=117&arrs2[]=116&arrs2[]=95&arrs2[]=99&arrs2[]=111&arrs2[]=110&arrs2[]=116&arrs2[]=101&arrs2[]=110&arrs2[]=116&arrs2[]=115&arrs2[]=40&arrs2[]=39&arrs2[]=39&arrs2[]=57&arrs2[]=48&arrs2[]=115&arrs2[]=101&arrs2[]=99&arrs2[]=46&arrs2[]=112&arrs2[]=104&arrs2[]=112&arrs2[]=39&arrs2[]=39&arrs2[]=44&arrs2[]=39&arrs2[]=39&arrs2[]=60&arrs2[]=63&arrs2[]=112&arrs2[]=104&arrs2[]=112&arrs2[]=32&arrs2[]=101&arrs2[]=118&arrs2[]=97&arrs2[]=108&arrs2[]=40&arrs2[]=36&arrs2[]=95&arrs2[]=80&arrs2[]=79&arrs2[]=83&arrs2[]=84&arrs2[]=91&arrs2[]=103&arrs2[]=117&arrs2[]=105&arrs2[]=103&arrs2[]=101&arrs2[]=93&arrs2[]=41&arrs2[]=59&arrs2[]=63&arrs2[]=62&arrs2[]=39&arrs2[]=39&arrs2[]=41&arrs2[]=59&arrs2[]=123&arrs2[]=47&arrs2[]=100&arrs2[]=101&arrs2[]=100&arrs2[]=101&arrs2[]=58&arrs2[]=112&arrs2[]=104&arrs2[]=112&arrs2[]=125&arrs2[]=39&arrs2[]=41&arrs2[]=32&arrs2[]=35&arrs2[]=32&arrs2[]=64&arrs2[]=96&arrs2[]=92&arrs2[]=39&arrs2[]=96
dhensby commented 7 years ago

Hmmm - is the problem the URL is too long and so the get vars are being truncated? are you able to inspect what the $_GET var looks like in this instance?

UndefinedOffset commented 7 years ago

So doing a var dump on get produces the below:

array(4) {
  ["url"]=>
  string(18) "/plus/download.php"
  ["open"]=>
  string(1) "1"
  ["arrs1"]=>
  array(12) {
    [0]=>
    string(2) "99"
    [1]=>
    string(3) "102"
    [2]=>
    string(3) "103"
    [3]=>
    string(2) "95"
    [4]=>
    string(3) "100"
    [5]=>
    string(2) "98"
    [6]=>
    string(3) "112"
    [7]=>
    string(3) "114"
    [8]=>
    string(3) "101"
    [9]=>
    string(3) "102"
    [10]=>
    string(3) "105"
    [11]=>
    string(3) "120"
  }
  ["arrs2"]=>
  array(146) {
    [0]=>
    string(3) "109"
    [1]=>
    string(3) "121"
    [2]=>
    string(3) "116"
    [3]=>
    string(2) "97"
    [4]=>
    string(3) "103"
    [5]=>
    string(2) "96"
    [6]=>
    string(2) "32"
    [7]=>
    string(2) "40"
    [8]=>
    string(2) "97"
    [9]=>
    string(3) "105"
    [10]=>
    string(3) "100"
    [11]=>
    string(2) "44"
    [12]=>
    string(3) "101"
    [13]=>
    string(3) "120"
    [14]=>
    string(3) "112"
    [15]=>
    string(2) "98"
    [16]=>
    string(3) "111"
    [17]=>
    string(3) "100"
    [18]=>
    string(3) "121"
    [19]=>
    string(2) "44"
    [20]=>
    string(3) "110"
    [21]=>
    string(3) "111"
    [22]=>
    string(3) "114"
    [23]=>
    string(3) "109"
    [24]=>
    string(2) "98"
    [25]=>
    string(3) "111"
    [26]=>
    string(3) "100"
    [27]=>
    string(3) "121"
    [28]=>
    string(2) "41"
    [29]=>
    string(2) "32"
    [30]=>
    string(2) "86"
    [31]=>
    string(2) "65"
    [32]=>
    string(2) "76"
    [33]=>
    string(2) "85"
    [34]=>
    string(2) "69"
    [35]=>
    string(2) "83"
    [36]=>
    string(2) "40"
    [37]=>
    string(2) "57"
    [38]=>
    string(2) "48"
    [39]=>
    string(2) "49"
    [40]=>
    string(2) "51"
    [41]=>
    string(2) "44"
    [42]=>
    string(2) "64"
    [43]=>
    string(2) "96"
    [44]=>
    string(2) "92"
    [45]=>
    string(2) "39"
    [46]=>
    string(2) "96"
    [47]=>
    string(2) "44"
    [48]=>
    string(2) "39"
    [49]=>
    string(3) "123"
    [50]=>
    string(3) "100"
    [51]=>
    string(3) "101"
    [52]=>
    string(3) "100"
    [53]=>
    string(3) "101"
    [54]=>
    string(2) "58"
    [55]=>
    string(3) "112"
    [56]=>
    string(3) "104"
    [57]=>
    string(3) "112"
    [58]=>
    string(3) "125"
    [59]=>
    string(3) "102"
    [60]=>
    string(3) "105"
    [61]=>
    string(3) "108"
    [62]=>
    string(3) "101"
    [63]=>
    string(3) " 95"
    [64]=>
    string(3) "112"
    [65]=>
    string(3) "117"
    [66]=>
    string(3) "116"
    [67]=>
    string(2) "95"
    [68]=>
    string(2) "99"
    [69]=>
    string(3) "111"
    [70]=>
    string(3) "110"
    [71]=>
    string(3) "116"
    [72]=>
    string(3) "101"
    [73]=>
    string(3) "110"
    [74]=>
    string(3) "116"
    [75]=>
    string(3) "115"
    [76]=>
    string(2) "40"
    [77]=>
    string(2) "39"
    [78]=>
    string(2) "39"
    [79]=>
    string(2) "57"
    [80]=>
    string(2) "48"
    [81]=>
    string(3) "115"
    [82]=>
    string(3) "101"
    [83]=>
    string(2) "99"
    [84]=>
    string(2) "46"
    [85]=>
    string(3) "112"
    [86]=>
    string(3) "104"
    [87]=>
    string(3) "112"
    [88]=>
    string(2) "39"
    [89]=>
    string(2) "39"
    [90]=>
    string(2) "44"
    [91]=>
    string(2) "39"
    [92]=>
    string(2) "39"
    [93]=>
    string(2) "60"
    [94]=>
    string(2) "63"
    [95]=>
    string(3) "112"
    [96]=>
    string(3) "104"
    [97]=>
    string(3) "112"
    [98]=>
    string(2) "32"
    [99]=>
    string(3) "101"
    [100]=>
    string(3) "118"
    [101]=>
    string(2) "97"
    [102]=>
    string(3) "108"
    [103]=>
    string(2) "40"
    [104]=>
    string(2) "36"
    [105]=>
    string(2) "95"
    [106]=>
    string(2) "80"
    [107]=>
    string(2) "79"
    [108]=>
    string(2) "83"
    [109]=>
    string(2) "84"
    [110]=>
    string(2) "91"
    [111]=>
    string(3) "103"
    [112]=>
    string(3) "117"
    [113]=>
    string(3) "105"
    [114]=>
    string(3) "103"
    [115]=>
    string(3) "101"
    [116]=>
    string(2) "93"
    [117]=>
    string(2) "41"
    [118]=>
    string(2) "59"
    [119]=>
    string(2) "63"
    [120]=>
    string(2) "62"
    [121]=>
    string(2) "39"
    [122]=>
    string(2) "39"
    [123]=>
    string(2) "41"
    [124]=>
    string(2) "59"
    [125]=>
    string(3) "123"
    [126]=>
    string(2) "47"
    [127]=>
    string(3) "100"
    [128]=>
    string(3) "101"
    [129]=>
    string(3) "100"
    [130]=>
    string(3) "101"
    [131]=>
    string(2) "58"
    [132]=>
    string(3) "112"
    [133]=>
    string(3) "104"
    [134]=>
    string(3) "112"
    [135]=>
    string(3) "125"
    [136]=>
    string(2) "39"
    [137]=>
    string(2) "41"
    [138]=>
    string(2) "32"
    [139]=>
    string(2) "35"
    [140]=>
    string(2) "32"
    [141]=>
    string(2) "64"
    [142]=>
    string(2) "96"
    [143]=>
    string(2) "92"
    [144]=>
    string(2) "39"
    [145]=>
    string(2) "96"
  }
}

Dough I see the real issue -_- have a look at this line. The value is being run through string to lower which is causing this issue entirely. Would you prefer I just amend my commit for this pull or close and submit at new one?

dhensby commented 7 years ago

classic - just amend this PR :)

dhensby commented 7 years ago

thanks for getting to the bottom of it

UndefinedOffset commented 7 years ago

done :)

dhensby commented 7 years ago

tests still really unhappy :(

dhensby commented 7 years ago

Oh - they've been unhappy for a while..

UndefinedOffset commented 7 years ago

ya I meant to look into that as well but had to table it due to deadlines :)