[PM-25685] - Migrate all Folder models #17077
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #17077 +/- ##
==========================================
+ Coverage 40.63% 41.25% +0.62%
==========================================
Files 3531 3546 +15
Lines 101119 102050 +931
Branches 15150 15316 +166
==========================================
+ Hits 41087 42098 +1011
+ Misses 58293 58185 -108
- Partials 1739 1767 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @jaasen-livefront's task —— View job Code Review - Recent ChangesI've reviewed the recent commits addressing shane-melton's feedback. Here's my assessment: Summary of Changes Since Last ReviewThe author made 5 commits addressing previous review feedback:
Status of Previous Review Items: ✅ RESOLVED: Optional fields made consistent (id and name now have defaults in Folder) Critical Issues Found❌ Finding 1: FolderView constructor doesn't validate folder state before accessing When a Folder is constructed via Recommendation: Consider validating folder has encrypted data before constructing view, or document that FolderView constructor expects properly encrypted folders. ❌ Finding 2: FolderView.fromJSON doesn't initialize revisionDate when null/undefined, violating type contract (folder.view.ts:27-29) The conditional assignment means Current code: if (obj.revisionDate != null) {
folderView.revisionDate = new Date(obj.revisionDate);
}Should be (matching CipherView pattern at cipher.view.ts:225-227): if (obj.revisionDate != null) {
folderView.revisionDate = new Date(obj.revisionDate);
} else {
folderView.revisionDate = new Date();
}Or make ❌ Finding 3: Redundant double nullish coalescing in FolderExport.build() (folder.export.ts:28) this.name = safeGetString(o.name ?? "") ?? "";The second
Simplify to: this.name = safeGetString(o.name ?? "");shane-melton correctly identified this. The pattern should match Cipher.fromJSON approach:
Current implementation: static fromJSON(obj: Jsonify<Folder>) {
if (obj == null) {
return null;
}
const folder = new Folder();
folder.id = obj.id;
folder.name = EncString.fromJSON(obj.name);
folder.revisionDate = new Date(obj.revisionDate);
return folder;
}Issues:
Recommended pattern (matching Cipher): static fromJSON(obj: Jsonify<Folder>) {
if (obj == null) {
return null;
}
const folder = new Folder();
folder.id = obj.id ?? "";
folder.name = EncString.fromJSON(obj.name) ?? new EncString("");
folder.revisionDate = obj.revisionDate ? new Date(obj.revisionDate) : new Date();
return folder;
}static toDomain(req: FolderExport, domain = new FolderDomain()) {
domain.name = new EncString(req.name);
return domain;
}Since Good Practices Observed
Action ItemsRequired:
Recommended: Response to Author's Question
✅ Confirmed: I searched the Desktop client codebase and found no explicit null checks for folder.id (patterns like The Browser and Web clients were correctly updated to use truthy checks ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Hinton
left a comment
Hinton
left a comment
There was a problem hiding this comment.
This seems to change the behaviour of the FolderExport model.
|
|
||
| static toDomain(req: FolderExport, domain = new FolderDomain()) { | ||
| domain.name = req.name != null ? new EncString(req.name) : null; | ||
| domain.name = new EncString(req.name); |
There was a problem hiding this comment.
issue: This is not functionally equivalent.
There was a problem hiding this comment.
@Hinton Indeed, but since the folder model requires name to be a type of EncString we can no longer return null.
There was a problem hiding this comment.
Fixed 939ec1b. name is now optional so this should now return apply the expected types (string | undefined)
There was a problem hiding this comment.
This is still a behavioural change. If name is now optional all changes to this file should be reverted.
There was a problem hiding this comment.
@Hinton Sorry about the confusion. Since FolderExport requires name we can just check for presence now. I've updated the code to reflect this.
| // 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) ?? ""; |
There was a problem hiding this comment.
issue: This is not functionally equivalent.
There was a problem hiding this comment.
@Hinton We're modelling FolderExport to match the Folder domain so we now requires name to be of type string. Since safeGetString can return undefined we must fallback to an empty string. Do you suggest we have name be optional?
| export class Folder extends Domain { | ||
| id: string; | ||
| name: EncString; | ||
| id: string = ""; |
There was a problem hiding this comment.
I'm pretty sure id can be optional, when you create a folder ID should be null/undefined.
|
|
||
| static toDomain(req: FolderExport, domain = new FolderDomain()) { | ||
| domain.name = req.name != null ? new EncString(req.name) : null; | ||
| domain.name = new EncString(req.name); |
There was a problem hiding this comment.
This is still a behavioural change. If name is now optional all changes to this file should be reverted.
There was a problem hiding this comment.
@Hinton I've reverted it back to close to the original. We're now checking for object equality instead of string equality which makes the tests less brittle.
| // 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 ?? "") ?? ""; |
There was a problem hiding this comment.
❌ 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 valuesNew behavior:
this.name = safeGetString(o.name ?? "") ?? ""; // Always returns stringWhen 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?
|
|
||
| static toDomain(req: FolderExport, domain = new FolderDomain()) { | ||
| domain.name = req.name != null ? new EncString(req.name) : null; | ||
| domain.name = req.name ? new EncString(req.name) : undefined; |
There was a problem hiding this comment.
❌ Finding 2: Changed from null to undefined and from != null to truthy check.
Original:
domain.name = req.name != null ? new EncString(req.name) : null;New:
domain.name = req.name ? new EncString(req.name) : undefined;This changes behavior for empty strings: toDomain({ name: "" }) now returns undefined instead of new EncString("") because "" is falsy.
| return null; | ||
| } | ||
| return new Folder({ | ||
| name: obj.name ?? "", |
There was a problem hiding this comment.
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.
| this.name = response?.name; | ||
| this.id = response?.id; | ||
| this.revisionDate = response?.revisionDate; | ||
| this.name = response.name ?? ""; |
There was a problem hiding this comment.
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:
- This is acceptable behavior (silent coercion vs. explicit migration)
- Existing user data needs migration
- The distinction between "no folder name" (null) vs. "empty folder name" ("") should be preserved
| return null; | ||
| } | ||
| return new Folder({ | ||
| name: obj.name ?? "", |
There was a problem hiding this comment.
❌ 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.
| async submit(): Promise<boolean> { | ||
| this.folder.name = this.formGroup.controls.name.value; | ||
| if (this.folder.name == null || this.folder.name === "") { | ||
| if (this.folder.name === "") { |
There was a problem hiding this comment.
🎨 We shouldn't remove this null check yet.
| id?: string; | ||
| name?: EncString; |
There was a problem hiding this comment.
❓ Why are these marked as optional in Folder but required in the other models? I would expect them to be consistent across models.
| return; | ||
| } | ||
|
|
||
| this.buildDomainModel( |
There was a problem hiding this comment.
🎨 If we're setting the name and revision date manually then we no longer need to call this buildDomainModel() method.
| constructor(obj?: FolderData) { | ||
| super(); | ||
| if (obj == null) { | ||
| this.revisionDate = new Date(); |
There was a problem hiding this comment.
💭 This is inconsistent with name. Why are we setting revisionDate to a default value and not name?
| folderView.revisionDate = this.revisionDate; | ||
| try { | ||
| folderView.name = await encryptService.decryptString(this.name, key); | ||
| folderView.name = await encryptService.decryptString(this.name ?? new EncString(""), key); |
There was a problem hiding this comment.
EncString("") to decryptString doesn't really mean anything or serve a purpose here. If the encrypted name is missing, we should just skip decryption.
| return null; | ||
| } | ||
| return new Folder({ | ||
| name: obj.name ?? "", |
There was a problem hiding this comment.
🎨 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.
| revisionDate: Date; | ||
|
|
||
| constructor(f?: Folder | DecryptedObject<Folder, "name">) { | ||
| constructor(f?: Folder | DecryptedObject<Folder, undefined>) { |
There was a problem hiding this comment.
❓ 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.
|
|
||
| this.id = f.id; | ||
| this.id = f.id ?? ""; | ||
| this.name = f.name?.decryptedValue ?? ""; |
There was a problem hiding this comment.
🎨 Since we never pass in a decrypted folder, I don't think we need to add this.
| const folderView = new FolderView(); | ||
| folderView.id = obj.id ?? ""; | ||
| folderView.name = obj.name ?? ""; | ||
| folderView.revisionDate = new Date(obj.revisionDate); |
There was a problem hiding this comment.
| undefined into new Date(). We need to keep the check and only assign if the obj.revisionDate has a value.
See here in CipherView:
clients/libs/common/src/vault/models/view/cipher.view.ts
Lines 225 to 227 in 7e5f02f
| id: string = null; | ||
| name: string = null; | ||
| revisionDate: Date = null; | ||
| id: string = ""; |
There was a problem hiding this comment.
ℹ️ 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.
There was a problem hiding this comment.
@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?
| folderView.name = obj.name ?? ""; | ||
| if (obj.revisionDate != null) { | ||
| folderView.revisionDate = new Date(obj.revisionDate); | ||
| } |
There was a problem hiding this comment.
❌ Critical: revisionDate may remain uninitialized
When obj.revisionDate is null or undefined, this property is never set, leaving it undefined despite being typed as non-optional Date (line 10). This violates the type contract.
Fix:
folderView.revisionDate = obj.revisionDate != null ? new Date(obj.revisionDate) : new Date();This matches the pattern used in the FolderView constructor (line 14) which initializes to current date when no folder is provided.
| // 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 ?? "") ?? ""; |
There was a problem hiding this comment.
❌ Redundant double nullish coalescing
The second ?? "" is unreachable:
- When
o.nameis nullish, first?? ""converts to"" safeGetString("")returns""(notnull)- Second
?? ""never executes
Simplify to:
this.name = safeGetString(o.name ?? "");
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-25685
📔 Objective
This PR migrates the existing folder models to enforce strict type checking. Please see ticket description for additional details.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes