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

Consider encoding everything by default #42

Closed
samdark opened this issue Nov 25, 2020 · 14 comments
Closed

Consider encoding everything by default #42

samdark opened this issue Nov 25, 2020 · 14 comments

Comments

@samdark
Copy link
Member

samdark commented Nov 25, 2020

See yiisoft/yii2#18404. In Yii 3 we can change it safely.

@dicrtarasov
Copy link

dicrtarasov commented Nov 25, 2020

Good idea!
But also we need an option to disable encoding in some situations where content is generated HTML, for example labels with markup.

@samdark
Copy link
Member Author

samdark commented Nov 25, 2020

Yes. That is true.

@samdark samdark added status:ready for adoption Feel free to implement this issue. help wanted and removed status:under discussion labels Nov 25, 2020
@samdark
Copy link
Member Author

samdark commented Dec 28, 2020

Method Encode by default Can change Pattern
tag Simple tag content
beginTag Simple tag content
endTag Simple tag content
style Simple tag content
script Simple tag content
cssFile ✔️ Attribute
jsFile ✔️ Attribute
a Simple tag content
mailto Simple tag content
img ✔️ Attribute
label Simple tag content
button Simple tag content
submitButton Simple tag content
resetButton Simple tag content
input ✔️ Attribute
buttonInput ✔️ Attribute
submitInput ✔️ Attribute
resetInput ✔️ Attribute
textInput ✔️ Attribute
hiddenInput ✔️ Attribute
passwordInput ✔️ Attribute
fileInput ✔️ Attribute
textarea ✔️ That's an exception because textarea can't accept tags anyway
radio ✔️ Attribute
checkbox ✔️ Attribute
dropDownList ✔️ ✔️ List
listBox ✔️ ✔️ List
checkboxList ✔️ ✔️ List
radioList ✔️ ✔️ List
div Simple tag content
span Simple tag content
p Simple tag content
ul ✔️ ✔️ List
ol ✔️ ✔️ List
renderSelectOptions ✔️ ✔️ List
renderTagAttributes ✔️ Attribute

@samdark
Copy link
Member Author

samdark commented Dec 28, 2020

General pattern now:

  1. For simple tags don't encode content. No way to enable it, use Html::encode().
  2. Always encode attributes. No way to disable it.
  3. Encode list items by default, allow disabling it.

@samdark samdark added status:under discussion and removed help wanted status:ready for adoption Feel free to implement this issue. labels Dec 28, 2020
@samdark
Copy link
Member Author

samdark commented Dec 28, 2020

It is quite logical as it is and results in not that verbose syntax in templates. If we'll change it we'll have to turn encoding off for majority of simple tags all the time:

Html::p('This was a <strong>strong</strong> feeling.', ['encode' => false]);

That will likely result in:

  1. More secure templates output by default.
  2. People using HTML directly more.

Both are alright.

@samdark samdark self-assigned this Dec 29, 2020
@samdark samdark added status:under development Someone is working on a pull request. and removed status:under discussion labels Dec 29, 2020
@dicrtarasov
Copy link

dicrtarasov commented Dec 30, 2020

"no way to disable/enable it" sounds not good. Especially to disable, because when it disabled by default, we can use Html::encode. Why not adding boolean "encode" option, which alter default behavior?

Html::p('Some text', [
  'encode' => false
]);

Then use:

$encode = ArrayHelper::remove($options, 'encode', true);

Please, do not force encoding with no way to disable it.

@Mister-42
Copy link
Contributor

If you give a good use case to not encode attributes, I am sure it will be changed.

@dicrtarasov
Copy link

Sure, you can imagine good case, to not make things imposdible to disable.

@Mister-42
Copy link
Contributor

Nope, never came across one.

@samdark
Copy link
Member Author

samdark commented Jan 11, 2021

@dicrtarasov of course, we want it to be configurable i.e. you will be able to disable encoding.

@samdark
Copy link
Member Author

samdark commented Jan 11, 2021

@Mister-42 the use-case to disable encoding is when you need some HTML. For example,

Html::p('Hello, <strong>' . Html::encode($username). '</strong>!', [
  'encode' => false
]);

@Mister-42
Copy link
Contributor

Mister-42 commented Jan 11, 2021

@Mister-42 the use-case to disable encoding is when you need some HTML. For example,

Html::p('Hello, <strong>' . Html::encode($username). '</strong>!', [
  'encode' => false
]);

No dispute there, but as you were specific about 'always encode attributes' that is what I replied on. Your example also does not contain attributes and is as such not a use case for my question.

@samdark
Copy link
Member Author

samdark commented Jan 11, 2021

Ah, right. For attribute values I have no idea why disabling encoding might make sense. @dicrtarasov do you have any?

@dicrtarasov
Copy link

dicrtarasov commented Jan 11, 2021

Ah, right. For attribute values I have no idea why disabling encoding might make sense. @dicrtarasov do you have any?

I completely agree.
I wrote about tag contents

@samdark samdark added help wanted and removed status:under development Someone is working on a pull request. labels Jan 30, 2021
@samdark samdark removed their assignment Jan 30, 2021
@samdark samdark closed this as completed Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants