From 681fdb7c51d316bf05a735427d69284d1857f798 Mon Sep 17 00:00:00 2001 From: Roshan Ramani Date: Mon, 7 Sep 2026 08:26:25 +0530 Subject: [PATCH 1/5] Refuse integers that cannot be sent exactly (#963) An integer above 2**53-1 was accepted and rounded on the way to salt, so `12345678901234567` arrived as `...68`. The command-line keeps the value the user typed, as you showed on the issue, so the number SaltGUI sends was simply a different one. The integer branch now returns an error, matching the float branch beside it that already refuses a value it cannot represent. Job-ids are unaffected: getPatJid() matches them as strings before this point. The three existing expectations for long integers compared against numeric literals, which the test file rounded identically, so they could not tell a correct value from a wrong one. They now compare against strings and assert the refusal, with the 2**53-1 boundary and a real job-id added either side. ai-assisted-by: Claude Code (Claude Opus 5) Signed-off-by: Roshan Ramani --- saltgui/static/scripts/ParseCommandLine.js | 11 +++++- tests/unit/ParseCommandLine.test.js | 41 ++++++++++++++++------ 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/saltgui/static/scripts/ParseCommandLine.js b/saltgui/static/scripts/ParseCommandLine.js index 2cce801ce..de01ce8ec 100644 --- a/saltgui/static/scripts/ParseCommandLine.js +++ b/saltgui/static/scripts/ParseCommandLine.js @@ -224,7 +224,16 @@ export class ParseCommandLine { // jobIds look like numbers but must be strings return { value: pStr }; } else if (patInteger.test(pStr)) { - return { value: Number.parseInt(pStr, 10) }; + const value = Number.parseInt(pStr, 10); + if (!Number.isSafeInteger(value)) { + // JS numbers are IEEE754 doubles, so an integer above 2**53-1 is rounded + // on the way in and salt would receive a different number than was typed. + // The salt command-line keeps the exact value (as an int, or as a string + // once it is long enough), so refusing is the only honest answer here. + // Job-ids are matched as strings before this point, see getPatJid(). + return { error: "Integer argument is too large to be sent exactly" }; + } + return { value }; } else if (patFloat.test(pStr)) { const value = Number.parseFloat(pStr); if (!Number.isFinite(value)) { diff --git a/tests/unit/ParseCommandLine.test.js b/tests/unit/ParseCommandLine.test.js index 52ff9eea2..ee0773a70 100644 --- a/tests/unit/ParseCommandLine.test.js +++ b/tests/unit/ParseCommandLine.test.js @@ -242,32 +242,53 @@ describe("Unittests for ParseCommandLine.js", () => { assert.equal(args[0], 0); assert.equal(Object.keys(params).length, 0); + // Integers beyond 2**53-1 cannot be sent exactly, so they are refused + // rather than silently rounded. These expectations compare against + // strings on purpose: written as numeric literals they would be rounded + // by the test itself and could not tell a correct value from a wrong one. + // an integer that almost looks like a jobid, but one digit less args = []; params = {}; result = ParseCommandLine.parseCommandLine("2018082000341133831", args, params); - assert.isNull(result); - assert.equal(args.length, 1); - assert.equal(args[0], 2018082000341133831); - assert.equal(Object.keys(params).length, 0); + assert.equal(result, "Integer argument is too large to be sent exactly"); + assert.equal(args.length, 0); // an integer that almost looks like a jobid, but one digit more args = []; params = {}; result = ParseCommandLine.parseCommandLine("201808200034113383170", args, params); - assert.isNull(result); - assert.equal(args.length, 1); - assert.equal(args[0], 201808200034113383170); - assert.equal(Object.keys(params).length, 0); + assert.equal(result, "Integer argument is too large to be sent exactly"); + assert.equal(args.length, 0); // an integer that almost looks like a jobid, just not a true date-time args = []; params = {}; result = ParseCommandLine.parseCommandLine("20182820003411338317", args, params); + assert.equal(result, "Integer argument is too large to be sent exactly"); + assert.equal(args.length, 0); + + // the largest integer that is still exact, and the first that is not + args = []; + params = {}; + result = ParseCommandLine.parseCommandLine("9007199254740991", args, params); assert.isNull(result); assert.equal(args.length, 1); - assert.equal(args[0], 20182820003411338317); - assert.equal(Object.keys(params).length, 0); + assert.equal(String(args[0]), "9007199254740991"); + + args = []; + params = {}; + result = ParseCommandLine.parseCommandLine("9007199254740993", args, params); + assert.equal(result, "Integer argument is too large to be sent exactly"); + + // a real jobid is matched as a string before the integer branch, so it + // still round-trips exactly + args = []; + params = {}; + result = ParseCommandLine.parseCommandLine("20180814033130818988", args, params); + assert.isNull(result); + assert.equal(args.length, 1); + assert.equal(args[0], "20180814033130818988"); // FLOAT From 98f9a81846cd1338c730afd83e813183761ad3a5 Mon Sep 17 00:00:00 2001 From: Roshan Ramani Date: Mon, 7 Sep 2026 08:33:46 +0530 Subject: [PATCH 2/5] Table-drive the new integer tests SonarQube flagged 39% duplication on new code: the added cases repeated the same four-line setup-and-assert block six times. Two loops over a list of values cover the same inputs in half the lines, and name the value in each assertion message so a failure still says which one broke. No change to what is asserted. ai-assisted-by: Claude Code (Claude Opus 5) Signed-off-by: Roshan Ramani --- tests/unit/ParseCommandLine.test.js | 79 ++++++++++++----------------- 1 file changed, 33 insertions(+), 46 deletions(-) diff --git a/tests/unit/ParseCommandLine.test.js b/tests/unit/ParseCommandLine.test.js index ee0773a70..5191cc348 100644 --- a/tests/unit/ParseCommandLine.test.js +++ b/tests/unit/ParseCommandLine.test.js @@ -243,52 +243,39 @@ describe("Unittests for ParseCommandLine.js", () => { assert.equal(Object.keys(params).length, 0); // Integers beyond 2**53-1 cannot be sent exactly, so they are refused - // rather than silently rounded. These expectations compare against - // strings on purpose: written as numeric literals they would be rounded - // by the test itself and could not tell a correct value from a wrong one. - - // an integer that almost looks like a jobid, but one digit less - args = []; - params = {}; - result = ParseCommandLine.parseCommandLine("2018082000341133831", args, params); - assert.equal(result, "Integer argument is too large to be sent exactly"); - assert.equal(args.length, 0); - - // an integer that almost looks like a jobid, but one digit more - args = []; - params = {}; - result = ParseCommandLine.parseCommandLine("201808200034113383170", args, params); - assert.equal(result, "Integer argument is too large to be sent exactly"); - assert.equal(args.length, 0); - - // an integer that almost looks like a jobid, just not a true date-time - args = []; - params = {}; - result = ParseCommandLine.parseCommandLine("20182820003411338317", args, params); - assert.equal(result, "Integer argument is too large to be sent exactly"); - assert.equal(args.length, 0); - - // the largest integer that is still exact, and the first that is not - args = []; - params = {}; - result = ParseCommandLine.parseCommandLine("9007199254740991", args, params); - assert.isNull(result); - assert.equal(args.length, 1); - assert.equal(String(args[0]), "9007199254740991"); - - args = []; - params = {}; - result = ParseCommandLine.parseCommandLine("9007199254740993", args, params); - assert.equal(result, "Integer argument is too large to be sent exactly"); - - // a real jobid is matched as a string before the integer branch, so it - // still round-trips exactly - args = []; - params = {}; - result = ParseCommandLine.parseCommandLine("20180814033130818988", args, params); - assert.isNull(result); - assert.equal(args.length, 1); - assert.equal(args[0], "20180814033130818988"); + // rather than silently rounded. Expectations compare against strings on + // purpose: written as numeric literals they would be rounded by the test + // itself and could not tell a correct value from a wrong one. + const tooLargeIntegers = [ + // almost a jobid, but one digit less + "2018082000341133831", + // almost a jobid, but one digit more + "201808200034113383170", + // jobid-shaped, just not a true date-time + "20182820003411338317", + // the first integer that is no longer exact + "9007199254740993" + ]; + for (const nr of tooLargeIntegers) { + args = []; + params = {}; + result = ParseCommandLine.parseCommandLine(nr, args, params); + assert.equal(result, "Integer argument is too large to be sent exactly", nr); + assert.equal(args.length, 0, nr); + } + + // The largest exact integer is still accepted, and a real jobid is matched + // as a string before the integer branch, so it round-trips unchanged. + const exactValues = ["9007199254740991", "20180814033130818988"]; + for (const nr of exactValues) { + args = []; + params = {}; + result = ParseCommandLine.parseCommandLine(nr, args, params); + assert.isNull(result, nr); + assert.equal(args.length, 1, nr); + assert.equal(String(args[0]), nr, nr); + assert.equal(Object.keys(params).length, 0, nr); + } // FLOAT From b3dcf924bfb31c74fb0b95ece2cab4e7d13cc552 Mon Sep 17 00:00:00 2001 From: Roshan Ramani Date: Mon, 7 Sep 2026 08:37:34 +0530 Subject: [PATCH 3/5] Name the assertion label separately from the value SonarQube read `assert.equal(String(args[0]), nr, nr)` as a likely copy-paste slip, which is fair: the same variable served as both the expected value and the failure message. Each loop now builds a label once and uses it for the messages, leaving the value argument on its own. --- tests/unit/ParseCommandLine.test.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/unit/ParseCommandLine.test.js b/tests/unit/ParseCommandLine.test.js index 5191cc348..6ef40db0d 100644 --- a/tests/unit/ParseCommandLine.test.js +++ b/tests/unit/ParseCommandLine.test.js @@ -257,24 +257,26 @@ describe("Unittests for ParseCommandLine.js", () => { "9007199254740993" ]; for (const nr of tooLargeIntegers) { + const label = "value " + nr; args = []; params = {}; result = ParseCommandLine.parseCommandLine(nr, args, params); - assert.equal(result, "Integer argument is too large to be sent exactly", nr); - assert.equal(args.length, 0, nr); + assert.equal(result, "Integer argument is too large to be sent exactly", label); + assert.equal(args.length, 0, label); } // The largest exact integer is still accepted, and a real jobid is matched // as a string before the integer branch, so it round-trips unchanged. const exactValues = ["9007199254740991", "20180814033130818988"]; for (const nr of exactValues) { + const label = "value " + nr; args = []; params = {}; result = ParseCommandLine.parseCommandLine(nr, args, params); - assert.isNull(result, nr); - assert.equal(args.length, 1, nr); - assert.equal(String(args[0]), nr, nr); - assert.equal(Object.keys(params).length, 0, nr); + assert.isNull(result, label); + assert.equal(args.length, 1, label); + assert.equal(String(args[0]), nr, label); + assert.equal(Object.keys(params).length, 0, label); } // FLOAT From ecbc965d1c7d97edb7e7cb554202f42172b78836 Mon Sep 17 00:00:00 2001 From: Roshan Ramani Date: Mon, 14 Sep 2026 07:16:33 +0530 Subject: [PATCH 4/5] Accept integers that render back as themselves isSafeInteger refuses everything above 2**53-1, but the value is sent as its own decimal rendering, so what matters is whether that rendering is the text that was typed. 2**53, 2**54 and 10**16 all survive; they were being refused. Comparing the rendering against the input accepts those and still refuses 12345678901234567, which comes back as ...68. The error now names the two ways out, as suggested in review. --- saltgui/static/scripts/ParseCommandLine.js | 14 ++++++------ tests/unit/ParseCommandLine.test.js | 25 ++++++++++++++++------ 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/saltgui/static/scripts/ParseCommandLine.js b/saltgui/static/scripts/ParseCommandLine.js index de01ce8ec..db351b5cd 100644 --- a/saltgui/static/scripts/ParseCommandLine.js +++ b/saltgui/static/scripts/ParseCommandLine.js @@ -225,13 +225,13 @@ export class ParseCommandLine { return { value: pStr }; } else if (patInteger.test(pStr)) { const value = Number.parseInt(pStr, 10); - if (!Number.isSafeInteger(value)) { - // JS numbers are IEEE754 doubles, so an integer above 2**53-1 is rounded - // on the way in and salt would receive a different number than was typed. - // The salt command-line keeps the exact value (as an int, or as a string - // once it is long enough), so refusing is the only honest answer here. - // Job-ids are matched as strings before this point, see getPatJid(). - return { error: "Integer argument is too large to be sent exactly" }; + // The number is sent as its own decimal rendering, so it survives exactly + // when that rendering is the text that was typed. That is a wider range + // than isSafeInteger allows: 2**53 and 10**16 render back unchanged, while + // 12345678901234567 becomes ...68. Job-ids are matched as strings before + // this point, see getPatJid(). + if (String(value) !== pStr.replace(/^\+/, "")) { + return { error: "Integer argument is too large to be sent exactly. Make it a string by surrounding it with quotes or make it a float by appending .0" }; } return { value }; } else if (patFloat.test(pStr)) { diff --git a/tests/unit/ParseCommandLine.test.js b/tests/unit/ParseCommandLine.test.js index 6ef40db0d..38af2db35 100644 --- a/tests/unit/ParseCommandLine.test.js +++ b/tests/unit/ParseCommandLine.test.js @@ -253,21 +253,34 @@ describe("Unittests for ParseCommandLine.js", () => { "201808200034113383170", // jobid-shaped, just not a true date-time "20182820003411338317", - // the first integer that is no longer exact - "9007199254740993" + // the first integer that no longer renders back as itself + "9007199254740993", + // loses its last digit: it renders back as ...68 + "12345678901234567" ]; for (const nr of tooLargeIntegers) { const label = "value " + nr; args = []; params = {}; result = ParseCommandLine.parseCommandLine(nr, args, params); - assert.equal(result, "Integer argument is too large to be sent exactly", label); + assert.equal( + result, + "Integer argument is too large to be sent exactly. Make it a string by surrounding it with quotes or make it a float by appending .0", + label); assert.equal(args.length, 0, label); } - // The largest exact integer is still accepted, and a real jobid is matched - // as a string before the integer branch, so it round-trips unchanged. - const exactValues = ["9007199254740991", "20180814033130818988"]; + // Anything whose decimal rendering is the text that was typed is accepted, + // which reaches past isSafeInteger: 2**53, 2**54 and 10**16 all render back + // unchanged. A real jobid is matched as a string before the integer branch, + // so it survives too. + const exactValues = [ + "9007199254740991", + "9007199254740992", + "18014398509481984", + "10000000000000000", + "20180814033130818988" + ]; for (const nr of exactValues) { const label = "value " + nr; args = []; From b54992fb216761c922f4e2742c85dcae36498d14 Mon Sep 17 00:00:00 2001 From: Roshan Ramani Date: Mon, 14 Sep 2026 14:39:24 +0530 Subject: [PATCH 5/5] Explain the round-trip check on its own terms Assisted-by: Claude Opus 5 (Claude Code) --- saltgui/static/scripts/ParseCommandLine.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/saltgui/static/scripts/ParseCommandLine.js b/saltgui/static/scripts/ParseCommandLine.js index db351b5cd..2f7122340 100644 --- a/saltgui/static/scripts/ParseCommandLine.js +++ b/saltgui/static/scripts/ParseCommandLine.js @@ -225,11 +225,10 @@ export class ParseCommandLine { return { value: pStr }; } else if (patInteger.test(pStr)) { const value = Number.parseInt(pStr, 10); - // The number is sent as its own decimal rendering, so it survives exactly - // when that rendering is the text that was typed. That is a wider range - // than isSafeInteger allows: 2**53 and 10**16 render back unchanged, while - // 12345678901234567 becomes ...68. Job-ids are matched as strings before - // this point, see getPatJid(). + // The number travels to salt as its own decimal rendering, so it arrives + // unchanged exactly when that rendering is the text that was typed. + // 12345678901234567 renders back as ...68, and sending a different number + // than the one that was typed is worse than refusing it here. if (String(value) !== pStr.replace(/^\+/, "")) { return { error: "Integer argument is too large to be sent exactly. Make it a string by surrounding it with quotes or make it a float by appending .0" }; }