CouchDB
  1. CouchDB
  2. COUCHDB-1416

the requested_path that is passed to a show is wrong on a vhost with a path

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.2
    • Fix Version/s: None
    • Component/s: HTTP Interface
    • Labels:
      None
    • Skill Level:
      Regular Contributors Level (Easy to Medium)

      Description

      In a show or list, it is impossible to construct a full url that an end user could use to re-request the resource, given the various combinations of vhosts and rewrites.
      The major one is if the vhost contains a path component, this path information is not passed to the show at all.

      I have created three tests that highlight the condition, currently failing for one test, with the two passing to prevent regressions.

      The commit can be found here:

      https://github.com/ryanramage/couchdb/commit/e9417480e2ce160f359d9508dcec3d4e56045a60

      I have talked this over with JasonSmith and bennoitc on #couchdb and they asked me to write the tests and raise the jira.

        Activity

        Hide
        Jan Lehnardt added a comment -

        I'm sorry, but the solution here is to make couchdb give you an error if you try to set up a vhost foo.com/bar telling you that it should just be foo.com.

        I'm happy to look at alternatives to solve the problem of putting multiple couchapps into one vhost, but this is not it.

        In response to @Jason:

        > 1. Your description is counter to the actual CouchDB code. Paths are valid vhost keys. example.com/foo and example.com/bar are distinct vhosts in 1.2.x.

        That is CouchDB expecting an RC2616 Host header (which can't include a path). If anything, this is a bug, not a feature. The fact that this works is an accident if anything.

        > 2. In any case, the bug in this ticket is that the path a browser sends to couch is somehow forgotten by the time it hits a _show function as req.requested_path. Ryan might fill in the details but AFAIK, this will fail.

        This looks like a more of a genuine issue, but I haven't been able to look at the patches.

        Show
        Jan Lehnardt added a comment - I'm sorry, but the solution here is to make couchdb give you an error if you try to set up a vhost foo.com/bar telling you that it should just be foo.com. I'm happy to look at alternatives to solve the problem of putting multiple couchapps into one vhost, but this is not it. In response to @Jason: > 1. Your description is counter to the actual CouchDB code. Paths are valid vhost keys. example.com/foo and example.com/bar are distinct vhosts in 1.2.x. That is CouchDB expecting an RC2616 Host header (which can't include a path). If anything, this is a bug, not a feature. The fact that this works is an accident if anything. > 2. In any case, the bug in this ticket is that the path a browser sends to couch is somehow forgotten by the time it hits a _show function as req.requested_path. Ryan might fill in the details but AFAIK, this will fail. This looks like a more of a genuine issue, but I haven't been able to look at the patches.
        Hide
        Benoit Chesneau added a comment -

        ALso to be really complete any patch that goes in should include a test with a vhost change which couldn't be done in js.

        Show
        Benoit Chesneau added a comment - ALso to be really complete any patch that goes in should include a test with a vhost change which couldn't be done in js.
        Hide
        Benoit Chesneau added a comment -

        So

        1. I'm not sure this is a bug. This have been done for some reasons I'm trying to recollect
        2. Such change may break some couchapps around.

        Anyway I need some days to figure where is the problem and if this change is appropriate. So please don't commit anything before I had time to review all of that. Let's talk about that again at the end of the week.

        Show
        Benoit Chesneau added a comment - So 1. I'm not sure this is a bug. This have been done for some reasons I'm trying to recollect 2. Such change may break some couchapps around. Anyway I need some days to figure where is the problem and if this change is appropriate. So please don't commit anything before I had time to review all of that. Let's talk about that again at the end of the week.
        Hide
        Adam Kocoloski added a comment -

        +JasonSmith: Actually, would anybody who is able please reopen COUCHDB-1416 so that Ryan can add an attachment. Thanks!

        Show
        Adam Kocoloski added a comment - +JasonSmith: Actually, would anybody who is able please reopen COUCHDB-1416 so that Ryan can add an attachment. Thanks!
        Hide
        Jason Smith added a comment -

        @Jan, would you kindly re-open this ticket (I cannot) so that Ryan can add an attachment? Thanks.

        @Ryan, would you please run this:

        git format-patch 47c81f4c25f5f9ec4ef60c4ea638d77118b9a9ee -1

        And attach the 0001-Testing-*.patch file to this ticket and click the copyright agreement.

        Show
        Jason Smith added a comment - @Jan, would you kindly re-open this ticket (I cannot) so that Ryan can add an attachment? Thanks. @Ryan, would you please run this: git format-patch 47c81f4c25f5f9ec4ef60c4ea638d77118b9a9ee -1 And attach the 0001-Testing-*.patch file to this ticket and click the copyright agreement.
        Hide
        Jason Smith added a comment -

        Forgot to mention, also the port is not involved in vhost lookups. That changed between 1.1 and 1.2.

        Show
        Jason Smith added a comment - Forgot to mention, also the port is not involved in vhost lookups. That changed between 1.1 and 1.2.
        Hide
        Jason Smith added a comment -

        Ryan, I believe there is still a bug here, triggered with just a vhost/rewrite combo. Would it be possible for you to modify the test to strictly use hostnames with no paths as the vhost key?

        Jan, two things:

        1. Your description is counter to the actual CouchDB code. Paths are valid vhost keys. example.com/foo and example.com/bar are distinct vhosts in 1.2.x.

        2. In any case, the bug in this ticket is that the path a browser sends to couch is somehow forgotten by the time it hits a _show function as req.requested_path. Ryan might fill in the details but AFAIK, this will fail.

        [vhosts]
        example.com = /db/_design/ddoc/_rewrite

        "_rewrites": [

        {"from":"show", "to":"_show/show_doc"}

        ]

        function(doc, req)

        { // show_doc return JSON.stringify(req.requested_path) }

        GET /foo
        Host: example.com

        The response should be "/foo" but it is not.

        Show
        Jason Smith added a comment - Ryan, I believe there is still a bug here, triggered with just a vhost/rewrite combo. Would it be possible for you to modify the test to strictly use hostnames with no paths as the vhost key? Jan, two things: 1. Your description is counter to the actual CouchDB code. Paths are valid vhost keys. example.com/foo and example.com/bar are distinct vhosts in 1.2.x. 2. In any case, the bug in this ticket is that the path a browser sends to couch is somehow forgotten by the time it hits a _show function as req.requested_path. Ryan might fill in the details but AFAIK, this will fail. [vhosts] example.com = /db/_design/ddoc/_rewrite "_rewrites": [ {"from":"show", "to":"_show/show_doc"} ] function(doc, req) { // show_doc return JSON.stringify(req.requested_path) } GET /foo Host: example.com The response should be "/foo" but it is not.
        Hide
        Jan Lehnardt added a comment -

        A vhost key should never include a path.

        It is strictly for matching the Host: example.com header which is defined by RFC 2616 as "the Internet host and port number of the resource being requested" — http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23

        Show
        Jan Lehnardt added a comment - A vhost key should never include a path. It is strictly for matching the Host: example.com header which is defined by RFC 2616 as "the Internet host and port number of the resource being requested" — http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23
        Hide
        Jason Smith added a comment -

        First crack at fixing this bug. This series passes Ryan's tests and does not seem to harm the other JS or etap tests. (I was concerned about oauth but it passed.)

        My branch for this: https://github.com/jhs/couchdb/tree/COUCHDB-1416

        It is based from Ryan's tests, and thus it is based on the 1.2.x branch for now.

        Show
        Jason Smith added a comment - First crack at fixing this bug. This series passes Ryan's tests and does not seem to harm the other JS or etap tests. (I was concerned about oauth but it passed.) My branch for this: https://github.com/jhs/couchdb/tree/COUCHDB-1416 It is based from Ryan's tests, and thus it is based on the 1.2.x branch for now.
        Hide
        Jason Smith added a comment -

        x-couchdb-vhost-path does not include the "VPath" part, i.e. the path part from the vhost definition.

        [vhosts]
        example.com/some_path = /some_db

        After vhost processing, "/some_path" is lost forever.

        Show
        Jason Smith added a comment - x-couchdb-vhost-path does not include the "VPath" part, i.e. the path part from the vhost definition. [vhosts] example.com/some_path = /some_db After vhost processing, "/some_path" is lost forever.
        Hide
        Benoit Chesneau added a comment - - edited

        It is isn't possible to use the test suite to tests vhosts. Since the vhost is depending on the Host header. It's unlikely that you can change it in your ajax request.

        I'm surprised you can't get this path since we are using it for the oauth check. Can't you use the header `x-couchdb-vhost-path` ?

        Anyway, the full problem right now, is that we don't pass any record while rewriting the uri. I can see 2 solutions in near future:

        1. Keeping a state and sendint oit back when the mochiweb request have been modified
        2. Keeping this state in the registry which was already proposed when we have to manage this step.

        I will have a look on it this week.

        Show
        Benoit Chesneau added a comment - - edited It is isn't possible to use the test suite to tests vhosts. Since the vhost is depending on the Host header. It's unlikely that you can change it in your ajax request. I'm surprised you can't get this path since we are using it for the oauth check. Can't you use the header `x-couchdb-vhost-path` ? Anyway, the full problem right now, is that we don't pass any record while rewriting the uri. I can see 2 solutions in near future: 1. Keeping a state and sendint oit back when the mochiweb request have been modified 2. Keeping this state in the registry which was already proposed when we have to manage this step. I will have a look on it this week.
        Hide
        Jason Smith added a comment -

        Thanks, Ryan.

        Hopefully this is a dupe of COUCHDB-981. Caolan?

        Firstly, yours would be the first test in the entire JavaScript suite to use vhosts. I do not know if vhosts were avoided all this time by design or accident. I made a small change to your hostname guesser (window.location.host) and it works for me.

        Next, this bugfix has a problem. CouchDB has an object, httpd{} which contains a Mochiweb object. Mochiweb is an upstream dependency. I thought this fix would happen by adding a a #httpd.requested_path however all the vhost/rewrite code works exclusively with mochiweb.

        So the question is how to remember state through all possible processing of a MochiReq.

        Show
        Jason Smith added a comment - Thanks, Ryan. Hopefully this is a dupe of COUCHDB-981 . Caolan? Firstly, yours would be the first test in the entire JavaScript suite to use vhosts. I do not know if vhosts were avoided all this time by design or accident. I made a small change to your hostname guesser (window.location.host) and it works for me. Next, this bugfix has a problem. CouchDB has an object, httpd{} which contains a Mochiweb object. Mochiweb is an upstream dependency. I thought this fix would happen by adding a a #httpd.requested_path however all the vhost/rewrite code works exclusively with mochiweb. So the question is how to remember state through all possible processing of a MochiReq.

          People

          • Assignee:
            Unassigned
            Reporter:
            Ryan Ramage
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development