[FEATURE] csp: enable tracking and serving of csp reports #323

tobiasso85 · 2020-05-20T12:38:35Z

With option serveCSPReports active

csp reports are gathered and can be retrieved with a request to "/.ui5/csp/csp-reports.json"

sample response:

{
  "csp-reports": [
    {
      "document-uri": "https://otherserver:8080/index.html",
      "referrer": "",
      "violated-directive": "script-src-elem",
      "effective-directive": "script-src-elem",
      "original-policy": "default-src 'self' myserver:443; report-uri /.ui5/csp/report.csplog",
      "disposition": "report",
      "blocked-uri": "inline",
      "line-number": 17,
      "source-file": "https://otherserver:8080/index.html",
      "status-code": 0,
      "script-sample": ""
    }
  ]
}

Reports file is written with the new config option "cspReportPath" set. json file written contains all csp-reports in an array.

coveralls · 2020-05-20T12:40:08Z

Coverage decreased (-0.07%) to 93.282% when pulling 0c7a241 on csp-reports-file into 2a7e208 on master.

adjust documentation

be set from the outside when serving

matz3 · 2020-05-27T16:07:56Z

lib/middleware/csp.js

+ * @param {string} cspReportPath path where the reports should be written to
+ * @returns {Function} Returns a server middleware closure.
+ */
+function createMiddleware(sCspUrlParameterName, oConfig, cspReportPath) {


The cspReportPath is part of the oConfig, isn't it?

good point, adjusted it

matz3 · 2020-05-27T16:16:48Z

lib/middleware/csp.js

+				// Write the violation into a csp-reports json file if cspReportPath parameter is set
+				if (cspReportPath && req.body && req.body["csp-report"]) {
+					// extract the csp-report and add it to the cspReportEntries list
+					cspReportEntries.push(req.body["csp-report"]);


Can we be sure that the body has been parsed?
If the body-parser middleware is not used (which we don't), then I would expect req.body to be a string.
So we should check and parse the body if necessary.

matz3 · 2020-05-27T16:19:03Z

lib/middleware/csp.js

+					// extract the csp-report and add it to the cspReportEntries list
+					cspReportEntries.push(req.body["csp-report"]);
+					// write the csp report file
+					writeCspReportsFiles(cspReportPath, cspReportEntries).then(() => {


Do we really want to write to the file for every single violation? I think this will cause a high disk I/O.
We should rather think how we can write the report before the server will be shut-down.
In the builder we have a way to define cleanup tasks, which seems to be quite similar to this use case.

only write upon server#close or sigint

Used
https://github.com/SAP/ui5-builder/blob/9f7d500bdabec327cdebf19d24eae967f5ecda62/lib/builder/builder.js#L141-L179

Use exit hook and server close function to write csp reports file. This avoids writing to a file with every request. The csp reports are stored in an array and only written once the server closes or gets killed (SIGINT)

report is served for URL "/.ui5/csp/csp-reports.json" when parameter serveCSPReports is active. Rather than saving the file on exit, the content now can be accessed on demand. Additionally the served content is valid json.

matz3 · 2020-06-02T10:30:30Z

lib/middleware/csp.js

-				oParsedURL.pathname.endsWith("/dummy.csplog") ) {
-				// In report-only mode there must be a report-uri defined
-				// For now just ignore the violation. It will be logged in the browser anyway.
+				oParsedURL.pathname.endsWith("/.ui5/csp/report.csplog") ) {


As the report-uri is absolute, this check should compare the whole URL

Suggested change

oParsedURL.pathname.endsWith("/.ui5/csp/report.csplog") ) {

oParsedURL.pathname === "/.ui5/csp/report.csplog") {

matz3 · 2020-06-02T10:31:49Z

lib/middleware/csp.js

+		} else if (serveCSPReports && req.method === "GET") {
+			// serve csp reports
+			if (req.headers["content-type"] === "application/json" &&
+				oParsedURL.pathname.endsWith("/.ui5/csp/csp-reports.json")) {


Same here, should be an absolute URL

Suggested change

oParsedURL.pathname.endsWith("/.ui5/csp/csp-reports.json")) {

oParsedURL.pathname === "/.ui5/csp/csp-reports.json") {

matz3 · 2020-06-02T10:33:06Z

lib/middleware/csp.js

 				return;
 			}
 			next();
 			return;
+		} else if (serveCSPReports && req.method === "GET") {
+			// serve csp reports
+			if (req.headers["content-type"] === "application/json" &&


Do we need to explicitly check for the requested content-type? This would require someone to pass this header explicitly and it's not possible to just load the file from the browser, right?

was changed to just compare the URL and the method to be a GET request

matz3 · 2020-06-02T10:36:15Z

lib/middleware/csp.js

+			if (req.headers["content-type"] === "application/json" &&
+				oParsedURL.pathname.endsWith("/.ui5/csp/csp-reports.json")) {
+				const body = JSON.stringify({
+					"csp-reports": cspReportEntries


I would opt for using some indentation, as otherwise the file is hard to read or needs to be formatted later on.
The current Java implementation uses tabs, so I think it makes sense to do the same.

matz3 · 2020-06-02T10:49:24Z

lib/middleware/csp.js

+				// Write the violation into an array
+				// They can be retrieved via a request to '/.ui5/csp/csp-reports.json'
+				if (serveCSPReports && req.body) {
+					const reqBodyObject = typeof req.body === "string" ? JSON.parse(req.body) : req.body;


It seems that there's no test case to cover the JSON.parse.
Error handling would also be interesting here. Will the server crash when invalid JSON is provided? I would expect that just this request fails with a proper error

Add body parser to parse request body in order to get csp-report. Add res.end() such that csp report requests are not stale (pending).

Instead of having the body-parser part of the MiddlewareManager it is now part of the csp middleware itself using router module. It allows also to split up the request branches.

RandomByte · 2020-06-03T15:56:35Z

lib/middleware/csp.js

+	// .csplog
+	// body parser is required to parse csp-report in body (json)
+	if (serveCSPReports) {
+		router.post("/.ui5/csp/report.csplog", bodyParser.json({type: "application/csp-report"}));


Do we really need the body-parser middleware? Can't we just parse the JSON string?

That's what I also thought initially, but the body-parser does way more than that. It reads the request stream, handles encoding, etc., so I don't think we should write that on our own.

But this way it's only done for that specific path, so no conflict with other middlewares and no overhead to handle other body content.

I see, sounds good 👍

matz3

LGTM

RandomByte · 2020-06-08T14:45:12Z

lib/middleware/csp.js

+	const router = new Router();
+	// .csplog
+	// body parser is required to parse csp-report in body (json)
+	router.post("/.ui5/csp/report.csplog", bodyParser.json({type: "application/csp-report"}));


As a minor optimization, maybe only require and use bodyParser if serveCSPReports is true?

done with 0c7a241

Update ui5-server dependency (and others) to include SAP/ui5-server#323 Setting the environment variable OPENUI5_SRV_CSP to true will configure the ui5-server to set the UI5 target CSP headers as well as to serve any reports at '/.ui5/csp/csp-reports.json' JIRA: CPOUI5FOUNDATION-212 Change-Id: I42057dd4d6127f4639385c3fe8dc71bef396a49a

[FEATURE] csp: enable writing of reports file

ef752ac

Reports file is written with the new config option "cspReportPath" set. json file written contains all csp-reports in an array.

tobiasso85 requested review from RandomByte and matz3 May 20, 2020 12:39

tobiasso85 mentioned this pull request May 20, 2020

[FEATURE] ui5 serve: Add serve-csp-reports option SAP/ui5-cli#339

Merged

tobiasso85 force-pushed the csp-reports-file branch from bd2f2ae to 8629195 Compare May 27, 2020 08:06

[FEATURE] csp: reduce test to relevant parts

44694f6

adjust documentation

tobiasso85 force-pushed the csp-reports-file branch from 8629195 to 44694f6 Compare May 27, 2020 08:08

[FEATURE] csp: Add cspReportPath parameter such that it can

264432c

be set from the outside when serving

matz3 requested changes May 27, 2020

View reviewed changes

[FEATURE] csp: only write file on exit

039e157

Use exit hook and server close function to write csp reports file. This avoids writing to a file with every request. The csp reports are stored in an array and only written once the server closes or gets killed (SIGINT)

tobiasso85 requested a review from matz3 May 28, 2020 13:37

tobiasso85 added 2 commits May 29, 2020 10:28

[FEATURE] csp: changed report to report serving

7a9f9ab

report is served for URL "/.ui5/csp/csp-reports.json" when parameter serveCSPReports is active. Rather than saving the file on exit, the content now can be accessed on demand. Additionally the served content is valid json.

[FEATURE] csp: added test removed middleUtil

481dcd4

tobiasso85 changed the title ~~[FEATURE] csp: enable writing of reports file~~ [FEATURE] csp: enable tracking and serving of csp reports May 29, 2020

matz3 requested changes Jun 2, 2020

View reviewed changes

tobiasso85 added 2 commits June 2, 2020 13:39

[FEATURE] csp: improve code

88301fe

[FEATURE] csp: add error handling

00e86de

Add body parser to parse request body in order to get csp-report. Add res.end() such that csp report requests are not stale (pending).

tobiasso85 requested a review from matz3 June 2, 2020 15:51

[FEATURE] csp: use router

7e5274c

Instead of having the body-parser part of the MiddlewareManager it is now part of the csp middleware itself using router module. It allows also to split up the request branches.

RandomByte reviewed Jun 3, 2020

View reviewed changes

tobiasso85 requested a review from RandomByte June 8, 2020 05:54

Change HTML middleware back to "use" / fix tests (async)

ebd252d

matz3 reviewed Jun 8, 2020

View reviewed changes

RandomByte reviewed Jun 8, 2020

View reviewed changes

[FEATURE] csp: lazy require body-parser

0c7a241

tobiasso85 requested a review from matz3 June 9, 2020 06:38

matz3 approved these changes Jun 9, 2020

View reviewed changes

tobiasso85 merged commit e0a0c5e into master Jun 9, 2020

tobiasso85 deleted the csp-reports-file branch June 9, 2020 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] csp: enable tracking and serving of csp reports #323

[FEATURE] csp: enable tracking and serving of csp reports #323

tobiasso85 commented May 20, 2020 •

edited

Loading

coveralls commented May 20, 2020 •

edited

Loading

matz3 May 27, 2020

tobiasso85 May 28, 2020

matz3 May 27, 2020

tobiasso85 May 28, 2020

matz3 May 27, 2020

tobiasso85 May 28, 2020

matz3 Jun 2, 2020

tobiasso85 Jun 3, 2020

matz3 Jun 2, 2020

tobiasso85 Jun 2, 2020

matz3 Jun 2, 2020

tobiasso85 Jun 3, 2020

matz3 Jun 2, 2020

tobiasso85 Jun 2, 2020

matz3 Jun 2, 2020

tobiasso85 Jun 2, 2020

RandomByte Jun 3, 2020

matz3 Jun 3, 2020

RandomByte Jun 3, 2020

matz3 left a comment

RandomByte Jun 8, 2020

tobiasso85 Jun 9, 2020

	oParsedURL.pathname.endsWith("/.ui5/csp/report.csplog") ) {
	oParsedURL.pathname === "/.ui5/csp/report.csplog") {

	oParsedURL.pathname.endsWith("/.ui5/csp/csp-reports.json")) {
	oParsedURL.pathname === "/.ui5/csp/csp-reports.json") {

[FEATURE] csp: enable tracking and serving of csp reports #323

[FEATURE] csp: enable tracking and serving of csp reports #323

Conversation

tobiasso85 commented May 20, 2020 • edited Loading

coveralls commented May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matz3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasso85 commented May 20, 2020 •

edited

Loading

coveralls commented May 20, 2020 •

edited

Loading