Utils#parse_query is broken with complex input #364

Closed
ddebernardy opened this Issue Mar 15, 2012 · 6 comments

Comments

Projects
None yet
2 participants
@ddebernardy

Utils#parse_query is broken, and it's used to parse Request#POST...

>> Rack::Utils.parse_query('foo=1&bar%5Bbaz%5D=baz')
=> {"foo"=>"1", "bar[baz]"=>"baz"} # instead of {"foo"=>"1", "bar"=>{"baz" => "baz"}}

This and the following issue:

#262

... make it inane to try to make a Rack app communicate with a separate RESTfull app...

@ddebernardy

This comment has been minimized.

Show comment Hide comment
@ddebernardy

ddebernardy Mar 15, 2012

In case this and the other ticket ever gets traction...


def parse_query(query)
  params = {}

  query.split('&').each do |param|
    key, value = param.split('=', 2)

    unless match = /\A
        (?<key>[^\[\]]*)
        (?<subkeys>(\[[^\[\]]*\])+)?
        \z/x.match(key)
      raise ArgumentError
    end

    key = Rack::Utils.unescape(match[:key])
    if subkeys = match[:subkeys]
      keys = []
      subkeys.scan(/\[[^\[\]]*\]/) do |subkey|
        keys << Rack::Utils.unescape(subkey[1..-2])
      end
    else
      keys = nil
    end

    value = Rack::Utils.unescape(value)

    if keys
      parent = params

      i = 0
      while key
        i += 1
        k = keys.first

        if !k
          case parent
          when Array
            parent << value
          when Hash
            parent[key] = value
          else
            break
          end
        elsif k.empty?
          case parent
          when Array
            parent << []
            parent = parent.last
          when Hash
            parent[key] = [] unless parent[key].is_a?(Array)
            parent = parent[key]
          else
            parent = []
          end
        else
          case parent
          when Array
            parent << {} unless parent.last.is_a?(Hash) && !parent.last.key?(k)
            parent = parent.last
          when Hash
            parent[key] = {} unless parent[key].is_a?(Hash)
            parent = parent[key]
          else
            parent = {}
          end
        end

        key = keys.shift
      end
    elsif cur = params[key]
        params[key] = [cur] unless cur.is_a?(Array)
        params[key] << value
    else
      params[key] = value
    end
  end

  params
end

And:

def test_parse_query
  [
    ['=foo',              {'' => 'foo'},                        ],
    ['foo=bar',           {'foo' => 'bar'},                     ],
    ['foo=bar&bar=baz',   {'foo' => 'bar', 'bar' => 'baz'},     ],
    ['foo=bar&foo=baz',   {'foo' => ['bar', 'baz']},            ],
    ['foo=%22bar%22',     {'foo' => '"bar"'},                   ],
    ['foo%3Dbaz=bar',     {'foo=baz' => 'bar'},                 ],
    ['foo=',              {'foo' => ''},                        ],
    ['foo[]=',            {'foo' => ['']},                      ],
    ['foo[bar][baz]=',    {'foo' => {'bar' => {'baz' => ''}}},  ],
    [
      'foo[bar][]=baz&foo[bar][]=baz',
      {'foo' => {'bar' => ['baz', 'baz']}},
    ],
    [
      'foo[bar%25][]=baz%25&foo%25[bar%25][]=baz%25',
      {'foo' => {'bar%' => ['baz%']}, 'foo%' => {'bar%' => ['baz%']}},
    ],
    [
      'my+weird+field=q1%212%22%27w%245%267%2Fz8%29%3F',
      {"my weird field" => "q1!2\"'w$5&7/z8)?"},
    ],
    [
      'x[y][][z]=1&x[y][][w]=a&x[y][][z]=2&x[y][][w]=3',
      {"x" => {"y" => [{"z" => "1", "w" => "a"}, {"z" => "2", "w" => "3"}]}},
    ],
  ].each do |args, expected|
    assert_equal  expected,
                  parse_query(args),
                  "`parse_query' - #{args}"
  end
end

In case this and the other ticket ever gets traction...


def parse_query(query)
  params = {}

  query.split('&').each do |param|
    key, value = param.split('=', 2)

    unless match = /\A
        (?<key>[^\[\]]*)
        (?<subkeys>(\[[^\[\]]*\])+)?
        \z/x.match(key)
      raise ArgumentError
    end

    key = Rack::Utils.unescape(match[:key])
    if subkeys = match[:subkeys]
      keys = []
      subkeys.scan(/\[[^\[\]]*\]/) do |subkey|
        keys << Rack::Utils.unescape(subkey[1..-2])
      end
    else
      keys = nil
    end

    value = Rack::Utils.unescape(value)

    if keys
      parent = params

      i = 0
      while key
        i += 1
        k = keys.first

        if !k
          case parent
          when Array
            parent << value
          when Hash
            parent[key] = value
          else
            break
          end
        elsif k.empty?
          case parent
          when Array
            parent << []
            parent = parent.last
          when Hash
            parent[key] = [] unless parent[key].is_a?(Array)
            parent = parent[key]
          else
            parent = []
          end
        else
          case parent
          when Array
            parent << {} unless parent.last.is_a?(Hash) && !parent.last.key?(k)
            parent = parent.last
          when Hash
            parent[key] = {} unless parent[key].is_a?(Hash)
            parent = parent[key]
          else
            parent = {}
          end
        end

        key = keys.shift
      end
    elsif cur = params[key]
        params[key] = [cur] unless cur.is_a?(Array)
        params[key] << value
    else
      params[key] = value
    end
  end

  params
end

And:

def test_parse_query
  [
    ['=foo',              {'' => 'foo'},                        ],
    ['foo=bar',           {'foo' => 'bar'},                     ],
    ['foo=bar&bar=baz',   {'foo' => 'bar', 'bar' => 'baz'},     ],
    ['foo=bar&foo=baz',   {'foo' => ['bar', 'baz']},            ],
    ['foo=%22bar%22',     {'foo' => '"bar"'},                   ],
    ['foo%3Dbaz=bar',     {'foo=baz' => 'bar'},                 ],
    ['foo=',              {'foo' => ''},                        ],
    ['foo[]=',            {'foo' => ['']},                      ],
    ['foo[bar][baz]=',    {'foo' => {'bar' => {'baz' => ''}}},  ],
    [
      'foo[bar][]=baz&foo[bar][]=baz',
      {'foo' => {'bar' => ['baz', 'baz']}},
    ],
    [
      'foo[bar%25][]=baz%25&foo%25[bar%25][]=baz%25',
      {'foo' => {'bar%' => ['baz%']}, 'foo%' => {'bar%' => ['baz%']}},
    ],
    [
      'my+weird+field=q1%212%22%27w%245%267%2Fz8%29%3F',
      {"my weird field" => "q1!2\"'w$5&7/z8)?"},
    ],
    [
      'x[y][][z]=1&x[y][][w]=a&x[y][][z]=2&x[y][][w]=3',
      {"x" => {"y" => [{"z" => "1", "w" => "a"}, {"z" => "2", "w" => "3"}]}},
    ],
  ].each do |args, expected|
    assert_equal  expected,
                  parse_query(args),
                  "`parse_query' - #{args}"
  end
end
@ddebernardy

This comment has been minimized.

Show comment Hide comment
@ddebernardy

ddebernardy Mar 15, 2012

Oh, and... I'm wondering if cookies and GET/POST should not be parsed using different functions.

Oh, and... I'm wondering if cookies and GET/POST should not be parsed using different functions.

@raggi

This comment has been minimized.

Show comment Hide comment
@raggi

raggi Mar 15, 2012

Owner
>> Rack::Utils.parse_nested_query 'foo=1&bar%5Bbaz%5D=baz'
=> {"foo"=>"1", "bar"=>{"baz"=>"baz"}}
Owner

raggi commented Mar 15, 2012

>> Rack::Utils.parse_nested_query 'foo=1&bar%5Bbaz%5D=baz'
=> {"foo"=>"1", "bar"=>{"baz"=>"baz"}}

@raggi raggi closed this Mar 15, 2012

@ddebernardy

This comment has been minimized.

Show comment Hide comment
@ddebernardy

ddebernardy Mar 15, 2012

cough -- sorry raggi, but that's irrelevant. You're using parse_query over in Request#GET and Request#POST

cough -- sorry raggi, but that's irrelevant. You're using parse_query over in Request#GET and Request#POST

@ddebernardy

This comment has been minimized.

Show comment Hide comment
@ddebernardy

ddebernardy Mar 15, 2012

if you use foo=1&foo=2&foo=3 as POST data, for instance, you end up with {'foo'=>3}. Whereas {'foo' => [1, 2, 3]} yields the former query string with build_query.

if you use foo=1&foo=2&foo=3 as POST data, for instance, you end up with {'foo'=>3}. Whereas {'foo' => [1, 2, 3]} yields the former query string with build_query.

@raggi

This comment has been minimized.

Show comment Hide comment
@raggi

raggi Mar 16, 2012

Owner

parse_query is not broken, parse_query is simple.

You want rails-like semantics, which is what parse_nested_query does.

They have different names for a reason. The title and content of this ticket is invalid as a change. Altering the semantics of a method to suit a preference, and break everyone else's code is not the correct change.

Owner

raggi commented Mar 16, 2012

parse_query is not broken, parse_query is simple.

You want rails-like semantics, which is what parse_nested_query does.

They have different names for a reason. The title and content of this ticket is invalid as a change. Altering the semantics of a method to suit a preference, and break everyone else's code is not the correct change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment