Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver-test.cc@463 PS3, Line 463: //Basic sanity check: the page should have the expected title. nit: add a space after the comment http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver-test.cc@464 PS3, Line 464: ASSERT_STR_CONTAINS(buf_.ToString(),"Kudu"); nit: missed space after the comma http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver-test.cc@473 PS3, Line 473: ASSERT_STR_NOT_CONTAINS(buf_.ToString(),kCspHeader); nit: missed space after the comma http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@93 PS3, Line 93: webserver_disable_csp We do have a bunch of boolean flags like that, and the majority of those named 'xxx_enabled' or 'xxx_enable_xxx'. For uniformity, rename this into 'webserver_enable_csp_header' and set the default value to 'true'. That would help to have clearer description for the flag as well :) http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@94 PS3, Line 94: Currently" : "the webserver is already impervious to XSS attacks due to being read-only. This is not very relevant for the description of the flag. You could add that into the commit description. http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@646 PS3, Line 646: char * style nit: stick the ampersand to the type http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@649 PS3, Line 649: string & style nit: stick the ampersand to the type http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@689 PS3, Line 689: FLAGS_webserver_disable_csp Since we are aiming to have the CSP header added by default, wrap this into PREDICT_TRUE or PREDICT_FALSE, as necessary. http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@692 PS3, Line 692: The easiest way to obtain this hash is through browser/js : // console. It is embedded in the error message. OK, I guess there is a way to do so using command-line tools, but we can update the comment later on as needed. Essentially, there should be a command that people could just copy-paste, get the new hash and update it here in the code. All right, let's aim for the follow-up patch to add those instructions. -- To view, visit http://gerrit.cloudera.org:8080/18285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106 Gerrit-Change-Number: 18285 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 14 Jul 2022 18:30:11 +0000 Gerrit-HasComments: Yes
