Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Comment thread
kunwp1 marked this conversation as resolved.
<div>
<button
nz-button
nzType="primary"
[nzLoading]="isCreatingVersion"
[disabled]="versionNameHasSlash"
(click)="onClickCreateVersion()"
Comment thread
kunwp1 marked this conversation as resolved.
class="create-version-button">
Submit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading