Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests for Newtonsoft.Json's handling of octal literals in version files #4227

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Oct 9, 2024

Motivation

A mod uploaded a release today with this in its version file:

		"PATCH":09,

This causes an inflation error:

New inflation error for <mod name>: Error parsing version file <path to version file>.version: Input string '09' is not a valid number. Path 'VERSION.PATCH', line 9, position 12.

Digging into this, apparently a 0 prefix for numbers isn't technically allowed at all in strict JSON (and a few online validators confirm this), but some parsers (including the most popular one for C#, Newtonsoft.Json) decided to be lenient and parse them as C-style octal literals. But 08 and 09 are not valid octal literals because only the digits 01234567 can be used in octal.

Changes

Now several new test cases are added to check and document exactly what is and is not allowed by Newtonsoft.Json's numeric parser, for future reference.

Note that the QuotedInvalidOctalNumber test is technically using an exploit, as the version file schema requires these fields to be integers, not strings, but CKAN does not enforce this.

@HebaruSan HebaruSan added Easy This is easy to fix Netkan Issues affecting the netkan data Tests Issues affecting the internal tests labels Oct 9, 2024
@JonnyOThan
Copy link
Contributor

for reference here is the official json grammar: https://www.json.org/json-en.html

@HebaruSan HebaruSan merged commit 8c02754 into KSP-CKAN:master Oct 9, 2024
3 checks passed
@HebaruSan HebaruSan deleted the add/avc-json-octal-tests branch October 9, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy This is easy to fix Netkan Issues affecting the netkan data Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants