Implement Content Security Policy, including moving all CSS and JS to external files #966

candrews · 2020-11-24T19:12:01Z

A strict content security policy is increasingly required and expected. This PR allows JavaMelody to function when a strict CSP is set, and in also sets a strict CSP for JavaMelody itself by default.

I've tested this pretty well on my own system, clicking around and checking various things to ensure everything looks and works correctly. There are no functional or visual changes; if I've done this right, you shouldn't be able to tell that I've done anything at all. :)

Add a configuration option for CSP header (enabled by default)
Eliminate all inline script and styles

Completes the work started in #782

candrews · 2020-11-30T16:54:47Z

@evernat have you had a chance to look at this PR? I'm eager to address any feedback in order to get this work merged - thank you again!

javamelody-core/src/main/java/net/bull/javamelody/Parameter.java

evernat · 2020-12-02T23:49:27Z

I looked at the PR, thank you for that. There is much to read and to test.

candrews · 2020-12-07T17:38:17Z

There is much to read and to test.

Yeah, it's a big one...

Is there anything I can do to help? As I mentioned, I'm quite eager to get this merged :)

evernat · 2020-12-09T20:11:58Z

The PR is not bad, but there are things broken by the PR. It needs tests and I will try to list some issues.

candrews · 2020-12-09T20:15:12Z

The PR is not bad

🎉

but there are things broken by the PR.

:( I'm eager to eager what so I can address those issues.

It needs tests and I will try to list some issues.

Do you mean some kind of automated tests are required? I don't see any testing framework in JavaMelody.

evernat · 2020-12-09T20:15:34Z

When having Maven versions in the webapp and when choosing a period by version at the top of the main report, it does nothing. It should refresh the page with the period of the version.
After opening the "Database" report in "System informations", choosing a database report in the list does nothing. It should refresh the page to display the choosen report.
Both are because the browser blocks inline scripts still written with "onchange".

evernat · 2020-12-09T20:18:47Z

Do you mean some kind of automated tests are required? I don't see any testing framework in JavaMelody.

There is junit. But given it's JS and CSP, I was speaking of manual tests.

evernat · 2020-12-09T20:21:22Z

When clicking on "Execute the garbage collector", the confim message displays <br> instead of a line break.

evernat · 2020-12-09T20:27:58Z

In the Threads details, clicking on the "Send a thread interrupt signal" or "Kill the tread" actions display a confirm message "null" instead of "Do you confirm ..." and "... Do you really want ..."

about:blank works for all browsers and doesn't use inline javascript therefore doesn't violate a content security policy which prohibits inline script.

innerHTML is unsafe; use document.createElement to create elements then set attributes on them.

Move the BOOMR.init call to boomerang.min.js and include boomerang.min.js using a script tag. Use attributes on the script tag to pass necessary data to the script which is used to call BOOMR.init.

Avoid a CSP inline script violation. See: prototypejs/prototype#320

Move all inline CSS and inline Javascript to monitoring.css and monitoring.js

Since no javascript is dynamically generated, methods to escape javascript are no longer necessary or used.

candrews · 2020-12-18T23:11:02Z

When having Maven versions in the webapp and when choosing a period by version at the top of the main report, it does nothing. It should refresh the page with the period of the version.
After opening the "Database" report in "System informations", choosing a database report in the list does nothing. It should refresh the page to display the choosen report.
Both are because the browser blocks inline scripts still written with "onchange".

I missed onchange - thank you for pointing those out. I've now addressed them.

When clicking on "Execute the garbage collector", the confim message displays <br> instead of a line break.

Fixed

In the Threads details, clicking on the "Send a thread interrupt signal" or "Kill the tread" actions display a confirm message "null" instead of "Do you confirm ..." and "... Do you really want ..."

Fixed

I've now addressed everything you reported; if you have any more findings or other feedback, please let know!

candrews · 2021-01-04T20:42:25Z

Happy 2021! 🥳

I have my 🤞 that the merging of this PR will be a great new start to the new year!

candrews · 2021-01-11T15:18:07Z

@evernat could you please take another look at this PR?

candrews · 2021-01-19T14:08:29Z

Hello @evernat - please let me know if there's anything I can do to help this PR along

evernat · 2021-01-24T23:51:01Z

I have made some tests and there are some issues:

In the database reports, the list to choose the database report does not work, That's because there is & in the URL instead of &. See http://javamelody.org/demo/monitoring?part=database
The Other charts link on the right does not work well: it displays the other charts but the items do not scroll below the charts like in http://javamelody.org/demo/monitoring.
The slider to zoom the graph does not work like it does in http://javamelody.org/demo/monitoring?part=graph&graph=usedMemory. The image should be zoomed with a new URL like http://javamelody.org/demo/monitoring?graph=usedMemory&width=1480&height=662
When clicking on Details in System information, just above Threads, the Threads part slips on the right, but it shoud not, see http://javamelody.org/demo/monitoring

candrews · 2021-01-31T20:56:45Z

I have made some tests and there are some issues:

1. In the database reports, the list to choose the database report does not work, That's because there is `&amp;` in the URL instead of `&`. See http://javamelody.org/demo/monitoring?part=database

I've pushed a fix for this issue.

2. The `Other charts` link on the right does not work well: it displays the other charts but the items do not scroll below the charts like in http://javamelody.org/demo/monitoring.

I've pushed a fix for this issue.

3. The slider to zoom the graph does not work like it does in http://javamelody.org/demo/monitoring?part=graph&graph=usedMemory. The image should be zoomed with a new URL like http://javamelody.org/demo/monitoring?graph=usedMemory&width=1480&height=662

I can't reproduce this one. I moved the slider and I can see that the image src keep changing, and does do in the same way as the demo site does.

4. When clicking on Details in System information, just above Threads, the Threads part slips on the right, but it shoud not, see http://javamelody.org/demo/monitoring

I've pushed a fix for this issue.

Thanks again for pointing out these issue - hopefully we're close to being able to merge this now 🤞

candrews · 2021-02-13T16:39:16Z

@evernat friendly bump - I'd love to make progress and get this change merged, thanks again!

candrews · 2021-02-21T02:05:31Z

@evernat is there anything I can do to help this along?

evernat · 2021-03-07T23:43:45Z

Sorry for the delay.
I have made some tests and I have some issues:

When I see the parameter content-security-policy I would think that the value should be default-src 'self'; object-src 'none';. So it may be better to rename it to content-security-policy-enabled. (true by default)
When I click on some actions such as Run GC. There is no more message giving the result of the action. It suppose that it is related to htmlCoreReport.toHtml(message, anchorNameForRedirect); replaced by htmlCoreReport.toHtml(); in the PR.
When the monitoring page is loaded, graphs of http and sql requests for example are loaded with URL like .../monitoring?graph=http211e03eeda2e3305b34429bf203bbe8e280d4664&width=100&height=50. Those graphs of http and sql requests should be loaded only when the mouse hovers over each request in the details in http and sql statistics. It is related to <img src='?graph= and the removing of the onmouseover script in the PR. That's important because on some applications there may up to 10000 http requests and 10000 sql requests in the statistics.
There are at least one issue with content-security-policy: in HtmlMBeansReport, there is still an inline <style type='text/css'>

evernat · 2021-04-05T20:00:31Z

Hi,
There are still issues with encoding. In particular, StringIndexOutOfBoundsException when a confirm message contains a quote, now encoded as ', because keys between # are intended to be translations keys in write*. For example, in the confirm message for GC in French.
Ignoring &# in the translation code is not a solution, because there would be ' in confirm messages anyway.

So my plan is:

I cut a release now, because it is overdue by too many months. I can still make another release when the content-security-policy work is ready.
I have merged the PR in the content-security-policy branch for reference.
Then probably:
I merge some bits such as moving some inline styles to css and moving some inline scripts to js files, to make the PR to come more readable. Of course, I say in the commits your github id and that it is based on this PR. And I rebase the branch with that.
I look again at which encoding methods are used where and what is broken by what, in order to fix the branch.
I add something like writeHtmlBegin before writeMessageIfNotNull in CollectorController.writeMessage.

The work in this PR is already huge and probably the bigger I have seen in a PR for javamelody. It's not bad if it is not ready yet given the task that it is.

candrews · 2021-04-07T00:57:41Z

The # issue can be easily fixed by using a different character representation. I've submitted a PR at #1003 that does just that.

I'm sorry I keep having issues... without comprehensive tests and since I'm not fully familiar with all of the functionality across the entire interface, it's really hard for me to find all the problems. I'm trying though!

Do you have any other issues? I'll happily fix them.

If you'd like the commits rebased for clarity, I can do that. I was (and still am) unclear on if you intend to squash all the commits.

I really want to get this into JavaMelody. I've been trying for almost 6 months; it's important to myself and my colleagues. Can we do anything to accelerate the process?

evernat · 2021-04-07T14:26:24Z

Given the number of commits in the branch, yes the commits would be squashed to be merged.
But I plan first to commit some css and js changes from the branch and then to rebase the branch.
The branch will be merged after a final review.

evernat · 2021-04-14T16:33:01Z

...y-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlJCacheInformationsReport.java

@@ -197,16 +198,16 @@ private void writeJCacheKeys(JCacheInformations jcacheInformations) throws IOExc
 				writeDirectly(htmlEncodeButNotSpace(myKey));
 				writeDirectly("</td>");
 				if (systemActionsEnabled) {
-					writeDirectly("<td class='noPrint' style='text-align: center;'>");
+					writeDirectly("<td class='noPrint containingIcon'");


> was missing here

Nice catch, thank you!

srsouto · 2021-06-21T14:41:59Z

Is there a status on the merge of the rest of this branch? I'm seeing that the css and js externalize work was merged, but has the CSP implementation been pushed through?

tonylulciuc · 2021-06-23T15:56:46Z

@evernat Do you know the state of this merged code?

candrews · 2021-06-23T16:32:16Z

This hasn't actually been merged to master - it's been "merged" to a branch: https://github.com/javamelody/javamelody/tree/content-security-policy

I don't know if or when @evernat intends to merge this work to master and include it in a release, nor do I know what's blocking it. I've been trying to get this done for 7 months but @evernat is very slow to respond and I'm honestly not sure how to progress this. I'm starting to think CSP will simply never happen in JavaMelody, despite my dedication to it. :-(

evernat · 2021-06-24T21:06:23Z

Generally speaking, you could note that my time to work on that is not infinite. And that it could not be merged if there are known issues or if after many reviews / fixes, review for merge is needed to be sure to avoid blocking regressions for a lot of users. See PR #1012 for example, where fixes of about one line of code out of five has a review, just to scratch the surface. In this PR #966 thanks to @candrews, work and changes were risky and big as the needed reviews. You may look at other major opensource projects, where most issues are not answered for years and closed without explanation, to compare with javamelody.
And I think I will have more time to work on that, starting 10 days from now.

evernat · 2021-07-09T23:13:37Z

...dy-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlCacheInformationsReport.java

-					+ "' onclick=\"javascript:return confirm('"
-					+ getStringForJavascript("confirm_purge_caches") + "');\">");
+			writeln("<a class='confirm' href='?action=clear_caches" + getCsrfTokenUrlPart()
+					+ "' data-confirm=\"" + I18N.htmlEncode("confirm_purge_caches", false, false)


The message is not translated anymore here.

evernat · 2021-07-09T23:18:39Z

...dy-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlCacheInformationsReport.java

 					writeDirectly(cacheNameEncoded);
 					writeDirectly("&amp;cacheKey=");
 					writeDirectly(urlEncode(myKey));
 					writeDirectly(csrfTokenUrlPart);
-					writeDirectly("' onclick=\"javascript:return confirm('");
+					writeDirectly("' data-confirm\"");


= is missing here

evernat · 2021-07-09T23:31:51Z

...y-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlJCacheInformationsReport.java

 					writeDirectly(confirmClearCache);
-					writeDirectly("');\">");
+					writeDirectly("\");\">");


\"); should not be here

evernat · 2021-07-11T13:13:44Z

...ody-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlJavaInformationsReport.java

-		write(id);
+		write("'>");
+		write("<em><img src='?graph=" + graph + "&width=100&height=50' id='");
+		write("id" + graph);
 		write("' alt='graph'/></em>");


after this change, these images are no longer lazy loaded when onmouseover

evernat · 2021-07-11T14:00:35Z

javamelody-core/src/main/resources/net/bull/javamelody/resource/monitoring.js

+	  });
+
+	  $$('.deploymentPeriod').invoke("observe", "click", function(event){
+		showHide('customPeriod');


if this was used, it would have displayed the custom period form instead of the deployment period form

evernat · 2021-07-11T14:12:07Z

javamelody-core/src/main/resources/net/bull/javamelody/resource/monitoring.js

+		var test = document.createElement('input'); test.type = 'date';
+		if(test.type === 'text') {
+		  document.customPeriodForm.pattern.value = '';
+		  document.getElementById('customPeriodPattern').style.display='inline';


it would have been better to not remove comments on this code

evernat · 2021-07-11T14:17:33Z

javamelody-core/src/main/resources/net/bull/javamelody/resource/monitoring.js

+		      return false;
+		   }
+		   if (periodForm.endDate.value.length == 0) {
+		      alert('Dates are mandatory');


This is no longer translated. Moreover the translations for "Dates are mandatory" are left in properties files for nothing.

evernat · 2021-07-11T14:43:28Z

javamelody-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlReport.java

@@ -212,62 +210,24 @@ private void writeHtmlHeader(boolean includeSlider, boolean includeCssInline)
 		if (includeSlider) {
 			writeln("<script type='text/javascript' src='?resource=slider.js'></script>");
 		}
+		writeln("<script type='text/javascript' src='?resource=monitoring.js'></script>");


when includeCssInline above, includes of prototype.js and monitoring.js are missing

evernat · 2021-07-11T15:40:46Z

javamelody-core/src/main/java/net/bull/javamelody/internal/web/CollectorController.java

-		writer.write("');location.href='");
-		writer.write(redirectTo);
-		writer.write("';</script>");
+		writeHtmlBegin(writer);


The same writeHtmlBegin(writer); is missing in the writeMessage method.

evernat · 2021-07-11T16:44:18Z

...lody-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlJobInformationsReport.java

+			final String endOnClickConfirm = "\">";
+			writeln("<a class='confirm' href='?action=pause_job&amp;jobId=all"
+					+ getCsrfTokenUrlPart() + onClickConfirm
+					+ I18N.htmlEncode(getString("confirm_pause_all_jobs"), false, false)
 					+ endOnClickConfirm);
 			writeln("<img src='?resource=control_pause_blue.png' width='18' height='18' alt=\"#Pause_all_jobs#\" /> #Pause_all_jobs#</a>");
 			writeln("&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;");
 			writeln("<a href='?action=resume_job&amp;jobId=all" + getCsrfTokenUrlPart()


class='confirm' is missing here

evernat · 2021-07-11T16:47:17Z

...lody-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlJobInformationsReport.java

-		final String onClickConfirm = "' onclick=\"javascript:return confirm('";
-		final String endOnClickConfirm = "');\">";
+		final String onClickConfirm = "' data-confirm=\"";
+		final String endOnClickConfirm = "\">";
 		writeln("<a href='?action=pause_job&amp;jobId=" + jobInformations.getGlobalJobId()


class='confirm' is also missing here

evernat · 2021-07-11T16:47:31Z

...lody-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlJobInformationsReport.java

 		writeln("<a href='?action=pause_job&amp;jobId=" + jobInformations.getGlobalJobId()
 				+ getCsrfTokenUrlPart() + onClickConfirm
-				+ getStringForJavascript("confirm_pause_job") + endOnClickConfirm);
+				+ I18N.htmlEncode(getString("confirm_pause_job"), false, false)
+				+ endOnClickConfirm);
 		writeln("<img src='?resource=control_pause_blue.png' width='18' height='18' alt=\"#Pause_job#\" title=\"#Pause_job#\" /></a>");
 		write("</td> <td align='center' class='noPrint'>");
 		writeln("<a href='?action=resume_job&amp;jobId=" + jobInformations.getGlobalJobId()


class='confirm' is also missing here

evernat · 2021-07-11T23:38:13Z

...melody-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlHeapHistogramReport.java

-					+ getStringForJavascript("ramasse_miette_desactive") + "');return false;\">");
+			writeln("<a class='alert' href='?part=heaphisto&amp;action=gc" + getCsrfTokenUrlPart()
+					+ "' data-alert=\""
+					+ I18N.htmlEncode(getString("ramasse_miette_desactive"), false, false) + "\">");


alert is not displayed instead of calling the action

by lack of case in monitoring.js

evernat · 2021-07-11T23:53:52Z

javamelody-core/src/main/java/net/bull/javamelody/internal/web/html/HtmlCoreReport.java

-			write("<a href='?action=gc" + getCsrfTokenUrlPart() + "' onclick=\"javascript:alert('"
-					+ getStringForJavascript("ramasse_miette_desactive") + "');return false;\">");
+			write("<a class='alert' href='?action=gc" + getCsrfTokenUrlPart() + "' data-alert=\""
+					+ I18N.htmlEncode(getString("ramasse_miette_desactive"), false, false) + "\">");


alert is not displayed here neither instead of calling the action

@candrews

based on PR #966 by @candrews + fixes

@candrews

based on PR javamelody#966 by @candrews + fixes

candrews force-pushed the content-security-policy branch from a50dc6c to d0cf7cb Compare November 24, 2020 20:32

evernat reviewed Dec 2, 2020

View reviewed changes

javamelody-core/src/main/java/net/bull/javamelody/Parameter.java Outdated Show resolved Hide resolved

candrews added 6 commits December 18, 2020 18:10

lightwindow.js: use about:blank instead of javascript:false

1d527fe

about:blank works for all browsers and doesn't use inline javascript therefore doesn't violate a content security policy which prohibits inline script.

resizable_tables.js: don't use innerHTML

b7482dc

innerHTML is unsafe; use document.createElement to create elements then set attributes on them.

RUM Injector: don't use inline script to run BOOMR.init

eb520aa

Move the BOOMR.init call to boomerang.min.js and include boomerang.min.js using a script tag. Use attributes on the script tag to pass necessary data to the script which is used to call BOOMR.init.

Only run PROBLEMATIC_ATTRIBUTE_READING check if IE

14ce7ef

Avoid a CSP inline script violation. See: prototypejs/prototype#320

Eliminate all inline script and styles

0330587

Move all inline CSS and inline Javascript to monitoring.css and monitoring.js

Remove no longer used methods

55a5ab7

Since no javascript is dynamically generated, methods to escape javascript are no longer necessary or used.

candrews force-pushed the content-security-policy branch from d0cf7cb to 2a643c5 Compare December 18, 2020 23:11

candrews added 3 commits March 9, 2021 19:48

Add a configuration option for CSP header (enabled by default)

11799d2

In the database reports, fix the dropdown that selects the report

9bc255d

Don't showHide twice

74d97c6

evernat changed the base branch from master to content-security-policy April 5, 2021 19:33

evernat merged commit a5b35f1 into javamelody:content-security-policy Apr 5, 2021

evernat added a commit that referenced this pull request Apr 14, 2021

externalize some css and js, based on PR #966 by @candrews

9f3baa3

evernat reviewed Apr 14, 2021

View reviewed changes

evernat reviewed Jul 9, 2021

View reviewed changes

evernat reviewed Jul 11, 2021

View reviewed changes

evernat mentioned this pull request Jul 11, 2021

follow up to prepare Content-Security-Policy, based on #966 #1030

Merged

evernat reviewed Jul 11, 2021

View reviewed changes

evernat added a commit that referenced this pull request Jul 12, 2021

follow up to prepare Content-Security-Policy (#1030)

0667afb

based on PR #966 by @candrews + fixes

evernat mentioned this pull request Jul 12, 2021

Enable content security policy by default #1031

Merged

goldyliang pushed a commit to goldyliang/javamelody that referenced this pull request Mar 17, 2022

externalize some css and js, based on PR javamelody#966 by @candrews

07780da

goldyliang pushed a commit to goldyliang/javamelody that referenced this pull request Mar 17, 2022

follow up to prepare Content-Security-Policy (javamelody#1030)

6bbb36a

based on PR javamelody#966 by @candrews + fixes

Implement Content Security Policy, including moving all CSS and JS to external files #966

Implement Content Security Policy, including moving all CSS and JS to external files #966

Conversation

candrews commented Nov 24, 2020

candrews commented Nov 30, 2020

evernat commented Dec 2, 2020

candrews commented Dec 7, 2020

evernat commented Dec 9, 2020

candrews commented Dec 9, 2020

evernat commented Dec 9, 2020

evernat commented Dec 9, 2020

evernat commented Dec 9, 2020 • edited Loading

evernat commented Dec 9, 2020

candrews commented Dec 18, 2020

candrews commented Jan 4, 2021

candrews commented Jan 11, 2021

candrews commented Jan 19, 2021

evernat commented Jan 24, 2021

candrews commented Jan 31, 2021

candrews commented Feb 13, 2021

candrews commented Feb 21, 2021

evernat commented Mar 7, 2021 • edited Loading

evernat commented Apr 5, 2021

candrews commented Apr 7, 2021

evernat commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srsouto commented Jun 21, 2021

tonylulciuc commented Jun 23, 2021 • edited Loading

candrews commented Jun 23, 2021

evernat commented Jun 24, 2021

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

evernat Jul 11, 2021 • 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

evernat commented Dec 9, 2020 •

edited

Loading

evernat commented Mar 7, 2021 •

edited

Loading

tonylulciuc commented Jun 23, 2021 •

edited

Loading

evernat Jul 11, 2021 •

edited

Loading