Re: [hackers] [quark][PATCH] Don't erase response on http_send_error_response
On Mon, 26 Oct 2020 11:34:17 +0100 José Miguel Sánchez García wrote: Dear José, > > I also don't see a reason for the constraints you mention. Just add > > an array of group-auth-pairs to the server struct and also add a > > group-auth-pair to the req-struct that you then fill when you parse > > the request fields in http_parse_header(). Then later, in > > http_prepare_header_buf(), you check if they match and either send > > an error-header (access denied) or allow access. > > > > In case the auth-field is empty but the file requires a password, > > you, in turn, send the desired header to ask for auth. > > You are absolutely right, and I just didn't see it when I was working > on it. Sorry for wasting your time. no problem! Sometimes it takes a few refactorings of an idea until it is implemented the best way. With best regards Laslo
Re: [hackers] [quark][PATCH] Don't erase response on http_send_error_response
On 10/26/2020 8:34 AM, Laslo Hunhold wrote: Definitely don't make exceptions here, because erasing the entire struct is a consistency measure and being inconsistent there complicates the semantics. I'll be careful then. I also don't see a reason for the constraints you mention. Just add an array of group-auth-pairs to the server struct and also add a group-auth-pair to the req-struct that you then fill when you parse the request fields in http_parse_header(). Then later, in http_prepare_header_buf(), you check if they match and either send an error-header (access denied) or allow access. In case the auth-field is empty but the file requires a password, you, in turn, send the desired header to ask for auth. You are absolutely right, and I just didn't see it when I was working on it. Sorry for wasting your time. Best regards, José Miguel
Re: [hackers] [quark][PATCH] Don't erase response on http_send_error_response
On Sun, 25 Oct 2020 11:04:26 +0100 José Miguel Sánchez García wrote: Dear José, > I'm currently relying on the req struct NOT being erased, because I'm > storing the realm the file belongs to there. Then, I'm using that > realm information to build the WWW-Authenticate header for the 401 > error response. > > I could just save that field before erasing everything else, but I > wonder if that's the way to go. If you are getting rid of everything, > maybe I shouldn't make exceptions? Definitely don't make exceptions here, because erasing the entire struct is a consistency measure and being inconsistent there complicates the semantics. I also don't see a reason for the constraints you mention. Just add an array of group-auth-pairs to the server struct and also add a group-auth-pair to the req-struct that you then fill when you parse the request fields in http_parse_header(). Then later, in http_prepare_header_buf(), you check if they match and either send an error-header (access denied) or allow access. In case the auth-field is empty but the file requires a password, you, in turn, send the desired header to ask for auth. With best regards Laslo
Re: [hackers] [quark][PATCH] Don't erase response on http_send_error_response
On 10/25/2020 8:39 AM, Laslo Hunhold wrote: Dear Laslo, No, this is supposed to be like this. I agree that the comment is a bit misleading, but http_parse_header() really builds a request from scratch and first sets it all to zero. With "fields" I'm referring to the struct fields in request, and this misleading comment will be fixed in an upcoming commit. I'm currently relying on the req struct NOT being erased, because I'm storing the realm the file belongs to there. Then, I'm using that realm information to build the WWW-Authenticate header for the 401 error response. I could just save that field before erasing everything else, but I wonder if that's the way to go. If you are getting rid of everything, maybe I shouldn't make exceptions? Best regards, José Miguel
Re: [hackers] [quark][PATCH] Don't erase response on http_send_error_response
On Sat, 24 Oct 2020 16:19:13 + José Miguel Sánchez García wrote: Dear José, thanks for taking your time reading the code and reporting this! > The comment before the offending line indicated it was intended to > only erase the fields, but it erased the whole response. It was most > likely a bug. > > /* empty all fields */ > - memset(req, 0, sizeof(*req)); > + memset(&(req->fields), 0, sizeof(req->fields)); No, this is supposed to be like this. I agree that the comment is a bit misleading, but http_parse_header() really builds a request from scratch and first sets it all to zero. With "fields" I'm referring to the struct fields in request, and this misleading comment will be fixed in an upcoming commit. With best regards Laslo
[hackers] [quark][PATCH] Don't erase response on http_send_error_response
The comment before the offending line indicated it was intended to only erase the fields, but it erased the whole response. It was most likely a bug. --- http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http.c b/http.c index f1e15a4..27d20f7 100644 --- a/http.c +++ b/http.c @@ -182,7 +182,7 @@ http_parse_header(const char *h, struct request *req) char *m, *n; /* empty all fields */ - memset(req, 0, sizeof(*req)); + memset(&(req->fields), 0, sizeof(req->fields)); /* * parse request line -- 2.29.0
