haoyangw / pe

0 stars 0 forks source link

Insufficient validation for data read from storage #12

Open haoyangw opened 1 year ago

haoyangw commented 1 year ago

Steps to reproduce: -Original (valid) data saved by app in data file 1.txt:

Hotpot
Test
1
Beef
1
Add beef

-Quit the app -Replace data in data file 1.txt with these invalid data:

Hotpot
Test
1
Beef
q
Add beef

Result: App crashes on launch with error message:

image.png

This means that there is insufficient validation for the data being read from storage, and the app expects that the data in storage is valid. However, the module recommends we support letting expert users modify the data file, and by human error it is very possible to accidentally enter q instead of 1 since '1' and 'q' are adjacent on the keyboard. This is thus a valid situation that should be supported by the app, so it's a valid bug.

The severity is high because the app crashes even though this mistake is not a major issue, and the app can just ignore the data in the storage and function normally, so the situation isn't severe enough to warrant a complete crash.

soc-pe-bot commented 1 year ago

Team's Response

Our UG states that incorrect formatting of the save file during modification will result in such errors which users are expected to remedy accordingly.

image.png

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: I disagree with your team's assessment.

The module recommends that we allow expert users to modify the data file, and my test case is a valid use case, not extreme user behaviour since 'q' and '1' are in close proximity on the keyboard, which means that this typo is actually very realistic. Hence, this use case should be caught by your program, which means displaying an error message and allowing the user to continue using your program, not crashing. Since this is a valid situation, it represents an incomplete feature if you expect your users to 'remedy accordingly', hence the FeatureFlaw label.

Besides, your UG makes no mention of your program crashing, so I don't see why you claim that '[your] UG states that incorrect formatting [...] will result in such errors'('recipe manager will not process the data correctly' is not the same as saying your program may crash). Even then, mentioning this in the UG is only appropriate for a FunctionalityFlaw(i.e. not working as intended), not a FeatureFlaw(incomplete feature).

As such, I don't think your team's reasoning fits my bug description.


## :question: Issue severity Team chose [`severity.Medium`] Originally [`severity.High`] - [x] I disagree **Reason for disagreement:** Your team has not explained why `Medium` severity is more appropriate actually, but I will further substantiate my reasoning for `High` severity regardless. Referring to the module website: ![image.png](https://raw.githubusercontent.com/haoyangw/pe/main/files/ae82c84f-85fe-404f-bd86-dc3f2f892f71.png) Firstly, since 'q' and '1' are very close on the keyboard, this mistake can be made by quite a few users in real life when modifying the data files, so it's not very `occasional` as the `Medium` severity entails. Secondly, the main justification for `High` severity is that your program **crashes** _upon launch_ as a result of this bug, which prevents users from continuing to use your program. In fact, it will be difficult for the user to tell what is the cause of the crash since the exception message is not specific(doesn't mention the data file being corrupted). Unfortunately, without resolving the cause of the crash, your program will be _unable to launch_ at all(crashes upon _every_ launch), making it _completely unusable_ until the user figures out the cause, so this bug causes major problems for users. As such, I continue to believe that the `High` severity label is warranted.