Skip to content
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

normalize_uri fixes (double slashes and trailing slash) #1397

Merged
merged 3 commits into from Jan 29, 2013

Conversation

wchen-r7
Copy link
Contributor

This pull request fixes mainly two issues:

  1. One of the intended behaviors for normalize_uri() is to eliminate all double slashes, but all it does is it removes the first one. So this pull request addresses the correct behavior. I also added more comments to better explain what the function does.
  2. Another problem is normalize_uri() removes the trailing slash, but sometimes that slash is actually needed. For example: if a module wants TARGETURI to be a directory, you should NOT strip the trailing slash. If you do, you may end up getting a 301 or 404 instead of 200 (expected output). This pull request only modifies those that need TARGETURI to be a directory.

Related to the following ticket:
http://dev.metasploit.com/redmine/issues/7727#change-34153

This will make sure all the double slashes are gone.  Also, the
function description is updated to clarify its purpose.
These modules require the target URI to be a directory path. So
if you remove the trailing slash, the web server might return a
301 or 404 instead of 200.

Related to: [SeeRM: rapid7#7727]
@@ -218,7 +218,7 @@ def run_host(ip)

#Get GlassFish version
edition, version, banner = get_version(res)
path = normalize_uri(datastore['PATH'])
path = normalize_uri(target_uri)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

target_uri is a URI object. You probably wanted target_uri.path

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, fixed.

@todb-r7
Copy link

todb-r7 commented Jan 29, 2013

You're sure it wouldn't be better to just return the old behavior? End with at most one slash, if the TARGET_URI ends with a slash? I feel like the last change that forced dropping slashes was wrong, now.

@todb-r7
Copy link

todb-r7 commented Jan 29, 2013

It restores the old "//" typos but that's the module's problem, not really the normalizer's problem. No?

@wchen-r7
Copy link
Contributor Author

Egypt and I talked about this problem. Basically, if you go back to the old behavior, you risk having double slashes. If you use normalize_uri(), a missing trailing slash when by default is needed can cause the module to malfunction, too. You are stuck between a rock, and a hard place.

I feel like this is the most ideal thing to do because many modules already check if there's a missing trailing slash after it's normalized. Although this kind of remedies our current problem, we'll have to rethink how a URI string should be handled. At one point we even talked about having a URI.join(), kinda like File.join(), for joining URIs.

@todb-r7
Copy link

todb-r7 commented Jan 29, 2013

This is all covered with some pretty exhaustive rspec tests, turns out:

(target_uri_fix) todb@aus-linux-1030:~/git/rapid7/metasploit-framework/spec/lib/msf/core/exploit/http$ rspec client_spec.rb
.......................

Finished in 0.02553 seconds
23 examples, 0 failures

Thanks @limhoff-r7 !

todb-r7 pushed a commit that referenced this pull request Jan 29, 2013
normalize_uri fixes (double slashes and trailing slash)
@todb-r7 todb-r7 merged commit 6002e35 into rapid7:master Jan 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants