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

Limit for "Instructions" field in creating and editing a session not correctly treated. #12455

Closed
pabloheika opened this issue May 26, 2023 · 6 comments · Fixed by #12503
Closed
Labels
good first issue Easy; restricted for first-time contributors

Comments

@pabloheika
Copy link


name: Bug Report
about: Reporting an error or defect in the application


Limit for "Instructions" field in creating and editing a session not correctly treated.

  • Environment:
    Development server
    branch master on commit 101cd63

Steps to reproduce

  • Log in as an instructor on the platform
  • Access the "Sessions" tab
  • Create or edit a session
  • In the "Instructions" field unlimited text can be entered in my test repeated 1102500 times the word "test "

Expected behavior
It is recommended that the development team review the text entry logic of the system and implement a proper limitation to avoid possible future problems.
This limitation should simply make it impossible to make more entries as is already the case in other fields

Actual behaviour
The text field has been accepting unlimited insertion of information and this has slowed down the browser.
After sending it to the server, it takes a long time to load and then gets an error.

Additional Information

image

@weiquu
Copy link
Contributor

weiquu commented May 26, 2023

Hi @pabloheika, I'm inclined to leave this as is since it's very unlikely anyone will enter such long text into the input field (and as far as I'm aware, this poses no security concern). Loading times also seem fine to me, I believe under 10s. Previous data isn't overwritten as well.

Perhaps an improvement to be made is to have a more descriptive error message, rather than impose an arbitrary limit on the text length of the input field. Let's see what others think.

As a quick benchmark, I checked this with 500,000 words for a total of 3,385,396 bytes/characters and didn't get an error. Loading times (rough estimate) were under 5s.

@weiquu weiquu added the s.ToDiscuss The issue/PR is undergoing discussion/review by the core team label May 26, 2023
@wkurniawan07
Copy link
Member

Generally speaking, we would only mark a UI issue as needing fix if it either poses a threat to system stability and/or security or inconvenience to a significant number of users.

  • Threat to system stability / security: someone needs to verify the impact of this to the backend, e.g. if it causes a more-than-usual backend processing time, then it is a vector for DDoS attack and thus needing fix. A localized impact on user browser is irrelevant here.
  • Inconvenience to a significant number of users: extremely unlikely. The user doing this clearly knows what s/he does.

However, we do need a hard limit on the backend. An unlimited upper bound, even without security concern, would bloat our DB storage cost (I'm not sure if there's a term for this kind of attack). The hard limit can be a sensibly large number such that anything beyond that is clearly an abuse attempt.

@weiquu weiquu added good first issue Easy; restricted for first-time contributors and removed s.ToDiscuss The issue/PR is undergoing discussion/review by the core team labels Jun 7, 2023
@JTrenerry
Copy link

What should the word limit be for it?

@weiquu
Copy link
Contributor

weiquu commented Jun 9, 2023

@wkurniawan07, any suggestions? I would say not more than 500 words, though it might be worth taking a look at how many words are usually used for the instructions field

@rexong
Copy link
Contributor

rexong commented Jun 29, 2023

Hi, I would like to work on this if there is not progress yet.

@weiquu
Copy link
Contributor

weiquu commented Jun 29, 2023

Hi @rexong, feel free to submit a PR for this issue. As for the max number of characters/words... will leave it up to you for now, just decide on a reasonable amount (:

jasonqiu212 pushed a commit that referenced this issue Aug 3, 2023
* Limit word and character count in text editor

* Fix snapshot testing and remove character limit

* Quick fix on word limit

* Fix snapshot test

* Change Word Count to Character Count

* Fix linting

* Update Snapshot test

* Change Character Count Limit

* Fix select all + paste bug

* Trauncate text and set cursor to end of pasted text

* Fix lint

---------

Co-authored-by: Dominic Lim <46486515+domlimm@users.noreply.github.com>
Co-authored-by: Wei Qing <48304907+weiquu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Easy; restricted for first-time contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants