[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[wub]

CRLF is explicitly defined as the line break in the HTTP protocol
specifications: RFC 2616 (obsolete) and RFC 7230.
---
 ui-shared.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index 9a38aa9..b463375 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -672,36 +672,36 @@ void cgit_print_http_headers(void)
 		return;
 
 	if (ctx.page.status)
-		htmlf("Status: %d %s\n", ctx.page.status, ctx.page.statusmsg);
+		htmlf("Status: %d %s\r\n", ctx.page.status, ctx.page.statusmsg);
 	if (ctx.page.mimetype && ctx.page.charset)
-		htmlf("Content-Type: %s; charset=%s\n", ctx.page.mimetype,
+		htmlf("Content-Type: %s; charset=%s\r\n", ctx.page.mimetype,
 		      ctx.page.charset);
 	else if (ctx.page.mimetype)
-		htmlf("Content-Type: %s\n", ctx.page.mimetype);
+		htmlf("Content-Type: %s\r\n", ctx.page.mimetype);
 	if (ctx.page.size)
-		htmlf("Content-Length: %zd\n", ctx.page.size);
+		htmlf("Content-Length: %zd\r\n", ctx.page.size);
 	if (ctx.page.filename) {
 		html("Content-Disposition: inline; filename=\"");
 		html_header_arg_in_quotes(ctx.page.filename);
-		html("\"\n");
+		html("\"\r\n");
 	}
 	if (!ctx.env.authenticated)
-		html("Cache-Control: no-cache, no-store\n");
-	htmlf("Last-Modified: %s\n", http_date(ctx.page.modified));
-	htmlf("Expires: %s\n", http_date(ctx.page.expires));
+		html("Cache-Control: no-cache, no-store\r\n");
+	htmlf("Last-Modified: %s\r\n", http_date(ctx.page.modified));
+	htmlf("Expires: %s\r\n", http_date(ctx.page.expires));
 	if (ctx.page.etag)
-		htmlf("ETag: \"%s\"\n", ctx.page.etag);
-	html("\n");
+		htmlf("ETag: \"%s\"\r\n", ctx.page.etag);
+	html("\r\n");
 	if (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD"))
 		exit(0);
 }
 
 void cgit_redirect(const char *url, bool permanent)
 {
-	htmlf("Status: %d %s\n", permanent ? 301 : 302, permanent ? "Moved" : "Found");
+	htmlf("Status: %d %s\r\n", permanent ? 301 : 302, permanent ? "Moved" : "Found");
 	html("Location: ");
 	html_url_path(url);
-	html("\n\n");
+	html("\r\n\r\n");
 }
 
 static void print_rel_vcs_link(const char *url)
-- 
2.8.1

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[john]

On Wed, May 11, 2016 at 05:48:51PM +0000, Juuso Lapinlampi wrote:
> CRLF is explicitly defined as the line break in the HTTP protocol
> specifications: RFC 2616 (obsolete) and RFC 7230.

Missing sign-off; see http://developercertificate.org/ for what this
means.

Otherwise,

Reviewed-by: John Keeping <john at keeping.me.uk>

> ---
>  ui-shared.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index 9a38aa9..b463375 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -672,36 +672,36 @@ void cgit_print_http_headers(void)
>  		return;
>  
>  	if (ctx.page.status)
> -		htmlf("Status: %d %s\n", ctx.page.status, ctx.page.statusmsg);
> +		htmlf("Status: %d %s\r\n", ctx.page.status, ctx.page.statusmsg);
>  	if (ctx.page.mimetype && ctx.page.charset)
> -		htmlf("Content-Type: %s; charset=%s\n", ctx.page.mimetype,
> +		htmlf("Content-Type: %s; charset=%s\r\n", ctx.page.mimetype,
>  		      ctx.page.charset);
>  	else if (ctx.page.mimetype)
> -		htmlf("Content-Type: %s\n", ctx.page.mimetype);
> +		htmlf("Content-Type: %s\r\n", ctx.page.mimetype);
>  	if (ctx.page.size)
> -		htmlf("Content-Length: %zd\n", ctx.page.size);
> +		htmlf("Content-Length: %zd\r\n", ctx.page.size);
>  	if (ctx.page.filename) {
>  		html("Content-Disposition: inline; filename=\"");
>  		html_header_arg_in_quotes(ctx.page.filename);
> -		html("\"\n");
> +		html("\"\r\n");
>  	}
>  	if (!ctx.env.authenticated)
> -		html("Cache-Control: no-cache, no-store\n");
> -	htmlf("Last-Modified: %s\n", http_date(ctx.page.modified));
> -	htmlf("Expires: %s\n", http_date(ctx.page.expires));
> +		html("Cache-Control: no-cache, no-store\r\n");
> +	htmlf("Last-Modified: %s\r\n", http_date(ctx.page.modified));
> +	htmlf("Expires: %s\r\n", http_date(ctx.page.expires));
>  	if (ctx.page.etag)
> -		htmlf("ETag: \"%s\"\n", ctx.page.etag);
> -	html("\n");
> +		htmlf("ETag: \"%s\"\r\n", ctx.page.etag);
> +	html("\r\n");
>  	if (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD"))
>  		exit(0);
>  }
>  
>  void cgit_redirect(const char *url, bool permanent)
>  {
> -	htmlf("Status: %d %s\n", permanent ? 301 : 302, permanent ? "Moved" : "Found");
> +	htmlf("Status: %d %s\r\n", permanent ? 301 : 302, permanent ? "Moved" : "Found");
>  	html("Location: ");
>  	html_url_path(url);
> -	html("\n\n");
> +	html("\r\n\r\n");
>  }
>  
>  static void print_rel_vcs_link(const char *url)
> -- 
> 2.8.1
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[john]

On Wed, May 11, 2016 at 07:30:49PM +0100, John Keeping wrote:
> On Wed, May 11, 2016 at 05:48:51PM +0000, Juuso Lapinlampi wrote:
> > CRLF is explicitly defined as the line break in the HTTP protocol
> > specifications: RFC 2616 (obsolete) and RFC 7230.
> 
> Missing sign-off; see http://developercertificate.org/ for what this
> means.
> 
> Otherwise,
> 
> Reviewed-by: John Keeping <john at keeping.me.uk>

Actually, NAK, this is wrong.  We're not talking HTTP here but CGI and
the CGI spec is clear that a single NL should be used after headers.

> > ---
> >  ui-shared.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/ui-shared.c b/ui-shared.c
> > index 9a38aa9..b463375 100644
> > --- a/ui-shared.c
> > +++ b/ui-shared.c
> > @@ -672,36 +672,36 @@ void cgit_print_http_headers(void)
> >  		return;
> >  
> >  	if (ctx.page.status)
> > -		htmlf("Status: %d %s\n", ctx.page.status, ctx.page.statusmsg);
> > +		htmlf("Status: %d %s\r\n", ctx.page.status, ctx.page.statusmsg);
> >  	if (ctx.page.mimetype && ctx.page.charset)
> > -		htmlf("Content-Type: %s; charset=%s\n", ctx.page.mimetype,
> > +		htmlf("Content-Type: %s; charset=%s\r\n", ctx.page.mimetype,
> >  		      ctx.page.charset);
> >  	else if (ctx.page.mimetype)
> > -		htmlf("Content-Type: %s\n", ctx.page.mimetype);
> > +		htmlf("Content-Type: %s\r\n", ctx.page.mimetype);
> >  	if (ctx.page.size)
> > -		htmlf("Content-Length: %zd\n", ctx.page.size);
> > +		htmlf("Content-Length: %zd\r\n", ctx.page.size);
> >  	if (ctx.page.filename) {
> >  		html("Content-Disposition: inline; filename=\"");
> >  		html_header_arg_in_quotes(ctx.page.filename);
> > -		html("\"\n");
> > +		html("\"\r\n");
> >  	}
> >  	if (!ctx.env.authenticated)
> > -		html("Cache-Control: no-cache, no-store\n");
> > -	htmlf("Last-Modified: %s\n", http_date(ctx.page.modified));
> > -	htmlf("Expires: %s\n", http_date(ctx.page.expires));
> > +		html("Cache-Control: no-cache, no-store\r\n");
> > +	htmlf("Last-Modified: %s\r\n", http_date(ctx.page.modified));
> > +	htmlf("Expires: %s\r\n", http_date(ctx.page.expires));
> >  	if (ctx.page.etag)
> > -		htmlf("ETag: \"%s\"\n", ctx.page.etag);
> > -	html("\n");
> > +		htmlf("ETag: \"%s\"\r\n", ctx.page.etag);
> > +	html("\r\n");
> >  	if (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD"))
> >  		exit(0);
> >  }
> >  
> >  void cgit_redirect(const char *url, bool permanent)
> >  {
> > -	htmlf("Status: %d %s\n", permanent ? 301 : 302, permanent ? "Moved" : "Found");
> > +	htmlf("Status: %d %s\r\n", permanent ? 301 : 302, permanent ? "Moved" : "Found");
> >  	html("Location: ");
> >  	html_url_path(url);
> > -	html("\n\n");
> > +	html("\r\n\r\n");
> >  }
> >  
> >  static void print_rel_vcs_link(const char *url)
> > -- 
> > 2.8.1
> > 
> > _______________________________________________
> > CGit mailing list
> > CGit at lists.zx2c4.com
> > http://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[wub]

On Wed, May 11, 2016 at 07:30:49PM +0100, John Keeping wrote:
> Missing sign-off; see http://developercertificate.org/ for what this
> means.

I am aware, but small changes like these are not generally recognized to
fit the threshold of originality for copyright protection. Thus, the
idea of Signed-off is quite often silly.

There are arguably cases where Signed-off is useful, for example, where
the maintainer has changed the commit message of the author or
co-authored code into the patch. An example from another repository I
maintain:

    commit d17c8830a8cb97fb48201990f5d57eb7a7b9e4ee
    Author: Eliot Whalan <ewhal at pantsu.cat>
    Date:   Thu Jan 21 11:41:17 2016 +1000
    
        Remove unused page specific CSS (old, ded)
    
        The tools page has only had alive and maintained tools on it since Pomf
        2.0.0. Ded and old tools have been removed in commit
        627937d0fa632a677f278af467886915c0a36f35.
    
        It is anticipated that these style rules will be never used again. If we
        did, we can add them back later: "you aren't gonna need it" (YAGNI). If
        it's dead, you should probably remove the tool from the tools page
        instead of beating a dead horse.
    
        [wub at partyvan.eu: Modified Git commit message, as discussed]
        Signed-off-by: Juuso Lapinlampi <wub at partyvan.eu>

In this case, the sole author is me and I don't think it's sensible to
use Signed-off-by. Not at least until Git makes it a default behavior.

I am the author of these patches with my name and email address in Git's
"Author:" field.

(Off-topic: Notice how the copyright headers are stuck in year 2014 in
the source file. Year ranges (2006-2014) have never been tested in court
as far as I'm aware, but comma seperated copyright years (2006, 2007,
2008, [...]) have been as far as I'm aware.)

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[john]

On Wed, May 11, 2016 at 07:38:24PM +0000, Juuso Lapinlampi wrote:
> On Wed, May 11, 2016 at 07:30:49PM +0100, John Keeping wrote:
> > Missing sign-off; see http://developercertificate.org/ for what this
> > means.
> 
> I am aware, but small changes like these are not generally recognized to
> fit the threshold of originality for copyright protection. Thus, the
> idea of Signed-off is quite often silly.

"generally recognized" is a bit nebulous, which is why a blanket policy
is safer as well as much simpler to police.

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[wub]

On Wed, May 11, 2016 at 08:57:52PM +0100, John Keeping wrote:
> "generally recognized" is a bit nebulous, which is why a blanket policy
> is safer as well as much simpler to police.

Guess we are going to wait for this bit to rot here over a silly blanket
policy then, as I have established my authorship already with Git
features and argued about threshold of originality. Same goes for the
other patches I submitted under the project's free software license
(GPLv2).

I know to be very reasonable with code review processes but this
Signed-off-by: policy is just too much.

By the way, get a CONTRIBUTING file.

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[john]

On Wed, May 11, 2016 at 08:15:27PM +0000, Juuso Lapinlampi wrote:
> On Wed, May 11, 2016 at 08:57:52PM +0100, John Keeping wrote:
> > "generally recognized" is a bit nebulous, which is why a blanket policy
> > is safer as well as much simpler to police.
> 
> Guess we are going to wait for this bit to rot here over a silly blanket
> policy then, as I have established my authorship already with Git
> features and argued about threshold of originality. Same goes for the
> other patches I submitted under the project's free software license
> (GPLv2).

"theshold of originality" is crap for two reasons:

1. if we use it as a criterion for requiring a sign-off then someone has
   to decide for each and every patch whether a sign-off is required,
   which increases the workload for maintainers with no benefit
2. IANAL but if you give a lawyer the choice between asserting that
   something is too small to matter for copyright or getting a sign-off
   certifying the DCO, I'll bet good money on them choosing the latter.

> I know to be very reasonable with code review processes but this
> Signed-off-by: policy is just too much.

If we were talking about a CLA I'd agree, but adding one line to the
commit message to certify that you have the rights to submit the patch
under the project's license doesn't seem that onerous to me (especially
when git-commit or git-format-patch will add it for you with "-s").

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[Jason]

On Wed, May 11, 2016 at 9:38 PM, Juuso Lapinlampi <wub at partyvan.eu> wrote:
> I am aware, but small changes like these are not generally recognized to
> fit the threshold of originality for copyright protection. Thus, the
> idea of Signed-off is quite often silly.

I don't care. I don't care to debate it either. Do what John says.

[PATCH 1/1] ui-shared: Use CRLF in HTTP headers as per RFC 7230

[Jason]

On Wed, May 11, 2016 at 7:48 PM, Juuso Lapinlampi <wub at partyvan.eu> wrote:
> CRLF is explicitly defined as the line break in the HTTP protocol
> specifications: RFC 2616 (obsolete) and RFC 7230.

But this is CGI, not HTTP.