Mailbox steal depot items bug, crash OTS bug [clone items]

[gesior]

Tested on TFS 1.4

Steps to reproduce

You will need:

• 2 players
• mailbox within 1 SQM from depot [as it's in Kazordoon on 13+], so player can stand next to depot and mailbox in same time
  

Test 1 is random guy who visits depot.
Test 2 is attacker who wants to steal items/crash OTS.

How to reproduce:

1. Test 1 goes to depot locker and opens it. Then he walks away.
2. Test 2 goes to same depot locker, so he is within 1 SQL from depot locker and mailbox.
3. Test 2 put Parcel addressed to Test 1 in his BP.
4. Test 2 opens BP in one window in client and Parcel in second window - he sees Parcel from outside [can throw it] and also content of Parcel.
5. Test 2 puts Parcel on mailbox next to him. Parcel converts into 'Stamped parcel'.
6. Now Test 2 can click 'move up' in 'Stamped parcel' and he is inside Test 1 Inbox (depot)!

Now Test 2 can steal items of Test 1.
If Test 1 logouts and Test 2 try to move item in Test 1 depot, it will crash server.
I also heard about people who were able to clone items without crashing server on otservbr, but I cannot reproduce it on TFS.

Problems:
Step 1 binds Test 1 item Inbox position to position on which player opened locker last time.
Step 5 checks, if item moved is within 1 SQM range and does not close it. If mailbox and 'Test 1 Inbox' positions are within 1 SQM from player who sends parcel (Test 2), it does not close sent Parcel.

Looks like someone who added Inbox system in TFS forgot to copy code that closes containers, when they are moved into someone's depot. Inbox is not inside Depot Chest, so it does not protect parcels anymore.

Expected behaviour

Parcel gets closed in client.

Actual behaviour

Parcel stays open. You can click 'move up' in client and get into other player depot!

Reported by Christianlb on Discord. He asked me for help with crash analysis, then he found crash reason himself and told me how to reproduce it.
Already reproduced [abused] on top OTSes. Gunzodus solution: block possibility to send parcels, when you are within 1 SQM from Mailbox 😃

[gesior]

I made PR with fix for TFS 1.4: #4537
If someone is able to test it on newest TFS (master) and reproduce, please create PR with same code for master branch.

[ArturKnopik]

1. Test 1 goes to depot locker and opens it. Then he walks away.
2. Test 2 goes to same depot locker, so he is within 1 SQL from depot locker and mailbox.
3. Test 2 put Parcel addressed to Test 1 in his BP.
4. Test 2 opens BP in one window in client and Parcel in second window - he sees Parcel from outside [can throw it] and also content of Parcel.
5. Test 2 puts Parcel on mailbox next to him. Parcel converts into 'Stamped parcel'.
6. Now Test 2 can click 'move up' in 'Stamped parcel' and he is inside Test 1 Inbox (depot)!

I can confirm, the problem is present on master!

[floki21uu]

confirmed the bug exists in the newest tfs server

[EPuncker]

confirmed the bug existed in the newes tfs server used to crash

do you mean it still exist or was it fixed with #4538 ?

[floki21uu]

confirmed the bug existed in the newes tfs server used to crash

do you mean it still exist or was it fixed with #4538 ?

IT STILL EXISTS after applying the code.
The parcel still turns into stamped parcel if the other player had opened the same depo before and moved away. If he hadn't opened the depo before, it works well, the window closes.

Although cloning items is impossible because it says "Sorry, not possible" but... server crashes sometimes when you open depo.

[gesior]

How to reproduce:

Test 1 goes to depot locker and opens it. Then he walks away.
Test 2 goes to same depot locker, so he is within 1 SQL from depot locker and mailbox.
Test 2 put Parcel addressed to Test 1 in his BP.
Test 2 opens BP in one window in client and Parcel in second window - he sees Parcel from outside [can throw it] and also content of Parcel.
Test 2 puts Parcel on mailbox next to him. Parcel converts into 'Stamped parcel'.
Now Test 2 can click 'move up' in 'Stamped parcel' and he is inside Test 1 Inbox (depot)!

@floki21uu
The parcel still turns into stamped parcel if the other player had opened the same depo before and moved away
Test scenario mentions that Player no 1 must open given depot first. What do you mean by 'other player'? Use 3rd player for tests or what? Is scenario you tested different than described in issue?
What engine do you use? TFS master with 12.x client?
Someone already reported crash issue, but he did not use TFS code: #4538 (comment)
I also heard that this does not fix problem on TFS downgrades, as there is no Inbox on old protocols.

[floki21uu]

By saying other player I mean player who opened the same depo before the second player. I use TFS master. I will have to make a youtube video because GIFs are too heavy,

[NRH-AA]

Is this still a problem?

[Codinablack]

I think this one fixed it

fix: close parcel send thru mail system to other player #4538

I don't know for sure though.

[EvilHero90]

@floki21uu can you update us on this, unless there is no video or better detailed description I'll close this as others mentioned it to be fixed.

[Codinablack]

I am pretty sure this is not fixed. Seems there are other ways to get around the fix in postremovenotification.. a more thorough investigation and fix is likely needed.

[gesior]

I am pretty sure this is not fixed. Seems there are other ways to get around the fix in postremovenotification.. a more thorough investigation and fix is likely needed.

We know what happened:
Parcels/inner containers do not close, when they are send to Inbox.
Inbox is virtual item without any real position in game, so code that closes containers 'in 1 sqm range' (for normal Containers) do not work.

We know why it happened:
Someone who added Inbox system into TFS did not know about safety feature inside postRemoveNotification, which already protected DepotChest (also virtual item with no real position in game).

We expect that this fix works:
Same protection is already used for DepotChest, when you move Container (BP/bag) from floor into DepotChest, it closes that container for players who are not owners of that DepotChest.
If something is wrong with this fix for Inbox, maybe DepotChest is also abusable.

Anyway, if anyone know some way around that fix please post reproduction scenario or gdb report after crash.

[Codinablack]

Yeah I tested it today myself. It indeed does resolve this, even when the parcel is on the ground, if multiple players have it open and someone else moves it, even if you address the parcel to yourself, literally tested all the ways that I could think of and it worked every single time!