Skip to content
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
94ebb25
enforce strict types on folders
jaasen-livefront Oct 28, 2025
a52ae75
fix folder api service
jaasen-livefront Oct 28, 2025
7d947f5
fix tests
jaasen-livefront Oct 30, 2025
c3ca6c8
fix test
jaasen-livefront Oct 30, 2025
6dc2a77
fix type issue
jaasen-livefront Oct 30, 2025
c19190f
fix test
jaasen-livefront Oct 31, 2025
87ccdf3
Merge branch 'main' into PM-25685
jaasen-livefront Oct 31, 2025
4b692f1
add extra checks for folders. add specs
jaasen-livefront Oct 31, 2025
7da57db
fix folder.id checks
jaasen-livefront Oct 31, 2025
51e9aaf
fix id logic
jaasen-livefront Oct 31, 2025
d9b522a
remove unecessary check
jaasen-livefront Oct 31, 2025
939ec1b
name name and id optional in folder model
jaasen-livefront Nov 7, 2025
093b99e
fix tests
jaasen-livefront Nov 7, 2025
705e232
Update folder and folderview
jaasen-livefront Nov 7, 2025
c878c77
fix folder with id export
jaasen-livefront Nov 7, 2025
ac18055
fix tests
jaasen-livefront Nov 8, 2025
6577b5f
fix tests
jaasen-livefront Nov 8, 2025
c2b7e7f
more defensive typing
jaasen-livefront Nov 8, 2025
d09853e
Merge branch 'main' into PM-25685
jaasen-livefront Nov 10, 2025
f58439e
fix tests
jaasen-livefront Nov 20, 2025
7a8ec65
no need to check for presence
jaasen-livefront Nov 20, 2025
1d5da94
check for empty name in folder toDomain
jaasen-livefront Nov 20, 2025
ab47d10
fixes to folder
jaasen-livefront Nov 21, 2025
cfb3218
initialize id in folder constructor. fix failing tests
jaasen-livefront Nov 21, 2025
e09cab2
remove optional param to folder constructor
jaasen-livefront Nov 21, 2025
a4fec9e
fix folder
jaasen-livefront Nov 21, 2025
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 @@ -437,7 +437,7 @@ describe("VaultPopupListFiltersService", () => {

describe("folders$", () => {
it('returns no folders when "No Folder" is the only option', (done) => {
folderViews$.next([{ id: null, name: "No Folder" }]);
folderViews$.next([{ id: "", name: "No Folder" }]);

service.folders$.subscribe((folders) => {
expect(folders).toEqual([]);
Expand All @@ -447,7 +447,7 @@ describe("VaultPopupListFiltersService", () => {

it('moves "No Folder" to the end of the list', (done) => {
folderViews$.next([
{ id: null, name: "No Folder" },
{ id: "", name: "No Folder" },
{ id: "2345", name: "Folder 2" },
{ id: "1234", name: "Folder 1" },
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ export class VaultPopupListFiltersService {
FolderView[],
PopupCipherViewLike[],
] => {
if (folders.length === 1 && folders[0].id === null) {
if (folders.length === 1 && !folders[0].id) {
// Do not display folder selections when only the "no folder" option is available.
return [filters as PopupListFilter, [], cipherViews];
}
Expand All @@ -399,7 +399,7 @@ export class VaultPopupListFiltersService {
folders.sort(Utils.getSortFunction(this.i18nService, "name"));
let arrangedFolders = folders;

const noFolder = folders.find((f) => f.id === null);
const noFolder = folders.find((f) => !f.id);

if (noFolder) {
// Update `name` of the "no folder" option to "Items with no folder"
Expand All @@ -409,7 +409,7 @@ export class VaultPopupListFiltersService {
};

// Move the "no folder" option to the end of the list
arrangedFolders = [...folders.filter((f) => f.id !== null), updatedNoFolder];
arrangedFolders = [...folders.filter((f) => f.id), updatedNoFolder];
}
return [filters as PopupListFilter, arrangedFolders, cipherViews];
},
Expand Down Expand Up @@ -552,11 +552,7 @@ export class VaultPopupListFiltersService {

// When the organization filter changes and a folder is already selected,
// reset the folder filter if the folder does not belong to the new organization filter
if (
currentFilters.folder &&
currentFilters.folder.id !== null &&
organization.id !== MY_VAULT_ID
) {
if (currentFilters.folder && currentFilters.folder.id && organization.id !== MY_VAULT_ID) {
// Get all ciphers that belong to the new organization
const orgCiphers = this.cipherViews.filter((c) => c.organizationId === organization.id);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ describe("vault filter service", () => {
];
folderViews.next(storedFolders);

await expect(firstValueFrom(vaultFilterService.filteredFolders$)).resolves.toEqual([
createFolderView("folder test id", "test"),
await expect(firstValueFrom(vaultFilterService.filteredFolders$)).resolves.toMatchObject([
{ id: "folder test id", name: "test" },
]);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export class FolderAddEditComponent implements OnInit {

async submit(): Promise<boolean> {
this.folder.name = this.formGroup.controls.name.value;
if (this.folder.name == null || this.folder.name === "") {
if (this.folder.name === "") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐ŸŽจ We shouldn't remove this null check yet.

this.toastService.showToast({
variant: "error",
title: this.i18nService.t("errorOccurred"),
Expand Down
10 changes: 4 additions & 6 deletions libs/common/src/models/export/folder.export.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { EncString } from "../../key-management/crypto/models/enc-string";
import { Folder as FolderDomain } from "../../vault/models/domain/folder";
import { FolderView } from "../../vault/models/view/folder.view";

import { safeGetString } from "./utils";

export class FolderExport {
name: string = "";

static template(): FolderExport {
const req = new FolderExport();
req.name = "Folder name";
Expand All @@ -19,14 +19,12 @@ export class FolderExport {
}

static toDomain(req: FolderExport, domain = new FolderDomain()) {
domain.name = req.name != null ? new EncString(req.name) : null;
domain.name = new EncString(req.name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This is not functionally equivalent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hinton Indeed, but since the folder model requires name to be a type of EncString we can no longer return null.

Copy link
Collaborator Author

@jaasen-livefront jaasen-livefront Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 939ec1b. name is now optional so this should now return apply the expected types (string | undefined)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a behavioural change. If name is now optional all changes to this file should be reverted.

Copy link
Collaborator Author

@jaasen-livefront jaasen-livefront Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hinton Sorry about the confusion. Since FolderExport requires name we can just check for presence now. I've updated the code to reflect this.

return domain;
}

name: string;

// Use build method instead of ctor so that we can control order of JSON stringify for pretty print
build(o: FolderView | FolderDomain) {
this.name = safeGetString(o.name);
this.name = safeGetString(o.name ?? "") ?? "";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โŒ Finding 1: Behavioral change - this now converts nullish folder names to empty strings in exports.

Original behavior:

this.name = safeGetString(o.name); // Returns null for nullish values

New behavior:

this.name = safeGetString(o.name ?? "") ?? ""; // Always returns string

When o.name is undefined, the original would pass it to safeGetString, which returns null. The new code converts to "" first, changing the export format.

Also: The double ?? "" is redundant. Simplify to:

this.name = safeGetString(o.name ?? "");

If folders can have no name, should this distinction be preserved in exports?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โŒ Redundant double nullish coalescing

The second ?? "" is unreachable:

  • When o.name is nullish, first ?? "" converts to ""
  • safeGetString("") returns "" (not null)
  • Second ?? "" never executes

Simplify to:

this.name = safeGetString(o.name ?? "");

}
}
19 changes: 12 additions & 7 deletions libs/common/src/vault/models/data/folder.data.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { Jsonify } from "type-fest";

import { FolderResponse } from "../response/folder.response";
Expand All @@ -10,12 +8,19 @@ export class FolderData {
revisionDate: string;

constructor(response: Partial<FolderResponse>) {
this.name = response?.name;
this.id = response?.id;
this.revisionDate = response?.revisionDate;
this.name = response.name ?? "";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Finding 5: Data migration concern - nullish values now coerced to empty strings.

Before: Properties could be undefined/null
After: Properties always have default values ("" or current date)

This changes the data layer contract from nullable to non-nullable. If any persisted state contains null values for these properties, they'll silently convert to "" on read.

Consider whether:

  1. This is acceptable behavior (silent coercion vs. explicit migration)
  2. Existing user data needs migration
  3. The distinction between "no folder name" (null) vs. "empty folder name" ("") should be preserved

this.id = response.id ?? "";
this.revisionDate = response.revisionDate ?? new Date().toISOString();
}

static fromJSON(obj: Jsonify<FolderData>) {
return Object.assign(new FolderData({}), obj);
static fromJSON(obj: Jsonify<FolderData | null>) {
if (obj == null) {
return null;
}
return new FolderData({
id: obj.id,
name: obj.name,
revisionDate: obj.revisionDate,
});
}
}
43 changes: 31 additions & 12 deletions libs/common/src/vault/models/domain/folder.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { mock, MockProxy } from "jest-mock-extended";

import { makeEncString, makeSymmetricCryptoKey, mockEnc, mockFromJson } from "../../../../spec";
import { EncryptService } from "../../../key-management/crypto/abstractions/encrypt.service";
import { EncryptedString, EncString } from "../../../key-management/crypto/models/enc-string";
import { EncString } from "../../../key-management/crypto/models/enc-string";
import { FolderData } from "../../models/data/folder.data";
import { Folder } from "../../models/domain/folder";

Expand Down Expand Up @@ -42,6 +42,30 @@ describe("Folder", () => {
});
});

describe("constructor", () => {
it("initializes properties from FolderData", () => {
const revisionDate = new Date("2022-08-04T01:06:40.441Z");
const folder = new Folder({
id: "id",
name: "name",
revisionDate: revisionDate.toISOString(),
});

expect(folder.id).toBe("id");
expect(folder.revisionDate).toEqual(revisionDate);
expect(folder.name).toBeInstanceOf(EncString);
expect((folder.name as EncString).encryptedString).toBe("name");
});

it("initializes empty properties when no FolderData is provided", () => {
const folder = new Folder();

expect(folder.id).toBe(undefined);
expect(folder.name).toBe(undefined);
expect(folder.revisionDate).toBeInstanceOf(Date);
});
});

describe("fromJSON", () => {
jest.mock("../../../key-management/crypto/models/enc-string");
jest.spyOn(EncString, "fromJSON").mockImplementation(mockFromJson);
Expand All @@ -50,17 +74,14 @@ describe("Folder", () => {
const revisionDate = new Date("2022-08-04T01:06:40.441Z");
const actual = Folder.fromJSON({
revisionDate: revisionDate.toISOString(),
name: "name" as EncryptedString,
name: "name",
id: "id",
});

const expected = {
revisionDate: revisionDate,
name: "name_fromJSON",
id: "id",
};

expect(actual).toMatchObject(expected);
expect(actual?.id).toBe("id");
expect(actual?.revisionDate).toEqual(revisionDate);
expect(actual?.name).toBeInstanceOf(EncString);
expect((actual?.name as EncString).encryptedString).toBe("name");
});
});

Expand All @@ -82,9 +103,7 @@ describe("Folder", () => {

const view = await folder.decryptWithKey(key, encryptService);

expect(view).toEqual({
name: "encName",
});
expect(view.name).toBe("encName");
});

it("assigns the folder id and revision date", async () => {
Expand Down
31 changes: 15 additions & 16 deletions libs/common/src/vault/models/domain/folder.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { Jsonify } from "type-fest";

import { EncryptService } from "../../../key-management/crypto/abstractions/encrypt.service";
Expand All @@ -9,20 +7,15 @@ import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-cr
import { FolderData } from "../data/folder.data";
import { FolderView } from "../view/folder.view";

export class Test extends Domain {
id: string;
name: EncString;
revisionDate: Date;
}

export class Folder extends Domain {
id: string;
name: EncString;
id?: string;
name?: EncString;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ“ Why are these marked as optional in Folder but required in the other models? I would expect them to be consistent across models.

revisionDate: Date;

constructor(obj?: FolderData) {
super();
if (obj == null) {
this.revisionDate = new Date();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ This is inconsistent with name. Why are we setting revisionDate to a default value and not name?

return;
}

Expand All @@ -35,8 +28,8 @@ export class Folder extends Domain {
},
["id"],
);

this.revisionDate = obj.revisionDate != null ? new Date(obj.revisionDate) : null;
this.name = new EncString(obj.name);
this.revisionDate = new Date(obj.revisionDate);
}

decrypt(): Promise<FolderView> {
Expand All @@ -48,10 +41,10 @@ export class Folder extends Domain {
encryptService: EncryptService,
): Promise<FolderView> {
const folderView = new FolderView();
folderView.id = this.id;
folderView.id = this.id ?? "";
folderView.revisionDate = this.revisionDate;
try {
folderView.name = await encryptService.decryptString(this.name, key);
folderView.name = await encryptService.decryptString(this.name ?? new EncString(""), key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Passing an EncString("") to decryptString doesn't really mean anything or serve a purpose here. If the encrypted name is missing, we should just skip decryption.

} catch (e) {
// Note: This should be replaced by the owning team with appropriate, domain-specific behavior.
// eslint-disable-next-line no-console
Expand All @@ -62,7 +55,13 @@ export class Folder extends Domain {
}

static fromJSON(obj: Jsonify<Folder>) {
const revisionDate = obj.revisionDate == null ? null : new Date(obj.revisionDate);
return Object.assign(new Folder(), obj, { name: EncString.fromJSON(obj.name), revisionDate });
if (obj == null) {
return null;
}
return new Folder({
name: obj.name ?? "",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Finding 3: Potential loss of encryption metadata.

The original implementation used EncString.fromJSON(obj.name) which properly deserializes encryption type information. The new approach passes the raw string to FolderData, which gets wrapped in new EncString(obj.name) in the constructor.

Original:

const revisionDate = obj.revisionDate == null ? null : new Date(obj.revisionDate);
return Object.assign(new Folder(), obj, { 
  name: EncString.fromJSON(obj.name),  // Preserves encryption metadata
  revisionDate 
});

While the new approach may work for simple cases, EncString.fromJSON() is designed to properly deserialize the encryption type and IV from the JSON format. Consider whether this simplified approach is safe for all encryption types used in production.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โŒ Critical: Loss of encryption metadata in deserialization

This implementation passes a plain string to the FolderData constructor, which then wraps it in new EncString(obj.name) at line 31. This only creates a basic AesCbc256_B64 EncString.

The original implementation used EncString.fromJSON(obj.name) which properly deserializes encryption type information (AES-CBC vs AES-GCM, IV, MAC, etc.) from the JSON representation.

Impact: If the JSON contains encrypted data with specific encryption metadata, this will be lost during deserialization, potentially causing decryption failures or security issues.

Original implementation:

return Object.assign(new Folder(), obj, { 
  name: EncString.fromJSON(obj.name),  // Preserves encryption metadata
  revisionDate 
});

Recommendation: Either revert to using EncString.fromJSON() or document this as an intentional breaking change if encryption metadata is never present in this deserialization path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐ŸŽจ Claude makes some potentially good points here, it seems we're changing/obsfucating this implementation here when we shouldn't need to.

We should be able to call the empty Folder() constructor and explicitly assign the the name and revisionDate properties. We should also continue to use the fromJson() helpers for EncString for consistency. See the Cipher.fromJSON method for an example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed ab47d10

revisionDate: obj.revisionDate,
id: obj.id ?? "",
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ export class FolderWithIdRequest extends FolderRequest {

constructor(folder: Folder) {
super(folder);
this.id = folder.id;
this.id = folder.id ?? "";
}
}
21 changes: 12 additions & 9 deletions libs/common/src/vault/models/view/folder.view.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { Jsonify } from "type-fest";

import { View } from "../../../models/view/view";
Expand All @@ -8,21 +6,26 @@ import { Folder } from "../domain/folder";
import { ITreeNodeObject } from "../domain/tree-node";

export class FolderView implements View, ITreeNodeObject {
id: string = null;
name: string = null;
revisionDate: Date = null;
id: string = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ„น๏ธ This is going to break our checks for the "No Folder" folder that assumes a null id. We'll need to do a thorough search of the clients to be sure we catch them all. I see you touched the browser and web services, but nothing for Desktop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shane-melton I didn't see any explicit checks for a null id in the desktop client. Can you think of anywhere we many be doing that?

name: string = "";
revisionDate: Date;

constructor(f?: Folder | DecryptedObject<Folder, "name">) {
constructor(f?: Folder | DecryptedObject<Folder, undefined>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ“ undefined doesn't seem right here?

On second look, I don't see any usage of the FolderView constructor where we pass in a decrypted Folder object. I think its safe to just remove this type.

Copy link
Collaborator Author

@jaasen-livefront jaasen-livefront Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed e09cab2

if (!f) {
this.revisionDate = new Date();
return;
}

this.id = f.id;
this.id = f.id ?? "";
this.name = f.name?.decryptedValue ?? "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐ŸŽจ Since we never pass in a decrypted folder, I don't think we need to add this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed ab47d10

this.revisionDate = f.revisionDate;
}

static fromJSON(obj: Jsonify<FolderView>) {
const revisionDate = obj.revisionDate == null ? null : new Date(obj.revisionDate);
return Object.assign(new FolderView(), obj, { revisionDate });
const folderView = new FolderView();
folderView.id = obj.id;
folderView.name = obj.name;
folderView.revisionDate = new Date(obj.revisionDate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ I'd expect this to be a type error as you can't pass a | undefined into new Date(). We need to keep the check and only assign if the obj.revisionDate has a value.

See here in CipherView:

if (obj.revisionDate) {
view.revisionDate = new Date(obj.revisionDate);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed ab47d10

return folderView;
}
}
6 changes: 3 additions & 3 deletions libs/common/src/vault/services/folder/folder-api.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ export class FolderApiService implements FolderApiServiceAbstraction {
const request = new FolderRequest(folder);

let response: FolderResponse;
if (folder.id == null) {
if (folder.id) {
response = await this.putFolder(folder.id, request);
} else {
response = await this.postFolder(request);
folder.id = response.id;
} else {
response = await this.putFolder(folder.id, request);
}

const data = new FolderData(response);
Expand Down
11 changes: 6 additions & 5 deletions libs/common/src/vault/services/folder/folder.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ describe("Folder Service", () => {
encryptedString: "ENC",
encryptionType: 0,
},
revisionDate: expect.any(Date),
});
});

Expand All @@ -132,7 +133,7 @@ describe("Folder Service", () => {
expect(result).toEqual({
id: "1",
name: makeEncString("ENC_STRING_" + 1),
revisionDate: null,
revisionDate: expect.any(Date),
});
});

Expand All @@ -150,12 +151,12 @@ describe("Folder Service", () => {
{
id: "1",
name: makeEncString("ENC_STRING_" + 1),
revisionDate: null,
revisionDate: expect.any(Date),
},
{
id: "2",
name: makeEncString("ENC_STRING_" + 2),
revisionDate: null,
revisionDate: expect.any(Date),
},
]);
});
Expand All @@ -167,7 +168,7 @@ describe("Folder Service", () => {
{
id: "4",
name: makeEncString("ENC_STRING_" + 4),
revisionDate: null,
revisionDate: expect.any(Date),
},
]);
});
Expand Down Expand Up @@ -203,7 +204,7 @@ describe("Folder Service", () => {

const folderViews = await firstValueFrom(folderService.folderViews$(mockUserId));
expect(folderViews.length).toBe(1);
expect(folderViews[0].id).toBeNull(); // Should be the "No Folder" folder
expect(folderViews[0].id).toEqual(""); // Should be the "No Folder" folder
});

describe("getRotatedData", () => {
Expand Down
Loading
Loading