You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2020/08/31 08:44:58 UTC
[GitHub] [struts] eozmen410 opened a new pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
eozmen410 opened a new pull request #434:
URL: https://github.com/apache/struts/pull/434
Hello Struts devs!
This PR is a follow up to [WW-5084: Add Content Security Policy support to Struts](https://github.com/apache/struts/pull/430) to make all Struts tags CSP ready. After our inital CSP implementation we realized that other Struts tags like `<s:doubleselect>`, `<s:head>` also include `<script>` or `<link/>` blocks, and we wanted to make sure enabling CSP will not compromise any of the functionality for the existing tags! Here's a summary of the changes we made:
* Modify the `UIBean` class to add the nonce value as a parameter so tags that need the nonce value can access it
* Add `nonce.ftl` and `<include />` it for tags that need the nonce attribute
* Modify the showcase JSP files to use `<s:script>` and `<s:link/>` instead of `<script>` and `<link/>`
* Add support for FreeMarker tags `<@s.script>` and `<@s.link>`
Co-authored-by: Ecenaz Jen Ozmen - @eozmen410
Co-authored-by: Giannis Chatziveroglou - @gchatz22
Co-authored-by: Santiago Diaz - @salcho
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] yasserzamani commented on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-690620638
Hi @JCgH4164838Gh792C124B5 , I've enabled integration tests only for JDK8's build (using STRUTS_IT=true variable in travis you can see) and it's the reason behind why it fails only. It really runs showcase app in an embedded jetty and tests the app via jwebunit real http requests/responses.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani commented on a change in pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on a change in pull request #434:
URL: https://github.com/apache/struts/pull/434#discussion_r486576988
##########
File path: core/src/main/resources/template/simple/nonce.ftl
##########
@@ -0,0 +1,23 @@
+<#--
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+-->
+<#if parameters.nonce?has_content>
+ nonce="${parameters.nonce}"<#rt/>
Review comment:
missed var and semicolon? i.e. shouldn't it be `var nonce="${parameters.nonce}";`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart merged pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
lukaszlenart merged pull request #434:
URL: https://github.com/apache/struts/pull/434
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-689135477
Hi. According to the Travis CI report, only the JDK 8 run failed (JDK 9 and 11 passed). There does not seem to be any changes in the PR that would pose a problem for JDK 8, so it seems weird.
Maybe @eozmen410 could confirm that their branch completes a local build using JDK 8 as well ? If a local JDK 8 build works (or fails), it might help narrow down a possible cause for the failure during the CI build.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart merged pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
lukaszlenart merged pull request #434:
URL: https://github.com/apache/struts/pull/434
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] salcho commented on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-691002860
Hi @JCgH4164838Gh792C124B5 ,
I've pushed a small commit that removes all the `<#rt>` tags as you mentioned. I agree that it's cleaner this way, thanks for that and thanks @yasserzamani for fixing the JDK issue 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] salcho commented on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-691002860
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls commented on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-690724356
[](https://coveralls.io/builds/33374295)
Coverage decreased (-0.002%) to 49.747% when pulling **f70999c401703c478b486c352978b1e50bf9a820 on salcho:post-ww-5083** into **6210afa747a67cabcddd6d72f675ed3d8023bda5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart edited a comment on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
lukaszlenart edited a comment on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-688055178
Looks good, yet some tests are failing - it looks like out integration tests cannot pass because an app cannot be started.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] salcho commented on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-691002860
Hi @JCgH4164838Gh792C124B5 ,
I've pushed a small commit that removes all the `<#rt>` tags as you mentioned. I agree that it's cleaner this way, thanks for that and thanks @yasserzamani for fixing the JDK issue 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-688055178
Looks good good, yet some tests are failing - it looks like out integration tests cannot pass because an app cannot be started.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-690724356
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-690724356
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] salcho commented on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-691002860
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-690724356
[](https://coveralls.io/builds/33388314)
Coverage increased (+0.02%) to 49.773% when pulling **4b8d7a15be8145f5dc921f8d4dad9451dbdd6035 on salcho:post-ww-5083** into **6210afa747a67cabcddd6d72f675ed3d8023bda5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-690724356
[](https://coveralls.io/builds/33388314)
Coverage increased (+0.02%) to 49.773% when pulling **4b8d7a15be8145f5dc921f8d4dad9451dbdd6035 on salcho:post-ww-5083** into **6210afa747a67cabcddd6d72f675ed3d8023bda5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #434:
URL: https://github.com/apache/struts/pull/434#issuecomment-690724356
[](https://coveralls.io/builds/33377186)
Coverage increased (+0.02%) to 49.773% when pulling **32bc34d797007009a6412533e9fbf9ecf6224edc on salcho:post-ww-5083** into **6210afa747a67cabcddd6d72f675ed3d8023bda5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #434: Refactoring Nonce Propagation in Struts Tags for CSP
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on a change in pull request #434:
URL: https://github.com/apache/struts/pull/434#discussion_r485180192
##########
File path: core/src/main/resources/org/apache/struts2/interceptor/debugging/console.ftl
##########
@@ -21,7 +21,9 @@
<!DOCTYPE html>
<html>
<head>
- <script type="text/javascript">
+ <script type="text/javascript" <#rt/>
+ <#include "/${parameters.templateDir}/simple/nonce.ftl" />
Review comment:
Should the changes being made in this PR be collapsed into single lines (where possible) to match the visual structure of the original tag(s) ?
For example in this case, utilize a single line such as:
`<script type="text/javascript" <#include "/${parameters.templateDir}/simple/nonce.ftl" /> >`
to replace the three lines would eliminate the need for extra `<#rt/>` tag processing in the template and produce a visual result closer to the original tag.
##########
File path: core/src/main/resources/template/css_xhtml/head.ftl
##########
@@ -18,5 +18,7 @@
* under the License.
*/
-->
-<link rel="stylesheet" href="<@s.url value='/struts/css_xhtml/styles.css' includeParams='none' encode='false' />" type="text/css" />
+<link
+ <#include "/${parameters.templateDir}/simple/nonce.ftl" />
Review comment:
And here:
`<link <#include "/${parameters.templateDir}/simple/nonce.ftl" /> rel="stylesheet" href="<@s.url value='/struts/css_xhtml/styles.css' includeParams='none' encode='false' />" type="text/css" />`
This sort of structure repeats in several of the PR's code changes, so those could be modified in a similar way if it makes sense.
##########
File path: core/src/main/resources/template/simple/form-close-tooltips.ftl
##########
@@ -24,7 +24,11 @@
--><#t/>
<#if (parameters.hasTooltip!false)><#t/>
<#lt/><!-- javascript that is needed for tooltips -->
- <#lt/><script type="text/javascript" src='<@s.url value="/struts/domTT.js" includeParams="none" encode="false" />'></script>
- <#lt/><link rel="stylesheet" type="text/css" href="<@s.url value="/struts/domTT.css" includeParams="none" encode="false" />"/>
+ <#lt/><script type="text/javascript" src='<@s.url value="/struts/domTT.js" includeParams="none" encode="false" />'<#rt/>
Review comment:
If the `<#rt/>` tag structure is kept, should this one include a space to be consistent with the others ?
i.e. `/>' <#rt/>`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org