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

inputTextarea: lack of user input validation (maxlength) #4420

Closed
cnsgithub opened this issue Jan 14, 2019 · 10 comments
Closed

inputTextarea: lack of user input validation (maxlength) #4420

cnsgithub opened this issue Jan 14, 2019 · 10 comments
Labels
🔒 security Security related issue or enhancement
Milestone

Comments

@cnsgithub
Copy link
Contributor

1) Environment

  • PrimeFaces version: 6.3-snapshot

2) Expected behavior

p:inputTextarea maxlength must be validated at server side as well.

3) Actual behavior

maxlength is validated on client side only. Client side validation can be bypassed.

4) Steps to reproduce

In the modified showcase shown below, execute $("textarea[maxlength=10]").val("A".repeat(100)) in the console and press the Submit button. Set breakpoint in BasicView.setText and notice 100 characters being written to the model.

5) Sample XHTML

Add to inputTextarea.xhtml of showcase:

        <h:form>            
            <p:inputTextarea id="foo" value="#{basicView.text}" maxlength="10"/>
            <p:commandButton value="Submit" update="foo"/>
        </h:form>

6) Sample bean

BasicView available in showcase is used.

@cnsgithub
Copy link
Contributor Author

Oops, this problem seems to be more general: improper input validation of maxlength is neither restricted to p:inputTextarea component nor to PrimeFaces (since Mojarra 2.3.2 and MyFaces 2.3.2 fail as well when testing h:inputText maxlength server side validation). 😢

@tandraschko tandraschko added the 🔒 security Security related issue or enhancement label Jan 14, 2019
cnsgithub added a commit to cnsgithub/mojarra-ajax that referenced this issue Jan 14, 2019
cnsgithub added a commit to cnsgithub/mojarra-ajax that referenced this issue Jan 14, 2019
@cnsgithub
Copy link
Contributor Author

Opened an issue at MyFaces as well: https://issues.apache.org/jira/browse/MYFACES-4279

@cnsgithub
Copy link
Contributor Author

Opened an issue at Mojarra as well: eclipse-ee4j/mojarra#4530

tandraschko added a commit that referenced this issue Jan 15, 2019
…puttextarea

fix #4420 - inputTextarea: lack of user input validation (maxlength)
@tandraschko tandraschko added this to the 7.0 milestone Jan 15, 2019
@cnsgithub
Copy link
Contributor Author

cnsgithub commented Jan 15, 2019

Just for documentation purposes:

According the user guide, maxlength attribute is also exposed to the following components: p:autoComplete, p:calendar, p:inputMask, p:inputNumber, p:inputText, p:keyboard, p:password, p:spinner.

This often makes no sense and is probably due to auto-generation of documentation based on components' PropertyKeys that are inherited from default JSF components if we extend them.

However, p:inputText maxlength is a useful example where we would expect the decode method to do some validation. And since PrimeFaces is using its own renderers, things won't become better even though issues in MyFaces + Mojarra would have been adressed some time.

Thoughts?

@melloware
Copy link
Member

Hmmm I never tried it but can you actually use maxLength on p:password?

@cnsgithub
Copy link
Contributor Author

In case of p:password, maxlength is not explicitly written in PasswordRenderer, however not sure if renderPassThruAttributes is doing so.

However, in case of p:inputText, maxlength is written explicitly in InputTextRenderer.

@cnsgithub
Copy link
Contributor Author

Just tested in showcase with <p:password id="nonFeedback" value="#{passwordView.password1}" maxlength="3" />.

Gets rendered as:
<input id="j_idt694:nonFeedback" name="j_idt694:nonFeedback" type="password" class="ui-inputfield ui-password ui-widget ui-state-default ui-corner-all" maxlength="3" role="textbox" aria-disabled="false" aria-readonly="false">

Meaning that maxlength is enforced at client but not at server side. That's odd.

@melloware
Copy link
Member

I thought that is what RenderPassthroughAttributes did. So yeah because those derive from HTML Input it respects all their props. Is there any way we can generically enforce your maxlength check on these types? I haven't studied it closely enough.

@tobiasdick
Copy link

Unfortunately, I can still reproduce the insecure behavior using p:inputText with the 8.0 release. It appears to be only fixed for p:inputTextarea itself.

See the following example based on the PoC by @cnsgithub and https://github.com/primefaces/primefaces-test.

Sample XHTML (src/main/webapp/test.xhtml):

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml"
	  xmlns:ui="http://java.sun.com/jsf/facelets"
	  xmlns:f="http://java.sun.com/jsf/core"
	  xmlns:p="http://primefaces.org/ui"
	  xmlns:h="http://java.sun.com/jsf/html">

	<h:head>
		<title>PrimeFaces Test</title>
	</h:head>
	<h:body>

		<h:form id="frmTest">
			<h1>#{testView.testString}</h1>
			<p:inputText value="#{testView.testString}" maxlength="10" />
			<p:commandButton type="submit" value="Submit" update="frmTest" />
			<p:commandButton type="submit" value="Hack" update="frmTest" onclick="document.querySelector('input[maxlength=&quot;10&quot;]').value='a'.repeat(100);" />
		</h:form>

	</h:body>
</html>

Sample bean (src/main/java/org/primefaces/test/TestView.java):

package org.primefaces.test;

import java.io.Serializable;
import javax.annotation.PostConstruct;
import javax.faces.view.ViewScoped;
import javax.inject.Named;

@Named
@ViewScoped
public class TestView implements Serializable {

	private String testString;

	@PostConstruct	
	public void init() {
		testString = "Welcome!";
	}

	public String getTestString() {
		return testString;
	}

	public void setTestString(String testString) {
		System.err.println("received input: " + testString + " with length: " + testString.length());
		this.testString = testString;
	}
}

@tandraschko
Copy link
Member

feel free to create a new issue + PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔒 security Security related issue or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants