Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.08k stars 886 forks source link

[Bug] Error messages do not output real HTML but write it out #5142

Open tabacitu opened 1 year ago

tabacitu commented 1 year ago

Bug report

What I did

Accidentally triggered a 500 error after merging https://github.com/Laravel-Backpack/CRUD/pull/5125

What I expected to happen

Clean error message

What happened

Error message shows code blocks in text:

CleanShot 2023-06-26 at 14 34 29@2x
pxpm commented 1 year ago

@tabacitu I asked you before if we should remove the output escaping, since now we are not overwriting user-land errors, and we control the text/html displayed on the exceptions.

I think we can safely do it, but you didn't answered me, and I think this needs two brains judging.

Let me know.

Cheers

pxpm commented 1 month ago

I've been investigating this a bit more. I think for safety reasons and unpredictable scenarios, not escaping exceptions messages could potentially be a breaking change.

There are two ways we can "safely" display the un-escaped message: 1 - Continue using the abort() helper, but pass a special header like: backpack-internal-error indicating that is a "developer error" and not a general application error. 2 - use a custom backpack_abort() helper that throw a BackpackInternalException in a similar fashion to the proposed on 1.

I think 1) is enough for our needs and would require the less code changes. I've already did a small poc just to see the feasability: image

Let me know what you think @tabacitu

Cheers

tabacitu commented 1 month ago

Hmmm I don't really have a strong opinion in one direction or another. So go with your gut, Pedro!

tabacitu commented 1 month ago

(plus, we can easily change this if we change our minds, it wouldn't be a breaking change right?)