Security lint #45
Merged
Security lint #45
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Address instances of "Implicit narrowing conversion in compound assignment"
heeheehee-kolmafia
approved these changes
Sep 28, 2021
fredg1
added a commit
to fredg1/kolmafia
that referenced
this pull request
Oct 5, 2021
* Set revision when building jar with gradle (#17) * Add revision to jar manifest and load in code * Override revision in tests * Remove RELEASED * Enable releases * move to StaticEntity, add build info * Fix tests for revision numbers * Change product name to CONSTANT_CASE * Capitalize getBuildInfo properly * We only have one MANIFEST file * Use r0 instead of rUnknown, make r0 pass all `since` checks * Remove another manifest loop * Change Modified classifier back to M * Use system-agnostic File constructor on gradle * Added shadow plugin * Fix version numbers - we have to use shadowJar * Final tidy-up * Do not run the PR tests twice Co-authored-by: Rinn <Kirchoff.Joseph.P@gmail.com> Co-authored-by: BadHorseMonkey <58645293+BadHorseMonkey@users.noreply.github.com> * Create jenkinsfile (#16) * Create jenkinsfile Adding the initial version of jenkinsfile for ci.kolmafia.us * Update jenkinsfile change build target to jar to update revision numbers * Update jenkinsfile Update target to shadowJar * feat: add portable Spacegate (open) (kolmafia#19) * dev: use 3-clause BSD license (kolmafia#18) * Publish test report in coverage, fix revision on release, fix NullPointerExecption (kolmafia#26) * Build shadowJar instead of jar for releases (kolmafia#31) * Remove individual licenses in favour of root license (kolmafia#21) * license which evaded the deletion (kolmafia#32) * Language Server - parsetree abstracts (kolmafia#30) ( from https://kolmafia.us/threads/ash-language-server-features.26098/post-163993 ) A few appetizers while we wait for the rest: * The classes CompositeType, CompositeValue and AggregateValue should be made abstract. * The method CompositeType.getDataType( Object ) can be made abstract. Rather than having Type.toString() and RecordType.toString() both be return this.name, just make Symbol.toString() { return this.name; } (this also incidentally adds the method to Variable and Function, which helps in the debugger) * Language Server - parseType records (kolmafia#29) ( from https://kolmafia.us/threads/ash-language-server-features.26098/post-163762 ) This changes the logic of parseType to attempt to parse records even when not allowed (again, with the continuous parsing, we need to try anyway). This is possible because the method parseRecord starts by looking for the token "record", immediately returning null if it doesn't find it. "record" is a reserved word exclusively used to declare new records, so nothing is hindered by checking for them right away. * Language Server - parseFunction vararg (kolmafia#28) ( from https://kolmafia.us/threads/ash-language-server-features.26098/post-163765 ) This modifies the logic of parseFunction a bit so that the vararg error checks happen after the variable name was read. This will allow us to have the full variable + type's location, rather than only the type, once those are implemented. Additionally, this allows the "Only one vararg parameter is allowed" error message to be reached. * Language Server - parseAggregateLiteral arrays (kolmafia#22) (from https://kolmafia.us/threads/ash-language-server-features.26098/post-163764 ) Alters parseAggregateLiteral a little so that we try to read key-value pairs (components of a map literal) even once we identified that the user is supplying an array literal (which doesn't expect keys). * Language Server - parseVariable initializations (kolmafia#27) ( from https://kolmafia.us/threads/ash-language-server-features.26098/post-163766 ) A bit more complex than the last few; this patch modifies parseVariable so that the signal indicating that we're parsing parameters is a boolean, rather than scope being null, and waits until the end to throw the error. This, again, will allow us to still keep trying to parse a parameter initialization, and will give us a full location once we throw the error. Also shuffles the parsing logic a little, for efficiency. * not supposed to touch that one either..? Why'd the test work anyway, then..? (context: kolmafia@d353d73) * way to pass the variable name (and location, in the future!) * Updated test to initialize TurnCounters prior to testing them. Not all systems need this, apparently. (kolmafia#33) * to_int() and the .id proxy record value can now extract the snarfblat from a $location. Non-adventure.php-locations will return -1 (kolmafia#34) * Update jenkinsfile (kolmafia#25) * Update jenkinsfile Update jenkinsfile to use --no-daemon and change the test build targets. * Update jenkinsfile Added a larger shallow depth * Update jenkinsfile * Update jenkinsfile Update Java version as 1.8 was causing issues on Jenkins instance. Now building and testing against Java 16. * Move over sourceforge references and remove unused donation section (kolmafia#35) * Move over sourceforge references and remove unused donation section * Include buffbot.xsl file in repo * Remove VariableReference(String, BasicScope) (kolmafia#39) * turn (Type) == (Type) into Type.equals( Type ) (kolmafia#36) * Prune old jars when building with gradle (kolmafia#40) * Prune old jars in when building with gradle * Force gradlew to always use unix line endings * Add line ending to end of file * Delete -M if not dirty, etc * Change some ints to longs in BasementRequest (kolmafia#42) * feat: try multiple faxbots if the first fails in the CLI (kolmafia#37) * feat: try multiple faxbots if the first fails in the CLI * chore: fmt * fix: also try another bot if many matches for command * Security lint (kolmafia#45) Addressed instances of Implicit narrowing conversion in compound assignment flagged by security checking. Trivial since the issue was just a local int that should have been changed when MP, HP, etc. went to longs. Remaining instances seem to require changing global structures or method signatures. Keeping this small to validate that I know how to "commit changes with git". * Update DiscoCombatHelper.java * Implicit naaarrowing Address instances of "Implicit narrowing conversion in compound assignment" * Language Server - Command class (kolmafia#20) * manually re-add to ef2d685 5672b9a a3d01c9 6a57eae * remove license from Command.java * Use Java 9, add spotless w/ google java style. (kolmafia#47) This is a proof-of-concept, and does not actually apply any formatting changes yet. I'd prefer to make that formatting change in a standalone commit. This also removes some comments in the middle of imports in KoLConstants.java because `./gradlew spotlessCheck` was yelling at me, because Google Java Format (gojf) doesn't like that. To reformat the code, you'll run `./gradlew spotlessApply`. This adds some flags so gojf continues to work for newer Java versions (16+). But for that, requires that the Java compiler be new enough to support exports, aka at least Java 9, which coincidentally is the highest version supported by OpenClover. We really should switch away from OpenClover to something like JaCoCo. (Coming soon!) * Implicit Narrowing part 2 (kolmafia#46) Address additional implicit narrowing warnings * Migrate from OpenClover to JaCoCo for coverage (kolmafia#48) * Use JaCoCo for coverage instead of Clover. They have support for Java > 9... * Remove Clover coverage comment. This branch, if merged, will mean that we're no longer using Clover, so it'll be moot. * Update jenkinsfile, too. I think this should work, but I could be wrong. * Separate core gradle plugins from third-party. This change also alphabetizes the plugins within each block to provide some semblance of structure. * Revert "Downgrade gradle wrapper to 7.0." (kolmafia#55) This reverts commit c77effb. Now that we're recommending people to use Java 17, they're having issues building. We need at least Gradle 7.1.1 to build with Java 17. * Updated to add JaCoCo test reports to Jenkins CI (kolmafia#54) * Update jenkinsfile Updated to add JaCoCo test reports to Jenkins CI * Update jenkinsfile * Correct merge mistakes brought by the rebase * Language server rebased spotless apply (#13) * Format the entirety of src/, test/. (kolmafia#50) This was done with `./gradlew spotlessApply`, followed by iterated application of sed to deal with my precommit hook complaining (tabs following space indentation as a result of commented-out blocks of code...) * Use Java 17 in actions (kolmafia#51) * Updating all dependencies except SwingX (kolmafia#58) ANTLR 3 Runtime 3.4 -> 3.52 FlatLaf 1.5 -> 1.6 FlatlLaf IntelliJ Themes Pack 1.5 -> 1.7 Add Flatlaf SwingX 1.6 JUnit Jupiter API 5.7.1 -> 5.8.0 JUnit Platform Launcher 1.7.2 -> 1.8.0 JUnit Vintage Engine 5.7.1 -> 5.8.0 Java Native Access 5.6.0 -> 5.9.0 Java Native Access Platform 5.6.0 -> 5.9.0 jansi 2.3.2 ->2.3.4 skipped SwingX, because it is causing conflicts with LockableListModel. working locally, passing tests. * Use browser launching code available since Java 1.6 (kolmafia#60) * Create CODEOWNERS file (kolmafia#61) This will just get github to automatically assign one of the team for reviewing PRs. I don't really recommend we use it in any more fine-grained way than this. * Run gradle checks for pull requests. (kolmafia#57) * Run gradle checks for pull requests. Among many other things, thiss will run spotlessCheck to make sure that code has been properly formatted. * Rename Run Checks -> Run Tests. Apparently our repository still requires _something_ to be called Run Tests. I guess I'd rather have a slightly misnamed task, than an empty or redundant task. * Require all files in test/root use newlines. (kolmafia#63) Our test setup specifies that the newline separator be \n so we can actually use golden files for testing output of scripts. This doesn't work so well when git mangles the golden files by adding newlines to them. Co-authored-by: heeheehee <heeheehee@users.sourceforge.net> * Deprecating ANT (kolmafia#41) * Deprecating ANT * Deprecating ANT * Enable gradle build cache (kolmafia#65) * Remove OSXAdapter and replace with Java built-ins (kolmafia#64) * Fix Taskbar API detection on unsupported platforms (kolmafia#67) * Only increment skill usage for successes. (kolmafia#56) * Only increment skill usage for successes. Thanks to phreddrickk @ kolmafia.us for reporting this issue. This adds an example of how one might use Mockito to inject a custom response. It's not super-well-documented, but I promise all the bits are necessary. In particular: we need to set session ID or else GenericRequest.run() will short-circuit, and we need to set max / current MP and "learn" the skill so UseSkillRequest.run() won't reject it before even passing it on to GenericRequest. These test cases follow the standard arrange / act / assert model. * Add a RequestTestBase for common setup / helpers. Right now, this provides some testing the happy path: you specify some response text for a spy, and we inject that data when you execute spy.run(). Important: in order for everything to work end-to-end, you need to provide a spy, not a regular mock. This allows us to exercise as much real behavior as possible. * Create README.md (kolmafia#62) * Create README.md Just our description and some useful status badges * Point release badge to the latest release page * Better revision capture for StaticEntity (kolmafia#68) * Better revision capture for StaticEntity * Check that attributes isn't null * Restore kolmafia-license.txt Somewhere we lost the license file and it's needed for About frame. This change returned the about box from the mists of time. If the correct answer is to remove the link for it, we coud do that, too. * Run github PR checks against latest mac/windows. (kolmafia#70) * Run checks against latest mac/windows. This also opts to use ubuntu-latest so we don't have to manually configure it in the future. Note that there are technically newer OSes than windows-latest and macos-latest at this time, but I anticipate that these labels will be automatically migrated over time (unlike a fixed label that will require periodic maintenance). * Add a placeholder dependent action. This is effectively a no-op, but we need to perform _some_ action in each step, and we need at least one step in each job. This allows us to get around the naming issues where the main branch expects a check with the name "Run Tests" to complete, while the matrix strategy modifies the name of each of the subjobs. * Comment on why we have this placeholder action. This also improves the stdout message from "1" to "All checks succeeded." because it doesn't cost anything to change a print statement... * Implicit Narrow (kolmafia#53) * Implicit Narrow Eliminate the last of the implicit narrowing by cast warnings. * Steps towards goal Added test, reformatted a source file. * Update SkillDatabaseTest.java * Handle implicit narrowing Create test for before/after comparison. Change code for easier access from test and then replace eventually incorrect calculation using Pow with something that is correct and won't overflow. * Fix Formatting Apparently telling IntelliJ to apply the Google format doesn't do what spotlessapply does. * Update SVNManager.java (kolmafia#71) * Remove Ant build (kolmafia#66) * Use grgit for updating the repo from origin/main. While we're at it, also remove the need for the git CLI for building from source. We do need this to be run in a git repository with a .git directory, though... And, remove the dependency on build.xml and default.properties, instead setting java.version in gradle.properties. * Delete build.xml, default.properties, jars. We haven't yet ported the javadoc task, exe / dmg building, or whatever experimental coverage viewer there was. We might not need any of those. * Also delete build.properties. I think this is only used in the Ant build. * Delete the download, toCopy parts of build.gradle. We no longer want to bundle jars explicitly (as we can / should instead rely on Gradle to handle dependency management), so there's no point in downloading them. * Only count revisions for commits on origin/main. This should prevent revision drift for those with lots of local commits. * Clean up build.gradle further. - Fix formatting, which doesn't seem to be addressed by spotless for some reason. - Remove svnRevList. We're steering hard into using git. - Remove redundant dependencies on gitRevList. pruneDist already relies on it, so things that rely on pruneDist shouldn't need to take an additional dependency. * Improve gitUpdate functionality. - Detect when no update is required. - Pull from the current tracked branch, as opposed to always origin/main. - If an update is required, "stash" the existing changes by creating a temp commit, rebase onto that temp commit, and then reset that commit. * Fix gradle formatting to use Eclipse style. I don't really care about the style as long as it actually works for nested blocks, which the previous one did not. * Format gradle files exclusively with greclipse(). I think this is causing check failures. * Add env variable for overriding the head commit. This should allow us to pass $GITHUB_SHA when we detect the number of revisions, rather than always using origin/main (which results in building the same revision multiple times if PRs are merged in quick succession). * Typo (kolmafia#73) Fixed a typo * Fix java source & target compatibility settings (kolmafia#75) * Link to the game in the README (kolmafia#76) * Test additional java releases (kolmafia#74) * Fix release build not using correct commit (kolmafia#72) * Option to pass commit to gradle * Fix release always using origin/main * Include commit in getRevision inputs * Remove Run Tests (I updated the branch protection) * switch to project property * Add info logging * Numeric cast (kolmafia#81) * Update BasementRequest.java Eliminate narrowing cast flagged by security * Update BasementRequest.java Clean ups suggested by IntelliJ's inspection. * Update BasementRequest.java spotlessApply * Update BasementRequest.java * Migrate JUnit4 tests to JUnit5 (kolmafia#78) * Migrate tests to JUnit 5. The most annoying ones were parameterized tests, since the structure changed appreciably. The others were mostly handled automatically by IntelliJ's migration actions, coupled with some search-and-replace to use assertions statically. Note that this is based on the `antless` branch, since I didn't want to deal with build.gradle merge conflicts shortly thereafter. * Remove leftover JUnit 4 dependencies. One test (SkillDatabaseTest) was added after I did the initial migration. This outright removes ErrorCollector from DataFileConsistencyTest, since it no longer exists in JUnit 5. We could continue using it, but I'd prefer not to if it means we can remove the dependency entirely. * Remove JUnit 4, vintage engine from build.gradle. To prevent backsliding in the future. * Publish test reports. While there's a comment insisting that it relies on pull_request_target (not pull_request), the example code for the action suggests using it with pull_request. Worth a shot, since I have no idea why a test is failing for Mac. * Convert CustomScriptTest to a parameterized test. I still have no idea why this is hanging in the MacOS build. ... Note after the fact: this is no longer hanging in the MacOS build, as of the migration to parameterized tests. I don't understand, but I'm not complaining. Could have been a transient issue with the github test runners. * Update BasementRequest.java (kolmafia#83) Somehow I failed to commit the most important line in a previous commit. * Add -M modified flag to version number inside KoLmafia code (for title bar, logs, about, CLI version, etc) (kolmafia#77) * handle dirty repo builds with explicit flag that matches build. * apply Spotless locally Co-authored-by: heeheehee-kolmafia <83351434+heeheehee-kolmafia@users.noreply.github.com> Co-authored-by: Joe Kirchoff <Kirchoff.Joseph.P@gmail.com> Co-authored-by: BadHorseMonkey <58645293+BadHorseMonkey@users.noreply.github.com> Co-authored-by: Samuel Gaus <sam@gaus.co.uk> Co-authored-by: Jamie Adams <82782908+jaadams5@users.noreply.github.com> Co-authored-by: heeheehee <heeheehee@users.sourceforge.net> Co-authored-by: Samuel Gaus <sam@gaus.co.uk> Co-authored-by: Rinn <Kirchoff.Joseph.P@gmail.com> Co-authored-by: BadHorseMonkey <58645293+BadHorseMonkey@users.noreply.github.com> Co-authored-by: Daniel Clements <fewyn@fewyn.net> Co-authored-by: Chris Midgley <midgleyc@gmail.com> Co-authored-by: Chris Midgley <chris.midgley@dunecomputers.co.uk> Co-authored-by: Jamie Adams <82782908+jaadams5@users.noreply.github.com> Co-authored-by: heeheehee-kolmafia <83351434+heeheehee-kolmafia@users.noreply.github.com> Co-authored-by: heeheehee <heeheehee@users.sourceforge.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addressed instances of Implicit narrowing conversion in compound assignment flagged by security checking. Trivial since the issue was just a local int that should have been changed when MP, HP, etc. went to longs. Remaining instances seem to require changing global structures or method signatures. Keeping this small to validate that I know how to "commit changes with git".