Fix the 37 SpotBugs findings and gate the build on all quality checks - #130
Conversation
Real fixes: - ShellFileCopy.copyFile: guard against Path.getFileName() returning null for a root path (NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE), with a unit test - WinRMSession: make the mutable handshake state volatile so writes on one per-call worker thread are visible to the next (AT_STALE_THREAD_WRITE_OF_PRIMITIVE, AT_NONATOMIC_64BIT_PRIMITIVE) - NtlmCrypto: build the OriginalContent header by concatenation so the protocol-mandated CRLF is not a format string (VA_FORMAT_STRING_USES_NEWLINE) - MD4.update: drop the dead trailing curBufferPos store (DLS_DEAD_LOCAL_STORE) - WqlQuery/WinRMWqlExecutor: defensive copies and unmodifiable views for the collection getters (EI_EXPOSE_REP/EI_EXPOSE_REP2), documented in README Justified suppressions (@SuppressFBWarnings with reasons): - CipherGen: EI_EXPOSE_REP/EI_EXPOSE_REP2 on the upstream NTLM crypto port - WinRMEndpoint: the password char[] is deliberately shared, not copied - NTLMMessage/Type2Message/Type3Message/WinRMEndpoint: CT_CONSTRUCTOR_THROW, parse/validate-in-constructor is the upstream design Build gating: mvn verify now fails on any Checkstyle, PMD, CPD or SpotBugs finding (checkstyle:check, pmd:check, pmd:cpd-check, spotbugs:check bound to the verify phase, pinned to the same versions as the site reports). Closes #124 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35668f60ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
WinRMWqlExecutor now copies each result row (not just the outer list) and returns rows that are unmodifiable down to the row level; WqlQuery.getSubPropertiesMap() wraps each subproperty set unmodifiable too. Both covered by new mutation tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec2a0de69e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
license:update-file-header does not cover test sources; header added manually, matching the other test files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What
Resolves every SpotBugs finding (closes #124) and — as requested — gates
mvn verifyon all four quality tools: a build now fails on any Checkstyle, PMD, CPD or SpotBugs finding.Real fixes (correctness findings fixed, not suppressed)
ShellFileCopy.copyFile(NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE):Path.getFileName()returnsnullfor a root path such asC:\; the method now rejects such paths with a clearIllegalArgumentExceptionbefore any remote interaction. Covered by a new unit test.WinRMSession(AT_STALE_THREAD_WRITE_OF_PRIMITIVE×3,AT_NONATOMIC_64BIT_PRIMITIVE×2): the mutable handshake state (negotiateFlags, signing keys, RC4 ciphers,authenticated) is nowvolatile— the session outlives individual operations and each operation runs on a freshUtils.executeworker thread, so cross-thread visibility is required (accesses are sequential, never concurrent).NtlmCrypto.encryptAndSign(VA_FORMAT_STRING_USES_NEWLINE): theOriginalContentheader is now built by plain concatenation, making it obvious the protocol-mandated CRLF is a deliberate literal.MD4.update(DLS_DEAD_LOCAL_STORE): removed the dead trailingcurBufferPosstore.WqlQuery/WinRMWqlExecutor(EI_EXPOSE_REP/EI_EXPOSE_REP2): collection getters now return unmodifiable views andWinRMWqlExecutorcopies the lists passed to its constructor. All internal callers only read these collections; the behavior change is documented in the README 2.x upgrade section.Justified suppressions (
@SuppressFBWarningswith stated reasons)CipherGen(EI_EXPOSE_REP×12,EI_EXPOSE_REP2): the upstream NTLM crypto port deliberately caches and returns its computed hashes/responses; defensive copies would churn the port for no security benefit. Member-level annotations (class-level triggeredUS_USELESS_SUPPRESSION_ON_CLASS) sharing one justification constant.WinRMEndpoint(EI_EXPOSE_REP/EI_EXPOSE_REP2): the passwordchar[]is deliberately shared so there is a single wipeable copy of the secret.NTLMMessage/Type2Message/Type3Message/WinRMEndpoint(CT_CONSTRUCTOR_THROW): parse/validate-in-constructor is the upstream design; the NTLM message classes cannot be subclassed outside their package, so no finalizer-attack surface.The new
spotbugs-annotationsdependency isprovided-scope (compile-time only), so the library remains dependency-free at runtime.Build gating
checkstyle:check,pmd:check,pmd:cpd-checkandspotbugs:checkare now bound to theverifyphase, pinned to the exact versions/configs the site reports use (Checkstyle 3.6.0 +checkstyle.xml, PMD 3.26.0 +pmd.xml, CPDminimumTokens=50, SpotBugs 4.9.3.0), so the gates and the reports always agree.Verification
target/spotbugsXml.xml: 0 bug instances; CPD, PMD and Checkstyle reports also at 0.mvn verify sitegreen on JDK 17 (like CI) with all four gates active: 80 unit tests + 1 IT pass.mvn formatter:format.🤖 Generated with Claude Code