From 32527984648151845eda0395211ade5db2b77926 Mon Sep 17 00:00:00 2001 From: Kunwoo Park Date: Tue, 6 Oct 2026 10:54:36 -0700 Subject: [PATCH 1/2] fix(file-service, frontend): reject slash in version names createDatasetVersion and createModelVersion build the version name "v{N} - {description}" from free text. The name is one segment of the logical path //owner/resource/version/file, which FileResolver splits on "/", so a "/" in the description shifted every later segment and none of that version's files could be opened again (presign-download returned a 500). Reject a description containing "/" with a 400 before the LakeFS commit, so the staged files stay staged and the user can fix the description and retry. The version uploader, which datasets and models both use, now shows the error and disables Submit instead of sending the request. Versions already stored with a "/" in their name are not repaired. Closes #8882 --- .../service/resource/DatasetResource.scala | 2 ++ .../service/resource/ModelResource.scala | 2 ++ .../service/resource/ResourceNaming.scala | 16 +++++++++++ .../resource/DatasetResourceSpec.scala | 22 +++++++++++++++ .../resource/ModelUploadResourceSpec.scala | 15 +++++++++++ .../service/resource/ResourceNamingSpec.scala | 27 ++++++++++++++++--- .../version-uploader.component.html | 6 +++++ .../version-uploader.component.scss | 5 ++++ .../version-uploader.component.spec.ts | 19 +++++++++++++ .../version-uploader.component.ts | 10 ++++++- 10 files changed, 120 insertions(+), 4 deletions(-) diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala index 76eb3ebd742..cff1d10f670 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala @@ -420,6 +420,8 @@ class DatasetResource extends LazyLogging { throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE) } + ResourceNaming.validateVersionDescription(versionName) + val dataset = getDatasetByID(ctx, did) val datasetName = dataset.getName val repositoryName = dataset.getRepositoryName diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala index 80a69cbda2a..a9a6fa6f1d0 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala @@ -741,6 +741,8 @@ class ModelResource extends LazyLogging { throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE) } + ResourceNaming.validateVersionDescription(versionName) + val model = getModelByID(ctx, mid) val modelName = model.getName val repositoryName = model.getRepositoryName diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/ResourceNaming.scala b/file-service/src/main/scala/org/apache/texera/service/resource/ResourceNaming.scala index 7a71362256c..b378e74d12f 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/ResourceNaming.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/ResourceNaming.scala @@ -54,6 +54,22 @@ object ResourceNaming { } } + /** + * Rejects a version description containing "/". The version name is a segment of the logical + * path //ownerEmail/resourceName/versionName/file, which FileResolver splits on "/", so + * none of such a version's files could be opened. A null or empty description is allowed: the + * version is then named by its number alone. + * + * @throws jakarta.ws.rs.BadRequestException if the description contains a "/". + */ + def validateVersionDescription(description: String): Unit = + if (description != null && description.contains("/")) { + throw new BadRequestException( + "Invalid version description: '/' is not allowed because the version name is part of " + + "the paths of its files." + ) + } + /** * Validates a file path supplied for a resource's contents. * diff --git a/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala b/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala index b728b748c5c..5766a3becda 100644 --- a/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala +++ b/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala @@ -3881,4 +3881,26 @@ class DatasetResourceSpec .get .getFilePath should startWith("/dataset/") } + + "createDatasetVersion" should "reject a description containing a slash without committing" in { + val dataset = seedDataset(s"version-slash-${System.nanoTime()}", ownerUser.getUid) + LakeFSStorageClient.initRepo(dataset.getRepositoryName) + LakeFSStorageClient.writeFileToRepo( + dataset.getRepositoryName, + "staged.txt", + new ByteArrayInputStream("content".getBytes(StandardCharsets.UTF_8)) + ) + + val ex = intercept[WebApplicationException] { + datasetResource.createDatasetVersion("2024/01 snapshot", dataset.getDid, sessionUser) + } + assertStatus(ex, 400) + + // Nothing was committed: the file is still staged and no version row exists, so the retry + // with a usable description succeeds as the first version. + datasetResource + .createDatasetVersion("2024-01 snapshot", dataset.getDid, sessionUser) + .datasetVersion + .getName shouldBe "v1 - 2024-01 snapshot" + } } diff --git a/file-service/src/test/scala/org/apache/texera/service/resource/ModelUploadResourceSpec.scala b/file-service/src/test/scala/org/apache/texera/service/resource/ModelUploadResourceSpec.scala index f4f300b7dcf..70bfc0d0d3a 100644 --- a/file-service/src/test/scala/org/apache/texera/service/resource/ModelUploadResourceSpec.scala +++ b/file-service/src/test/scala/org/apache/texera/service/resource/ModelUploadResourceSpec.scala @@ -326,6 +326,21 @@ class ModelUploadResourceSpec .map(_.getName) should contain("weights.pt") } + it should "reject a description containing a slash without committing to LakeFS" in { + val model = newModel() + val mid = model.model.getMid + + uploadOneShot(mid, "weights.pt", Array.fill[Byte](32)(0x1)).getStatus shouldEqual 200 + + intercept[BadRequestException] { + modelResource.createModelVersion("2024/01 snapshot", mid, sessionUser) + } + + // The staged change survived, so a retry with a usable description works. + val recovered = modelResource.createModelVersion("2024-01 snapshot", mid, sessionUser) + recovered.modelVersion.getName should endWith("2024-01 snapshot") + } + "retrieveModelVersionRootFileNodes" should "404 for a version belonging to another model" in { val modelA = newModel() val modelB = newModel() diff --git a/file-service/src/test/scala/org/apache/texera/service/resource/ResourceNamingSpec.scala b/file-service/src/test/scala/org/apache/texera/service/resource/ResourceNamingSpec.scala index 71e6812eb6b..fd23cdd2438 100644 --- a/file-service/src/test/scala/org/apache/texera/service/resource/ResourceNamingSpec.scala +++ b/file-service/src/test/scala/org/apache/texera/service/resource/ResourceNamingSpec.scala @@ -23,9 +23,9 @@ import jakarta.ws.rs.BadRequestException import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -// Focused unit tests for ResourceNaming.validateAndNormalizeFilePathOrThrow, which guards -// every upload/lookup path. These call the pure helper directly and avoid the heavy -// DatasetResourceSpec integration harness (DB + LakeFS). +// Focused unit tests for ResourceNaming's pure validators: validateAndNormalizeFilePathOrThrow, +// which guards every upload/lookup path, and validateVersionDescription. These call the helpers +// directly and avoid the heavy DatasetResourceSpec integration harness (DB + LakeFS). class ResourceNamingSpec extends AnyFlatSpec with Matchers { "validateAndNormalizeFilePathOrThrow" should "reject a null path" in { @@ -76,4 +76,25 @@ class ResourceNamingSpec extends AnyFlatSpec with Matchers { "a/./b/../c.csv" ) shouldBe "a/c.csv" } + + "validateVersionDescription" should "reject a description containing a slash anywhere" in { + Seq("/", "2024/01 snapshot", "/leading", "trailing/", "a//b").foreach { description => + val ex = intercept[BadRequestException] { + ResourceNaming.validateVersionDescription(description) + } + ex.getMessage should include("'/'") + } + } + + it should "accept a null or empty description, which names the version by its number alone" in { + ResourceNaming.validateVersionDescription(null) + ResourceNaming.validateVersionDescription("") + } + + it should "accept any description without a slash" in { + Seq("second upload", "2024-01 snapshot", "a\\b", "naïve 数据 \uD83D\uDE00", " ") + .foreach { + ResourceNaming.validateVersionDescription + } + } } diff --git a/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.html b/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.html index 384d706f600..b316b2fb95e 100644 --- a/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.html +++ b/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.html @@ -161,11 +161,17 @@ (keydown.enter)="onClickCreateVersion()" class="version-input" /> +
+ A version description cannot contain '/'. +
diff --git a/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.spec.ts b/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.spec.ts index 6a517d95a81..499a84034ec 100644 --- a/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.spec.ts @@ -1194,6 +1194,27 @@ describe("VersionUploaderComponent", () => { expect(submit.disabled).toBe(false); }); + it("announces the slash error to assistive tech and ties it to the name field", () => { + const el = withPendingChanges(); + const input = q(el, ".version-input"); + expect(input.getAttribute("aria-invalid")).toBeNull(); + expect(input.getAttribute("aria-describedby")).toBeNull(); + + typeName(el, "/"); + + const error = q(el, ".version-name-error"); + expect(error.getAttribute("role")).toBe("alert"); + expect(input.getAttribute("aria-invalid")).toBe("true"); + // The input must point at the message that is actually on the page. + expect(error.id).not.toBe(""); + expect(input.getAttribute("aria-describedby")).toBe(error.id); + + typeName(el, "2024-01 snapshot"); + + expect(input.getAttribute("aria-invalid")).toBeNull(); + expect(input.getAttribute("aria-describedby")).toBeNull(); + }); + it("submits the version straight from the name field with Enter", () => { const el = withPendingChanges(); diff --git a/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.ts b/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.ts index 317588bc38e..860d3226a30 100644 --- a/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.ts +++ b/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.ts @@ -457,7 +457,7 @@ export class VersionUploaderComponent implements OnInit { * named with one could never have its files opened. */ get versionNameHasSlash(): boolean { - return this.versionName?.includes("/") ?? false; + return this.versionName.includes("/"); } onClickCreateVersion(): void {