This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new adc6a4ac4d fix(file-service, frontend): reject slash in version names
(#8883)
adc6a4ac4d is described below
commit adc6a4ac4d79f0302534228d119ea2c22f393db7
Author: Kunwoo (Chris) <[email protected]>
AuthorDate: Sat Oct 10 22:43:11 2026 +0000
fix(file-service, frontend): reject slash in version names (#8883)
### What changes were proposed in this PR?
Root cause. `createDatasetVersion` names a version `v{N} -
{description}` from whatever the user typed, and the UI accepted a `/`.
The version name is one segment of the logical path
`/<prefix>/owner/resource/version/file`, and
`FileResolver.parsePrefixedPath` splits that path on `/`. For the path
in the issue, `/texera/bughunt-version-name/v3 - //file_name.png`, the
version becomes `v3 - ` and `/file_name.png` becomes the file path, so
the version lookup misses and `presign-download` returns a 500.
`createModelVersion` builds its name the same way, so model versions
have the same problem.
Fix. `ResourceNaming.validateVersionDescription` rejects a description
containing `/` with a 400. `createDatasetVersion` and
`createModelVersion` call it right after the write-access check, 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, shows the error under the input and disables Submit,
and `Enter` no longer submits either.
```
Before: description "/" -> version "v3 - /" -> every file in it fails
with presign-download 500
After: description "/" -> 400 "Invalid version description" (UI:
inline error, Submit disabled)
```
Not included. Versions already stored with a `/` in their name stay
unopenable, and repairing them needs a data migration. A `%` or `+` in a
name has a separate problem, because `resolveVersionedFile` URL-decodes
`filePath` a second time.
### Any related issues, documentation, discussions?
Closes #8882
### How was this PR tested?
Tests were added for the validator, for both resources, and for the
version uploader, and they were written first and failed against the old
code.
```
sbt "FileService/testOnly
org.apache.texera.service.resource.ResourceNamingSpec
org.apache.texera.service.resource.DatasetResourceSpec
org.apache.texera.service.resource.ModelUploadResourceSpec"
cd frontend && yarn ng test --watch=false
--include='**/version-uploader.component.spec.ts'
```
The first command passes 177 tests and the second passes 73.
`scalafixAll`, `scalafmtAll`, `prettier-eslint` and `eslint` ran clean
on the changed files.
https://github.com/user-attachments/assets/8c7bb472-b845-4bed-9b51-da38ef3b3397
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code, Claude Sonnet 5.5
---
.../texera/service/resource/DatasetResource.scala | 2 ++
.../texera/service/resource/ModelResource.scala | 2 ++
.../texera/service/resource/ResourceNaming.scala | 16 +++++++++
.../service/resource/DatasetResourceSpec.scala | 17 +++++++++
.../service/resource/ModelUploadResourceSpec.scala | 15 ++++++++
.../service/resource/ResourceNamingSpec.scala | 27 +++++++++++++--
.../version-uploader.component.html | 10 ++++++
.../version-uploader.component.scss | 5 +++
.../version-uploader.component.spec.ts | 40 ++++++++++++++++++++++
.../version-uploader/version-uploader.component.ts | 10 +++++-
10 files changed, 140 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 4da407fd31..ac9b5a9d62 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
@@ -423,6 +423,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 80a69cbda2..a9a6fa6f1d 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 7a71362256..b378e74d12 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 /<prefix>/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 db4c2437eb..9361e46a70 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
@@ -3975,4 +3975,21 @@ class DatasetResourceSpec
assertStatus(ex, 400)
datasetResource.getDatasetVersionList(dataset.getDid, sessionUser) should
have size 1
}
+
+ it should "reject a description containing a slash without committing" in {
+ val dataset = seedDatasetWithStagedFile("version-slash")
+
+ val ex = intercept[WebApplicationException] {
+ createVersion(dataset, "2024/01 snapshot")
+ }
+ 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.
+ LakeFSStorageClient
+ .retrieveUncommittedObjects(dataset.getRepositoryName)
+ .map(_.getPath) shouldBe List("staged.txt")
+ val retried = createVersion(dataset, "2024-01 snapshot")
+ retried.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 f4f300b7dc..70bfc0d0d3 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 71e6812eb6..fd23cdd243 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 384d706f60..1829cc611e 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
@@ -158,14 +158,24 @@
[(ngModel)]="versionName"
placeholder="Describe the new version (Optional)"
[disabled]="isCreatingVersion"
+ [attr.aria-invalid]="versionNameHasSlash ? 'true' : null"
+ [attr.aria-describedby]="versionNameHasSlash ? 'version-name-error'
: null"
(keydown.enter)="onClickCreateVersion()"
class="version-input" />
</div>
+ <div
+ *ngIf="versionNameHasSlash"
+ id="version-name-error"
+ role="alert"
+ class="version-name-error">
+ A version description cannot contain '/'.
+ </div>
<div>
<button
nz-button
nzType="primary"
[nzLoading]="isCreatingVersion"
+ [disabled]="versionNameHasSlash"
(click)="onClickCreateVersion()"
class="create-version-button">
Submit
diff --git
a/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.scss
b/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.scss
index 788b226a40..d4944a608f 100644
---
a/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.scss
+++
b/frontend/src/app/dashboard/component/user/version-uploader/version-uploader.component.scss
@@ -90,6 +90,11 @@
padding: 6px;
}
+.version-name-error {
+ margin-top: 6px;
+ color: #ff4d4f;
+}
+
.create-version-button {
display: flex; /* Use flexbox for centering */
align-items: center; /* Center vertically */
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 754bf14eeb..499a84034e 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
@@ -1175,6 +1175,46 @@ describe("VersionUploaderComponent", () => {
expect(createVersionSpy).toHaveBeenCalledWith("second cut");
});
+ it("explains why a name with a slash is refused and keeps Submit from
sending it", () => {
+ const el = withPendingChanges();
+ const submit = q<HTMLButtonElement>(el, ".create-version-button");
+
+ const input = typeName(el, "/");
+
+ expect(text(q<HTMLElement>(el, ".version-name-error"))).toBe("A
version description cannot contain '/'.");
+ expect(submit.disabled).toBe(true);
+ // Enter submits straight from the field, so it must not get around
the disabled button.
+ input.dispatchEvent(new KeyboardEvent("keydown", { key: "Enter",
bubbles: true }));
+ expect(createVersionSpy).not.toHaveBeenCalled();
+
+ // Fixing the name takes the error away and re-enables Submit.
+ typeName(el, "2024-01 snapshot");
+
+ expect(el.querySelector(".version-name-error")).toBeNull();
+ 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<HTMLInputElement>(el, ".version-input");
+ expect(input.getAttribute("aria-invalid")).toBeNull();
+ expect(input.getAttribute("aria-describedby")).toBeNull();
+
+ typeName(el, "/");
+
+ const error = q<HTMLElement>(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 cfdaca02e6..860d3226a3 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
@@ -452,8 +452,16 @@ export class VersionUploaderComponent implements OnInit {
this.userHasPendingChanges = this.pendingChangesCount > 0;
}
+ /**
+ * The name becomes a segment of the version's file paths, which are split
on "/", so a version
+ * named with one could never have its files opened.
+ */
+ get versionNameHasSlash(): boolean {
+ return this.versionName.includes("/");
+ }
+
onClickCreateVersion(): void {
- if (!this.resourceId || this.isCreatingVersion) {
+ if (!this.resourceId || this.isCreatingVersion ||
this.versionNameHasSlash) {
return;
}
this.isCreatingVersion = true;
