Time Nick Message 04:40 MTDiscord Bump https://github.com/minetest/minetest/pull/10449 13:24 MTDiscord base64 is still wrong Krock / SmallJoker 13:24 MTDiscord fixing it 13:37 MTDiscord https://github.com/minetest/minetest/pull/10515 13:56 Krock though currently it works 13:56 Krock perhaps just inaccurate 14:02 MTDiscord well, it has false positives 14:02 MTDiscord "A=A" is invalid base64, but would be accepted by the current implementation 14:03 MTDiscord on a sidenote, my fix should increase performance as there is no need to check every character against = 14:07 Krock there would only be a single '=' comparison in the whole process because if previous logical OR conditions evaluate to "true", the other checks are skipped 14:11 MTDiscord true, but I save you the "or" 14:12 rubenwardy The performance increase will be negligible 14:13 rubenwardy A fix to correct behaviour would be the benefit 14:14 MTDiscord yes 14:15 sfan5 please don't put that unrelated change in there 14:18 MTDiscord ok 14:18 pgimeno base64 has been gotten wrong three times by the engineers at Linden Lab; pay close attention to the implementation and test it thoroughly and with invalid cases, like a string starting with a non-base64, of with a =, or with a character and a =, or with a character and an invalid symbol, etc. 14:20 MTDiscord I'm pretty certain that my fix works 14:20 pgimeno for XORing in particular: first we had llXorBase64Strings, then llXorBase64StringsCorrect, then llXorBase64, and that one also had a bug that got caught early enough (but now they have to live with the other two because of compatibility) 14:28 pgimeno I may be missing something, but I don't see a check for i % 4 14:28 pgimeno this base64 string is invalid, but it seems to be accepted: A= 14:29 pgimeno this one is wrong too: ABCDE== 14:29 pgimeno basically, if number of base64 chars % 4 = 1, the string is wrong 14:30 sfan5 the code should calculate and check the amount of required padding 14:33 MTDiscord Well, we allow omitting padding 14:33 pgimeno it could be more permissive with what it accepts, and accept strings without padding signs, but then care must be taken to avoid the last character if the total length % 4 = 1, so that e.g. a base64 string with 1 base64 char, optionally followed by padding, is taken as an empty string 14:33 MTDiscord We could check that IF padding is used, it has to be proper 14:34 sfan5 the simplified check is fine then 14:34 pgimeno in that case, every string is valid base64 14:34 pgimeno no need to check it 14:34 rubenwardy This is the sort of code that can be unit tested very easily 14:34 pgimeno yes, but first a specification is needed 14:35 MTDiscord True, and it should be documented in lua_api.txt 14:35 MTDiscord We have to keep allowing the omission of padding 14:35 MTDiscord How we deal with padding remains the question 14:37 pgimeno for rigorous base64, where two base64 strings are equal iff the binary strings they represent are equal, additional checks need to be done on the last character in case i % 4 = 2 or 3 14:37 pgimeno I'd vote for "any string is a base64 string" 14:38 pgimeno i.e. don't check padding, just consume base64 chars and omit the last if the total number of them % 4 = 1 14:39 pgimeno (stopping only at the first non-base64, or at end of string) 14:39 sfan5 I don't know why you are making this so complicated, base64 decoding currently allows = in the middle of a string despite being invalid 14:39 pgimeno and doesn't that stop the decoding? 14:39 sfan5 and this PR changes it to only accept = at the end, which is closer to what it should do 14:39 sfan5 I have no idea but that doesn't matter because it isn't supposed to decode invalid base64 14:40 MTDiscord It decodes invalid base64 anyways tho 14:40 sfan5 ¯\_(ツ)_/¯ 14:40 MTDiscord From a glance it appears it stops if it encounters padding? 14:41 MTDiscord while (in_len-- && ( encodedstring[in] != '=') && is_base64(encodedstring[in])) 14:41 pgimeno I'm not making it complicated at all, just bool base64_is_valid(std::string const& s) { return true; } 14:41 pgimeno and for the decoder, the idea is to stop at the first break in a loop similar to this: https://github.com/minetest/minetest/pull/10515/commits/8409aef3f2bb5af295bb131a9977c5924ebc4dc3 14:41 pgimeno it's actually the simplest 14:41 MTDiscord The decoder already does it 14:41 pgimeno just additionally do: if (i % 4 == 1) --i; 14:42 sfan5 then replace "complicated" with "nonsense" 14:42 MTDiscord Yeah, such an implementation will lead to strange bugs 14:42 sfan5 this PR improves base64 validation and your suggestion is "it should attempt to decode whatever garbage the user gives in, php style" 14:42 MTDiscord Also, people might use decode_base64 to validate it 14:42 MTDiscord "returns string or nil for invalid base64" - lua_api.txt 14:43 pgimeno sfan5: well, if you want a mid term (not rigorous base64 but not relaxed), then you need to define what you want 14:43 MTDiscord base64_is_valid is currently not exposed 14:43 MTDiscord I'd say we go with optional padding, but if padding is used, check whether it's right 14:43 MTDiscord Note: My PR doesn't check the amount of padding characters, it just ensures it's between 0 and 2 at the end 14:44 MTDiscord Let me add this... 14:44 sfan5 pgimeno: true and I think "base64 with or without padding" is a good enough specification 14:46 pgimeno here's my suggestion for a mid term: construct a base64_is_valid that checks for rigorous base64 including pad bits as well as padding chars, and expose it to Lua; and a base64 decoder that accepts anything 14:46 Krock or keep base64_is_valid as fast sanity check prior passing it to the real decoder 14:47 MTDiscord In that case you could save the is_base64 check in the decoder loop BTW 14:48 pgimeno yeah, that's my point, it's not necessary and since everything is a base64 string, the decoder is not penalized 14:48 pgimeno it just needs to stop at the first base64 character 14:48 pgimeno non* base64 14:49 pgimeno user code that cares about the well-formedness of the string, can check with the exposed lua check; user code that doesn't, can just throw it into the decoder 14:51 MTDiscord hmm, padding should be base64 size % 4 ? 14:51 pgimeno padding should make the total string length % 4 = 0 14:52 pgimeno so, 3-size%4 14:53 MTDiscord yes 14:53 MTDiscord thanks 14:54 pgimeno sorry, 3-(size+3)%4 14:58 pgimeno additionally, for it to be a canonical base64 string, when i % 4 = 2, the last character can only be one of [AQgw], and when i % 4 = 3, one of [AEIMQUYcgkosw048] 14:58 pgimeno see https://www.ietf.org/rfc/rfc4648.txt section 3.5 15:00 appguru That document uses another alphabet 15:01 pgimeno what? is Minetest not using A-Za-z0-9+/ ? 15:01 appguru Sorry my fault 15:02 appguru The alphabet is right 15:02 appguru Was looking at the safe alphabet... 15:12 appguru alright, done 15:13 appguru now I'm not so sure anymore, it will need testing... 15:13 rubenwardy by unit tests 15:14 rubenwardy https://github.com/minetest/minetest/blob/master/src/unittest/test_utilities.cpp 15:21 appguru Exposed it to the Lua API now 15:29 pgimeno http://www.formauri.es/personal/pgimeno/pastes/valid64.lua 15:32 pgimeno reload for more test cases 15:33 pgimeno consider that file public domain 15:34 MTDiscord hmm nice 15:34 pgimeno anyway, appguru, I'm not a code developer, I can only give advice, so better do as sfan5 says 15:34 pgimeno code -> core 18:18 Krock #10510 can this be kept close or shall I re-open it? 18:18 pgimeno https://github.com/minetest/minetest/issues/10510 -- Switching backend 18:18 ShadowBot Krock: Error: That URL appears to have no HTML title within the first 4KB. 18:18 Krock <3 pgimeno 18:19 pgimeno a temporary solution while the bot is fixed :) 18:19 Krock I love it. 18:23 Krock sfan5: would it be possible that you meant "position vector" or "vector" here? https://github.com/minetest/minetest/pull/10512#discussion_r507188051 18:42 sfan5 I meant to draw attention to the fact that it still says "tables" 18:53 Krock ah. to me it looked like a suggestion 21:28 paramat merging trivial #10520 in 15 mins. will merge fairly trivial #10493 in 24 hrs if no objections 21:28 ShadowBot paramat: Error: That URL appears to have no HTML title within the first 4KB. 21:28 pgimeno https://github.com/minetest/minetest/pull/10520 -- Contributing doc: Minor improvements and a clarification by paramat 21:28 ShadowBot paramat: Error: That URL appears to have no HTML title within the first 4KB. 21:28 pgimeno https://github.com/minetest/minetest/pull/10493 -- Devtest: Automatically enable zoom capability by paramat 21:29 paramat thanks pgibot 22:22 pgimeno lol