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

prevent JavaScript injection with setValue() #30

Open
wants to merge 1 commit into
base: v24
Choose a base branch
from

Conversation

vt512
Copy link

@vt512 vt512 commented Apr 19, 2024

It is a bad idea to concatenate a JavaScript expression with a value that may be from a user input. The call of
autocommit.setValue("xy");alert("Hello World!"); leads to the alert window shown in the browser.

It is a bad idea to concatenate a JavaScript expression with a value that may be from a user input.
The call of
autocommit.setValue("xy\");alert(\"Hello World!");
leads to the alert window shown in the browser.
@thevaadinman
Copy link

It appears to me that there is no functional difference between the old code and the new.
Take a look at https://github.com/vaadin/flow/blob/dc3f6b9a9d390d3cd33797a69d61426ccf1f9495/flow-server/src/main/java/com/vaadin/flow/dom/Element.java#L1363 and notice that the argument is still just turned into a string and concatenated straight into an evaluated string, making it possible to cause code execution.

That said, this is a real issue; a proper fix would be to sanitize the input string using e.g. JSoup.

@vt512
Copy link
Author

vt512 commented Apr 19, 2024

I had tested the two versions executeJs and callJsFunction. With callJsFunction the alert was not displayed.

callJsFunction does only concatenate the functionName and calls scheduleJavaScriptInvocation() with the expression "return $0._setValue($1)". This is packed together with an array of the parameters (the value) to the client side. I don't see a string concatenation of the value.

That means: I don't think that sanitization is needed. Just exchange executeJs with callJsFunction and it is fine.

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.

2 participants