Skip to content

Prevent HTML injection #107

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

Merged
merged 2 commits into from
Dec 23, 2024
Merged

Prevent HTML injection #107

merged 2 commits into from
Dec 23, 2024

Conversation

AuditeMarlow
Copy link
Contributor

@AuditeMarlow AuditeMarlow commented Jun 27, 2024

Unfortunately these changes also mean that things like var_dump(...) and dd(...) will not render on the page but instead will show up as plain text. I'm not sure if there is a way to get Web Tinker to behave the same way as the Tinker shell would inside of a terminal, meaning it'd use a different strategy for things like var_dump(...) and dd(...).

Closes #106.

I thought it'd be better to sanitize the output before hitting the
output modifiers as building output modifiers yourself will still
contain the HTML injections unless you specifically address this in your
output modifier.
@freekmurze freekmurze merged commit 2a0be1f into spatie:main Dec 23, 2024
@freekmurze
Copy link
Member

Thanks!

@xHeaven
Copy link
Contributor

xHeaven commented Dec 28, 2024

@freekmurze isn't it a better idea to just DOMPurify the output on the client, instead of straight up nuking the output on the server? That way dump and other styles would stay, but XSS would be filtered. See https://github.com/luminarix/laravel-web-tinker/pull/10/files

I'm not sure we should drop the styling in order to fix this issue, especially because this little note.

@freekmurze
Copy link
Member

@xHeaven That might be better, yeah. I'd be open for a PR that adds that.

@xHeaven
Copy link
Contributor

xHeaven commented Dec 30, 2024

@xHeaven That might be better, yeah. I'd be open for a PR that adds that.

Alright, I'll open a PR a bit later this afternoon reverting this change and implementing the HTML purifying on the client.

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

Successfully merging this pull request may close these issues.

HTML injection
3 participants