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

HTML: addChild() & addChildren() #223

Open
dakur opened this issue May 25, 2020 · 4 comments
Open

HTML: addChild() & addChildren() #223

dakur opened this issue May 25, 2020 · 4 comments

Comments

@dakur
Copy link

dakur commented May 25, 2020

▸ Is your feature request related to a problem? Please describe.

There is no intuitive way to nest element and nest them safely. I can use one of following methods:

  • Html::el('ul')->addHtml(Html::el('li'))
    • the problem is, that addHtml()'s parameter accepts string as well which opens it for potential XSS issues
  • Html::el('ul')->create('li')
    • safe one, but not much intuitive and requires a loop in case of nesting multiple elements into one parent

▸ Explain your intentions.

I suggest to create addChild(Html $child) and addChildren(Html[] $children) methods on Html objects. As it accepts only Html instances it is safe and its name also goes with industry standard.

▸ It's up to you to make a strong case to convince the project's developers of the merits of this feature.

I did above.

I can make a pull request if you agree on this proposal as well.

@jkuchar
Copy link
Contributor

jkuchar commented May 26, 2020

When passing Html object as an parameter there is no space for XSS. If you do not trust input, you can use ->addText() which also accepts Html. This means escaping for string and no escaping for passed Html object.

@dakur
Copy link
Author

dakur commented May 26, 2020

@jkuchar My point is that if you pass a variable into addHtml(), you can not be sure if there is not an unsanitized string by mistake and as addHtml() does not typecheck it is IHtmlString you have potential XSS in there.

@jkuchar
Copy link
Contributor

jkuchar commented May 26, 2020

I still do not see a point. I you are calling ->addHtml(), you are adding – well – HTML, so it is up to programmer to check this.

@dakur
Copy link
Author

dakur commented May 26, 2020

@jkuchar The point is that you want to use API which helps you to avoid working with unsafe content as much as possible. So if there is addHtml(IHtmlString $html), I would prefer it over addHtml(IHtmlString|string $html) so that I never by accident pass an unsafe string in there in future. And if I need to pass an unsafe string, I should have to call something like setDangerouslyInnerHtml in React – explicitely saying it is not safe.

As I do not want to make BCs, I do not propose to change the parameter type but create new method with strictly typed parameter.

But that is only part of the proposal. The second one is about nesting more children at once – addChildren() complement.

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

No branches or pull requests

2 participants