[MGNLSTK-1453] Do not allow selection of invalid redirection targets Created: 19/Dec/14  Updated: 20/Jan/16  Resolved: 04/May/15

Status: Closed
Project: Magnolia Standard Templating Kit (closed)
Component/s: templates
Affects Version/s: 2.8.6
Fix Version/s: 2.8.9

Type: Bug Priority: Major
Reporter: Lars Fischer Assignee: Christoph Meier
Resolution: Fixed Votes: 0
Labels: stk, support, uzh, validation
Σ Remaining Estimate: Not Specified Remaining Estimate: Not Specified
Σ Time Spent: Not Specified Time Spent: Not Specified
Σ Original Estimate: Not Specified Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLSTK-1521 Allow RedirectTargetValidator to cons... Closed
Sub-Tasks:
Key
Summary
Type
Status
Assignee
MGNLSTK-1520 Support external targets Sub-task Closed  
Template:
Acceptance criteria:
Empty
Date of First Response:

 Description   

The current stkRedirect template dialog does not validate the selected target path for redirection.

So it's possible to select the redirect page itself as target for redirection. This could cause an infinite loop problem.

The template dialog should validate if the selected path is different than the path of the current page.



 Comments   
Comment by Magnolia International [ 08/Apr/15 ]

What if the target is another page, which is itself also a redirect, and the target of the latter is the page you're currently editing ?
What I'm getting at is – do we really need this check, or should we have a broader safety net (somewhere else), or just let users make mistakes and ensure they can fix them ?

Comment by Christoph Meier [ 24/Apr/15 ]

All commits are on branch "MGNLSTK-1453" (see http://goo.gl/16O8qX).

Comment by Christoph Meier [ 24/Apr/15 ]

The validator assumes that both nodes current page and target page are both on website repository (which also indicates unique node paths) and considers the redirect path as invalid, if:
— It is the path of the redirecting page itself (pointing to itself).
— It leads to a redirection cascade which ends up in a "loop".
— It leads to a redirection cascade where the last redirection page has no path set and no child page. (Per default, the redirect target redirects to 1st child page, if no target path is set.)

Comment by Philip Mundt [ 28/Apr/15 ]

Small things only:

  • Variable redirecTemplateId should be called redirectTemplateId
  • Method getErrorMessage() has to be fromatted (according to coding conventions)
  • Would be nice to proper javadoc on method findCascadedRedirectionTarget(String, Session, String)
  • errorMessageType of type circularRedirection is a constant, "redirectTo404" isn't. Would be nice to use the same pattern.
  • I would not consider the validation result as "good" when there is a RepositoryException.
  • Minor: in method isValidValue(String targetPagePath) I wouldn't test for StringUtils.isEmpty(indirectRedirectionPath) but rather for indirectRedirectionPath == null
  • RedirectTargetValidatorTest:
    • Member variable private RedirectTargetValidator redirectTargetValidator is never used
    • Logger is never used
    • Try using org.junit.Rule & ExpectedException expectedException for the test cases where you expect an Exception + want to check the message
  • Tip: try using the final modifier throughout the code where possible
Comment by Christoph Meier [ 28/Apr/15 ]

Re-resolved according to review comments.
Using org.junit.Rule & org.junit.rules.ExpectedException also reduced the amount of tests.
Last commits together with precedings are squashed onto branch "MGNLSTK-1453-sq2" (see http://goo.gl/PNCJ2T).

Comment by Christopher Zimmermann [ 30/Apr/15 ]

Please integate on 2.9 branch.

Generated at Mon Feb 12 07:36:42 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.