|
From: Vest <no...@gi...> - 2026-06-03 03:14:04
|
Branch: refs/heads/master Home: https://github.com/PCGen/pcgen Commit: c4cbc43ba1f8a283791d4426c79d54bbdee68998 https://github.com/PCGen/pcgen/commit/c4cbc43ba1f8a283791d4426c79d54bbdee68998 Author: Vest <Ve...@us...> Date: 2026-06-03 (Wed, 03 Jun 2026) Changed paths: M code/src/java/pcgen/core/analysis/SkillModifier.java M code/src/java/pcgen/gui2/dialog/DataInstaller.java A code/src/utest/pcgen/gui2/dialog/DataInstallerZipSlipTest.java Log Message: ----------- fix: address CodeQL findings (zip-slip + implicit narrowing) (#7576) * DataInstaller: reject archive entries that escape the install dir CodeQL java/zipslip flagged populateFileAndDirLists at lines 750/753; the actual sink is createFiles at line 619 (FileOutputStream) and createDirectories at line 617 (mkdirs), reached via a string-concat correctFileName that didn't normalise '..' segments. correctFileName now resolves each entry against its base directory and rejects any path whose canonical form leaves that base. Both canonicalisations happen on the resolved File, so symlinks, '.' and '..' segments are all collapsed before comparison. The two callers that didn't already throw IOException (checkOverwriteOK and createDirectories) catch and abort the install with the existing error-dialog pattern. A hostile data set containing 'data/../../etc/whatever' would now be refused before any file is written. * SkillModifier: make double-to-int truncation explicit CodeQL java/implicit-cast-in-compound-assignment flagged 12 sites in this file where a 'double' bonus was accumulated into an 'int' via +=, which silently inserts (int) at every addition. The intent was always truncating accumulation -- the function returns Integer and uses .intValue() for the formula path -- so the warning is purely about making the cast visible. Fix: write the cast at every site. No behaviour change; the bytecode already contained the same i2d/d2i pair. * DataInstaller: regression test for zip-slip rejection Reflective unit test so the same class compiles against both the pre-fix (`private`, no checked exception) and post-fix (package-private, throws IOException) signatures of correctFileName. Verified by hand: temporarily reverting DataInstaller to master and re-running this test yields 1 pass + 2 failures (both '..'-escape cases are silently accepted under the old logic), and restoring the fix flips it to 3 passes -- which makes the test a real regression guard rather than a tautology. correctFileName goes from `private` to package-private to give the test in the same package direct access; reflection was needed only for the cross-state run. To unsubscribe from these emails, change your notification settings at https://github.com/PCGen/pcgen/settings/notifications |