CORPORATION: Rewrite validation code for strings of price and quantity

[catloversg]

This PR was a part of "Fix lint errors 2" (it's still WIP). Corporation code is too messed up, so I have to split this part to a separate PR to discuss it easier. This PR deals with eval code in 2 files: src\Corporation\Actions.ts and src\Corporation\Division.ts.

Part 1: Actions.ts sellMaterial and sellProduct process the price string and the amount string in different (wrong) ways. Their code are inconsistent, confusing, and wrong. I'll try to make a non-exhaustive list:

• sellMaterial:
  • Allow only 1 instance of MP, but in Division.ts, this condition is irrelevant.
  • Allow "INV", but in Division.ts, "INV" is not replaced by the real value.
  • When it replaces "INV", it uses material.productionAmount instead of material.stored.
  • Price: "123+456" will be "eval" to 579. The final value is 579.
• sellProduct:
  • Allow "INV", but in Division.ts, "INV" is not replaced by the real value.
  • Price: "123+456" will be passed to parseFloat. The final value is 123.

The new implementation is much more consistent. One thing to note is the new behavior of the price string. The new implementation uses the behavior of the old sellProduct (pass the string to parseFloat). This is a breaking change, but I don't think it's a big deal.

Part 2: Division.ts All current changes are just small changes. I want to hear d0sboots's opinion about sCost before moving forward. Let's check this code in the product part: https://github.com/bitburner-official/bitburner-src/blob/c7cf0b853b787b9899e2e07805102f288c2cdcaf/src/Corporation/Division.ts#L864 https://github.com/bitburner-official/bitburner-src/blob/c7cf0b853b787b9899e2e07805102f288c2cdcaf/src/Corporation/Division.ts#L898-L909 sCost is declared as a number, but if the product's price is not set, sellPrice is an empty string, and sCost will be undefined after that. The entire code path after that part assumes that sCost is a number, but it's undefined.

This problem is even easier to see if we check the material part: https://github.com/bitburner-official/bitburner-src/blob/c7cf0b853b787b9899e2e07805102f288c2cdcaf/src/Corporation/Division.ts#L543 sCost is "any" here. Lint will complain about a lot of lines after that.

@d0sboots What do you think about this case?

[catloversg]

@d0sboots To minimize the number of behavior changes, I tried to reuse as much code as possible. It seems that you are okay with some behavior changes, so I'll summarize the response to your feedback here.

• Comment about regex: I'll update the comment. Ideally, we should use a regex that enforces "MP" as a string, not ["M" or "P"], but I don't know how to write that regex. If you come up with a better regex, please let me know.
• Replace whitespace characters: This is old code. I'm fine with removing them.
• Returned type of convertPriceString and convertAmountString: I tried to use string only at first as you suggested, but in the end, I preserved the "string | number". More about the reason later.
• The block of "converting it to a number": It's the same as above. I did it due to another reason. More about this later.
• "Can't these be placeholders to avoid needing to pass in real values? ...": You are right. I'll do that. The old code did that, so I was hesitant to change that part.
• "this should return "123+456": I'm fine either way. I'll change it after we agree on behavior changes.

Now, about the reason that I mentioned above. At first, let's talk about the current design of the cost/quantity string. We receive a string from the player, parse it, then save it in desiredSellPrice and desiredSellAmount. Generally, the logic is that:

• Does it contain placeholders (MP, MAX, PROD, INV)? If yes, validate it, then save the string.
• Otherwise, "process" it, and then save the number. It's inconsistent in this step. It may be "calling parseFloat" or "calling eval". That's why "123+456" is 579 in sellMaterial, but 123 in sellProduct.

In the SALE state, these "string | number" variables are used:

• string: replace placeholders, then call eval.
• number: use it as-is.

This is why I preserved the "string | number" and wrote the block of "converting it to a number".

A solution:

• Change desiredSellPrice and desiredSellAmount to string. Currently, it's "string | number". We will have to migrate the save data and deal with all the code assuming those variables being "string | number".
• When we receive a string from the player, validate it, then save it as a string.
• In the SALE state, always eval them.

This is a pretty big change.

By the way, I want to hear your opinion about the sCost that I mentioned in the description of this PR.

[d0sboots]

Oops, I started writing this comment a couple days ago and then got called away on something.

Comment about regex: I'll update the comment. Ideally, we should use a regex that enforces "MP" as a string, not ["M" or "P"], but I don't know how to write that regex. If you come up with a better regex, please let me know.

The regex whitelist exists because we are "eval"ing this string later, and we don't want executable code to slip in. I'm not actually certain that it is sufficient; the existence of https://jsfuck.com/ shows just how hard it is to avoid arbitrary code execution through eval. But, at the same time we are already allowing arbitrary code execution, everywhere, so this is mostly academic and if someone manages to craft an exploit through execution of malicious code in eval, kudos to them.

All that exists to say: We don't need to try overhard in filtering. A simple regex that doesn't block things we know we want to allow, and is easy to understand, is more important.

A solution:

• Change desiredSellPrice and desiredSellAmount to string. Currently, it's "string | number". We will have to migrate the save data and deal with all the code assuming those variables being "string | number".
• When we receive a string from the player, validate it, then save it as a string.
• In the SALE state, always eval them.

Yes, that is what I was suggesting. I overlooked the codepath in SALE where the number is used as-is. My thought was, if we are making a major change here, we should go all the way and clean up this code fully: Don't have two separate paths that introduce bugs like this, but simply validate in one way, and evaluate (with eval) in one way.

Note that there is no need for save file migration (although it should be tested/checked): Eval of a number returns the number.

[catloversg]

New changes:

• Remove the regex that replaces whitespace characters.
• convertPriceString and convertAmountString return a string.
• No usage of parseFloat.
• The SALE state always eval desiredSellPrice and desiredSellAmount.
• sellMaterial does not automatically "correct" the empty string input. price is converted to the upper case string. I make these changes to make it consistent with how sellProduct and amt work. For example, before these changes, "max" is ok, but "mp" is invalid.

I have played for a while (manually via UI and with my scripts). It works properly.

[d0sboots]

Great! BTW, my logic on why behavior changes were OK here was that this affects manual play exclusively (or almost exclusively). Scripts don't set the type of strings that should see any differences here.