ruby / uri

URI is a module providing classes to handle Uniform Resource Identifiers
https://ruby.github.io/uri/
Other
78 stars 42 forks source link

Regression on URI.split in version 0.12.1 #95

Closed lucaskanashiro closed 2 weeks ago

lucaskanashiro commented 8 months ago

In Debian and Ubuntu, I have experienced some test failures in other ruby gems like this one:

  4) Failure:
TestURIUtils#test_split_file_uri [/<<PKGBUILDDIR>>/test/test_uri_utils.rb:41]:
--- expected
+++ actual
@@ -1 +1 @@
-["file", nil, "/etc/fstab", nil]
+["file", "", "/etc/fstab", nil]

This is a simple test:

  def test_split_file_uri
    parts = split_file_uri("file://localhost/etc/fstab")
    assert_equal ['file', 'localhost', '/etc/fstab', nil], parts

    parts = split_file_uri("file:///etc/fstab")
    assert_equal ['file', nil, '/etc/fstab', nil], parts
    [...]
  def split_file_uri(uri)
      scheme, _, host, _, _, path, _, query, _ = URI.split(uri)
      [...]

After some digging, I noticed that this issue would be fixed by this commit:

https://github.com/ruby/uri/commit/ffbab83de6d8748c9454414e02db5317609166eb

Applying it on top of we have indeed fixed the issue. But when applying all changes in version 0.12.1, the same test fails again. The problem seems to be this commit:

https://github.com/ruby/uri/commit/eaf89cc31619d49e67c64d0b58ea9dc38892d175

Since the regex is not easy to read I was not able yet to pinpoint the issue introduced by this change, sorry.

This is a straightforward reproducer for the behavior change:

1- Without the security fix:

irb(main):001:0> require 'uri'
=> true
irb(main):002:0> URI.split("file:///etc/fstab")
=> ["file", nil, nil, nil, nil, "/etc/fstab", nil, nil, nil]
irb(main):003:0> 

2 - With the security fix in 0.12.1 applied:

irb(main):001:0> require 'uri'
=> true
irb(main):002:0> URI.split("file:///etc/fstab")
=> ["file", nil, "", nil, nil, "/etc/fstab", nil, nil, nil]
irb(main):003:0> 

Not sure if this change is intentional. BTW the link in the commit message here is not accessible without logging in, it would be great if the info is public.

hsbt commented 2 weeks ago

https://hackerone.com/reports/1444501 is public now.