Backport Accept check from chttpd #284
Conversation
try to access a database when they aren't logged in or need to change their login
|
Really strange errors, but I will look at them, CI worked for the 1.6.x branch. |
|
Oh, don't waste your time on it - that's my fault. Just fixed it. Sorry for that. |
|
Thanks kxepal, no problem. I really like that Couchdb has a good CI, because there are so many moving parts. |
|
hrm, but this code was ported to chttpd from couch_httpd originally... |
|
rnewson That's what the commit log says but it must of been cleaned up before that because I don't see it being used in couch_httpd. Maybe it's from rcouch? |
|
Here I found something: https://gitorious.org/refugeio/couch_core/commit/03ede5b |
|
There they are reverting from the correct behaviour to the incorrect behaviour. Using Mochi accepts_content_type to unroll the accept string which is really bad when you are trying to compare against two or more options. |
|
Oh, so this is actually a rollback for some old commit? |
|
kxepal, when I look at the old code then I see that the initial commit sort of worked. Then people added more to it so it broke completely. Mochi's accepts_content_type does everything correct but the problem is that the json check is before the html check and all web browsers promise to support basically everything with the q argument, so we get json when we should be getting a redirect. So yes it's a rollback but not just of one commit but many commits, that have tried to get the initial state back. One thing to look at is maybe the default behaviour when not having accept should be json, but we would have to change that in both places. I have a patch where I do this, instead of accepts_content_type: check_header(undefined,_) -> check_header(Header, Value) -> But the problem might be the xhtml browsers. The most important thing is that it's consistent in both couch_httpd and couch_chttpd |
|
Sorry I tend to complicate things with explanations :) It's really simple, this patch is a rollback because bug. It also brings the 1.x.x series in line with what's in 2.0 This bug is one of the first ones I hit many years ago when I started using Couchdb, at the time I didn't know it was a bug. Just a really strange behavior, which I think is the case with most people. Futon and Fauxton have workarounds for this buggy behavior. People are using require_valid_user unnecessarily because of this with all it's Basic Authentication mess, having to close the browser to log out etc, and not being able to mix and match closed and open databases. It's just a couple of lines of code that will make Couchdb much better and Couchapp are a lot better, just try it. Apply read protection on a database and visit it with this patch. |
|
@olafura awesome! could you give me a case how to reproduce it? You can write etap test for it or just put here |
|
kxepal This is my first etap so hopefully it's up to scratch. |
|
It's funny it's the first one I can find to use SecObj, had to use ejson to get the structure right, I was very close. |
|
+1, @rnewson what do you say? |
|
This is reverting commit ecb23f5 which deliberately switched this behavior. The timeline is; a) the pre-ecb23f5af2 exists At no point does chttpd get the merge up from ecb23f5 for this change, so it is behind, rather than ahead of couch_httpd. Whichever way we go, chttpd.erl and couch_httpd.erl should do the same thing. It's not clear which of the two behaviors that is. -1 until this is resolved. |
|
@rnewson oh yea, this is need to be fixed in the first place. @olafura how about to open issue about old vs new Accept behavior to track it on? |
|
@rnewson That thing is that the change was a non feature, it added constraints to the pattern matching so it would pick up q= value, which breaks any comparison you can make. Because browsers say they support really anything with that value: http:https://stackoverflow.com/questions/10496570/what-is-the-purpose-of-the-q-values-in-the-http-accept-request-header. Every framework it seems at some point has someone that adds support for those thing and everything breaks. The reason why the initial patch didn't break badly at first is that it only did the comparison on The intentions were good and I have a patch which is stricter about matching it to If you do a web search you can find a lot of bugs caused by respecting the q value plus wild card usage when doing comparisons. It's also an overkill, it does not add any value, just takes it away. The browsers say with the My test takes a real world use of the There are three options:
I'll also make patches for chttp for the two latter options if you choose that. http:https://www.django-rest-framework.org/api-guide/content-negotiation/ |
|
@kxepal Sorry I haven't yet learned the structure on how you do things, the only reason I submitted a pull request is that I saw that you were ok with that. I feel it's simpler than creating a bug report then submitting the patch and so on. I can try make make some documentation on how this behavior works, I've read enough about it because of this bug :) Is it really and open issue, I don't know. I don't think we do any more comparisons like that, but I can look. The links I sent fall into two categories "this q thing is weird and broken, let's ignore it" (easy option) or "let's try respecting the q value" (the more complex), also add a lot of test cases to make sure that it does not break anything. At this point I'm just worried that people just see the complexity of decision and will just run away and this will bit rot. The old version is pretty nice, if you include html in you accept then you get html otherwise you get json, do we really need anything more? |
|
I'm all for improving this, I filed COUCHDB-1175 myself because neither the behavior before or after the original change here satisfied every use. That ticket, as you can see, ran aground without really solving the problem. I simply don't know if there's a solution that works correctly for all the cases. Is it your claim that there is, given that browsers send Accept with |
|
@rnewson It's usually sent with q= value but the problem is that the matching is not done together, so the correct behavior is giving the match two possible behaviors and letting it decide the best candidate. I'll make a patch this weekend. But I'm going to need somebodies help with making all the test cases, I saw that some other people had done those kinds of tests for their framework, it can be a starting place what we need to cover. What bugs you about the old behavior, where you getting html when you didn't want that or were you getting json when you didn't want that. And what were the Accept headers that were causing you problems. I don't know of a case myself where this old behavior would break, maybe except if you only accepted xhtml and not html, then why are you calling it or you want json if xhtml isn't available. All the browsers send something containing html and they are the primary use case for not sending json to. |
|
Where are we returning text/plain, it might be good having the option of sending different responses based on preference but it's a much larger patch for more of the system, and still respect q value. Still doable. The problem with the what this patch is trying to address is a very serious problem that CouchDB has and should be fixed as soon as possible. We have probably lost a lot of people over this, it's a really nasty bug. |
|
I see that Mochi has an option of passing two arguments to the accepted_content_types so it can be the bases for the patch. It's going to be much more robust, but I'm wondering if we should be checking the functionality of the function or Mochi, I'll check the source to see how they test it. |
|
I spoke too soon it doesn't decide just tells you that it supports both so maybe if it supports html then give them html otherwise not, should work. |
|
Sorry about spamming but it does seem to try to do the right thing, but we should still decide if text/html or application/json comes first, because it uses that order when they are both equal. |
|
That's the trick. If they're equal (which they usually are), then preferring html over json "breaks jquery" (and other libs I guess) and preferring json over html "breaks authentication". |
|
I proposed this semantic on the ticket; if Accept line is exactly "application/json", then send application/json, otherwise send text/html. That puts a surmountable requirement on XHR calls and doesn't require browsers to send sensible Accept headers (which they probably never will). At this point, someone from the Fauxton team should chip in, perhaps all this ugliness is really because of Futon and we can make a clean break? |
|
Sound reasonable, if I remember right looking over the code, they have a fix for this incorrect behavior not the other way round. It takes a lot of code to check if it's alright to send people on their way instead of things just working. I think I something like that when I first encountered the bug. There are also so many cookie timeout things that just work when this behavior is right. Dealing with a 302 is very easy, just pipe the location to window.location. |
…the begining since firefox kindly adds to the headers information it chooses at the end per request from @rnewson
Conflicts: src/couchdb/couch_httpd.erl
|
I'll look over the build failure tomorrow |
|
If I'm reading this correctly it's a Latex error that I don't think has anything to do with my patch, and it's just in one version |
|
I think I have to send in some patches to clean up some of the warnings, so it's easier to read through the log |
|
I've now run the test of my own with kerl for the test that failed and it was successful for me. |
|
@rnewson what do you think of the latest patch should I rework it towards using accepted_content_types or is this simple enough to work. I have the patch now in production on our service and it seems to work fine. My only complaint would not be with it but maybe we need a special use case for eventsource as I don't get the status code, then again I get an error which is basically the same thing. I just have to read an other standard now to see if we should be giving some other answer. |
|
@kxepal I would also appreciate you opinion. |
|
@olafura I really appreciate you did this. Can you tell me if it went anywhere? |
|
That awkward feeling when you miss notification and see it year after Can you elaborate problem with eventsource if you didn't forget about? |
|
Bump. @kxepal do you think you might be able to help out here? |
|
@wohali Sure. Bookmarked. |
|
JIRA says wontfix |
Error 'Command not found' when sname is used
Backport "Accept check" from chttpd to correctly handle when browser
try to access a database when they aren't logged in or need to change their login
This fixes so many bugs that have been littering the bug system for years.
The problem was that browsers over promise and we were using that to check
if it was json or html. I have another patch that is only 4 lines added and two
changed but since it was changed in chttpd I thought merging that was the best
thing.
Also if this patch doesn't get accepted I have an other solution in a form of allowing
rewrite to change headers.
This closes #283
COUCHDB-1175
COUCHDB-2435