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..1bc8588417 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 @@ -151,12 +151,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..0d13730ad3 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