You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by "reiern70 (via GitHub)" <gi...@apache.org> on 2023/04/05 11:49:11 UTC
[GitHub] [wicket] reiern70 opened a new pull request, #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
reiern70 opened a new pull request, #569:
URL: https://github.com/apache/wicket/pull/569
do CSP related header generation both at onRequestHandlerResolved and onRequestHandlerExecuted
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] reiern70 commented on pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on PR #569:
URL: https://github.com/apache/wicket/pull/569#issuecomment-1498557849
> Not beautiful, but it seems to work.
>
> Maybe let #onRequestHandlerResolved() and #onRequestHandlerExecuted() both delegate to a private method #protect() ... the way the one calls the other now I find confusing.
This sounds like a goog idea. I will do and push those changes and merge. After that I can try to explore the other idea you proposed (that seems less fragile that doing it this way). Wondering if we should also add some tests covering actual generation of headers for these corrner cases.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] solomax commented on a diff in pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "solomax (via GitHub)" <gi...@apache.org>.
solomax commented on code in PR #569:
URL: https://github.com/apache/wicket/pull/569#discussion_r1159399210
##########
wicket-core/src/main/java/org/apache/wicket/coep/CrossOriginEmbedderPolicyRequestCycleListener.java:
##########
@@ -36,15 +36,15 @@
* <code>Cross-Origin-Embedder-Policy</code>. The header is not set for the paths that are exempted
* from COEP. The only valid value of COEP is <code>require-corp</code>, so if the listener is
* enabled the policy value will be specified as so.
- *
+ * <p>
Review Comment:
I believe, this one will produce JavaDoc warning :(
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] reiern70 commented on a diff in pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on code in PR #569:
URL: https://github.com/apache/wicket/pull/569#discussion_r1159427006
##########
wicket-core/src/main/java/org/apache/wicket/coep/CrossOriginEmbedderPolicyRequestCycleListener.java:
##########
@@ -36,15 +36,15 @@
* <code>Cross-Origin-Embedder-Policy</code>. The header is not set for the paths that are exempted
* from COEP. The only valid value of COEP is <code>require-corp</code>, so if the listener is
* enabled the policy value will be specified as so.
- *
+ * <p>
Review Comment:
> you are right :) no specific warning at this line, sorry for the noise :(
No problem. Everything that makes wicket code better is welcomed :-)
>
> I'm not using IJ, I believe checkstyle and javadoc maven plugins can be configured to be more strict
Oh! I didn't check the result of those. Just that code build with maven clean install on terminal in my linux box.
At work we have the same "warnings war" because some people use IJ and other use eclipse and we use gradle instead of maven and it is very diffcult to get all the parties agree about what a warning is :-(
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] svenmeier commented on pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "svenmeier (via GitHub)" <gi...@apache.org>.
svenmeier commented on PR #569:
URL: https://github.com/apache/wicket/pull/569#issuecomment-1497413462
Can't check ATM, but don't we have to check for duplicated headers then?
I wonder whether we should go with a completely different solution:
Aren't the headers specific to the page? We could add a behavior to the page (via Component instantiation/Initialization listener) and thus always add the header when the page is rendered.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] reiern70 commented on pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on PR #569:
URL: https://github.com/apache/wicket/pull/569#issuecomment-1497419624
> Can't check ATM, but don't we have to check for duplicated headers then?
>
Maybe yes... but they do not seem to be duplicated. I do agree thsi solution "sucks"
> I wonder whether we should go with a completely different solution: Aren't the headers specific to the page? We could add a behavior to the page (via Component instantiation/Initialization listener) and thus always add the header when the page is rendered.
Do you mean...
1. The setting register a compoent instantiation listener
2. The listener adds a behavior to teh page that adds these headers?
This idea could work and it maybe be cleaner that what we have now. Mind that teh same problem we have for CrossOriginEmbedderPolicyRequestCycleListener
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] svenmeier commented on pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "svenmeier (via GitHub)" <gi...@apache.org>.
svenmeier commented on PR #569:
URL: https://github.com/apache/wicket/pull/569#issuecomment-1497926276
Not beautiful, but it seems to work.
Maybe let #onRequestHandlerResolved() and #onRequestHandlerExecuted() both delegate to a private method #protect() ... the way the one calls the other now I find confusing.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] reiern70 commented on a diff in pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on code in PR #569:
URL: https://github.com/apache/wicket/pull/569#discussion_r1159405132
##########
wicket-core/src/main/java/org/apache/wicket/coep/CrossOriginEmbedderPolicyRequestCycleListener.java:
##########
@@ -36,15 +36,15 @@
* <code>Cross-Origin-Embedder-Policy</code>. The header is not set for the paths that are exempted
* from COEP. The only valid value of COEP is <code>require-corp</code>, so if the listener is
* enabled the policy value will be specified as so.
- *
+ * <p>
Review Comment:
:-( I had a warining locally... And this fixed it. DO we have some IJ settings I could use as default for wicket?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] solomax commented on a diff in pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "solomax (via GitHub)" <gi...@apache.org>.
solomax commented on code in PR #569:
URL: https://github.com/apache/wicket/pull/569#discussion_r1159414745
##########
wicket-core/src/main/java/org/apache/wicket/coep/CrossOriginEmbedderPolicyRequestCycleListener.java:
##########
@@ -36,15 +36,15 @@
* <code>Cross-Origin-Embedder-Policy</code>. The header is not set for the paths that are exempted
* from COEP. The only valid value of COEP is <code>require-corp</code>, so if the listener is
* enabled the policy value will be specified as so.
- *
+ * <p>
Review Comment:
you are right :)
no specific warning at this line, sorry for the noise :(
I'm not using IJ, I believe checkstyle and javadoc maven plugins can be configured to be more strict
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] reiern70 commented on pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on PR #569:
URL: https://github.com/apache/wicket/pull/569#issuecomment-1497355736
@svenmeier ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] reiern70 commented on pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on PR #569:
URL: https://github.com/apache/wicket/pull/569#issuecomment-1497439735
> Can't check ATM, but don't we have to check for duplicated headers then?
>
> I wonder whether we should go with a completely different solution: Aren't the headers specific to the page? We could add a behavior to the page (via Component instantiation/Initialization listener) and thus always add the header when the page is rendered.
There is no need to check for duplication it seems.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] reiern70 merged pull request #569: [WICKET-7028] do CSP related header generation both at onRequestHandl…
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 merged PR #569:
URL: https://github.com/apache/wicket/pull/569
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org