adobe / aem-core-wcm-components

Standardized components to build websites with AEM.
https://docs.adobe.com/content/help/en/experience-manager-core-components/using/introduction.html
Apache License 2.0
747 stars 753 forks source link

[Form] Simplify dispatcher whitelisting of form POST requests #1142

Open kwin opened 4 years ago

kwin commented 4 years ago

Currently in https://github.com/adobe/aem-core-wcm-components/blob/3453838f52ab2ec2b5fe6052d7b1a9ab24404e3e/content/src/content/jcr_root/apps/core/wcm/components/form/container/v2/container/container.html#L18 the action is being retrieved from https://github.com/adobe/aem-core-wcm-components/blob/3453838f52ab2ec2b5fe6052d7b1a9ab24404e3e/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/models/v1/form/ContainerImpl.java#L121 which IMHO just points to the current page url. The selector form is not used for which https://github.com/adobe/aem-core-wcm-components/blob/3453838f52ab2ec2b5fe6052d7b1a9ab24404e3e/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/CoreFormHandlingServlet.java#L56 is registered. The servlet filter implemented by https://github.com/adobe/aem-core-wcm-components/blob/3453838f52ab2ec2b5fe6052d7b1a9ab24404e3e/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/CoreFormHandlingServlet.java#L123 does some internal request dispatching then to trigger the servlet.

This architecture makes it complicated to just enable certain request paths for POST in the dispatcher (like outlined in https://docs.adobe.com/content/help/en/experience-manager-dispatcher/using/configuring/dispatcher-configuration.html#example-filter-enable-post-requests, which relies on the selector form). What is the reason why the form action just points to the page itself but not to the container component with the specific selector? That way also the filter could be limited to only filter certain requests!

kwin commented 4 years ago

It would be great to have the request architecture laid out in https://github.com/adobe/aem-core-wcm-components/blob/master/content/src/content/jcr_root/apps/core/wcm/components/form/container/v2/container/README.md

stefanseifert commented 4 years ago

the magic in com.day.cq.wcm.foundation.forms.FormsHandlingServletHelper works like this:

in recent AEM versions like AEM 6.5.5 this handling is blocked by default in the author instance by the CSRF filter, but it should work OOTB on publish instances.

in #56 there was a discussion about replacing all this foundation forms stuff with a modern implementation, i do not know when it will happen. the roadmap lists several entries around improving the form support.

kwin commented 4 years ago

@stefanseifert Thanks for the hints. The main problem with relying on INTERNAL(!) request dispatching is that it is impossible to whitelist those request properly in the dispatcher with this approach: https://docs.adobe.com/content/help/en/experience-manager-dispatcher/using/configuring/dispatcher-configuration.html#example-filter-enable-post-requests Also the default dispatcher publish configuration in https://github.com/adobe/aem-project-archetype/blob/97692b5cef60fe9cbd18dd4cd7d193226610230c/src/main/archetype/dispatcher.ams/src/conf.dispatcher.d/filters/ams_publish_filters.any#L25 and https://github.com/adobe/aem-project-archetype/blob/97692b5cef60fe9cbd18dd4cd7d193226610230c/src/main/archetype/dispatcher.cloud/src/conf.dispatcher.d/filters/default_filters.any#L32 blocks all POSTs except for those having the selector form!

So in its current form this is incompatible with the dispatcher filter.

I don't really understand why the action can not target the component path of the container itself (and currently goes via the page path only).

ky940819 commented 4 years ago

Just a couple thoughts, Haven't tested any of them.

With the current set up, what will happen if there are multiple forms on a page? Sending the POST request to the specific resource and including the .form. selector would resolve the above issues.

That said, only whitelisting POST requests to page or resource paths with a .form. selector from the dispatcher doesn't really help all that much, since if there was malicious intent then you could add the .form. selector to any page.

Something I haven't tried, but have a lot of interest in for an upcoming project, is how well forms work inside of an XF. Do they work properly once embedded in the page? Do they post to the current page instead of the resource page? Do all of the form utilities that are used work properly with XFs?

If they do work, then an interesting approach would be to

  1. Create a folder along the lines of /content/experience-fragments/forms
  2. Create an XF template that has a structural or initial content of a form container (depending on your requirements)
  3. Create an XF wrapper component that edits the dialog to only pick XF's from the /content/experience-fragments/forms (optional if you currently allow authors to place any XF they want already, useful if you don't allow that so you want a restricted XF component to allow this one very specific use case)
  4. Create your form XF's
  5. Reference them on the page using the newly created "Form XF" component (or standard XF component).
  6. whitelist POST to /content/experience-fragments/forms/*
ky940819 commented 4 years ago

Just a follow up to this. I took the time today to test using XFs for forms.

In general it does work. There is one slight snag to the solution i proposed above. The form submits to the currentPage instead of the resourcePage. This means that once the form XF is embedded in the page, you must allow POST requests everywhere on your site.

This behavior can be corrected by simply changing currentPage -> resourcePage in the form/ContainerImpl.java model. Is this a change that we want to make? It would have no impact on anyone who is currently using the Form Container on a standard page; however, it would change the action URL for anyone who happens to have a Form Container used in an XF or as part of a template structure.

bartoszWesolowski commented 3 years ago

Hello @gabrielwalt , any updates here ? Isn't it a critical issue that makes this feature not production ready ?