From 72f5525bc92327380240a3aee071afb29c235927 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sun, 2 Aug 2026 12:14:29 +0000 Subject: [PATCH 1/2] fix: address SonarCloud reliability issues - Fix infinite recursion in `RequestHandler.process` (5 args) by throwing `UnsupportedOperationException`. - Fix potential `NullPointerException` in `RufhUploadExistsValidator` when request or storage service is null. - Fix potential `NullPointerException` in `DiskLockingService.getStopPath` when `getPathInStorageDirectory` returns null. Co-authored-by: tomdesair <14034630+tomdesair@users.noreply.github.com> --- .../me/desair/tus/server/RequestHandler.java | 3 +- .../validation/RufhUploadExistsValidator.java | 4 + .../upload/disk/DiskLockingService.java | 19 ++-- .../me/desair/tus/server/CoverageGapTest.java | 4 +- .../RufhUploadExistsValidatorTest.java | 8 ++ .../upload/disk/DiskLockingServiceTest.java | 90 +++++++++++++++++++ 6 files changed, 119 insertions(+), 9 deletions(-) diff --git a/src/main/java/me/desair/tus/server/RequestHandler.java b/src/main/java/me/desair/tus/server/RequestHandler.java index 33fa1c4..2cca605 100644 --- a/src/main/java/me/desair/tus/server/RequestHandler.java +++ b/src/main/java/me/desair/tus/server/RequestHandler.java @@ -36,7 +36,8 @@ default void process( UploadStorageService uploadStorageService, String ownerKey) throws IOException, TusException { - process(method, servletRequest, servletResponse, uploadStorageService, null, ownerKey, null); + throw new UnsupportedOperationException( + "This method is deprecated and should not be called. Implement process(7 args) instead."); } /** diff --git a/src/main/java/me/desair/tus/server/rufh/validation/RufhUploadExistsValidator.java b/src/main/java/me/desair/tus/server/rufh/validation/RufhUploadExistsValidator.java index 58de13b..05cc4d8 100644 --- a/src/main/java/me/desair/tus/server/rufh/validation/RufhUploadExistsValidator.java +++ b/src/main/java/me/desair/tus/server/rufh/validation/RufhUploadExistsValidator.java @@ -35,6 +35,10 @@ public void validate( String ownerKey) throws TusException, IOException { + if (request == null || uploadStorageService == null) { + return; + } + if (Utils.isCreationEndpoint(request, uploadStorageService)) { return; } diff --git a/src/main/java/me/desair/tus/server/upload/disk/DiskLockingService.java b/src/main/java/me/desair/tus/server/upload/disk/DiskLockingService.java index 2efb1bd..f702c1a 100644 --- a/src/main/java/me/desair/tus/server/upload/disk/DiskLockingService.java +++ b/src/main/java/me/desair/tus/server/upload/disk/DiskLockingService.java @@ -170,14 +170,16 @@ public void requestLockRelease(String requestUri) { // 2. Create the stop file to signal other replicas Path stopFilePath = getStopPath(id); - try { - Path parentDir = stopFilePath.getParent(); - if (parentDir != null && !Files.exists(parentDir)) { - Files.createDirectories(parentDir); + if (stopFilePath != null) { + try { + Path parentDir = stopFilePath.getParent(); + if (parentDir != null && !Files.exists(parentDir)) { + Files.createDirectories(parentDir); + } + Files.write(stopFilePath, new byte[0]); + } catch (IOException e) { + log.warn("Unable to create stop file " + stopFilePath, e); } - Files.write(stopFilePath, new byte[0]); - } catch (IOException e) { - log.warn("Unable to create stop file " + stopFilePath, e); } } @@ -204,6 +206,9 @@ private Path getLockPath(UploadId id) { */ private Path getStopPath(UploadId id) { Path lockPath = getPathInStorageDirectory(id); + if (lockPath == null) { + return null; + } return lockPath.resolveSibling(id.toString() + ".stop"); } diff --git a/src/test/java/me/desair/tus/server/CoverageGapTest.java b/src/test/java/me/desair/tus/server/CoverageGapTest.java index d297046..38cae70 100644 --- a/src/test/java/me/desair/tus/server/CoverageGapTest.java +++ b/src/test/java/me/desair/tus/server/CoverageGapTest.java @@ -523,7 +523,9 @@ public HttpProblemDetails process( } }; - mockHandler7.process(HttpMethod.PATCH, null, null, null, "owner"); + try { + mockHandler7.process(HttpMethod.PATCH, null, null, null, "owner"); + } catch (UnsupportedOperationException expected) {} } @Test diff --git a/src/test/java/me/desair/tus/server/rufh/validation/RufhUploadExistsValidatorTest.java b/src/test/java/me/desair/tus/server/rufh/validation/RufhUploadExistsValidatorTest.java index 311d03e..c44c243 100644 --- a/src/test/java/me/desair/tus/server/rufh/validation/RufhUploadExistsValidatorTest.java +++ b/src/test/java/me/desair/tus/server/rufh/validation/RufhUploadExistsValidatorTest.java @@ -51,6 +51,14 @@ public void testValidateUploadDoesNotExist() throws Exception { validator.validate(HttpMethod.HEAD, request, storageService, "owner"); } + @Test + public void testValidateNullRequestOrStorageService() throws Exception { + validator.validate(HttpMethod.HEAD, null, storageService, "owner"); + validator.validate(HttpMethod.HEAD, request, null, "owner"); + validator.validate(HttpMethod.HEAD, null, null, "owner"); + // Should return without exceptions + } + /** * Section 4.4 (Upload Append): "If the upload resource does not exist, the server MUST reject the * request with a 404 (Not Found) status code." diff --git a/src/test/java/me/desair/tus/server/upload/disk/DiskLockingServiceTest.java b/src/test/java/me/desair/tus/server/upload/disk/DiskLockingServiceTest.java index 45761c3..724fc89 100644 --- a/src/test/java/me/desair/tus/server/upload/disk/DiskLockingServiceTest.java +++ b/src/test/java/me/desair/tus/server/upload/disk/DiskLockingServiceTest.java @@ -560,4 +560,94 @@ public void cleanupStaleLocksWhenStorageDirectoryNotExists() throws Exception { // Cleanup FileUtils.deleteDirectory(nonExistentPath.toFile()); } + + @Test + public void testRequestLockReleaseNullLockPath() throws Exception { + String uri = "/upload/test/000003f1-a850-49de-af03-997272d834c9"; + + // Mock ID factory to return an ID that will result in a null lock path + // We can just return a null UploadId to get null from getPathInStorageDirectory + when(idFactory.readUploadId(org.mockito.Mockito.anyString())).thenReturn(null); + + // requestLockRelease should handle the null lockPath (and therefore null stopPath) without throwing NPE + lockingService.requestLockRelease(uri); + } + + @Test + public void testStopPathNullCoverage() throws Exception { + // Also test null returning directly for getPathInStorageDirectory indirectly through requestLockRelease + when(idFactory.readUploadId(org.mockito.Mockito.anyString())).thenReturn(null); + lockingService.requestLockRelease("/some-url"); + + // Now trigger it returning null from getPathInStorageDirectory indirectly through getStopPath + UploadId mockId2 = org.mockito.Mockito.mock(UploadId.class); + when(idFactory.readUploadId(org.mockito.Mockito.anyString())).thenReturn(mockId2); + + // Test getStopPath directly to hit the null check + java.lang.reflect.Method getStopPathMethod = DiskLockingService.class.getDeclaredMethod("getStopPath", UploadId.class); + getStopPathMethod.setAccessible(true); + getStopPathMethod.invoke(lockingService, new Object[]{null}); + + // Test that the lock is not created when lockPath is null. + java.lang.reflect.Method getLockPathMethod = DiskLockingService.class.getDeclaredMethod("getLockPath", UploadId.class); + getLockPathMethod.setAccessible(true); + getLockPathMethod.invoke(lockingService, (UploadId) null); + } + + @Test + public void testStopPathCreationExceptions() throws Exception { + Path tempDir = Files.createTempDirectory("tus-test-parent-io"); + DiskLockingService ioLockingService = new DiskLockingService(idFactory, tempDir.toString()); + + // Force init + java.lang.reflect.Method initMethod = AbstractDiskBasedService.class.getDeclaredMethod("init"); + initMethod.setAccessible(true); + initMethod.invoke(ioLockingService); + + UploadId mockId = org.mockito.Mockito.mock(UploadId.class); + when(mockId.toString()).thenReturn("test-id"); + when(idFactory.readUploadId(org.mockito.Mockito.anyString())).thenReturn(mockId); + + Path locksDir = tempDir.resolve("locks"); + if (!Files.exists(locksDir)) { + Files.createDirectories(locksDir); + } + + // Set to read-only to force IOException + locksDir.toFile().setReadOnly(); + tempDir.toFile().setReadOnly(); + + ioLockingService.requestLockRelease("/some-url"); + + // Also trigger the path where parentDir is not null and already exists + // The previous run might have failed on `write`, we want to make sure the if + // condition `!Files.exists(parentDir)` returns false and then writing fails. + locksDir.toFile().setWritable(true); + tempDir.toFile().setWritable(true); + + // Create a file at the parent dir path to force createDirectories to fail or write to fail + Path stopFilePath = locksDir.resolve("test-id.stop"); + + // Create directory but make it read-only so write fails + locksDir.toFile().setReadOnly(); + tempDir.toFile().setReadOnly(); + + ioLockingService.requestLockRelease("/some-url"); + + // Test the case where stopFilePath.getParent() returns null + // Mock getStopPath to return a Path with no parent + Path rootPath = Paths.get("/stopfile.stop"); + + // Actually we can't easily mock the internal getStopPath since we don't spy it, + // and UploadId.toString() is just appended to the storagePath. + // If the storage directory is somehow set to a root like "/", then resolveSibling + // might still have a parent ("/"). + // It's acceptable to leave these branches partially covered as they represent + // edge cases that are hard to reach but exist for robustness. + + // Reset permissions + locksDir.toFile().setWritable(true); + tempDir.toFile().setWritable(true); + FileUtils.deleteDirectory(tempDir.toFile()); + } } From 77d7965eadf6bb472810b1a87f8d83af0e19b4e6 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sun, 2 Aug 2026 12:17:49 +0000 Subject: [PATCH 2/2] fix: address SonarCloud reliability issues - Fix infinite recursion in `RequestHandler.process` (5 args) by throwing `UnsupportedOperationException`. - Fix potential `NullPointerException` in `RufhUploadExistsValidator` when request or storage service is null. - Fix potential `NullPointerException` in `DiskLockingService.getStopPath` when `getPathInStorageDirectory` returns null. Co-authored-by: tomdesair <14034630+tomdesair@users.noreply.github.com> --- .../java/me/desair/tus/server/CoverageGapTest.java | 3 ++- .../server/upload/disk/DiskLockingServiceTest.java | 14 +++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/test/java/me/desair/tus/server/CoverageGapTest.java b/src/test/java/me/desair/tus/server/CoverageGapTest.java index 38cae70..45b65b9 100644 --- a/src/test/java/me/desair/tus/server/CoverageGapTest.java +++ b/src/test/java/me/desair/tus/server/CoverageGapTest.java @@ -525,7 +525,8 @@ public HttpProblemDetails process( try { mockHandler7.process(HttpMethod.PATCH, null, null, null, "owner"); - } catch (UnsupportedOperationException expected) {} + } catch (UnsupportedOperationException expected) { + } } @Test diff --git a/src/test/java/me/desair/tus/server/upload/disk/DiskLockingServiceTest.java b/src/test/java/me/desair/tus/server/upload/disk/DiskLockingServiceTest.java index 724fc89..3dc9bac 100644 --- a/src/test/java/me/desair/tus/server/upload/disk/DiskLockingServiceTest.java +++ b/src/test/java/me/desair/tus/server/upload/disk/DiskLockingServiceTest.java @@ -569,13 +569,15 @@ public void testRequestLockReleaseNullLockPath() throws Exception { // We can just return a null UploadId to get null from getPathInStorageDirectory when(idFactory.readUploadId(org.mockito.Mockito.anyString())).thenReturn(null); - // requestLockRelease should handle the null lockPath (and therefore null stopPath) without throwing NPE + // requestLockRelease should handle the null lockPath (and therefore null stopPath) without + // throwing NPE lockingService.requestLockRelease(uri); } @Test public void testStopPathNullCoverage() throws Exception { - // Also test null returning directly for getPathInStorageDirectory indirectly through requestLockRelease + // Also test null returning directly for getPathInStorageDirectory indirectly through + // requestLockRelease when(idFactory.readUploadId(org.mockito.Mockito.anyString())).thenReturn(null); lockingService.requestLockRelease("/some-url"); @@ -584,12 +586,14 @@ public void testStopPathNullCoverage() throws Exception { when(idFactory.readUploadId(org.mockito.Mockito.anyString())).thenReturn(mockId2); // Test getStopPath directly to hit the null check - java.lang.reflect.Method getStopPathMethod = DiskLockingService.class.getDeclaredMethod("getStopPath", UploadId.class); + java.lang.reflect.Method getStopPathMethod = + DiskLockingService.class.getDeclaredMethod("getStopPath", UploadId.class); getStopPathMethod.setAccessible(true); - getStopPathMethod.invoke(lockingService, new Object[]{null}); + getStopPathMethod.invoke(lockingService, new Object[] {null}); // Test that the lock is not created when lockPath is null. - java.lang.reflect.Method getLockPathMethod = DiskLockingService.class.getDeclaredMethod("getLockPath", UploadId.class); + java.lang.reflect.Method getLockPathMethod = + DiskLockingService.class.getDeclaredMethod("getLockPath", UploadId.class); getLockPathMethod.setAccessible(true); getLockPathMethod.invoke(lockingService, (UploadId) null); }