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

fix: Add row limit to DataCommunicator row data requests #12415

Merged
merged 6 commits into from Sep 30, 2021

Conversation

TatuLund
Copy link
Contributor

No description provided.

@OlliTietavainenVaadin OlliTietavainenVaadin merged commit 2fc98ea into master Sep 30, 2021
@TatuLund TatuLund added this to the 8.14.1 milestone Sep 30, 2021
OlliTietavainenVaadin pushed a commit to OlliTietavainenVaadin/framework that referenced this pull request Oct 5, 2021
* Add row limit to DataCommunicator row data requests

* Add missing constant

* Add unit test

* Add test for extending Grid

* Fixed test
TatuLund added a commit that referenced this pull request Oct 5, 2021
* Improve thread safety (#12395)

See: https://vaadin.com/forum/thread/17522264/concurrentmodificationexception-in-vaadin-shared-on-karaf-4-2-x

* Fix incompatible selenium version in test module. (#12397)

* Fixed a dependency version in a karaf test module. (#12399)

* Checkstyle tweaks to DateField widgets. (#12400)

- Added and updated JavaDocs.
- Updated comments.
- Updated to use non-deprecated method calls.
- Removed unnecessary warning suppressions.
- Suppressed warnings for unavoidable deprecation.

* fix: set Vaadin session attribute using lock in reinitializeSession (#12401)

* Cherry picked unit test from Flow

See vaadin/flow#11538

* Fix missing import

* Cherry pick fix from Flow

* deprecate vaadin-snasphots repo (#12405)

* deprecate vaadin-snasphots repo

* Update chrome version to 93

* add more screenshots

* fix: Add MPR UI id request parameter (#12412)

* fix: Add MPR UI id request parameter

Related-to vaadin/multiplatform-runtime#85

* test: Remove redundant non-empty param test

* test: Remove leftovers

* fix: Init window.mprUiId earlier than window.vaadin

* Add missing '='

* Update links shown by license checker (#12402)

vaadin.com/pro does no longer have the info

* fix: Add row limit to DataCommunicator row data requests (#12415)

* Add row limit to DataCommunicator row data requests

* Add missing constant

* Add unit test

* Add test for extending Grid

* Fixed test

Co-authored-by: Tatu Lund <tatu@vaadin.com>
Co-authored-by: Anna Koskinen <Ansku@users.noreply.github.com>
Co-authored-by: Zhe Sun <31067185+ZheSun88@users.noreply.github.com>
Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
@ZheSun88 ZheSun88 deleted the dc-limit-rows branch October 6, 2021 05:43
@gbougiakas
Copy link

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

@OlliTietavainenVaadin
Copy link
Member

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
Copy link

gbougiakas commented Nov 2, 2021

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
Copy link
Member

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

@TatuLund
Copy link
Contributor Author

TatuLund commented Nov 2, 2021

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
Copy link

gbougiakas commented Nov 2, 2021

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
Copy link

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

@TatuLund
Copy link
Contributor Author

TatuLund commented Nov 2, 2021

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

#12466

@gbougiakas
Copy link

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
Copy link

mletenay commented Nov 3, 2021

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
Copy link
Contributor Author

TatuLund commented Nov 3, 2021

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
Copy link

mletenay commented Nov 3, 2021

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
Copy link
Contributor Author

TatuLund commented Nov 3, 2021

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
Copy link

mletenay commented Nov 3, 2021

@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
Copy link
Contributor Author

TatuLund commented Nov 3, 2021

@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
Copy link

cdir commented Nov 16, 2021

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
Copy link
Contributor Author

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

@wuwu2000
Copy link

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
Copy link

rPraml commented Jun 8, 2022

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)

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.

None yet

7 participants