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

User input content length / size limits #18783

Closed
fnetX opened this issue Feb 16, 2022 · 13 comments
Closed

User input content length / size limits #18783

fnetX opened this issue Feb 16, 2022 · 13 comments
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/summary This issue aggregates a bunch of other issues

Comments

@fnetX
Copy link
Contributor

fnetX commented Feb 16, 2022

Feature Description

Recently, in #16765, Gitea migrated the issue comment size to allow 4GB text size, an easy way to shoot of every Gitea instance (small on Raspberry - does it even fit in RAM?, and large ones like Codeberg, too).

In many places, Gitea does not do any size checking and just relies on the database to handle stuff (it either throws a 500 error if data is too long, or if SQL mode is on truncate it just truncates data).

It would be great, if could go through / create a list of user input fields and either choose sensible defaults or make it configurable, improve UI feedback (ideally before submission).

Related issues that are kind-of included here:

Screenshots

No response

@lunny
Copy link
Member

lunny commented Feb 16, 2022

Every input form has a struct in services/forms which has some limitations. You can just send PR to change them.

@lunny lunny added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Feb 16, 2022
@fnetX
Copy link
Contributor Author

fnetX commented Feb 16, 2022

Implementing hard limits does not prevent users in the UI. But yes, I'll check it out and see if I can do something. It will at least help us to prevent being shot with large issue comments.

@fnetX
Copy link
Contributor Author

fnetX commented Feb 16, 2022

Do the form structs also work for the API? I'm basically having a summary issue here - we need both the limits and UI feedback rather than server errors and truncated data.

The limits should thus obviously apply for the API, too.

@lunny
Copy link
Member

lunny commented Feb 16, 2022

API also has API's form, it maybe different.

@fnetX
Copy link
Contributor Author

fnetX commented Feb 16, 2022

It's notable that the recent PRs from @wxiaoguang that only set LONGTEXT for every thing kinda defeats the purpose of " Restrict some tables' columns size #14075 " which tries to limit some column sizes for various reasons. LONGTEXT sounds just wrong, especially with no limits. And if we set limits again, LONGTEXT would be unnecessarily too much. IMHO, this sounds unreasonable.

And IIRC, the database was once migrated from MEDIUMTEXT to TEXT, too.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 16, 2022

It's notable that the recent PRs from @wxiaoguang that only set LONGTEXT for every thing kinda defeats the purpose of " Restrict some tables' columns size #14075 " which tries to limit some column sizes for various reasons. LONGTEXT sounds just wrong, especially with no limits. And if we set limits again, LONGTEXT would be unnecessarily too much. IMHO, this sounds unreasonable.

And IIRC, the database was once migrated from MEDIUMTEXT to TEXT, too.

No, it is not the case. All the databases except MySQL treat TEXT as nearly "no-limitation". So my PR only made these database behaviors consistent. I have made a comment in the PR #16765 clearly: And, no other database has such limitation as MySQL. So it's good to make issue system's behavior the same with different databases.

So, what I did is not wrong. You should keep in mind that you must use a correct mechanism to protect your server. For example, an attacker can post very large data to your server to do DoS. That's why nginx has an option called client_max_body_size. By no mean you should try to limit anything from database side to achieve security purpose.

@fnetX
Copy link
Contributor Author

fnetX commented Feb 17, 2022

Yes, I know that the protection should not occur at database level (that's why we don't just modify the column again ;-)), but it should happen at the application level (in Gitea - thus the feature request). And also, you should consider what "no-limitation" means. Does it really need to store 4GB? Or would MEDIUMTEXT be enough, for example?

If we would limit the max body size via proxy, as you propose, we'd be no better than the error 500 Gitea shows when the mysql truncation mode is on. If we do limit, and I think this is important, there should be some feedback to the user, ideally before even submitting a form (but another check afterwards to give feedback to API clients, too).

Also, if you really require someone to fiddle around with the webserver config in order to protect Gitea, this is a topic for the docs. I bet there are many many Gitea instances (if not >99%) that don't use such means of "protection".

@wxiaoguang
Copy link
Contributor

Yes, I know that the protection should not occur at database level (that's why we don't just modify the column again ;-)), but it should happen at the application level (in Gitea - thus the feature request). And also, you should consider what "no-limitation" means. Does it really need to store 4GB? Or would MEDIUMTEXT be enough, for example?

If you think it is a problem, then there are a lot of new problems, for example, do you want to limit how many issues/comments can a user create? An attacker can create a lot of issues with MEDIUMTEXT to occupy the 4G storage too. So the LONGTEXT is not wrong, MEDIUMTEXT is not necessary and doesn't bring benefits.

If we would limit the max body size via proxy, as you propose, we'd be no better than the error 500 Gitea shows when the mysql truncation mode is on. If we do limit, and I think this is important, there should be some feedback to the user, ideally before even submitting a form (but another check afterwards to give feedback to API clients, too).

I have no objection to valid the input size by code, I was talking about that "LONGTEXT is not a problem" (which was considered to be a problem in the previous comment).

Also, if you really require someone to fiddle around with the webserver config in order to protect Gitea, this is a topic for the docs. I bet there are many many Gitea instances (if not >99%) that don't use such means of "protection".

It's a new topic, I just took it as an example to explain why column limitation on database side doesn't really help security.

@stale
Copy link

stale bot commented Apr 19, 2022

This issue has been automatically marked as stale because it has not had recent activity. I am here to help clear issues left open even if solved or waiting for more insight. This issue will be closed if no further activity occurs during the next 2 weeks. If the issue is still valid just add a comment to keep it alive. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 19, 2022
@fnetX
Copy link
Contributor Author

fnetX commented Apr 19, 2022

Still relevant IMO.

@stale stale bot removed the issue/stale label Apr 19, 2022
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Apr 19, 2022
@lunny lunny closed this as completed Mar 29, 2023
@wxiaoguang
Copy link
Contributor

I see this gets closed. Some of my thoughts:

It would be great, if could go through / create a list of user input fields and either choose sensible defaults or make it configurable, improve UI feedback (ideally before submission).

There are so many, at the moment I have no idea about how to "list" them or make them "configurable" (overcomplicated?).

We can always improve anything, but I have no idea about how to do that.

Still relevant IMO.

Could you elaborate about the details of the proposal? And if there could be a detailed feasible document or a PR demo, it could help.

@lunny
Copy link
Member

lunny commented Mar 29, 2023

I think the issue is just like a summary for all input forms but not a concreate problem. Maybe we one by one. I list some of them, it's welcome to improve them.

  • Issue title and content length / size limitation
  • Comment content length / size limitation
  • Code editor filepath and content length / size limitation
  • Release title and content length/size limitation
  • Login name input box length/size limitation
  • Login password input box length/size limitation

@lunny lunny reopened this Mar 29, 2023
@lunny lunny added type/summary This issue aggregates a bunch of other issues and removed issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail labels Mar 29, 2023
@wxiaoguang
Copy link
Contributor

Since Codeberg is running a self-maintained fork, I don't see any end user really needs this. So I think it can be closed.

Again, #16765 doesn't bring any real or new problem.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/summary This issue aggregates a bunch of other issues
Projects
None yet
Development

No branches or pull requests

3 participants