ComboBox with pagelength throws exception for requesting too many rows of data

[evanzel]

With the recent change https://github.com/vaadin/framework/pull/12415 in version 8.14.1 for limiting rows in the client, I have unneseccary exceptions in the following situation:

I have a ComboBox with a setDataProvider(FetchItemsCallback fetchItems, SerializableToIntFunction sizeCallback)

I have set a small pagelength, say 15 rows.

The exception is thrown when the user's first action is to click the arrow. Surely, the pagelength should limit the number of rows both in the client and the number of rows the server prepares.

• Vaadin Framework version: 8.14.1

[TatuLund]

This exception is intentional and is related to this https://vaadin.com/security/cve-2021-33609

Now comboBox.setScrollToSelectedItem(true) can trigger this exception, if you have ComboBoxwith really many items.

For these rare cases there is a workaround possible to override DataCommunicator to use different limit

    public class MyDataCommunicator extends DataCommunicator<String> {

        @Override
        protected int getMaximumAllowedRows() {
            return 1000; // Return here limit that fits your application better than the default
        }
    }

    public class MyCombo extends ComboBox<String> {
        private MyDataCommunicator dataCommunicator;

        public MyCombo() {
            dataCommunicator = new MyDataCommunicator();
        }

        @Override
        public DataCommunicator<String> getDataCommunicator() {
            return dataCommunicator;
        }
    }

[evanzel]

Thanks for the response. You are right. I checked with disabled setScrollToSelectedItem and the exception is not triggered. Now I have to decide if i will just leave it with no ScrollToSelectedItem or if I will use the workaround.

I do wonder though, if I override the getMaximumAllowedRows, will I be vulnerable to that CVE, or does the small pagelength protects from calls with too many data.

[TatuLund]

I do wonder though, if I override the getMaximumAllowedRows, will I be vulnerable to that CVE, or does the small pagelength protects from calls with too many data.

This threat is very specific to your application and environment. If you are running the application in well protected environment in trusted organization, the risk is minimal. This risk is analogous to having a Label in your view, which is set html content allowed mode, and who's input is directly coming from a TextField or TextArea without sanitization. This poses a risk of XSS, which could be exploited by malicious user that has access to the application and that view. The same here. There might be justified cases of exceptions. There we have API's / workarounds to circumvent these limitations. The default path is protected, and you need to make conscious decision and balance the risks when doing the non-default.

[evanzel]

That makes sense. I appreciate the response. The issue can be closed, I believe.

[tangomannX]

@TatuLund: I dont think the usecase is rare. How do you come to that conclusion? The workaround makes the application vulnerable again, no valid workaround for serious applications. Is the vaadin team willing to provide a real solution for this?

[TatuLund]

@tangomannX we will follow the case. I partially agree that implementation of setScrollToSelectedItemcould be better. However this nature of the implementation has been warned in the JavaDoc, copying here:

    /**
     * Sets whether to scroll the selected item visible (directly open the page
     * on which it is) when opening the combo box popup or not.
     * <p>
     * This requires finding the index of the item, which can be expensive in
     * many large lazy loading containers.
     *
     * @param scrollToSelectedItem
     *            true to find the page with the selected item when opening the
     *            selection popup
     */

[mletenay]

Current fix for the CVE introduced in 8.14.1 is breaking a lot of existing code and I would strongly suggest different approach. Having hardcoded limit ignoring actual data provider and paging size is IMHO very bad idea. We had to downgrade back to 8.14.0 since our application started to crash in many places.

Try even this simple scenario:

List<String> items = new ArrayList<>();
for (int i = 0; i < 700; i++) {
  items.add("Item " + i);
}
ListSelect<String> listSelect = new ListSelect<>();
listSelect.setDataProvider(DataProvider.ofCollection(items));
addComponent(listSelect)

it will fail with the exception from #12415.

So I'd like to kindly ask you to re-open this ticket and provide better fix for the CVE, without impact to existing (legitimate) code.