fix: Add row limit to DataCommunicator row data requests

[gbougiakas]

Same issue here with TwinColSelect component. We do need to use more than 500 rows, is there anyway to override this?

[OlliTietavainenVaadin]

Same issue here with TwinColSelect component. We do need to use more than 500 rows, is there anyway to override this?

@gbougiakas yes, check the GridTest.java in this very pull request

[gbougiakas]

Same issue here with TwinColSelect component. We do need to use more than 500 rows, is there anyway to override this?

@gbougiakas yes, check the GridTest.java in this very pull request

Thanks for the reply, I checked GridTest.java and Grid appears to have a constructor that takes a DataCommunicator as an argument. TwinColSelect does not have such a constructor. I need to override the setItems method but it's not obvious to me of what I should do in there.

Can you help me?

[OlliTietavainenVaadin]

Create a new DataCommunicator subclass like in GridTest. Override getDataCommunicator in your TwinColSelect subclass and return an instance of your custom DataCommunicator.

[TatuLund]

With TwinColSelect it looks like this

    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 MyTwinSelect extends TwinColSelect<String> {
        private MyDataCommunicator dataCommunicator;

        public MyTwinSelect() {
            dataCommunicator = new MyDataCommunicator();
            addExtension(dataCommunicator);
        }

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

[gbougiakas]

Create a new DataCommunicator subclass like in GridTest. Override getDataCommunicator in your TwinColSelect subclass and return an instance of your custom DataCommunicator.

With TwinColSelect it looks like this

    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 MyTwinSelect extends TwinColSelect<String> {
        private MyDataCommunicator dataCommunicator;

        public MyTwinSelect() {
            dataCommunicator = new MyDataCommunicator();
            addExtension(dataCommunicator);
        }

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

Thank you both for the swift replies. Here is my custom TwinColSelect

` public class TwinColSelectWithCustomRowLimit extends TwinColSelect {

private DataCommunicatorWithCustomRowLimit<T> dataCommunicatorWithCustomRowLimit;

public TwinColSelectWithCustomRowLimit() {
    dataCommunicatorWithCustomRowLimit = new DataCommunicatorWithCustomRowLimit<T>();
    addExtension(dataCommunicatorWithCustomRowLimit);
}

@Override
public DataCommunicator<T> getDataCommunicator() {
    return dataCommunicatorWithCustomRowLimit;
}

public class DataCommunicatorWithCustomRowLimit<T> extends DataCommunicator<T> {
    @Override
    protected int getMaximumAllowedRows() {
        return 2000;
    }
}

}

`

Unfortunately now I get a NullPointerException upon calling the constructor of the TwinColSelectWithCustomRowLimit.

Any further help is much appreciated.

[gbougiakas]

Seems like the the overriden getDataCommunicator() is called before the constructor for some reason, hence the custom DataCommunicator instance is null.

[TatuLund]

I, see some components are trickier to extend. I opened PR for easier configurability

https://github.com/vaadin/framework/pull/12466

[gbougiakas]

I, see some components are trickier to extend. I opened PR for easier configurability

12466

Thanks for the swift response. I will remain at 8.14.0 until there is an easy way to bypass this for the components we need more than 500 rows in our dataset.

Regards

[mletenay]

As I've just commented in #12428, this limit to 500 items in ListSelect, TwinColSelect, ComboBox broke potentially a lot of code without proper warning that a 8.14.1 made a breaking change to previously legitimate and working code.

We have potentially many places were such crash may happen with backend data providers of unknown size. Having to manually made hacky overrides to (what limit size ?) throughout the application is quite a bad idea.

Could the CVE be solved with proper way, without breaking code working fine for many years ?

[TatuLund]

Could the CVE be solved with proper way, without breaking code working fine for many years ?

No, that is not the good approach in this case.

We want to follow the same principle here as we have for XSS for example. By default our components do not allow HTML content (say Label, tooltip etc.), and you need to specifically with the API define that HTML content is allowed. I.e. then application developer takes the responsibility to take care that HTML content does not possess harm. Here we are working analogous way. By default framework limits the size, but you can override it. See the #12466, we will introduce API take makes the override less hacky.

[mletenay]

I don't see relation to XSS here. Until 8.14.1 it was totally fine to have e.g. ListSelect with 501 items. Now it is not possible without explicitly allowing that. And why actually 500 ? Why not 100 or 1000 ?

And even if there is such limit (which I still thing is a bad idea), it needs to be properly documented in all the possible places. And a big warning in release notes.

[TatuLund]

it needs to be properly documented in all the possible places. And a big warning in release notes.

@mletenay That is a valid argument, I improved the release note

https://github.com/vaadin/framework/releases/tag/8.14.1

[mletenay]

@TatuLund Thanks for the release notes adjustment, but again I'm not sure this is sufficient. Most of the Vaadin application developers don't even know there exists a DataComunicator, it's rather low-level framework class. I suggest to notify framework users that any UI component subclassing AbstractListing (i.e. ComboBoxes, ListSelects, Grids ...) have now this limitation.

[TatuLund]

@mletenay We will update some documentation when #12466 is released. After that the following will be possible

@Theme("mytheme")
public class MyUI extends UI {

    @Override
    protected void init(VaadinRequest vaadinRequest) {
        final VerticalLayout layout = new VerticalLayout();

        TwinColSelect<String> twin = new TwinColSelect<>();
        twin.getDataCommunicator().setMaximumAllowedRows(1000);

        ComboBox<String> combo = new ComboBox<>();
        combo.getDataCommunicator().setMaximumAllowedRows(1000);

        List<String> items = new ArrayList<>();
        for (int i=0;i<800;i++) {
            items.add("Item "+i);
        }

        twin.setItems(items);
        twin.setItemCaptionGenerator(item -> item.toUpperCase());
        combo.setItems(items);
        combo.setScrollToSelectedItem(true);

        layout.addComponents(twin, combo);

        setContent(layout);
    }

    @WebServlet(urlPatterns = "/*", name = "MyUIServlet", asyncSupported = true)
    @VaadinServletConfiguration(ui = MyUI.class, productionMode = false)
    public static class MyUIServlet extends VaadinServlet {
    }
}

[cdir]

We also ran in this problem and I understand the point of @mletenay very well. We and our customers have some ComboBos with a lot of entries and we disabled pagination because our users did not see easily how to go down to the next page (no real scrollbar, buttons nearly unvisible). However even with pagination there would be the problem if I understood #12428 correctly.

However I do not see why it was not fixed without breaking the behavior at least for ListDataProvider (or maybe other InMemoryDataProvider). In a ListDataProvider the server knows exactly how many Items there are at most and could set this number of items automatically. When do you plan to release the fix to specify the allowed rowes?

[TatuLund]

Vaadin 8.14.3 was released today with the API to override the limit if so needed.

[wuwu2000]

I'm sorry but this fix is stupid. It's not fixing the core problem of the attack but makes life for developers harder. So now we have to estimate a number which could be possible. And if the number is big enough we reenabled the weakness to be attackable.

[rPraml]

We have combo boxes in our application, where we do not really have the size under control. A user can create new data and a distinct set is shown in the combobox. We expected, that paging would solve the problem, but only a page-size ~ 100 will work (the client fetches the rows 40-460 in a second step) so we decide to increase maximum allowed rows to 5000 (configurable via system property)