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

Test failures with patch for CVE-2020-8161 #1655

Closed
utkarsh2102 opened this issue May 21, 2020 · 2 comments
Closed

Test failures with patch for CVE-2020-8161 #1655

utkarsh2102 opened this issue May 21, 2020 · 2 comments

Comments

@utkarsh2102
Copy link
Contributor

Hi @tenderlove,

With the patch for CVE-2020-8161 (that is, commit: dddb7ad), there are following test failures:

  1) Failure:
Rack::Directory#test_0009_uri escape path parts [/home/utkarsh/debian/ruby-team/rack/ruby-rack/test/spec_directory.rb:112]:
Expected /\/cgi\/test\+directory\/test\+file/ to match # encoding: ASCII-8BIT
#    valid: true
"<html><head>\n  <title>&#x2F;cgi&#x2F;test+directory</title>\n  <meta http-equiv=\"content-type\" content=\"text/html; charset=utf-8\" />\n  <style type='text/css'>\ntable { width:100%; }\n.name { text-align:left; }\n.size, .mtime { text-align:right; }\n.type { width:11em; }\n.mtime { width:15em; }\n  </style>\n</head><body>\n<h1>&#x2F;cgi&#x2F;test+directory</h1>\n<hr />\n<table>\n  <tr>\n    <th class='name'>Name</th>\n    <th class='size'>Size</th>\n    <th class='type'>Type</th>\n    <th class='mtime'>Last Modified</th>\n  </tr>\n<tr><td class='name'><a href='../'>Parent Directory</a></td><td class='size'></td><td class='type'></td><td class='mtime'></td></tr>\n</table>\n<hr />\n</body></html>\n".

  2) Failure:
Rack::Directory#test_0011_correctly escape script name [/home/utkarsh/debian/ruby-team/rack/ruby-rack/test/spec_directory.rb:149]:
Expected /\/script-path\/cgi\/test\+directory\/test\+file/ to match # encoding: ASCII-8BIT
#    valid: true
"<html><head>\n  <title>&#x2F;cgi&#x2F;test+directory</title>\n  <meta http-equiv=\"content-type\" content=\"text/html; charset=utf-8\" />\n  <style type='text/css'>\ntable { width:100%; }\n.name { text-align:left; }\n.size, .mtime { text-align:right; }\n.type { width:11em; }\n.mtime { width:15em; }\n  </style>\n</head><body>\n<h1>&#x2F;cgi&#x2F;test+directory</h1>\n<hr />\n<table>\n  <tr>\n    <th class='name'>Name</th>\n    <th class='size'>Size</th>\n    <th class='type'>Type</th>\n    <th class='mtime'>Last Modified</th>\n  </tr>\n<tr><td class='name'><a href='../'>Parent Directory</a></td><td class='size'></td><td class='type'></td><td class='mtime'></td></tr>\n</table>\n<hr />\n</body></html>\n".

  3) Failure:
Rack::Directory#test_0010_correctly escape script name with spaces [/home/utkarsh/debian/ruby-team/rack/ruby-rack/test/spec_directory.rb:132]:
Expected /\/foo%20bar\/omg%20omg\.txt/ to match "<html><head>\n  <title>&#x2F;foo bar&#x2F;</title>\n  <meta http-equiv=\"content-type\" content=\"text/html; charset=utf-8\" />\n  <style type='text/css'>\ntable { width:100%; }\n.name { text-align:left; }\n.size, .mtime { text-align:right; }\n.type { width:11em; }\n.mtime { width:15em; }\n  </style>\n</head><body>\n<h1>&#x2F;foo bar&#x2F;</h1>\n<hr />\n<table>\n  <tr>\n    <th class='name'>Name</th>\n    <th class='size'>Size</th>\n    <th class='type'>Type</th>\n    <th class='mtime'>Last Modified</th>\n  </tr>\n<tr><td class='name'><a href='../'>Parent Directory</a></td><td class='size'></td><td class='type'></td><td class='mtime'></td></tr>\n</table>\n<hr />\n</body></html>\n".
@utkarsh2102
Copy link
Contributor Author

utkarsh2102 commented May 21, 2020

I am unsure if we could apply the following patch removing the 3 tests since we already this new test added here: 775c836

I'll be happy to raise a PR if so? :)

--- a/test/spec_directory.rb
+++ b/test/spec_directory.rb
@@ -91,55 +91,6 @@ describe Rack::Directory do
     res.must_be :not_found?
   end
 
-  it "uri escape path parts" do # #265, properly escape file names
-    mr = Rack::MockRequest.new(Rack::Lint.new(app))
-
-    res = mr.get("/cgi/test%2bdirectory")
-
-    res.must_be :ok?
-    res.body.must_match(%r[/cgi/test\+directory/test\+file])
-
-    res = mr.get("/cgi/test%2bdirectory/test%2bfile")
-    res.must_be :ok?
-  end
-
-  it "correctly escape script name with spaces" do
-    Dir.mktmpdir do |dir|
-      space_dir = "foo bar"
-      full_dir = File.join(dir, space_dir)
-      FileUtils.mkdir full_dir
-      FileUtils.touch File.join(full_dir, "omg omg.txt")
-      app = Rack::Directory.new(dir, FILE_CATCH)
-      env = Rack::MockRequest.env_for(Rack::Utils.escape_path("/#{space_dir}/"))
-      status, _, body = app.call env
-
-      assert_equal 200, status
-
-      str = ''.dup
-      body.each { |x| str << x }
-      assert_match "/foo%20bar/omg%20omg.txt", str
-    end
-  end
-
-  it "correctly escape script name" do
-    _app = app
-    app2 = Rack::Builder.new do
-      map '/script-path' do
-        run _app
-      end
-    end
-
-    mr = Rack::MockRequest.new(Rack::Lint.new(app2))
-
-    res = mr.get("/script-path/cgi/test%2bdirectory")
-
-    res.must_be :ok?
-    res.body.must_match(%r[/script-path/cgi/test\+directory/test\+file])
-
-    res = mr.get("/script-path/cgi/test+directory/test+file")
-    res.must_be :ok?
-  end
-
   it "return error when file not found for head request" do
     res = Rack::MockRequest.new(Rack::Lint.new(app)).
       head("/cgi/missing")

@tenderlove
Copy link
Member

I think I fixed it in e7ba1b0

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

No branches or pull requests

2 participants