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

Particle Rework (Copyright, Social) #2214

Merged
merged 5 commits into from Jan 3, 2019

Conversation

Projects
None yet
3 participants
@thexmanxyz
Copy link
Contributor

thexmanxyz commented Jan 17, 2018

Copyright Particle:
I fixed the wrong namespace for link access within the copyright.html.twig because accessing the link information can not be achieved via particle.owner.link. The correct namespace is actually particle.link.

Social Particle:
I think that the Social Particle needs a rework and therefore I added now these changes to this pull request. I'm using these in my projects and it think they make sense in general:

  • The first thing i noticed is that we don't have an option to change the title and aria-label attribute other than setting the text. I extended the particle with a title field which is taken if title is set. Otherwise the default old behaviour is preserved (text is used).

  • Second the link target handling. I do not understand why there is a option for self which sets the target to _parent idk if this is a mistake. I cleared that up and now provide five options none, _self, _top, _parent and _blank with the correct namings.

  • The third thing is the default target behaviour of the links within the particle. We should finally switch them from _blank to none or at least _self. Because it's not contemporary anymore to use _blank. There are many suggestions to only use _blank in special situations and only then. Social links are non of them. Also it's a security issue even if not very likely in this case. I renounce to provide you with in deep information on this topic but I assume you are aware of this. It's just good practise to change this and if someone needs _blank it can still be changed.

  • Fourth add rel="noopener noreferrer" if _blank is selected for the target attribute.

PS.: sorry for the flood of commits but wanted the code to be clean ... should check better before pushing

@thexmanxyz thexmanxyz changed the title [Bug] Copyright Particle Fixed Particle Rework (Copyright, Social) Jan 23, 2018

@mahagr mahagr added the feature label Feb 19, 2018

@thexmanxyz thexmanxyz force-pushed the thexmanxyz:develop branch from 453d60b to 43e55d2 Sep 12, 2018

@thexmanxyz thexmanxyz force-pushed the thexmanxyz:develop branch from 43e55d2 to 48d048a Dec 29, 2018

@N8Solutions

This comment has been minimized.

Copy link

N8Solutions commented Jan 2, 2019

@thexmanxyz Thank you for creating this pull request! I was just looking at the Copyright particle this past weekend and wondering why the link was not working.

@thexmanxyz

This comment has been minimized.

Copy link
Contributor Author

thexmanxyz commented Jan 2, 2019

@N8Solutions np, I found it worth pushing because I noticed the bug myself as I used the Copyright Particle. The link namespace is simply wrong, not a big deal however. Hope that the PR is merged soon because it is already open quite long. @mahagr @newkind

@mahagr

This comment has been minimized.

Copy link
Member

mahagr commented Jan 3, 2019

I'd prefer using empty target ('') if there's no target to be set.

@mahagr mahagr merged commit 660a4d1 into gantry:develop Jan 3, 2019

@mahagr mahagr added this to the 5.4.28 milestone Jan 3, 2019

@thexmanxyz

This comment has been minimized.

Copy link
Contributor Author

thexmanxyz commented Jan 3, 2019

@mahagr First and foremost, thanks for merging. I'd prefer the empty target '' too it's sleeker. Just out of curiosity do we really need particle.target|e? Because we output the attributes |raw afterwards. So basically there is no need to escape at the point of constructing the attribute HTML. But I think that doesn't really matter. Just noticed within your applied commit 7735e5e.

@mahagr

This comment has been minimized.

Copy link
Member

mahagr commented Jan 4, 2019

You need to escape unsafe strings, meaning that if I generate HTML and use |raw filter afterwards.

@thexmanxyz

This comment has been minimized.

Copy link
Contributor Author

thexmanxyz commented Jan 4, 2019

@mahagr Do we really need HTML context escaping for a parameter that is only set to four predefined strings? I can't imagine myself a scenario where this might be unsafe or generate invalid HTML markup at all...I don't want to bug you I simply want to understand the intention. Because we also have |raw outputs everywhere in Particles - which is obviously necessary to provide certain functionality - so I don't see why this point might be exploitable or unsafe.

@mahagr

This comment has been minimized.

Copy link
Member

mahagr commented Jan 11, 2019

|raw should really not be used anywhere where there's (potentially) unfiltered content coming in, including any content in database and yaml files. The reason is that if you assume user input as safe, someone will find a way to eventually do XSS just because of the template may be used in an unexpected way, if not now, maybe in the future.

We're getting a lot of security reports for Grav Admin, just because of administrator can himself introduce XSS attacks. Yeah, we can ignore those in both Gantry and Grav, but they are still considered attacks. What if you allow someone limited access to the admin and he gets pissed off when he gets fired?

So basically all input which can be changed by any user, including administrator, needs to be escaped unless the content is specified as being HTML. And even in that case it should be filtered or reported if it has something potentially fishy in it.

@thexmanxyz

This comment has been minimized.

Copy link
Contributor Author

thexmanxyz commented Jan 11, 2019

@mahagr Thanks for your explanations I really appreciate it. I'm fully aware of the basics of string escaping, XSS attacks and why you should never ever allow user generated content to be written out unfiltered. Except the case you laid down yourself when the user has elevated rights or the role which allows him to modify or add stuff intentionally without a malicious intent.

But let me rephrase what I actually meant. We have in the same particle (ever had - even before my PR) fields that are written out |raw like here. As I said this is an obvious case because we want the option for the title field to be entered as pure HTML. That's fine. But also every person who tinkers with the system or at least wants to do some damage plus has the privileges to modify backend fields or the respective data files knows that and can simply exploit all these fields that are already there. And can even find them easily all over the place.

So I do understand what you are saying and I also understand that escaping more fields reduces the number of attack points. However if someone wants to do damage this won't stop the person and in this case it is even easier to take the input.text over a input.selectize. Because as long as there are |raw outputs which obviously is the case on many places...escaping a fixed value field doesn't add any real security benefits. I accept your decision and your intention (close as many doors as possible) but I see no real advantage when the same Particle has at least one field written out unfiltered as |raw. Yes it reduced a potentially risk a miniscule bit but not really help much in the discussed case.

@mahagr

This comment has been minimized.

Copy link
Member

mahagr commented Jan 14, 2019

Personally, I consider all of those |raw filters as likely security bugs if the content is coming from a user. And yeah, we should be checking and fixing all of them. As I said, nothing coming from the user should be having |raw filter on it and there should be no exceptions on that. If you want the user to be able to pass HTML, there should be a new filter that checks the input against XSS.

@thexmanxyz

This comment has been minimized.

Copy link
Contributor Author

thexmanxyz commented Jan 15, 2019

@mahagr You're totally right, the best way would be to completely eliminate all |raw outputs by applying an appropriate filter. I would like to say thanks for the very useful and constructive discussion on the topic. So basically there is already a specific filter just for HTML output planed? I assume this is not that trivial without cutting down functionality and still offering enough freedom. But TBH I don't know if there are already well established methods which work good for that case or even if symphony does offer a predefined filter for that scenario in the latest version - but as far as I know there is not.

@mahagr

This comment has been minimized.

Copy link
Member

mahagr commented Jan 21, 2019

I guess that you cannot really make a safe HTML filter, but we have already started to look into this by detecting if a page is using certain HTML tags and displaying a message in admin if they are. In my example using |raw filter is the correct way to do (because of the HTML is being generated in the template file).

@thexmanxyz

This comment has been minimized.

Copy link
Contributor Author

thexmanxyz commented Jan 23, 2019

@mahagr There are ways, one example is implemented by mail clients (e.g. Thunderbird) which only renders a reduced (light) set of HTML to prevent XSS and other malicious content that might be included in the message body. However, it would obviously not be appropriate at all occurrences of |raw because it would definitely break things depending on where it is used.

Good to know that there are upcoming changes that at least address the issue in the back-end by informing the maintainer on possible issues.

@mahagr

This comment has been minimized.

Copy link
Member

mahagr commented Jan 30, 2019

Not only that, but stripping html is really, really slow in PHP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.