CouchDB
  1. CouchDB
  2. COUCHDB-383

HTTP get parameter strictness/looseness

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0
    • Fix Version/s: 0.10
    • Component/s: HTTP Interface
    • Labels:
      None
    • Environment:

      Linux Debian 5.0 (VMware image)

      Description

      The use of GET params in a URL is either too loose or too strict depending on your point of view.

      Adding extra GET params that are not needed to a query works on every request except for /dbname/_all_docs, in this event CouchDB returns an error.

      Test Case:
      /foobar/all_docs?callback=jsonp1244562168058&=1244562168079

      Returns: jsonp1244562168058(

      {"error":"query_parse_error","reason":"Invalid URL parameter: \"_\""}

      );

      NOTE: this is not just a JSONP error, this is just how I found it.

      Ideally, CouchDB either needs to accept this or to error on every request with that contains erroneous GET parameters.

        Issue Links

          Activity

          Hide
          Simon Thulbourn added a comment -

          Adding content.

          Show
          Simon Thulbourn added a comment - Adding content.
          Hide
          Paul Joseph Davis added a comment -

          The root of this is that the parse_view_parameters function is much more strict that the average URL in CouchDB. Most end points only check what they use if they even check beyond the standard Erlang assume true, barf on error method.

          As Simon mentions, the two avenues are either a) go through and beef up URL parameters throughout, or b) relax parameter handling for view queries. The relaxation that'd be needed would be fairly trivial. The code is already there to even trigger it with an ignore_extra=True (for this case at least). On the other hand, tightening all other URL's would be quite arduous at best. Someone would have to find every URL, and then find every parameter that might be used, and then write all the logic for values those parameters might take.

          I'm open to either approach.

          Show
          Paul Joseph Davis added a comment - The root of this is that the parse_view_parameters function is much more strict that the average URL in CouchDB. Most end points only check what they use if they even check beyond the standard Erlang assume true, barf on error method. As Simon mentions, the two avenues are either a) go through and beef up URL parameters throughout, or b) relax parameter handling for view queries. The relaxation that'd be needed would be fairly trivial. The code is already there to even trigger it with an ignore_extra=True (for this case at least). On the other hand, tightening all other URL's would be quite arduous at best. Someone would have to find every URL, and then find every parameter that might be used, and then write all the logic for values those parameters might take. I'm open to either approach.
          Hide
          Chris Anderson added a comment -

          I vote relax!

          Show
          Chris Anderson added a comment - I vote relax!
          Hide
          Curt Arnold added a comment -

          This feature/bug relates to COUCHDB-257 which addressed the inability to work around IE's overly aggressive caching of XmlHttpRequests. A potential work around would be to throw in an extra ignored parameter with a constantly changing value to prevent the inappropriate cache hit, however this does not work since CouchDB returns a query_parse_error for the extra parameter.

          I have not observed other queries accepting extra parameters either with or without an ignore_extra parameter. I'm using 0.10.0a777361 which is a few weeks old, so perhaps that behavior have changed in the interim.

          I vote relax.

          Show
          Curt Arnold added a comment - This feature/bug relates to COUCHDB-257 which addressed the inability to work around IE's overly aggressive caching of XmlHttpRequests. A potential work around would be to throw in an extra ignored parameter with a constantly changing value to prevent the inappropriate cache hit, however this does not work since CouchDB returns a query_parse_error for the extra parameter. I have not observed other queries accepting extra parameters either with or without an ignore_extra parameter. I'm using 0.10.0a777361 which is a few weeks old, so perhaps that behavior have changed in the interim. I vote relax.
          Hide
          Simon Thulbourn added a comment -

          I found it by using jQuery with JSONP.

          It would be nice to relax it.

          Show
          Simon Thulbourn added a comment - I found it by using jQuery with JSONP. It would be nice to relax it.
          Hide
          Paul Joseph Davis added a comment -

          No one opposed, so I relaxed URL parameter parsing as of r786337

          Show
          Paul Joseph Davis added a comment - No one opposed, so I relaxed URL parameter parsing as of r786337
          Hide
          Brian Candler added a comment -

          If you're gonna fully relax, then I think you might also want to accept things like reduce=false on a map-only view. (Perhaps even ignore reduce=true too?)

          Right now, nonsense like 'reducz=false' is accepted, but the reasonable 'reduce=false' is not:

          $ curl http://127.0.0.1:5984/

          {"couchdb":"Welcome","version":"0.10.0a787002"}

          $ curl 'http://127.0.0.1:5984/test/_design/test/_view/test?reduce=false'

          {"error":"query_parse_error","reason":"Invalid URL parameter `reduce` for map view."}

          This is a pain, given that map-reduce views default to reduce=true. That is, if you know you only want the map part of the view, but you don't know if it has a reduce part or not, you may end up having to submit the query twice.

          Show
          Brian Candler added a comment - If you're gonna fully relax, then I think you might also want to accept things like reduce=false on a map-only view. (Perhaps even ignore reduce=true too?) Right now, nonsense like 'reducz=false' is accepted, but the reasonable 'reduce=false' is not: $ curl http://127.0.0.1:5984/ {"couchdb":"Welcome","version":"0.10.0a787002"} $ curl 'http://127.0.0.1:5984/test/_design/test/_view/test?reduce=false' {"error":"query_parse_error","reason":"Invalid URL parameter `reduce` for map view."} This is a pain, given that map-reduce views default to reduce=true. That is, if you know you only want the map part of the view, but you don't know if it has a reduce part or not, you may end up having to submit the query twice.
          Hide
          Paul Joseph Davis added a comment -

          I don't find it very relaxing to accept a request that is invalid just because we think we might know what the user wanted. This may seem trivial enough but its the first step on the path to dragons.

          However, I would support adding a new parameter that is map=true to force viewing a map that would obviously work for either map-only or map-reduce views.

          Another solution I once described was to split urls like:

          /db_name/_design/foo/_view/bar

          becomes

          /db_name/_design/foo/_map/bar
          /db_name/_design/foo/_red/bar

          I support the splitting from an CouchDB internals point of view plus this exact reduce=false messiness. Obviously, that's quite a huge break for clients though.

          Show
          Paul Joseph Davis added a comment - I don't find it very relaxing to accept a request that is invalid just because we think we might know what the user wanted. This may seem trivial enough but its the first step on the path to dragons. However, I would support adding a new parameter that is map=true to force viewing a map that would obviously work for either map-only or map-reduce views. Another solution I once described was to split urls like: /db_name/_design/foo/_view/bar becomes /db_name/_design/foo/_map/bar /db_name/_design/foo/_red/bar I support the splitting from an CouchDB internals point of view plus this exact reduce=false messiness. Obviously, that's quite a huge break for clients though.
          Hide
          Brian Candler added a comment -

          I'd say there are dragons in accepting random unknown query strings too. A few times I have done something like
          ....?include_attachments=true
          instead of
          ....?attachments=true

          (or is it the other way round?) Having erroneous parameters fail silently is not, IMO, very helpful. However I agree that the situation wasn't very consistent as it was.

          Having "map=true" doesn't seem sensible to me, given that it's just the negative of "reduce=false".

          Having views default to reduce=false would suit me well (i.e. if you want it reduced, you have to ask for reduce=true), but would be hugely non backwards compatible. Ditto for your suggestion of _map and _red.

          Perhaps we could even have multiple reduce functions behind the same map - so you'd have to select which one you wanted anyway.

          Show
          Brian Candler added a comment - I'd say there are dragons in accepting random unknown query strings too. A few times I have done something like ....?include_attachments=true instead of ....?attachments=true (or is it the other way round?) Having erroneous parameters fail silently is not, IMO, very helpful. However I agree that the situation wasn't very consistent as it was. Having "map=true" doesn't seem sensible to me, given that it's just the negative of "reduce=false". Having views default to reduce=false would suit me well (i.e. if you want it reduced, you have to ask for reduce=true), but would be hugely non backwards compatible. Ditto for your suggestion of _map and _red. Perhaps we could even have multiple reduce functions behind the same map - so you'd have to select which one you wanted anyway.
          Hide
          Curt Arnold added a comment -

          For COUCHDB-257, it would be hugely beneficial to have an always accepted parameter that is always ignored by CouchDB.

          Not quite the same issue, but CouchDB (at least a few months ago) had problems when it was behind an authenticated proxy. The credentials were processed properly by the proxy server but they carried through to CouchDB which would reject the request, even though credentials weren't intended to be used by CouchDB and the request would have worked fine without credentials. There is the potential that CouchDB may see parameters that were included for some other layer in the processing chain. There should be at least an option to accept unrecognized parameters.

          My possibly uninformed opinion is:

          1. Add an ignore=... parameter for COUCHDB-257
          2. Add a strict=true|false to control whether to reject unrecognized parameters with a default of true (behavior prior to r786337)
          3. Accept reduce=false for map-only views regardless of strict setting.
          4. Accept reduce=true for map-only views when strict=false.

          Show
          Curt Arnold added a comment - For COUCHDB-257 , it would be hugely beneficial to have an always accepted parameter that is always ignored by CouchDB. Not quite the same issue, but CouchDB (at least a few months ago) had problems when it was behind an authenticated proxy. The credentials were processed properly by the proxy server but they carried through to CouchDB which would reject the request, even though credentials weren't intended to be used by CouchDB and the request would have worked fine without credentials. There is the potential that CouchDB may see parameters that were included for some other layer in the processing chain. There should be at least an option to accept unrecognized parameters. My possibly uninformed opinion is: 1. Add an ignore=... parameter for COUCHDB-257 2. Add a strict=true|false to control whether to reject unrecognized parameters with a default of true (behavior prior to r786337) 3. Accept reduce=false for map-only views regardless of strict setting. 4. Accept reduce=true for map-only views when strict=false.
          Hide
          Chris Anderson added a comment -

          > Having views default to reduce=false would suit me well (i.e. if you want it reduced, you have to ask for reduce=true), but would be hugely non backwards compatible.

          This idea is the most sensible of the bunch as far as I'm concerned. Backwards compatibility is still less important than good API design at this point.

          I'm not opposed to the introduction of the strict=true option, but I'd suggest that defaulting to false is more relaxing.

          Show
          Chris Anderson added a comment - > Having views default to reduce=false would suit me well (i.e. if you want it reduced, you have to ask for reduce=true), but would be hugely non backwards compatible. This idea is the most sensible of the bunch as far as I'm concerned. Backwards compatibility is still less important than good API design at this point. I'm not opposed to the introduction of the strict=true option, but I'd suggest that defaulting to false is more relaxing.
          Hide
          Vinay Sajip added a comment -

          I don't see that this is particularly related to COUCHDB-257 - that issue is about HTTP headers, this is about interpretation of HTTP query parameters.

          Show
          Vinay Sajip added a comment - I don't see that this is particularly related to COUCHDB-257 - that issue is about HTTP headers, this is about interpretation of HTTP query parameters.
          Hide
          Curt Arnold added a comment -

          The typical work-around used with other services to workaround IE's XmlHttpRequest ignoring requests not to cache responses is to add nonsense query parameters. Since CouchDB (until the recent patch) returns sees unrecognized parameters as errors, that work-around isn't available.

          Show
          Curt Arnold added a comment - The typical work-around used with other services to workaround IE's XmlHttpRequest ignoring requests not to cache responses is to add nonsense query parameters. Since CouchDB (until the recent patch) returns sees unrecognized parameters as errors, that work-around isn't available.
          Hide
          Sam Bisbee added a comment -

          Resolved for a while. Closing.

          Show
          Sam Bisbee added a comment - Resolved for a while. Closing.

            People

            • Assignee:
              Paul Joseph Davis
              Reporter:
              Simon Thulbourn
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development