[MGNLUI-3863] After creating nodes with (invalid) characters, such as commercial/at/@ Magnolia apps might crash when addressing such nodes Created: 27/Apr/16  Updated: 11/Jul/16  Resolved: 11/Jul/16

Status: Closed
Project: Magnolia UI
Component/s: None
Affects Version/s: None
Fix Version/s: 5.4.8

Type: Bug Priority: Neutral
Reporter: Zdenek Skodik Assignee: Milan Divilek
Resolution: Fixed Votes: 0
Labels: devwl, support
Remaining Estimate: 0d
Time Spent: 0.5h
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLUI-322 Location fragment should be escaped Closed
relates to MGNLUI-3924 Pulse messages doesn't show for user ... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Sprint: Kromeriz 51
Story Points: 8

 Description   

Commercial at (@) is currently forbidden character in node name. Forbidden by Magnolia that is, for historical reasons, see more at https://documentation.magnolia-cms.com/display/DOCS/Managing+pages#Managingpages-Invalidcharacters.

Due to those characters being historically forbidden by Magnolia, there are various parts that do not handle such chars well as it was not required in the past. Said commercial character is however no longer treated consistently and most apps allow it's use as node or property name.

Without changing functionality of Magnolia where commercial is allowed on purpose (user subapp), we need to treat nodes with commercial consistently. Therefore:

  • AddNodeAction should validate label name and not create node with invalid name
  • SaveRoleDialogAction should create roles with valid node names
  • JcrContentConnector should not fail on NPE when node contains @
  • remove duplicated code between JcrContentConnector and JcrItemUtil

Once we MGNLUI-322 is resolved, commercial character can be enabled again for all apps, by simply removing it from the validation method and enabling it in all places at once.



 Comments   
Comment by Mikaël Geljić [ 08/Jul/16 ]

Hi Milan,

I agree, users can still be created with @ in node name. I was mislead by the change in AddNodeActionTest, see for yourself:

@Test
public void doesNotCreateNodeWithInvalidName() throws Exception {
    ...
    definition.setBaseName("pepa@pepa.cz");
    AddNodeAction action = new AddNodeAction(definition, new JcrNodeAdapter(root), eventBus);
    ...
    assertAddedNewNode(root, "pepa-pepa.cz", NodeTypes.Content.NAME, nodeCountBefore + 1);
}

However, few things which do still bother me:

1. I have no idea what the above *concretely* fixes.

  • Using an email address as baseName sounds hardly realistic
  • We never configure the baseName either, as far as demo can tell (and fallback is "untitled"); yet even if some silly guys want to do that, why shouldn't they? (more on that below)

2. Claim that @ is "invalid" is invalid

  • gotta love self-referencing our own docu without the rationale
  • this was only done for pages, or any content eventually mapped to URIs, just as Zdenek originally pointed out
  • as per that famous URL RFC 3986, an @ may be used in the authority component of the URI: [user-info@]host[:port]. Disclaimer: as this doesn't even apply to the path component, I'm even beginning to question why do it, but nevermind this one.
  • Yet NO, it does not apply to fragments either.

Quoting Zdenek again:

commercial at is a reserved char by URL specification

—This is correct per se. However terminology of character categories is highly important; reserved does not mean invalid, it simply defines an arbitrary range of characters, for reference in other component definitions. The reserved category is not part of the definition of the fragment URI component.

If you doubt this, here's the relevant chunk of the URI ABNF, mind the @ within the pchar def:

unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
pct-encoded   = "%" HEXDIG HEXDIG
sub-delims    = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="
pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
fragment      = *( pchar / "/" / "?" )

3. I agree all apps are already highly inconsistent towards sanitization via Path#getValidatedLabel.

  • I just can't see how less inconsistent we're making it when we explicitly put users out of the mix
  • and when I think we are currently overly sanitizing paths throughout all our content apps.

4. Zdenek filed the ticket, so that we consolidate this; as you mentioned, “The problem guys have on support was already fixed” elsewhere, so there was zero reason to rush that in. Besides the NPE stuff maybe, I don't see consolidation. Unlike for the escaping—which I didn't mention just for decoration, I'd happily have ditched this one based on false claims instead.

5. Last but not least, when will you start telling us the WHY in those commit messages, or in your previous reply? We can already figure out the WHAT by the diff.

Cheers,

Comment by Aleksandr Pchelintcev [ 09/Jul/16 ]

If I may propose something that might help with the issue: browser history api - this theoretically would allow us to use normal urls instead of fragments and also store as much stuff in the state as we want.

One question is the browser support though. If we don't need to maintain IE 10, then we seem to be covered (http://caniuse.com/#feat=history)

Comment by Jan Haderka [ 11/Jul/16 ]

1. I have no idea what the above concretely fixes.

Please do not reopen ticket just because you have no idea what it fixes. Talk to people and try to get understanding instead of just turning ticket into discussion forum.

2. Claim that @ is "invalid" is invalid
this was only done for pages

Actually this is where you are wrong. Until Magnolia 5.0 this character was consistently disallowed everywhere in names. That's also one of the reasons it being chosen for separation of node name from property name in the fragments at the time of 5.0 development.

Quoting Zdenek again:
commercial at is a reserved char by URL specification
—This is correct per se.

Reserved in URL, however not in URI, we have no issue here.

3. I agree all apps are already highly inconsistent towards sanitization via

And that is what the ticket is fixing. I've updated description to make it more clear.

4. Zdenek filed the ticket, so that we consolidate this; as you mentioned, “The problem guys have on support was already fixed” elsewhere, so there was zero reason to rush that in. Besides the NPE stuff maybe, I don't see consolidation.

The NPE you refer to, actually killed whole app or subapp in a way that forced you to close it and start again. That is serious enough to get the issue fixed. IF you have problem with prioritization of the tickets, please talk to me or Michi.

One question is the browser support though. If we don't need to maintain IE 10

No, IE 10 is no longer supported.

Generated at Mon Feb 12 09:10:51 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.