Skip to content

fix: resolve MAJOR Sonar bugs in HttpClientAdapter and OasImportConverter#420

Open
jlouvel wants to merge 1 commit intomainfrom
fix/sonar-major-bugs
Open

fix: resolve MAJOR Sonar bugs in HttpClientAdapter and OasImportConverter#420
jlouvel wants to merge 1 commit intomainfrom
fix/sonar-major-bugs

Conversation

@jlouvel
Copy link
Copy Markdown
Contributor

@jlouvel jlouvel commented May 6, 2026

Related Issue

Closes #417, closes #418, closes #419


What does this PR do?

Phase 1 of the Sonar Bug Remediation blueprint — resolve all 4 MAJOR bugs reported by the SonarQube quality gate (run #1516, main @ dcfd01c):

Rule File Lines Fix
java:S2116 HttpClientAdapter.java 103, 115 Replace getPassword().toString() with new String(getPassword())
java:S2259 OasImportConverter.deriveResourceName 211–219 Guard path and slug against null; fall back to "root"
java:S5850 OasImportConverter.toKebabCase 513 Replace ambiguous "^-|-$" with explicitly-grouped "(^-)|(-$)"

The S2116 fix is the only one with runtime impact — before this change, every Basic and Digest authenticated outbound HTTP call was sending the JVM array identity (e.g. [C@5a3b7c) as the secret instead of the configured password. The two existing auth tests masked the bug because they only asserted assertNotNull(getSecret()).


Tests

  • HttpClientAdapterTest.basicAuthenticationShouldSetIdentifierAndSecret and digestAuthenticationShouldSetIdentifierAndSecret: strengthened from assertNotNull(getSecret()) to assertEquals("<expected>", String.valueOf(getSecret())). These strengthened assertions fail on main (proving the S2116 bug), and pass after the fix. A comment in each test explains why this assertion strengthening is itself the non-regression guard.
  • OasImportConverterTest.deriveResourceNameShouldFallBackToRootWhenPathIsNull (new): verifies the null-path fallback. Fails with NPE before the fix.
  • OasImportConverterTest.toKebabCaseShouldTrimLeadingAndTrailingHyphens (new): verifies the regex still trims both ends. Passes against both old and new regex (S5850 is a code-quality issue, not a behavioral bug); kept as a defensive regression test for the new explicitly-grouped form.

Verification

HttpClientAdapterTest:  5 tests, 0 failures (was 2 failing before fix)
OasImportConverterTest: 41 tests, 0 failures (was 1 failing before fix)
Full suite:             923 tests, 16 pre-existing flaky failures
                        (Step2-8 MCP client and Step10 REST adapter
                        tutorial tests — verified pre-existing on main,
                        unrelated to these fixes)

Checklist

  • CI is green (build, tests, schema validation, security scans)
  • Rebased on latest main
  • Small and focused — one concern per PR (4 MAJOR Sonar bugs as Phase 1 of the blueprint)
  • Commit messages follow Conventional Commits

Agent Context (optional)

agent_name: GitHub Copilot
llm: claude-opus-4.7
tool: copilot-chat
confidence: high
source_event: SonarQube quality gate run #1516
discovery_method: code_review
review_focus: HttpClientAdapter.java:90-118 and OasImportConverter.java:210-225, 503-516

…rter

Phase 1 of the Sonar bug remediation blueprint: resolve all 4 MAJOR
bugs reported by the SonarQube quality gate (run #1516, main @ dcfd01c).

Fixes:

- S2116 (HttpClientAdapter.java:103, 115): Basic and Digest auth read
  the password as a char[] from the spec, then called .toString() on
  the array which produced the JVM array identity (e.g. "[C@5a3b7c")
  instead of the password contents. Every authenticated outbound
  request was therefore sending a garbage secret. Replace with
  new String(getPassword()).

- S2259 (OasImportConverter.deriveResourceName): the method
  dereferenced path.replaceAll(...) without a null guard and called
  slug.startsWith(...) on a slug that could be null. Both inputs are
  now guarded; the method falls back to "root" instead of throwing
  NullPointerException.

- S5850 (OasImportConverter.toKebabCase): the regex "^-|-$" parses
  correctly under Java but reads ambiguously to humans. Replace with
  the explicitly-grouped "(^-)|(-$)" — same behavior, unambiguous
  intent.

Tests:

- HttpClientAdapterTest: the existing basic/digest auth tests only
  asserted assertNotNull(getSecret()), which let the [C@... garbage
  secret pass review. They now compare the actual secret content
  against the expected value, with a comment explaining why this
  assertion strengthening is itself the non-regression test for S2116.

- OasImportConverterTest: added
  deriveResourceNameShouldFallBackToRootWhenPathIsNull (NPE guard) and
  toKebabCaseShouldTrimLeadingAndTrailingHyphens (regex behavior).

Closes #417, #418, #419.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses MAJOR SonarQube-reported bugs in the HTTP client adapter and OpenAPI import converter, including one runtime-impacting authentication defect where Basic/Digest passwords were incorrectly serialized.

Changes:

  • Fix Basic/Digest authentication secret handling by converting char[] passwords to their actual string contents before Mustache resolution.
  • Make OasImportConverter.deriveResourceName null-safe with a "root" fallback.
  • Clarify the hyphen-trimming regex in toKebabCase and add regression tests for the above behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/main/java/io/naftiko/engine/consumes/http/HttpClientAdapter.java Fixes Basic/Digest password conversion so the resolved secret is the real password content.
src/main/java/io/naftiko/spec/openapi/OasImportConverter.java Adds null fallback in deriveResourceName and clarifies regex grouping in toKebabCase.
src/test/java/io/naftiko/engine/consumes/http/HttpClientAdapterTest.java Strengthens auth assertions to validate secret contents (non-regression for S2116).
src/test/java/io/naftiko/spec/openapi/OasImportConverterTest.java Adds regression tests for null-path fallback and leading/trailing hyphen trimming.

@jlouvel
Copy link
Copy Markdown
Contributor Author

jlouvel commented May 6, 2026

⚠️ CI is red but not because of this PR.

The Quality Gate / Unit Tests + JaCoCo + SonarQube + Security job fails on mvn test (before Sonar ever evaluates the gate) because 8 tutorial integration test classes depend on the live mock at https://mocks.naftiko.net/, which is currently returning non-JSON responses (plain text starting with "The…" and content-type INI).

Tracking this in #421.

Verification this is unrelated:

Run Branch UTC Result
25451669628 main (a19009c) 17:48
25459118475 fix/sonar-major-bugs 20:24 ❌ same 14 tutorial failures
25459630553 fix/sonar-major-bugs 20:35 ❌ same 14 tutorial failures
25459630553 (re-run) fix/sonar-major-bugs ~21:00 ❌ same 14 tutorial failures

The fixes in this PR touch HttpClientAdapter (Basic/Digest password reading) and OasImportConverter (null guards + regex grouping). None of the failing tutorial tests use Basic/Digest auth or go through OasImportConverter.

HttpClientAdapterTest (5/5 pass) and OasImportConverterTest (41/41 pass) — the directly relevant classes — are green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

2 participants