[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: |
|
||||||||||||
| 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:
Once we |
| 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.
2. Claim that @ is "invalid" is invalid
Quoting Zdenek again:
—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.
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 ] |
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.
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.
Reserved in URL, however not in URI, we have no issue here.
And that is what the ticket is fixing. I've updated description to make it more clear.
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.
No, IE 10 is no longer supported. |