Skip to content

fix: migrate auth spec credentials to AtomicReference + defensive copies (S3077, Phase 4)#428

Open
jlouvel wants to merge 2 commits intomainfrom
fix/sonar-volatile-char-array
Open

fix: migrate auth spec credentials to AtomicReference + defensive copies (S3077, Phase 4)#428
jlouvel wants to merge 2 commits intomainfrom
fix/sonar-volatile-char-array

Conversation

@jlouvel
Copy link
Copy Markdown
Contributor

@jlouvel jlouvel commented May 6, 2026

Summary

Phase 4 (final phase) of the Sonar bug remediation blueprint. Migrates the credential fields on BasicAuthenticationSpec and DigestAuthenticationSpec away from volatile to AtomicReference<T>, with the char[] password defensively cloned on both set and get.

This is the third and final S3077 PR, after #423 (Phase 2 — spec POJOs) and #425 (Phase 3 — engine layer). The blueprint's bug count drops from 59 → 0 once Phases 1–4 land.

Closes #427.

What changed

File Fields migrated Pattern
io.naftiko.spec.consumes.http.BasicAuthenticationSpec username (String), password (char[]) AtomicReference<String> (Pattern A) + AtomicReference<char[]> with defensive clone on set/get
io.naftiko.spec.consumes.http.DigestAuthenticationSpec username (String), password (char[]) same

Each setPassword(char[]) clones the input before storing; each getPassword() clones the stored array before returning. Constructors do the same. The public API stays exactly as String / char[] — Jackson and existing callers are unaffected.

Why defensive clone, not just AtomicReference<char[]>

The blueprint's "Atomic Password Storage" pattern is explicit: defensive copies on set and get are not optional. Without them, getPassword() hands out the live internal array — a caller doing spec.getPassword()[0] = 'x' silently rewrites the stored credential. The new test suite exercises this exact scenario and would have caught it before this PR.

This pairs naturally with AtomicReference because both are about safe handoff of a credential value across threads or callers: atomic-swap for the reference itself, defensive-copy for the array contents.

Test plan

New AuthenticationSpecThreadSafetyTest (8 cases, two-tier):

  1. Meta-tests (2) — reflection-based, parameterized over both classes; assert no field is volatile. Same regression-guard style as SpecFieldThreadSafetyTest (fix: migrate spec POJOs to AtomicReference + immutable snapshots (S3077) #423) and EngineFieldThreadSafetyTest (fix: migrate engine fields to AtomicReference snapshots (S3077, Phase 3) #425).
  2. Behavioral tests (6) — assert setPassword/getPassword defensively clone, and setPassword(null) round-trips correctly. These test a real bug: on main, callers can mutate the stored credential in place via the live array reference.

Bug Workflow followed strictly:

  • Failing test committed first (commit b3d00d6) — 6/8 fail on main as expected (2 meta + 4 defensive-copy)
  • Migration committed second (commit b55a70b) — all 8 now pass

Full local suite: 929 tests, 14 failures, 0 errors. The 14 are the pre-existing LLM-flaky Step{2-8,10}Shipyard*IntegrationTest cases — same set as on main and unrelated.

Notes for reviewers

  • The blueprint scope was the 2 volatile char[] password fields. I also migrated the 2 volatile String username fields in the same classes for internal consistency — same approach as Phase 2 (which migrated 27 files for 44 hits) and Phase 3 (which migrated 13 fields for 9 explicit hits).
  • OasImportConverter is the only production caller of setPassword(char[]) outside tests — both call sites pass freshly-allocated "{{PASSWORD}}".toCharArray() and discard the reference, so the new defensive clone has no observable effect on them.
  • Phase 1 (fix: resolve MAJOR Sonar bugs in HttpClientAdapter and OasImportConverter #420) already fixed HttpClientAdapter's char[].toString() bug (S2116), so the read path now sees the correct password value through new String(password).

jlouvel added 2 commits May 6, 2026 18:36
…ve copies (S3077)

Phase 4 (final phase) of the Sonar bug remediation blueprint. Migrates the volatile username/password fields on BasicAuthenticationSpec and DigestAuthenticationSpec to AtomicReference<T>, with the char[] password defensively cloned on both set and get so callers cannot mutate the stored credential in place.

Also fixes a real credential-hygiene bug surfaced by the new test suite: getPassword() previously handed back the live internal array, so any caller doing 'spec.getPassword()[0] = char' silently rewrote the stored password. The blueprint's atomic password storage pattern guards against exactly this.

Closes #427
@jlouvel jlouvel self-assigned this May 6, 2026
@jlouvel jlouvel requested review from eskenazit May 6, 2026 23:33
@jlouvel jlouvel added the bug Something isn't working label May 6, 2026
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

Development

Successfully merging this pull request may close these issues.

bug: 4 char[]/String fields in BasicAuth/DigestAuth specs use volatile (Sonar S3077)

1 participant