Default to escape true on all column types except markdown and custom_html

[tabacitu]

Some column types do NOT have "escape" to "true" by default. That means the output is not filtered for XSS, so if the admin stored some malicious JS code in the database, that code will be run for whoever previews it (perhaps another admin).

The same is true if the content did NOT come from the admin, but from something a user would input, outside Backpack. For example a contact form or something. A malicious user plugs in something like afafa"><img src=x onerror=confirm(1)> then when the admin previews it, that code will run if it's in any of the columns addressed in this PR.

Not sure how/whether we should be doing this... What do you guys think? It'd definitely be a breaking change, but I'm wondering if it's even worth doing. If it's a Backpack problem indeed. If we should add a warning, or default all columns to escaping. Or if any of the columns here actually need to not be escaped. I don't really remember why we chose to leave these ones unescaped by default...

[tabacitu]

I think we should do this. I think it's reasonable to default to secure, and then let the dev make it unsecure if they specifically want to. But it's also a breaking change. So I propose that we do this in 4.2, so that we can add a note in the upgrade guide about it.

But before we merge this into 4.2, let's take a little time to

• carefully consider how it would impact each column type, and if it's ok to default to "true" for each one of them;
• add warnings to these column types in the 4.1 docs, saying something like "As opposed to most other Backpack columns, this columns' output is NOT escaped by default. That means if the database value contains malicious JS, that JS will be run. If you do not trust what's shown by this column, please add the attribute "escaped" => true (or ->escaped(true)) to your column definition."

[tabacitu]

Update: just pushed Laravel-Backpack/demo@5ed0799 in the demo, these lines have to be commented out for the issue to be reproducible.

[pxpm]

@tabacitu

I think those columns were "unescaped" by default, because they were "unescaped" when we wanted to push the ability to escape them.

I DON'T agree with escaped by default, it would create some BC like in custom_html, closure or markdown for example where user would need to setup escaped => false to display the html, and be open to some XSS anyway.

I think the best defence would be to prevent those things to be stored inside the database, but I get that it's dificult to manage for example in a WYSIWYG editor.

Thing is that WYSIWYG editors would be unusable when escaping the output, as generaly they need to output html.

I don't get why would someone allow non admin users to add html to a database. If you are dealing with "user-admins" where they can create pages and content for them for example, I would rather use bb-code or something that could be "pre-parsed" before entering the database.

I would not merge this PR. Change my mind! 🙃

Best,
Pedro

[tabacitu]

I've gone through each column type and here are my thoughts on each one: I think it'll be OK to default to escaped on all of them except custom_html and markdown. That's because:

• 🟩 array - will probably not impact anyone
• 🟩 array_count - will probably not impact anyone
• 🟨 closure - most people probably don't put HTML inside the closure
• 🟥 custom_html - does not make sense when escaped
• 🟥 markdown - does not make sense escaped; we could strip_tags before it's passed to markdown, for better security;
• 🟨 model_function - most people probably don't put HTML inside it, but text
• 🟨 model_function_attribute - most people probably don't put HTML inside it, but text
• 🟩 relationship_count - will probably not impact anyone
• 🟨 textarea - will probably impact a lot of people, but it's a huge boost to default security

If you want a column to output HTML instead of text, you have to use the escaped attribute, period. Everything's safe by default, but you can make it unsafe if you want to.

That will be a breaking change for most people, but:

• it'll be a boost in default security (in case they haven't sanitized their inputs or outputs);
• it'll be an easy fix where needed (just add ->escaped(false) where you need it);
• it'll make it easy to understand which columns are escaped and which are not (EVERYTHING is escaped by default, except for custom_html which does not make sense escaped);
• the columns docs won't be littered by notices that CAREFUL, THIS IS COLUMN UNESCAPED, like 9 times;

What do you think now @pxpm ? I didn't make a very good argument, did I? 😅

[scrutinizer-notifier]

The inspection completed: No new issues

[tabacitu]

I've just double-checked, edited and added docs for this. I still think we need to do this. We can't assume the dev knows what he's doing in terms of security... And personally anything a software can do to force security on me... Even if I sometimes grumble... I appreciate it, I'm sloppy sometimes too.

However, @pxpm - I think you were right in this being too strict for some columns, thanks for the reality check, I've edited the PR and docs to address your concerns. You said:

it would create some BC like in custom_html, closure or markdown for example where user would need to setup escaped => false to display the html, and be open to some XSS anyway.

Which of course I agree with 😅 So I've decided to increase security for some columns, but not all columns. More exactly, I've increased security for everything except the columns that don't really make sense escaped (markdown and custom_html). I think this is a sweet spot:

• columns that can be used to output something other than HTML are now "secure by default" (eg. closure, model_function, model_function_attribute, textarea etc.); it will be inconvenient during the upgrade process to add 'escaped' => true in all places you output HTML, but it's also an opportunity to question if what that HTML is safe;
• columns that cannot be used escaped (custom_html and markdown) have big fat notices in the docs, that they should sanitize the input or output; I've included both a package recommendation and examples; I've even submitted a PR to that package to make it even easier to use, by just casting that attribute in your model to CleanHtml::class, CleanHtmlInput::class or CleanHtmlOutput::class; it's dead-simple now!

I've edited the PR so that it's:

• as easy as possible to upgrade (add 'escaped' => false in a few places where you output HTML);
• not inconvenient at all for people using 4.2 directly;

You also said:

I think the best defence would be to prevent those things to be stored inside the database, but I get that it's dificult to manage for example in a WYSIWYG editor.

I agree with that too. So I added notices that the user should sanitize the input/output when using those columns. I didn't add the notices in fields too because... you can probably trust the admin not to add malicious JS. But if you do create a form to edit user-introduced HTML, if you'd like the admin to see it... you have to use the textarea column. And it'll escape it. Then you'll run into the notice that you should sanitize the input/output. So we don't protect the user when building his app, but when he builds the admin panel for it, we remind him and recommend a security improvement.

@promatik what do you think about this? Good/bad? Sweet spot or not? Eager to merge this.

[promatik]

I agree with it — 100%. Security first, and if while upgrading backpack to v4.2 a column that used to be;

Value

becomes;

<b>Value</b>

The first thing that comes to a developer mind is "escaping", I think developers will easily figure out what's the problem. Also, since they are now forced to use the "escaped" => false in those cases, it will enforce devs to use it only when it's needed and that is a good security behavior.

[promatik]

@tabacitu can we please avoid this change here?

It is also done here; #3936, that way we avoid conflicts.