Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Bug fix for relative directory removal #2055

Merged
merged 2 commits into from Dec 6, 2012

Conversation

Projects
None yet
2 participants
Contributor

chernjie commented Dec 6, 2012

This patch attempts to fix two bugs:

  • for segments that ends with ".." e.g. /user/username../details, this should not be replaced
  • current solution only replace double slashes, this solution removes an infinite number of recurring slashes
Contributor

chernjie commented Dec 6, 2012

UnitTest: https://gist.github.com/4221865

time php UnitTest-e8af5d1.php
. ../
. /../
. abc/../
. ../def
. /../def
. abc/../../def
. abc../
. abc/def../
. abc/def../ghi
. abc//def
. abc///def
. abc////def
. 0/abc
. 00/abc
. abc/0
. abc/00
. abc/0/def
. abc/00/def
Bug fix for relative directory removal
This fixes two bugs:
- for segments that ends with ".." e.g. /user/username../details, this should not be replaced
- current solution only replace double slashes, this solutions removes the infinite number of recurring slashes
Contributor

narfbg commented Dec 6, 2012

Nice! However, in order to get this into CI, some adjustments need to be made in order to meet the styleguide requiremens. I'll comment on the diff.

Edit: Also, if this fixes a bug - it should be noted in the changelog.

@narfbg narfbg commented on an outdated diff Dec 6, 2012

system/core/URI.php
@@ -219,7 +219,26 @@ protected function _parse_request_uri()
}
// Do some final cleaning of the URI and return it
- return str_replace(array('//', '../'), '/', trim($uri, '/'));
+ return $this->_remove_relative_directory_str($uri);
+ }
+
+ // --------------------------------------------------------------------
+
+ /**
+ * Remove relative directory (../) and multi slashes (///)
+ * @param string $url
@narfbg

narfbg Dec 6, 2012

Contributor

Use only tabs as separators - no spaces (between 'string' and '$url' + although followed by a tab, you have spaces after both @param and @return).
Additionally, for the sake of consistency there should be an empty description line between 228 & 229.

@narfbg narfbg and 1 other commented on an outdated diff Dec 6, 2012

system/core/URI.php
@@ -219,7 +219,26 @@ protected function _parse_request_uri()
}
// Do some final cleaning of the URI and return it
- return str_replace(array('//', '../'), '/', trim($uri, '/'));
+ return $this->_remove_relative_directory_str($uri);
+ }
+
+ // --------------------------------------------------------------------
+
+ /**
+ * Remove relative directory (../) and multi slashes (///)
+ * @param string $url
+ * @return string
+ */
+ private function _remove_relative_directory_str($url)
@narfbg

narfbg Dec 6, 2012

Contributor

No private methods please (protected is good) - this makes it impossible to extend parts of the core.
Also, why does this method need to be separated if it's only used by _parse_request_uri()?

@chernjie

chernjie Dec 6, 2012

Contributor

For readability, would you prefer if it's merged into the previous method _parse_request_uri()?

@narfbg

narfbg Dec 6, 2012

Contributor

Unless it would be used elsewhere - yes, otherwise it doesn't make sense (functionally). Just leave a comment above it to describe what it does.

@narfbg narfbg and 1 other commented on an outdated diff Dec 6, 2012

system/core/URI.php
- return str_replace(array('//', '../'), '/', trim($uri, '/'));
+ return $this->_remove_relative_directory_str($uri);
+ }
+
+ // --------------------------------------------------------------------
+
+ /**
+ * Remove relative directory (../) and multi slashes (///)
+ * @param string $url
+ * @return string
+ */
+ private function _remove_relative_directory_str($url)
+ {
+ $uris = array();
+ $tok = strtok($url, '/');
+ while ($tok !== false)
@narfbg

narfbg Dec 6, 2012

Contributor

false -> FALSE

Also might be a good idea to just do this and remove lines 235 & 239 (the latter doesn't seem correct anyway - shouldn't $url be present in there?):

while (($tok = strtok($url, '/')) !== FALSE)
@chernjie

chernjie Dec 6, 2012

Contributor

According to PHP manual: http://php.net/manual/en/function.strtok.php

<?php
$string = "This is\tan example\nstring";
/* Use tab and newline as tokenizing characters as well  */
$tok = strtok($string, " \n\t");

while ($tok !== false) {
    echo "Word=$tok<br />";
    $tok = strtok(" \n\t");
}
?>
@narfbg

narfbg Dec 6, 2012

Contributor

Okay, agreed.

@narfbg narfbg and 1 other commented on an outdated diff Dec 6, 2012

system/core/URI.php
+ }
+
+ // --------------------------------------------------------------------
+
+ /**
+ * Remove relative directory (../) and multi slashes (///)
+ * @param string $url
+ * @return string
+ */
+ private function _remove_relative_directory_str($url)
+ {
+ $uris = array();
+ $tok = strtok($url, '/');
+ while ($tok !== false)
+ {
+ ($tok != '..' && ! empty($tok) || $tok === '0') && $uris[] = $tok;
@narfbg

narfbg Dec 6, 2012

Contributor

This one looks really weird, should be converted to an if, with several style changes:

if (( ! empty($tok) && $tok !== '0') && $tok !== '..')
{
    $uris[] = $tok;
}
@chernjie

chernjie Dec 6, 2012

Contributor

It does not work :(

$ time php UnitTest-e8af5d1.php | grep -E '^F'
F 0/abc
F abc/0
F abc/0/def
@narfbg

narfbg Dec 6, 2012

Contributor

Yep, my fault ... should've been this:

if (( ! empty($tok) OR $tok === '0') && $tok !== '..')
@chernjie

chernjie Dec 6, 2012

Contributor

Okay fixed, committing now

narfbg added a commit that referenced this pull request Dec 6, 2012

Merge pull request #2055 from chernjie/develop
Bug fix for relative directory removal

@narfbg narfbg merged commit 3fd3345 into bcit-ci:develop Dec 6, 2012

1 check was pending

default The Travis build is in progress
Details
Contributor

chernjie commented Dec 6, 2012

@narfbg I realized there's another snippet that also shares the same code
chernjie/CodeIgniter@5addf14

Contributor

narfbg commented Dec 6, 2012

Contributor

chernjie commented Dec 6, 2012

That's fast! :p

nonchip pushed a commit to nonchip/CodeIgniter that referenced this pull request Jun 29, 2013

Merge pull request #2055 from chernjie/develop
Bug fix for relative directory removal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment