diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/AppliedChangeLedger.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/AppliedChangeLedger.java index 6447568812..05fe0e949e 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/AppliedChangeLedger.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/AppliedChangeLedger.java @@ -84,6 +84,8 @@ private static AppliedChange fromPending(PendingAppliedChange pac) { return AppliedChange.builder() .originalLineFrom(pac.lineFrom()) .originalLineTo(pac.lineTo()) + .declaredLineFrom(pac.declaredLineFrom()) + .declaredLineTo(pac.declaredLineTo()) .deltaLines(pac.deltaLines()) .comparisonCode(pac.comparisonCode()) .lineNormalizedCode(pac.lineNormalizedCode()) diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/HunkClassifier.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/HunkClassifier.java index d9eb175472..a78a9129db 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/HunkClassifier.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/HunkClassifier.java @@ -80,13 +80,13 @@ public List classifyRemediationHunks(Remediation remediation, Path */ private HunkOutcome classifyRange(int lineFrom, int lineTo, List applied, String candidateComparisonCode) { for (AppliedChange ac : applied) { - if (ac.coversFully(lineFrom, lineTo)) { + if (ac.coversDeclaredRange(lineFrom, lineTo)) { if (ac.contentCovers(candidateComparisonCode, lineFrom, lineTo)) { return HunkOutcome.SUPERSEDED; } return HunkOutcome.POSSIBLY_REMEDIATED; } - if (ac.overlapsPartially(lineFrom, lineTo)) { + if (ac.overlapsDeclaredRangePartially(lineFrom, lineTo)) { return HunkOutcome.CONFLICTS; } } diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/PendingAppliedChange.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/PendingAppliedChange.java index b714e621f8..9603cf8cde 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/PendingAppliedChange.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/PendingAppliedChange.java @@ -14,6 +14,6 @@ import java.nio.file.Path; -public record PendingAppliedChange(Path filePath, int lineFrom, int lineTo, int deltaLines, - String comparisonCode, String lineNormalizedCode) { +public record PendingAppliedChange(Path filePath, int lineFrom, int lineTo, int declaredLineFrom, int declaredLineTo, + int deltaLines, String comparisonCode, String lineNormalizedCode) { } diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChange.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChange.java index f37376b2fc..29c6814d16 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChange.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChange.java @@ -26,18 +26,22 @@ public final class AppliedChange { private final int originalLineFrom; private final int originalLineTo; + private final int declaredLineFrom; + private final int declaredLineTo; private final int deltaLines; private final String comparisonCode; private final String[] lineNormalizedContent; public AppliedChange(int originalLineFrom, int originalLineTo, int deltaLines, String comparisonCode) { - this(originalLineFrom, originalLineTo, deltaLines, comparisonCode, null); + this(originalLineFrom, originalLineTo, originalLineFrom, originalLineTo, deltaLines, comparisonCode, null); } @Builder - public AppliedChange(int originalLineFrom, int originalLineTo, int deltaLines, String comparisonCode, String lineNormalizedCode) { + public AppliedChange(int originalLineFrom, int originalLineTo, int declaredLineFrom, int declaredLineTo, int deltaLines, String comparisonCode, String lineNormalizedCode) { this.originalLineFrom = originalLineFrom; this.originalLineTo = originalLineTo; + this.declaredLineFrom = declaredLineFrom; + this.declaredLineTo = declaredLineTo; this.deltaLines = deltaLines; this.comparisonCode = comparisonCode; // Store line-by-line normalized content for offset-anchored comparison (newlines preserved, each line normalized) @@ -63,8 +67,21 @@ public boolean coversFully(int lineFrom, int lineTo) { /** True if [lineFrom, lineTo] overlaps this change's original range without either side fully containing the other. */ public boolean overlapsPartially(int lineFrom, int lineTo) { - boolean disjoint = lineTo < originalLineFrom || lineFrom > originalLineTo; - return !disjoint; + boolean overlaps = lineFrom <= originalLineTo && originalLineFrom <= lineTo; + boolean candidateContainsThis = lineFrom <= originalLineFrom && originalLineTo <= lineTo; + return overlaps && !coversFully(lineFrom, lineTo) && !candidateContainsThis; + } + + /** True if this change's declared range fully contains [lineFrom, lineTo]. */ + public boolean coversDeclaredRange(int lineFrom, int lineTo) { + return declaredLineFrom <= lineFrom && lineTo <= declaredLineTo; + } + + /** True if [lineFrom, lineTo] overlaps this change's declared range without either side fully containing the other. */ + public boolean overlapsDeclaredRangePartially(int lineFrom, int lineTo) { + boolean overlaps = lineFrom <= declaredLineTo && declaredLineFrom <= lineTo; + boolean candidateContainsThis = lineFrom <= declaredLineFrom && declaredLineTo <= lineTo; + return overlaps && !coversDeclaredRange(lineFrom, lineTo) && !candidateContainsThis; } /** diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/writer/FileWriteCoordinator.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/writer/FileWriteCoordinator.java index 58dbaccd1e..07633a414c 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/writer/FileWriteCoordinator.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/writer/FileWriteCoordinator.java @@ -130,7 +130,8 @@ private void processFileChanges(Remediation remediation, FileChange fileChange, int linesAfterChange = updatedContent.split("\n", -1).length; int delta = linesAfterChange - linesBeforeChange; String lineNormalizedCode = hunk.lineNormalizedCode(filename); - ledger.stage(new PendingAppliedChange(filePath, actualLineFrom, actualLineTo, delta, comparisonCode, lineNormalizedCode)); + ledger.stage(new PendingAppliedChange(filePath, actualLineFrom, actualLineTo, declaredLineFrom, declaredLineTo, + delta, comparisonCode, lineNormalizedCode)); appliedKeysOut.add(key); appliedInThisFile++; } @@ -151,12 +152,25 @@ public void commitRemediationWrites(String instanceId, Map rollbacks = new ArrayList<>(); for (PendingFileWrite pendingWrite : pendingWrites.values()) { + Path filePath = pendingWrite.filePath(); + // A permission failure is rejected atomically before any bytes are written, so a file + // that's already known unwritable needs no rollback entry at all: attempting one would + // just retry the same failing write. Checking this upfront (rather than relying on + // Files.write's exception to prove the file was untouched) matters once a write CAN + // start: e.g. running out of disk space mid-write can truncate/partially overwrite the + // file before failing, so a write that's confirmed writable must be recorded for + // rollback before attempting it, not after. + if (!Files.isWritable(filePath)) { + throw new RemediationCommitException( + "Source code file is not writable: '" + pendingWrite.filename() + "'", + new IOException("File not writable: " + filePath), rollbacks); + } try { - byte[] originalBytes = Files.readAllBytes(pendingWrite.filePath()); - rollbacks.add(new RollbackFileWrite(pendingWrite.filename(), pendingWrite.filePath(), originalBytes)); + byte[] originalBytes = Files.readAllBytes(filePath); + rollbacks.add(new RollbackFileWrite(pendingWrite.filename(), filePath, originalBytes)); LOG.debug("Writing remediation {} to '{}' using staged bytes; encodedBytes={}", instanceId, pendingWrite.filename(), pendingWrite.updatedBytes().length); - Files.write(pendingWrite.filePath(), pendingWrite.updatedBytes()); + Files.write(filePath, pendingWrite.updatedBytes()); } catch (Exception e) { throw new RemediationCommitException("Error writing source code file '" + pendingWrite.filename() + "'", e, rollbacks); } diff --git a/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/processor/RemediationProcessorTest.java b/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/processor/RemediationProcessorTest.java index da9a03ca5b..7aa65dcdd0 100644 --- a/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/processor/RemediationProcessorTest.java +++ b/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/processor/RemediationProcessorTest.java @@ -678,6 +678,39 @@ void illegalFilenameIsSkippedAndValidRemediationsStillApply() throws Exception { assertEquals("before\nREPLACED\nafter\n", Files.readString(sourceFile)); } + /** + * A source file whose write fails (e.g., made read-only) must be skipped via + * {@code SOURCE_WRITE_FAILED} on its own, not abort the whole batch: the rollback attempt for + * that failed write must not itself retry writing to the same permission-denied file, which + * previously escalated into a batch-aborting {@code RollbackRemediationException} and caused + * every other remediation in the run - even ones touching unrelated, writable files - to fail. + */ + @Test + @EnabledOnOs(OS.WINDOWS) + void readOnlyFileWriteFailureIsSkippedAndOtherRemediationsStillApply() throws Exception { + Path readOnlyFile = writeSourceFile("ReadOnly.java", "before\nTARGET\nafter\n"); + Path goodFile = writeSourceFile("Good.java", "before\nTARGET\nafter\n"); + readOnlyFile.toFile().setReadOnly(); + try { + Path fprPath = buildFpr(List.of( + new MultiHunkRemediationSpec("blocked", List.of(new FileSpec("ReadOnly.java", List.of( + new HunkSpec(2, 2, 1, 1, "before\ntarget\nafter", "TARGET", "REPLACED"))))), + new MultiHunkRemediationSpec("valid", List.of(new FileSpec("Good.java", List.of( + new HunkSpec(2, 2, 1, 1, "before\ntarget\nafter", "TARGET", "REPLACED"))))))); + + RemediationMetric metric = apply(fprPath); + + assertEquals(2, metric.totalRemediations()); + assertEquals(1, metric.appliedRemediations(), "the remediation for the writable file must still be applied"); + assertEquals(1, metric.skippedRemediations()); + assertEquals(Map.of("Source file write failed", 1), metric.skippedByReason()); + assertEquals("before\nTARGET\nafter\n", Files.readString(readOnlyFile)); + assertEquals("before\nREPLACED\nafter\n", Files.readString(goodFile)); + } finally { + readOnlyFile.toFile().setWritable(true); + } + } + /** * The offset ledger must record where a hunk actually landed via the fuzzy anchor, not where * it was declared: {@code relocated} declares line 12 but its context/OriginalCode only exist @@ -777,6 +810,41 @@ void secondFprDoesNotApplyOverAFixTheFirstFprAlreadyRewrote() throws Exception { assertEquals(afterFirst, Files.readString(sourceFile), "the second FPR must leave the file untouched"); } + /** + * Regression test for coordinate-space mismatch bug: HunkClassifier must compare declared + * (pristine-file) line numbers against declared ranges, not actual (post-shift) ranges, when + * pre-classifying narrower hunks nested inside a broader fix that shifted them. The broader + * fix (wide-deletes) declares lines 1-5 and deletes them, replacing with 2 lines (delta -3). + * The narrower fix (narrow-inside) declares line 3, which sits inside 1-5's declared range. + * Before the fix, classifyRange would compare declared-3 against the broader hunk's ACTUAL + * range (which is now at a shifted position), missing the nesting; after the fix, it compares + * declared-against-declared and correctly classifies as POSSIBLY_REMEDIATED. + */ + @Test + void narrowerRemediationInsideBroaderDeleteIsClassifiedAsPossiblyRemediated() throws Exception { + String originalSource = "line1\nline2\nline3\nline4\nline5\nline6\n"; + writeSourceFile("Example.java", originalSource); + Path fprPath = buildFpr(List.of( + new MultiHunkRemediationSpec("wide-deletes", List.of(new FileSpec("Example.java", List.of( + new HunkSpec(1, 5, 0, 0, + "line1\nline2\nline3\nline4\nline5", + "line1\nline2\nline3\nline4\nline5", + "replacement1\nreplacement2"))))), + new MultiHunkRemediationSpec("narrow-inside", List.of(new FileSpec("Example.java", List.of( + new HunkSpec(3, 3, 1, 1, "line2\nline3\nline4", + "line3", "line3-modified"))))))); + + RemediationMetric metric = apply(fprPath); + + assertEquals(2, metric.totalRemediations()); + assertEquals(1, metric.appliedRemediations(), "the broader fix must be applied"); + assertEquals(1, metric.possiblyRemediatedRemediations(), + "the narrower fix's declared line 3 sits inside the broader fix's declared 1-5, so it must be " + + "pre-classified as POSSIBLY_REMEDIATED even though its actual position shifted"); + assertEquals(0, metric.skippedRemediations(), + "no remediation should be skipped; the narrower one must not fail with ANCHOR_DOES_NOT_MATCH"); + } + private record HunkSpec(int lineFrom, int lineTo, int contextBefore, int contextAfter, String context, String originalCode, String newCode) {}