Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WICKET-7107 improved BaseWicketTester#WicketTesterServletWebResponse #846

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dr0ps
Copy link
Contributor

@dr0ps dr0ps commented Apr 25, 2024

WicketTesterServletWebResponse does implement IMetaDataBufferingWebResponse but fails to buffer header data. Headers are therefore lost in some cases. I took the provided test from WICKET-7107 and reduced it to the minimum required to demonstrate the issue. I also added the header buffering to BaseWicketTester#WicketTesterServletWebResponse.
I believe that this is an improvement but it ultimately falls short of actually testing wicket core functionality. It only tests the test infrastructure. Also, WICKET-7107 is still valid. In the quick start, the buffered response sent after the redirect does not contain the CSP header.

@dr0ps dr0ps marked this pull request as draft April 25, 2024 19:59
@dr0ps
Copy link
Contributor Author

dr0ps commented Apr 25, 2024

ResetResponseException$ResponseResettingDecorator#respond(RequestCycle) calls reset() on the response object. The current implementation in BaseWicketTester$WicketTesterServletWebResponse does nothing which is not representative of the implementation in BufferedWebResponse or ServletWebResponse. I therefore added the clearing of cookies and headers which makes the tests fail.
As the ResetResponseException then continues to call IRequestHandler#respond(RequestCycle) the onRequestHandlerResolved and onRequestHandlerExecuted in CSPRequestCycleListener will not be called. Therefore the CSP values are missing.

@dr0ps
Copy link
Contributor Author

dr0ps commented Apr 25, 2024

I replaced CSPRequestCycleListener#onRequestHandlerResolved and CSPRequestCycleListener#onRequestHandlerExecuted with onUrlMapped which fixes the tests. Is this a good idea?

Copy link
Contributor

@reiern70 reiern70 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the way to go. This seems to fix a previous ISSUE

https://issues.apache.org/jira/browse/WICKET-7028

But this I had to check by manual testing. IMHO, we should not invest more time on fixing corner cases and invest some time doing

https://issues.apache.org/jira/browse/WICKET-7040


@Override
public void onRequestHandlerExecuted(RequestCycle cycle, IRequestHandler handler)
public void onUrlMapped(RequestCycle cycle, IRequestHandler handler, Url url)
Copy link
Contributor

@reiern70 reiern70 May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried this change with

https://issues.apache.org/jira/browse/WICKET-7028

sample application and this change seems to be Ok. But to be honest I'm not sure this is the way we should go. See

https://issues.apache.org/jira/browse/WICKET-7040

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WICKET-7040 is never going to work, I think. Due to ajax requests, components can be instantiated long after the original page has been created and long after the original CSP has been sent to the browser. Additional CPSs could be delivered to the client as meta tags but weakening the original CSP is not allowed ( https://www.w3.org/TR/CSP3/#multiple-policies ). Therefore the strictest possible set of policies has to be known before the page is rendered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants