[b90-beta] Bug in varsFromStrProp() and getStrProp()

[wolph42]

Good to see you back here! Your absence did not go unnoticed!

[Azhrei]

Well, and now I've been gone for a bit.

But I should be back for the short term. I've opened a bunch of browser tabs to keep track of all the threads I've missed. In particular, your thread that is tracking remaining bugs to be fixed is *very* helpful -- thank you!

I've got a couple of hotel room nights where I hope to spend a few hours on MT but then I'm back to the grind for a few weeks. Hopefully I'll get an official b90 out the door. I appreciate everyone testing b90beta via the link in your signature; when I last spent some significant time here I didn't have much testing feedback yet.

Anyway... Carry on!

[wolph42]

Well, and now I've been gone for a bit.

But I should be back for the short term. I've opened a bunch of browser tabs to keep track of all the threads I've missed. In particular, your thread that is tracking remaining bugs to be fixed is *very* helpful -- thank you!

I've got a couple of hotel room nights where I hope to spend a few hours on MT but then I'm back to the grind for a few weeks. Hopefully I'll get an official b90 out the door. I appreciate everyone testing b90beta via the link in your signature; when I last spent some significant time here I didn't have much testing feedback yet.

Anyway... Carry on!

I will. I guessed it might come in handy, also when a new version is released its also easier to have an initial checklist of things to test.

Have you seen the pm i send you a while bag concerning the bag of tricks? It points to a thread in the moderators forum (or whatever its named).

[Azhrei]

@wolph I checked this out an, unfortunately, these aren't bugs per se. Everything, works as intended under the hood.

Yes, I'm wondering why this problem has taken so long to surface...

As Lee points out, the parser checks first for JSONs (by looking for a leading '[' or '{') and then for numbers and then treats any remaining value as a string.

Just as you're having a problem with token ids being converted to numbers there are similar problems when a string starts with a bracket or brace and ends with a bracket or brace. It's possible to build a string that contains those characters but it will (in A LOT of situations!) be treated as a JSON and that's wrong. But there's no simple way to fix it. Ditto for this issue with numbers.

I think we might be able to change MapTool's GUID object so that the first character is always alphabetic and it wouldn't hurt backward compatibility: campaign assets will already have GUIDs assigned so those will be wrong, but any future ones created will not have this problem. But as Lee says if we're going to make such a change shouldn't we just use the newer and presumably more reliable built-in generator? And if so, can we use the built-in generator when the MapTool GUID object is created so that the GUID is simply a front-end for the built-in data type? If so, then we would need to verify that certain things work correctly, like the asset:// URLs but otherwise we should be good to go. But now you're talking about a potentially very intrusive change -- something that could cause older campaigns to not load properly and we wouldn't find out until much further down the road.

Case #2: As it is written, the source decodes a value (i.e. Integer.decode) in order to ascertain integer values from a string. What triggered this in your case was that your string started with a "0", which is the hint for octal decoding. Without more feedback from users, it is "dangerous" to touch this as we don't know how many users actually value this (intended?/unintended?) functionality. I'll let Azhrei, et. al, decide.

I can't think of any reason why it doesn't use BigDecimal. Maybe the Integer.decode() was significantly faster or something? But again, this is something that could break campaign macros that currently work fine if we make a change.

In any case, these two macro functions should probably treat all values as strings and let the user force them to numbers using the Wiki: number() function, but we come right back around to how requiring the use of a conversion function might break existing code. (Sigh. Don't you just love trying to be backward compatibility? It's the scourge of every programmer!)

[wolph42]

Lets start with the second issue: it's a simple trade off: if you fix it you *might* break existing campaigns but if you do NOT fix it you still might break existing campaigns as the function will return unexpected results in certain cases.

So the question becomes which off the two will break more, basically anyone using these functions is prone to this issue now how many of those did actually :
1. Found this exploit and
2. Use it (for what on earth?) and
3. Did NOT report it....and
4. Released a framework using this exploit

I have a VERY difficult time believing that there is even one person walking this earth that fits this description. So IMO not fixing it will break much more than fixing it.

As for the first issue (token ID). Here I find the argument that you do not apply a simple solution now because you want to do something thorough in a new version which is actually too much work, so in the end you refrain from fixing anything, a bit lame.

As you say adding one letter is a simple solution and it does not interfere with the passed nor the future, so go this route for now. Note that it is not quite unlikely that this issue will not only occur for these functions!!

Edit: Forcing the ouput of these functions to string won't break anything because:

Code: Select all

[h:a=string(3)][a+2] 

simply returns 5.
So you won't need the number() conversion.

[Azhrei]

Yeah, your conclusion is what I'm thinking now ... using a string in a numeric expression will force a numeric. I'll consider making this change and we'll see where it leads.

[wolph42]

Yeah, your conclusion is what I'm thinking now ... using a string in a numeric expression will force a numeric. I'll consider making this change and we'll see where it leads.

My gratitude is, as always, as infinite as humanly possible

[Lee]

Well I sort of did submit a patch for this the first time wolph brought it up. I even said so on the first page of this thread.

Just saying, so the tracker can be updated