{"thread":{"id":"27545","subject":"[PATCH] gitweb: Fix usability of $prevent_xss","startedAt":"2011-06-04T08:43:35Z","lastAt":"2011-06-14T01:33:57Z","messageCount":14,"participants":["Jakub Narebski","Matt McCutchen","J.H.","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"169300","messageId":"1307177015-880-1-git-send-email-jnareb@gmail.com","threadId":"27545","inReplyTo":null,"subject":"[PATCH] gitweb: Fix usability of $prevent_xss","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-04T08:43:35Z","receivedAt":"2011-06-04T08:43:35Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"With XSS prevention on (enabled using $prevent_xss), blobs\n('blob_plain') of all types except a few known safe ones are served\nwith \"Content-Disposition: attachment\".  However the check was too\nstrict; it didn't take into account optional parameter attributes,\n\n  media-type     = type \"/\" subtype *( \";\" parameter )\n\nas described in RFC 2616\n\n  http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.17\n  http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7\n\nThis fixes that, and it for example treats following as safe MIME\nmedia type:\n\n  text/plain; charset=utf-8\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThe fact that it this buglet was present for so long, since its\nintroduction by Matt McCutchen in 7e1100e (gitweb: add $prevent_xss\noption to prevent XSS by repository content, 2009-02-07) without\ncomplaint shows that not many people are using this feature...\n\nThat, and that we don't have automated tests for that.\n\n gitweb/gitweb.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex dc3f37d..85acbed 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -6139,7 +6139,7 @@ sub git_blob_plain {\n \t# want to be sure not to break that by serving the image as an\n \t# attachment (though Firefox 3 doesn't seem to care).\n \tmy $sandbox = $prevent_xss &&\n-\t\t$type !~ m!^(?:text/plain|image/(?:gif|png|jpeg))$!;\n+\t\t$type !~ m!^(?:text/plain|image/(?:gif|png|jpeg))(?:[ ;]|$)!;\n \n \tprint $cgi->header(\n \t\t-type => $type,\n-- \n1.7.5\n"},{"id":"169307","messageId":"1307222101.5994.13.camel@localhost","threadId":"27545","inReplyTo":"1307177015-880-1-git-send-email-jnareb@gmail.com","subject":"Prevalence of $prevent_xss","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2011-06-04T21:15:01Z","receivedAt":"2011-06-04T21:15:01Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Sat, 2011-06-04 at 10:43 +0200, Jakub Narebski wrote:\n> The fact that it this buglet was present for so long, since its\n> introduction by Matt McCutchen in 7e1100e (gitweb: add $prevent_xss\n> option to prevent XSS by repository content, 2009-02-07) without\n> complaint shows that not many people are using this feature...\n\nYes.  Well, I'm still using it, and I found a few mentions on the web:\n\nhttps://android.git.kernel.org/?p=tools/gerrit.git;a=blob;f=gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java;h=947fbb423f1f8cf46db9876f4b80c600cdf9ee41;hb=HEAD#l193\nhttp://ao2.it/wiki/How_to_setup_a_GIT_server_with_gitosis_and_gitweb\nhttp://www.digitalfoo.net/posts/2009/11/git,_gitosis,_gitweb_on_FreeBSD/\n\nAnd there are probably others who did their own custom things (GitHub?)\nbefore the feature was added upstream.\n\n-- \nMatt\n"},{"id":"169308","messageId":"201106042353.08942.jnareb@gmail.com","threadId":"27545","inReplyTo":"1307222101.5994.13.camel@localhost","subject":"Re: Prevalence of $prevent_xss","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-04T21:53:08Z","receivedAt":"2011-06-04T21:53:08Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 4 Jul 2011, Matt McCutchen wrote:\n> On Sat, 2011-06-04 at 10:43 +0200, Jakub Narebski wrote:\n\n> > The fact that it this buglet was present for so long, since its\n> > introduction by Matt McCutchen in 7e1100e (gitweb: add $prevent_xss\n> > option to prevent XSS by repository content, 2009-02-07) without\n> > complaint shows that not many people are using this feature...\n\nWell, and the fact that it is just minor issue (and might not be visible\nat all if there is mime.types file, and text files do use mime.types\nextensions).\n\n> Yes.  Well, I'm still using it, and I found a few mentions on the web:\n> \n> https://android.git.kernel.org/?p=tools/gerrit.git;a=blob;f=gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java;h=947fbb423f1f8cf46db9876f4b80c600cdf9ee41;hb=HEAD#l193\n> http://ao2.it/wiki/How_to_setup_a_GIT_server_with_gitosis_and_gitweb\n> http://www.digitalfoo.net/posts/2009/11/git,_gitosis,_gitweb_on_FreeBSD/\n\nThanks for research.  Nice to know.\n \n> And there are probably others who did their own custom things (GitHub?)\n> before the feature was added upstream.\n\nGitHub does not use gitweb, but its own [integrated] custom web interface.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"169312","messageId":"201106051103.59541.jnareb@gmail.com","threadId":"27545","inReplyTo":"1307222101.5994.13.camel@localhost","subject":"Implementing CSP (Content Security Policy) for gitweb in the future","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-05T09:03:55Z","receivedAt":"2011-06-05T09:03:55Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 4 Jul 2011, Matt McCutchen qrote:\n> On Sat, 2011-06-04 at 10:43 +0200, Jakub Narebski wrote:\n\n> > The fact that it this buglet was present for so long, since its\n> > introduction by Matt McCutchen in 7e1100e (gitweb: add $prevent_xss\n> > option to prevent XSS by repository content, 2009-02-07) without\n> > complaint shows that not many people are using this feature...\n> \n> Yes.  Well, I'm still using it, and I found a few mentions on the web:\n> \n> https://android.git.kernel.org/?p=tools/gerrit.git;a=blob;f=gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java;h=947fbb423f1f8cf46db9876f4b80c600cdf9ee41;hb=HEAD#l193\n> http://ao2.it/wiki/How_to_setup_a_GIT_server_with_gitosis_and_gitweb\n> http://www.digitalfoo.net/posts/2009/11/git,_gitosis,_gitweb_on_FreeBSD/\n> \n> And there are probably others who did their own custom things (GitHub?)\n> before the feature was added upstream.\n\nIn the future however it might be better solution for gitweb to implement\n(as an option) support for CSP (Content Security Policy), which IIRC did\nnot exists in 2009, in addition to current $prevent_xss.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"169316","messageId":"1307278350.23564.5.camel@localhost","threadId":"27545","inReplyTo":"201106051103.59541.jnareb@gmail.com","subject":"Re: Implementing CSP (Content Security Policy) for gitweb in the future","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2011-06-05T12:52:30Z","receivedAt":"2011-06-05T12:52:30Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Sun, 2011-06-05 at 11:03 +0200, Jakub Narebski wrote:\n> In the future however it might be better solution for gitweb to implement\n> (as an option) support for CSP (Content Security Policy), which IIRC did\n> not exists in 2009, in addition to current $prevent_xss.\n\nSure.  CSP is not a substitute for designing to prevent harmful HTML\ninjection, but a mitigation for some of its worst effects in case some\ninjection points are overlooked.  There's no reason not to enable it by\ndefault with $prevent_xss, though third parties adding functionality to\ngitweb would need to know to disable it or modify the policy\naccordingly.\n\n-- \nMatt\n"},{"id":"169317","messageId":"201106051533.51735.jnareb@gmail.com","threadId":"27545","inReplyTo":"1307278350.23564.5.camel@localhost","subject":"Re: Implementing CSP (Content Security Policy) for gitweb in the future","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-05T13:33:51Z","receivedAt":"2011-06-05T13:33:51Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 5 July 2011, Matt McCutchen wrote:\n> On Sun, 2011-06-05 at 11:03 +0200, Jakub Narebski wrote:\n\n> > In the future however it might be better solution for gitweb to implement\n> > (as an option) support for CSP (Content Security Policy), which IIRC did\n> > not exists in 2009, in addition to current $prevent_xss.\n> \n> Sure.  CSP is not a substitute for designing to prevent harmful HTML\n> injection, but a mitigation for some of its worst effects in case some\n> injection points are overlooked.  There's no reason not to enable it by\n> default with $prevent_xss, though third parties adding functionality to\n> gitweb would need to know to disable it or modify the policy\n> accordingly.\n\nI propose CSP support _in addition to_ and not replacing $prevent_xss\n(which would be nice to have more fine-grained control over).\n\nWell, while we can whitelist HTML fragment from README.html, or render\nREADME.md / README.rs / README.pod etc. instead of blocking it like gitweb\ncurrently does if $prevent_xss is enabled, I don't think it would be\nfeasible to do the same for text/html 'blob_plain' pages. \n\nServing HTML pages etc. from 'blob_plain' view with path_info links\nis quite useful feature; this way one can use gitweb as a cheap and easy\nway to deploy web pages and web apps; or just test results of development.\nCSP would serve this purpose well; current $prevent_xss behavior of\nserving as attachment (forcing download), or serving them as text/plain\nas e.g. GitHub does simply remove this feature.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"169323","messageId":"1307292370.23564.10.camel@localhost","threadId":"27545","inReplyTo":"201106051533.51735.jnareb@gmail.com","subject":"Re: Implementing CSP (Content Security Policy) for gitweb in the future","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2011-06-05T16:46:10Z","receivedAt":"2011-06-05T16:46:10Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Sun, 2011-06-05 at 15:33 +0200, Jakub Narebski wrote:\n> On Sun, 5 July 2011, Matt McCutchen wrote:\n> > On Sun, 2011-06-05 at 11:03 +0200, Jakub Narebski wrote:\n> \n> > > In the future however it might be better solution for gitweb to implement\n> > > (as an option) support for CSP (Content Security Policy), which IIRC did\n> > > not exists in 2009, in addition to current $prevent_xss.\n> > \n> > Sure.  CSP is not a substitute for designing to prevent harmful HTML\n> > injection, but a mitigation for some of its worst effects in case some\n> > injection points are overlooked.  There's no reason not to enable it by\n> > default with $prevent_xss, though third parties adding functionality to\n> > gitweb would need to know to disable it or modify the policy\n> > accordingly.\n> \n> I propose CSP support _in addition to_ and not replacing $prevent_xss\n> (which would be nice to have more fine-grained control over).\n> \n> Well, while we can whitelist HTML fragment from README.html, or render\n> README.md / README.rs / README.pod etc. instead of blocking it like gitweb\n> currently does if $prevent_xss is enabled, I don't think it would be\n> feasible to do the same for text/html 'blob_plain' pages. \n> \n> Serving HTML pages etc. from 'blob_plain' view with path_info links\n> is quite useful feature; this way one can use gitweb as a cheap and easy\n> way to deploy web pages\n\nYes.\n\n> and web apps;\n\nProbably not: the browser features needed to make a nontrivial web app\nare probably the same ones that are dangerous to other web apps.\n\n> or just test results of development.\n> CSP would serve this purpose well; current $prevent_xss behavior of\n> serving as attachment (forcing download), or serving them as text/plain\n> as e.g. GitHub does simply remove this feature.\n\nCSP is not intended to be used by itself as a sandbox, although it might\nalmost work for the purpose.  It would be more appropriate to set up a\nwildcard virtual host and appropriate rewrite rules to expose each\nrepository at a different DNS name and take advantage of the usual\nsame-origin policy.\n\n-- \nMatt\n"},{"id":"169587","messageId":"201106081227.19495.jnareb@gmail.com","threadId":"27545","inReplyTo":"1307292370.23564.10.camel@localhost","subject":"Re: Implementing CSP (Content Security Policy) for gitweb in the future","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-08T10:27:19Z","receivedAt":"2011-06-08T10:27:19Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun 5 June 2011 Matt McCutchen wrote:\n> On Sun 2011-06-05 at 15:33 +0200 Jakub Narebski wrote:\n>> On Sun 5 July 2011 Matt McCutchen wrote:\n>>> On Sun 2011-06-05 at 11:03 +0200 Jakub Narebski wrote:\n>> \n>>>> In the future however it might be better solution for gitweb to implement\n>>>> (as an option) support for CSP (Content Security Policy) which IIRC did\n>>>> not exists in 2009 in addition to current $prevent_xss.\n>>> \n>>> Sure. CSP is not a substitute for designing to prevent harmful HTML\n>>> injection but a mitigation for some of its worst effects in case some\n>>> injection points are overlooked. There's no reason not to enable it by\n>>> default with $prevent_xss though third parties adding functionality to\n>>> gitweb would need to know to disable it or modify the policy\n>>> accordingly.\n>> \n>> I propose CSP support _in addition to_ and not replacing $prevent_xss\n>> (which would be nice to have more fine-grained control over).\n>> \n>> Well while we can whitelist HTML fragment from README.html or render\n>> README.md / README.rs / README.pod etc. instead of blocking it like gitweb\n>> currently does if $prevent_xss is enabled I don't think it would be\n>> feasible to do the same for text/html 'blob_plain' pages. \n>> \n>> Serving HTML pages etc. from 'blob_plain' view with path_info links\n>> is quite useful feature; this way one can use gitweb as a cheap and easy\n>> way to deploy web pages\n> \n> Yes.\n> \n>> and web apps;\n> \n> Probably not: the browser features needed to make a nontrivial web app\n> are probably the same ones that are dangerous to other web apps.\n\n\"Deploying\" with gitweb doesn't allow for server-side scripting, so it\nis \"web apps\" only as far as there can be web application done entirely\non client-side: HTML or HTML5 + JavaScript.  Well, there is demo of a\ngame played in HTML5+JavaScript played entirely in URL bar ;-)\n\nWith CSP you would be restricted to prerequisites (web page itself,\nscripts, stylesheets, images) to be also hosted/deployed via gitweb.\n\nWhat features would non server-side nontrivial web app need that would\nbe dangerous to other web apps?\n\n>> or just test results of development.\n>> CSP would serve this purpose well; current $prevent_xss behavior of\n>> serving as attachment (forcing download) or serving them as text/plain\n>> as e.g. GitHub does simply remove this feature.\n> \n> CSP is not intended to be used by itself as a sandbox although it might\n> almost work for the purpose. It would be more appropriate to set up a\n> wildcard virtual host and appropriate rewrite rules to expose each\n> repository at a different DNS name and take advantage of the usual\n> same-origin policy.\n\nCould this virtualhost + DNS + same-origin sandboxing be used for gitweb?\nIf not, then perhaps it is better solution in other cases, but not for\ngitweb.\n\n\nP.S. I don't know how difficult implementing CSP support for gitweb would\nbe, given that gitweb is quite configurable wrt. external resources it\nuses: $javascript, @stylesheets, various *logo variables...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"169648","messageId":"4DEFB1DE.7070102@eaglescrag.net","threadId":"27545","inReplyTo":"201106081227.19495.jnareb@gmail.com","subject":"Re: Implementing CSP (Content Security Policy) for gitweb in the future","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2011-06-08T17:31:10Z","receivedAt":"2011-06-08T17:31:10Z","isPatch":false,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"On 06/08/2011 03:27 AM, Jakub Narebski wrote:\n> On Sun 5 June 2011 Matt McCutchen wrote:\n>> On Sun 2011-06-05 at 15:33 +0200 Jakub Narebski wrote:\n>>> On Sun 5 July 2011 Matt McCutchen wrote:\n>>>> On Sun 2011-06-05 at 11:03 +0200 Jakub Narebski wrote:\n>>>\n>>>>> In the future however it might be better solution for gitweb to implement\n>>>>> (as an option) support for CSP (Content Security Policy) which IIRC did\n>>>>> not exists in 2009 in addition to current $prevent_xss.\n>>>>\n>>>> Sure. CSP is not a substitute for designing to prevent harmful HTML\n>>>> injection but a mitigation for some of its worst effects in case some\n>>>> injection points are overlooked. There's no reason not to enable it by\n>>>> default with $prevent_xss though third parties adding functionality to\n>>>> gitweb would need to know to disable it or modify the policy\n>>>> accordingly.\n>>>\n>>> I propose CSP support _in addition to_ and not replacing $prevent_xss\n>>> (which would be nice to have more fine-grained control over).\n>>>\n>>> Well while we can whitelist HTML fragment from README.html or render\n>>> README.md / README.rs / README.pod etc. instead of blocking it like gitweb\n>>> currently does if $prevent_xss is enabled I don't think it would be\n>>> feasible to do the same for text/html 'blob_plain' pages. \n>>>\n>>> Serving HTML pages etc. from 'blob_plain' view with path_info links\n>>> is quite useful feature; this way one can use gitweb as a cheap and easy\n>>> way to deploy web pages\n>>\n>> Yes.\n>>\n>>> and web apps;\n>>\n>> Probably not: the browser features needed to make a nontrivial web app\n>> are probably the same ones that are dangerous to other web apps.\n> \n> \"Deploying\" with gitweb doesn't allow for server-side scripting, so it\n> is \"web apps\" only as far as there can be web application done entirely\n> on client-side: HTML or HTML5 + JavaScript.  Well, there is demo of a\n> game played in HTML5+JavaScript played entirely in URL bar ;-)\n> \n> With CSP you would be restricted to prerequisites (web page itself,\n> scripts, stylesheets, images) to be also hosted/deployed via gitweb.\n> \n> What features would non server-side nontrivial web app need that would\n> be dangerous to other web apps?\n> \n>>> or just test results of development.\n>>> CSP would serve this purpose well; current $prevent_xss behavior of\n>>> serving as attachment (forcing download) or serving them as text/plain\n>>> as e.g. GitHub does simply remove this feature.\n>>\n>> CSP is not intended to be used by itself as a sandbox although it might\n>> almost work for the purpose. It would be more appropriate to set up a\n>> wildcard virtual host and appropriate rewrite rules to expose each\n>> repository at a different DNS name and take advantage of the usual\n>> same-origin policy.\n> \n> Could this virtualhost + DNS + same-origin sandboxing be used for gitweb?\n> If not, then perhaps it is better solution in other cases, but not for\n> gitweb.\n> \n> \n> P.S. I don't know how difficult implementing CSP support for gitweb would\n> be, given that gitweb is quite configurable wrt. external resources it\n> uses: $javascript, @stylesheets, various *logo variables...\n> \n\nThe long and the short of it is, until browsers other than\nMozilla/Firefox support CSP, it's a dead standard and we shouldn't spend\nany time on it.  There are few enough resources on gitweb as it is to be\ntilting at windmills.\n\n- John 'Warthog9' Hawley\n"},{"id":"169841","messageId":"201106101401.19108.jnareb@gmail.com","threadId":"27545","inReplyTo":"1307177015-880-1-git-send-email-jnareb@gmail.com","subject":"[PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-10T12:01:18Z","receivedAt":"2011-06-10T12:01:18Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"One of mechanism enabled by setting $prevent_xss to true is 'blob_plain'\nview protection.  With XSS prevention on, blobs of all types except a\nfew known safe ones are served with \"Content-Disposition: attachment\" to\nmake sure they don't run in our security domain.\n\nInstead of serving text/* type files, except text/plain (and including\ntext/html), as attachements, downgrade it to text/plain.  This way HTML\npages in 'blob_plain' (raw) wiew would be displayed in browser, but\nsafely as a source, and not asked to be saved.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis is what GitHub interface does for text/html pages (*.html), if\nI remember it correctly...\n\n gitweb/gitweb.perl |   10 +++++++++-\n 1 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 85acbed..470793a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -6139,7 +6139,15 @@ sub git_blob_plain {\n \t# want to be sure not to break that by serving the image as an\n \t# attachment (though Firefox 3 doesn't seem to care).\n \tmy $sandbox = $prevent_xss &&\n-\t\t$type !~ m!^(?:text/plain|image/(?:gif|png|jpeg))(?:[ ;]|$)!;\n+\t\t$type !~ m!^(?:text/[a-z]+|image/(?:gif|png|jpeg))(?:[ ;]|$)!;\n+\n+\t# serve text/* as text/plain\n+\tif ($prevent_xss &&\n+\t    $type =~ m!^text/([a-z]+)\\b(.*)$!) {\n+\t\tmy ($subtype, $rest) = ($1, $2);\n+\t\t$rest = defined $rest ? $rest : '';\n+\t\t$type = \"text/plain$rest\" if ($subtype ne 'plain');\n+\t}\n \n \tprint $cgi->header(\n \t\t-type => $type,\n-- \n1.7.5\n"},{"id":"169954","messageId":"7v8vt5ptj4.fsf@alter.siamese.dyndns.org","threadId":"27545","inReplyTo":"201106101401.19108.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-13T16:47:59Z","receivedAt":"2011-06-13T16:47:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> +\t# serve text/* as text/plain\n> +\tif ($prevent_xss &&\n> +\t    $type =~ m!^text/([a-z]+)\\b(.*)$!) {\n> +\t\tmy ($subtype, $rest) = ($1, $2);\n> +\t\t$rest = defined $rest ? $rest : '';\n> +\t\t$type = \"text/plain$rest\" if ($subtype ne 'plain');\n\nHmph, wouldn't it be more straightforward if you dropped the statement\nmodifier?  I.e.\n\n\tmy ($subtype, $rest) = ($1, $2);\n        $rest = '' unless defined $rest;\n        $type = \"text/plain$rest\";\n\nOther than that, looks good to me.\n\nThanks.\n"},{"id":"169967","messageId":"201106132350.00161.jnareb@gmail.com","threadId":"27545","inReplyTo":"7v8vt5ptj4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-13T21:49:59Z","receivedAt":"2011-06-13T21:49:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > +\t# serve text/* as text/plain\n> > +\tif ($prevent_xss &&\n> > +\t    $type =~ m!^text/([a-z]+)\\b(.*)$!) {\n> > +\t\tmy ($subtype, $rest) = ($1, $2);\n> > +\t\t$rest = defined $rest ? $rest : '';\n> > +\t\t$type = \"text/plain$rest\" if ($subtype ne 'plain');\n> \n> Hmph, wouldn't it be more straightforward if you dropped the statement\n> modifier?  I.e.\n> \n> \tmy ($subtype, $rest) = ($1, $2);\n> \t$rest = '' unless defined $rest;\n> \t$type = \"text/plain$rest\";\n\nYes, of course.\n\nI don't know why I decided that avoiding rewriting 'text/plain; \ncharset=utf-8' case was important.  It cretainly is not worth making \ncode harder to follow.\n \nCan you fix it during applying, or should I resend it?\n\n> Other than that, looks good to me.\n\nThanks.\n-- \nJakub Narebski\nPoland\n"},{"id":"169971","messageId":"7vy615mily.fsf@alter.siamese.dyndns.org","threadId":"27545","inReplyTo":"201106132350.00161.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-13T23:12:09Z","receivedAt":"2011-06-13T23:12:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>> Hmph, wouldn't it be more straightforward if you dropped the statement\n>> modifier?  I.e.\n>> \n>> \tmy ($subtype, $rest) = ($1, $2);\n>> \t$rest = '' unless defined $rest;\n>> \t$type = \"text/plain$rest\";\n>\n> Yes, of course.\n>\n> I don't know why I decided that avoiding rewriting 'text/plain; \n> charset=utf-8' case was important.\n\nJust to make sure I understand what you are saying...\n\n    my $type = 'text/plain; charset=utf-8';\n    if ($type =~ m|^text/([a-z]+)\\b(.*)$|) {\n \tmy ($subtype, $rest) = ($1, $2);\n \t$rest = '' unless defined $rest;\n \t$type = \"text/plain$rest\";\n        print \"Type is now <$type>\\n\";\n    }\n\n\ndoes yield \"text/plain; charset=utf-8\". It does rewrite but rewrite to\nexactly the same thing, so...\n"},{"id":"169974","messageId":"201106140333.58351.jnareb@gmail.com","threadId":"27545","inReplyTo":"7vy615mily.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-14T01:33:57Z","receivedAt":"2011-06-14T01:33:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>>> Hmph, wouldn't it be more straightforward if you dropped the statement\n>>> modifier?  I.e.\n>>> \n>>> \tmy ($subtype, $rest) = ($1, $2);\n>>> \t$rest = '' unless defined $rest;\n>>> \t$type = \"text/plain$rest\";\n>>\n>> Yes, of course.\n>>\n>> I don't know why I decided that avoiding rewriting 'text/plain; \n>> charset=utf-8' case was important.\n> \n> Just to make sure I understand what you are saying...\n> \n>     my $type = 'text/plain; charset=utf-8';\n>     if ($type =~ m|^text/([a-z]+)\\b(.*)$|) {\n>  \tmy ($subtype, $rest) = ($1, $2);\n>  \t$rest = '' unless defined $rest;\n>  \t$type = \"text/plain$rest\";\n>         print \"Type is now <$type>\\n\";\n>     }\n> \n> \n> does yield \"text/plain; charset=utf-8\". It does rewrite but rewrite to\n> exactly the same thing, so...\n\nYes, it does rewrite to the same thing.  And the code is simpler,\ntherefore better. \n\n-- \nJakub Narebski\nPoland\n"}]}