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

BUGFIX: Configurable uriPathSuffix in EventSourcedFrontendNodeRoutePa… #5189

Open
wants to merge 3 commits into
base: 9.0
Choose a base branch
from

Conversation

samsauter
Copy link

Only for Neos 9.

Author & Contributor @sjsone

With this PR it is possible to use a different uriPathSuffix than the one configured in Neos.Neos.sitePresets.

The Routes.yaml config has to be "dynamic", e.g.

-
  name: 'Neos :: Frontend :: Document node with json format'
  uriPattern: '{node}'
  defaults:
    '@package': Neos.Neos
    '@controller': Frontend\Node
    '@action': show
    '@format': json
  routeParts:
    node:
      handler: Neos\Neos\FrontendRouting\FrontendNodeRoutePartHandlerInterface
      options:
        uriPathSuffix: '.json'
  appendExceedingArguments: true

So uriPattern: '{node} instead of uriPattern: '{node}.json'.

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@samsauter
Copy link
Author

for the sake of completeness, we talked about this in the neos-general Slack channel https://neos-project.slack.com/archives/C050C8FEK/p1722245234573379

@@ -117,7 +117,7 @@ class NodeController extends ActionController
* @Flow\SkipCsrfProtection We need to skip CSRF protection here because this action could be called
* with unsafe requests from widgets or plugins that are rendered on the node
* - For those the CSRF token is validated on the sub-request, so it is safe to be skipped here
*/
*/
Copy link
Member

Choose a reason for hiding this comment

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

A fluke?

* - For those the CSRF token is validated on the sub-request, so it is safe to be skipped here
* We need to skip CSRF protection here because this action could be called with unsafe requests from widgets or plugins that are rendered on the node - For those the CSRF token is validated on the sub-request, so it is safe to be skipped here
* @Flow\SkipCsrfProtection
* @Flow\IgnoreValidation("node")
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this should be needed – IIRC IngoreValidation should be only needed for object arguments!? What happens if you omit this?

Copy link
Author

@samsauter samsauter Aug 12, 2024

Choose a reason for hiding this comment

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

If I submit a form which redirects to a node (e.g. via Neos.Form or a Neos.Form:Builder rendered form) without this change, the request ends in a 404 and I get the error in the logs CSRF: token was empty but a valid token is required for Neos\Neos\Controller\Frontend\NodeController::showAction().

Copy link
Member

Choose a reason for hiding this comment

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

That should be solved by the SkipCsrfProtection, the IgnoreValidation is also necessary and gives you the same error otherwise?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, exactly @kitsunet. Without the IgnoreValidation, I get the mentioned error.

Copy link
Member

Choose a reason for hiding this comment

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

i wonder if the problem here is in any way possibly related to #4909 ? ill have to experiment with this myself

@mhsdesign
Copy link
Member

The core change (the uri path options thing) will be fixed with #5224. For the other changes we can keep this pr and its discussion open :)

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

Successfully merging this pull request may close these issues.

4 participants