Heading partial

[jmsfwk]

WHY

BEFORE - What was wrong? What was happening before this PR?

The header was vulnerable to cross site scripting. #5839

AFTER - What is happening after this PR?

The header escapes its contents, but can also be published if the developer wants to restore the previous behaviour.

HOW

How did you achieve that, in technical terms?

• Extracted a header template include and replace its uses
• Introduced a subtitle_fallback variable that can be passed from the including template

Is it a breaking change?

Yes. On their next upgrade users might want to adjust any published templates.

How can we test the before & after?

Set an entity name to an XSS attack.

$this->crud->setEntityNameStrings('', '<img src="x" onerror="alert(\'XSS\')">');

View the edit page for then entity, it should show the HTML rather than triggering a JavaScript alert.

[welcome]

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

• if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
• even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
• not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
• we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!

--
Justin Case
The Backpack Robot

[pxpm]

Hey @jmsfwk thanks for the PR.

This is a major breaking change, something that we are not willing to do at this stage of v7, unless it was something really really important (like we discussed in the issue you opened, I don't think this warrants such a breaking change like this).

I won't mind adding a way to escape the headers if that's a requirement, but this way it's a no no.

What do you think about an operation setting ?

Something like:

$this->crud->setOperationSetting('escapeHeaderText', true);

And in the header templates:

@php
$heading = isset($crud->getOperationSetting('escapeHeaderText')) && $crud->getOperationSetting('escapeHeaderText') ? e($crud->getHeading() ?? $crud->entity_name_plural) : ($crud->getHeading() ?? $crud->entity_name_plural);
$subheading = .... similar thing as above 
@endphp


<h1 class="text-capitalize mb-0" bp-section="page-heading">{!! $heading !!}</h1>
  <p class="ms-2 ml-2 mb-0" id="datatable_info_stack_{{$tableId}}" bp-section="page-subheading">{{ $crud->getSubheading() ?? '' }}</p>
</section>

What do you think ?

[jmsfwk]

Hi @pxpm

I think introducing a setting is way more complicated than using the basic functionality that is built into Laravel.

I would prefer to wait for a simpler solution in a future version rather than introducing a setting quickly that (ideally) would need to be removed in the future.

[pxpm]

Hey @jmsfwk I was just revisiting this with a fresh perspective, but I still agree with the old me.
Forcing the headers to be escaped are not "always" desired. Some developers might want to use emojis on their page headings and other kind of html formating. And that's totally fine, as we talked previously, you, the developer, can safely add html or anything you need into those headings, as long as you are not using "user" supplied input.
In case you do use user supplied input (and I think that's the niche use case, not the general use case), you should probably have a way to escape it without having to overwrite the files. We agree on that 👍

So if you are willing to provide the PR like I proposed, or a similar approach (mainly that leaves freedom for the developers to decide about it), I would consider merging it into v7 👍

Once again, thanks for your work here, and for your inputs 🙏

Cheers