diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/DataAccess.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/DataAccess.java index bccaf58edfc..d046fa4661d 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/DataAccess.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/DataAccess.java @@ -331,14 +331,23 @@ public static boolean uploadToDatasetAllowed(Dataset d, String storageIdentifier String driverId = DataAccess.getStorageDriverFromIdentifier(storageIdentifier); String effectiveDriverId = d.getEffectiveStorageDriverId(); if(!effectiveDriverId.equals(driverId)) { + //Not allowed unless this is a remote store and you're uploading to the basestore if(getDriverType(driverId).equals(REMOTE)) { String baseDriverId = RemoteOverlayAccessIO.getBaseStoreIdFor(driverId); if(!effectiveDriverId.equals(baseDriverId)) { + //Not allowed - wrong base driver allowed = false; + } else { + //Only allowed if baseStore allows it + allowed = StorageIO.isDirectUploadEnabled(baseDriverId); } } else { + //Not allowed - wrong main driver allowed=false; } + } else { + //Only allowed if main store allows it + allowed = StorageIO.isDirectUploadEnabled(driverId); } return allowed; } diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java index 1c60981c82a..d5f00b9868f 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java @@ -29,10 +29,13 @@ import java.io.FileOutputStream; // NIO imports: import java.nio.file.Files; +import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.Paths; import java.util.List; import java.util.logging.Logger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; // Dataverse imports: import edu.harvard.iq.dataverse.DataFile; @@ -303,7 +306,7 @@ public Path getAuxObjectAsPath(String auxItemTag) throws IOException { throw new IOException("Null or invalid Auxiliary Object Tag."); } if(isDirectAccess()) { - //Overlay case + //includes overlay case return Paths.get(physicalPath.toString() + "." + auxItemTag); } String datasetDirectory = getDatasetDirectory(); @@ -427,8 +430,8 @@ public void deleteAllAuxObjects() throws IOException { } } - - + + @Override public String getStorageLocation() { // For a local file, the "storage location" is a complete, absolute @@ -656,4 +659,28 @@ private String stripDriverId(String storageIdentifier) { } return storageIdentifier; } + + //Confirm inputs are of the form of a relative file path that doesn't contain . or .. + protected static boolean isValidIdentifier(String driverId, String storageId) { + String pathString = storageId.substring(storageId.lastIndexOf("//") + 2); + String basePath = "/tmp/"; + try { + String rawPathString = basePath + pathString; + Path normalized = Paths.get(rawPathString).normalize(); + if(!rawPathString.equals(normalized.toString())) { + logger.warning("Non-normalized path in submitted identifier " + storageId); + return false; + } + logger.fine(normalized.getFileName().toString()); + if (!usesStandardNamePattern(normalized.getFileName().toString())) { + logger.warning("Unacceptable file name in submitted identifier: " + storageId); + return false; + } + + } catch (InvalidPathException ipe) { + logger.warning("Invalid Path in submitted identifier " + storageId); + return false; + } + return true; + } } diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/RemoteOverlayAccessIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/RemoteOverlayAccessIO.java index bc421949ed7..c8e42349318 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/RemoteOverlayAccessIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/RemoteOverlayAccessIO.java @@ -630,4 +630,5 @@ protected static boolean isValidIdentifier(String driverId, String storageId) { public static String getBaseStoreIdFor(String driverId) { return System.getProperty("dataverse.files." + driverId + ".base-store"); } + } diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java index e550cae1373..3c9cef04980 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java @@ -937,9 +937,9 @@ public String generateTemporaryDownloadUrl(String auxiliaryTag, String auxiliary // them for some servers, we check whether the protocol is in the url and then // normalizing to use the part without the protocol String endpointServer = endpoint; - int protocolEnd = endpoint.indexOf("://"); + int protocolEnd = endpoint.indexOf(DataAccess.SEPARATOR); if (protocolEnd >=0 ) { - endpointServer = endpoint.substring(protocolEnd + 3); + endpointServer = endpoint.substring(protocolEnd + DataAccess.SEPARATOR.length()); } logger.fine("Endpoint: " + endpointServer); // We're then replacing @@ -998,9 +998,9 @@ private String generateTemporaryS3UploadUrl(String key, Date expiration) throws // them for some servers, we check whether the protocol is in the url and then // normalizing to use the part without the protocol String endpointServer = endpoint; - int protocolEnd = endpoint.indexOf("://"); + int protocolEnd = endpoint.indexOf(DataAccess.SEPARATOR); if (protocolEnd >=0 ) { - endpointServer = endpoint.substring(protocolEnd + 3); + endpointServer = endpoint.substring(protocolEnd + DataAccess.SEPARATOR.length()); } logger.fine("Endpoint: " + endpointServer); // We're then replacing @@ -1274,5 +1274,37 @@ public boolean isMainDriver() { public void setMainDriver(boolean mainDriver) { this.mainDriver = mainDriver; } + + public static String getDriverPrefix(String driverId) { + return driverId+ DataAccess.SEPARATOR + getBucketName(driverId) + ":"; + } + + //Confirm inputs are of the form s3://demo-dataverse-bucket:176e28068b0-1c3f80357c42 + protected static boolean isValidIdentifier(String driverId, String storageId) { + String storageBucketAndId = storageId.substring(storageId.lastIndexOf("//") + 2); + String bucketName = getBucketName(driverId); + if(bucketName==null) { + logger.warning("No bucket defined for " + driverId); + return false; + } + int index = storageBucketAndId.lastIndexOf(":"); + if(index<=0) { + logger.warning("No bucket defined in submitted identifier: " + storageId); + return false; + } + String idBucket = storageBucketAndId.substring(0, index); + String id = storageBucketAndId.substring(index+1); + logger.fine(id); + if(!bucketName.equals(idBucket)) { + logger.warning("Incorrect bucket in submitted identifier: " + storageId); + return false; + } + if (!usesStandardNamePattern(id)) { + logger.warning("Unacceptable identifier pattern in submitted identifier: " + storageId); + return false; + } + return true; + } + } diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java index 46b4b13a889..acb441267ee 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java @@ -594,10 +594,14 @@ public static String getDriverPrefix(String driverId) { return driverId+ DataAccess.SEPARATOR; } + public static boolean isDirectUploadEnabled(String driverId) { + return Boolean.getBoolean(System.getProperty("dataverse.files." + driverId + ".download-redirect", "false")); + } + //Check that storageIdentifier is consistent with store's config //False will prevent direct uploads protected static boolean isValidIdentifier(String driverId, String storageId) { - return true; + return false; } //Utility to verify the standard UUID pattern for stored files. @@ -607,4 +611,5 @@ protected static boolean usesStandardNamePattern(String identifier) { Matcher m = r.matcher(identifier); return m.find(); } + } diff --git a/src/main/resources/db/migration/V5.11.1.6__storageconstraint.sql b/src/main/resources/db/migration/V5.11.1.6__storageconstraint.sql new file mode 100644 index 00000000000..c2629213e98 --- /dev/null +++ b/src/main/resources/db/migration/V5.11.1.6__storageconstraint.sql @@ -0,0 +1,10 @@ +DO $$ +BEGIN + + BEGIN + ALTER TABLE dvobject ADD CONSTRAINT chk_dvobject_storageidentifier check (strpos(storageidentifier,'..') = 0); + EXCEPTION + WHEN duplicate_object THEN RAISE NOTICE 'Table constraint chk_dvobject_storageidentifier already exists'; + END; + +END $$; diff --git a/src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java b/src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java index 286fe8a6595..95621dd8750 100644 --- a/src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java @@ -9,6 +9,8 @@ import edu.harvard.iq.dataverse.Dataset; import edu.harvard.iq.dataverse.Dataverse; import edu.harvard.iq.dataverse.mocks.MocksFactory; +import edu.harvard.iq.dataverse.util.FileUtil; + import java.io.BufferedReader; import java.io.BufferedWriter; import java.io.ByteArrayInputStream; @@ -24,6 +26,9 @@ import org.junit.After; import org.junit.Test; import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + import org.junit.Before; /** @@ -285,4 +290,31 @@ public void testGetAuxFileAsInputStream() throws IOException { } assertEquals("This is a test string\n", sb.toString()); } + + @Test + public void testFileIdentifierFormats() throws IOException { + System.setProperty("dataverse.files.filetest.type", "file"); + System.setProperty("dataverse.files.filetest.label", "Ftest"); + System.setProperty("dataverse.files.filetest.directory", "/tmp/mydata"); + + FileUtil.generateStorageIdentifier(); + assertTrue(DataAccess.isValidDirectStorageIdentifier("filetest://" + FileUtil.generateStorageIdentifier())); + //The tests here don't use a valid identifier string + assertFalse(DataAccess.isValidDirectStorageIdentifier(dataFile.getStorageIdentifier())); + //bad store id + String defaultType = System.getProperty("dataverse.files.file.type"); + //Assure file isn't a defined store before test and reset afterwards if it was + System.clearProperty("dataverse.files.file.type"); + assertFalse(DataAccess.isValidDirectStorageIdentifier("file://" + FileUtil.generateStorageIdentifier())); + if(defaultType!=null) { + System.out.println("dataverse.files.file.type reset to " + defaultType); + System.setProperty("dataverse.files.file.type", defaultType); + } + //breakout + assertFalse(DataAccess.isValidDirectStorageIdentifier("filetest://../" + FileUtil.generateStorageIdentifier())); + + System.clearProperty("dataverse.files.filetest.type"); + System.clearProperty("dataverse.files.filetest.label"); + System.clearProperty("dataverse.files.filetest.directory"); + } } diff --git a/src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java b/src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java index e2756d70663..0e8e8500a1e 100644 --- a/src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java @@ -9,6 +9,9 @@ import edu.harvard.iq.dataverse.Dataset; import edu.harvard.iq.dataverse.api.UtilIT; import edu.harvard.iq.dataverse.mocks.MocksFactory; +import edu.harvard.iq.dataverse.util.FileUtil; + +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -42,11 +45,21 @@ public void setup() throws IOException { dataSet = MocksFactory.makeDataset(); dataFile.setOwner(dataSet); dataFileId = UtilIT.getRandomIdentifier(); - dataFile.setStorageIdentifier("s3://bucket:"+dataFileId); - dataSetAccess = new S3AccessIO<>(dataSet, null, s3client, "s3"); - dataFileAccess = new S3AccessIO<>(dataFile, null, s3client, "s3"); + System.setProperty("dataverse.files.s3test.type", "s3"); + System.setProperty("dataverse.files.s3test.label", "S3test"); + System.setProperty("dataverse.files.s3test.bucket-name", "thebucket"); + + dataFile.setStorageIdentifier("s3test://thebucket:"+dataFileId); + dataSetAccess = new S3AccessIO<>(dataSet, null, s3client, "s3test"); + dataFileAccess = new S3AccessIO<>(dataFile, null, s3client, "s3test"); } + @AfterEach + public void tearDown() { + System.clearProperty("dataverse.files.s3test.type"); + System.clearProperty("dataverse.files.s3test.label"); + System.clearProperty("dataverse.files.s3test.bucket-name"); + } /* createTempFile getStorageLocation @@ -99,7 +112,7 @@ void keyNullstorageIdInvalid_getMainFileKey() throws IOException { @Test void default_getUrlExpirationMinutes() { // given - System.clearProperty("dataverse.files.s3.url-expiration-minutes"); + System.clearProperty("dataverse.files.s3test.url-expiration-minutes"); // when & then assertEquals(60, dataFileAccess.getUrlExpirationMinutes()); } @@ -107,7 +120,7 @@ void default_getUrlExpirationMinutes() { @Test void validSetting_getUrlExpirationMinutes() { // given - System.setProperty("dataverse.files.s3.url-expiration-minutes", "120"); + System.setProperty("dataverse.files.s3test.url-expiration-minutes", "120"); // when & then assertEquals(120, dataFileAccess.getUrlExpirationMinutes()); } @@ -115,9 +128,20 @@ void validSetting_getUrlExpirationMinutes() { @Test void invalidSetting_getUrlExpirationMinutes() { // given - System.setProperty("dataverse.files.s3.url-expiration-minutes", "NaN"); + System.setProperty("dataverse.files.s3test.url-expiration-minutes", "NaN"); // when & then assertEquals(60, dataFileAccess.getUrlExpirationMinutes()); } + @Test + void testS3IdentifierFormats() throws IOException { + assertTrue(DataAccess.isValidDirectStorageIdentifier("s3test://thebucket:" + FileUtil.generateStorageIdentifier())); + //The tests here don't use a valid identifier string + assertFalse(DataAccess.isValidDirectStorageIdentifier(dataFile.getStorageIdentifier())); + //bad store id + assertFalse(DataAccess.isValidDirectStorageIdentifier("s3://thebucket:" + FileUtil.generateStorageIdentifier())); + //bad bucket + assertFalse(DataAccess.isValidDirectStorageIdentifier("s3test://bucket:" + FileUtil.generateStorageIdentifier())); + } + }