antalya-26.3: ClickHouseVersion parser accepts malformed version strings instead of rejecting them (PR #1653)

[CarlosFelipeOR]

ClickHouseVersion parser accepts malformed version strings instead of rejecting them (PR #1653)

Source: Altinity/ClickHouse PR #1653 – audit review
Context: Antalya 26.3 patch for SettingsChangesHistory + ClickHouseVersion parsing update (base branch antalya-26.3)
AI audit note: The original review was generated by AI (gpt-5.3-codex).

---

Summary

Code audit of the Antalya 26.3 SettingsChangesHistory / version-parsing patch identified 1 confirmed defect (Medium): the updated ClickHouseVersion constructor silently accepts malformed version strings that the previous implementation rejected with BAD_ARGUMENTS.

---

Confirmed defects

1. Medium: Malformed compatibility versions are now accepted instead of rejected

• Impact: Typos in compatibility values can silently apply an unintended compatibility profile instead of failing fast, causing hard-to-diagnose settings behavior.
• Anchor: src/Common/ClickHouseVersion.cpp / ClickHouseVersion::ClickHouseVersion
• Trigger: A non-numeric middle component such as 26..1, 26.1., or 26.x.1 in a parsed version string.
• Why defect: The constructor now treats the first non-numeric token (including empty tokens) as suffix and stops parsing, while previous behavior rejected such strings with BAD_ARGUMENTS.
• Fix direction (short): Restrict suffix handling to an explicit terminal suffix pattern; keep empty/intermediate non-numeric components invalid.
• Regression test direction (short): Add parser tests asserting exceptions for malformed inputs and success for valid suffixed forms like 26.1.3.20001.altinityantalya.

---

Notes

• The second item flagged in the original audit comment (stateless check in 02995_new_settings_history.sh) was confirmed by the dev to be the intended purpose of the change, not a defect, and is therefore not tracked here.

[il9ue]

Spent a bit of time investigating this issue. While the requirement seems straight forward, there are three reasonable ways to fix the parser

1. keep the existing suffix handling, but only treat a non-numeric token as the suffix when it's the last token and isn't empty. Small change, and it's enough to reject 26..1, 26.1. and 26.x.1 while 26.3.1.20001.altinityantalya and short compatibility values like 25.3 still parse fine. Close to what the audit suggested.
2. validate the whole string with a regex like ^\d+(\.\d+)*(\.[A-Za-z][A-Za-z0-9]*)?$ before parsing. Reads clearly, but it's a second validation pass on top of the split we already do, and too strict a pattern could reject some future suffix and may have poorer readability.
3. The strict check from Fix version parsing for Antalya branch #1644. It correctly rejects the bad inputs, and it leaves short versions like 25.3 working. But it only recognizes one exact suffix, the literal text altinityantalya, so any other suffix such as altinitystable would fail. It also just remembers a yes/no flag (is_antalya) instead of saving the actual suffix text.

Leaning towards (1), because this preserves on valid input most organically and maintenance with readability. (It seems (2), using regex would work too, but IMO, this would have some maintenance issue. Avoid (3) because it's too passive solution) Let me check the code and confirm the behavior first, then follow up with a patch and tests.

[il9ue]

Thanks @zvonand for review.
Reading again the audit: the only valid suffixed form it lists is 26.1.3.20001.altinityantalya. The 26.altinityantalya / 26.1.altinityantalya cases in my test were my own addition, not from the spec, and treating "accepts more inputs" as a feature was the wrong framing on my part. I'll drop them.

The parser change still holds for the core bug: 26..1, 26.1., 26.x.1 are rejected, and empty/intermediate non-numeric tokens stay invalid, which matches the audit's fix direction. What the audit doesn't pin down is the exact terminal-suffix pattern, so I need to confirm,

1. Should a suffix be valid only after exactly four numeric components (MAJOR.MINOR.PATCH.BUILD.altinityantalya), or is any number of leading components acceptable?
2. Should only altinityantalya be accepted, or should other suffixes like altinitystable / altinityfips parse too?

Once confirmed, I'll tighten the parser and tests to match and re-open from the Altinity repo so CI runs.

[ianton-ru]

@il9ue Now we care about two cases now:
4 digit groups -<digits>.<digits>.<digits>.<digits> from upstream and altinity stable versions.
4 digit groups and specific postfix -<digits>.<digits>.<digits>.<digits>.altinityantalya from antalya branch.

May be make sense to support other custom ClickHouse versions like TinyBird, but I don't know how they change version string.

[mkmkme]

@ianton-ru IIUC we also need to support 2-digit groups due to SettingsChangesHistory.cpp. The version entry gets converted to ClickHouseVersion, and quite many of them are in the format of YY.M

[ianton-ru]

@mkmkme Hm, looks like yes.

[il9ue]

Thanks @ianton-ru @mkmkme.
I checked the codebase mentioned above to lock the spec down. See the summary below.

Accepted formats (4)

#	Form	Example	Where it comes from
1	2 numeric groups (YY.M)	26.3, 25.8	61 SettingsChangesHistory keys; compatibility short form
2	3 numeric groups	18.12.17	1 legacy SettingsChangesHistory key (pre-YY.M scheme)
3	4 numeric groups	26.3.1.20001	upstream + altinity stable server version()
4	4 numeric groups + altinityantalya	26.1.3.20001.altinityantalya	antalya branch; 3 history keys

One thing worth a note: the legacy 3-group key 18.12.17 is parsed at static-init time, so if the parser rejected 3-group forms the binary wouldn't start. That's why 3 groups is in the accepted set, not just 2 and 4.

Rule matrix (numeric groups × suffix)

Numeric groups	no suffix	.altinityantalya	any other suffix
1 (26)	❌ reject	❌ reject	❌ reject
2 (26.3)	✅ accept	❌ reject	❌ reject
3 (18.12.17)	✅ accept	❌ reject	❌ reject
4 (26.3.1.20001)	✅ accept	✅ accept	❌ reject
5+	❌ reject	❌ reject	❌ reject

On TinyBird and other vendor suffixes

Since the exact version string format isn't known, I've left them out of scope for now; only altinityantalya is accepted as a suffix. The parser checks the suffix explicitly, so adding another vendor later is a one-line change once its format is confirmed. Will handle it differently if you'd prefer.

[mkmkme]

@il9ue 3-segment version is not "legacy pre-YY.M" format, it is YY.M.minor, the same upstream is still using. It's just so YY.MM.minor means "every build of this minor version"