Skip to content
Merged
Changes from 1 commit
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
57 changes: 30 additions & 27 deletions backend/src/services/secret-folder/secret-folder-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,19 +231,19 @@
tx
);

return doc;
});
const [folderWithFullPath] = await folderDAL.findSecretPathByFolderIds(projectId, [doc.id], tx);

const [folderWithFullPath] = await folderDAL.findSecretPathByFolderIds(projectId, [folder.id]);
if (!folderWithFullPath) {
throw new NotFoundError({
message: `Failed to retrieve path for folder with ID '${doc.id}'`
});
}

if (!folderWithFullPath) {
throw new NotFoundError({
message: `Failed to retrieve path for folder with ID '${folder.id}'`
});
}
return { ...doc, path: folderWithFullPath.path };
});

await snapshotService.performSnapshot(folder.parentId as string);
return { ...folder, path: folderWithFullPath.path };
return folder;
};

const updateManyFolders = async ({
Expand Down Expand Up @@ -461,7 +461,7 @@
}
}

const newFolder = await folderDAL.transaction(async (tx) => {
const { newFolder, newFolderPath, oldFolderPath } = await folderDAL.transaction(async (tx) => {
const [doc] = await folderDAL.update(
{ envId: env.id, id: folder.id, parentId: parentFolder.id, isReserved: false },
{ name, description },
Expand Down Expand Up @@ -495,34 +495,37 @@
}
]
},
tx
);
if (!doc) throw new NotFoundError({ message: `Failed to update folder with ID '${id}'`, name: "UpdateFolder" });
return doc;
});

Check notice on line 501 in backend/src/services/secret-folder/secret-folder-service.ts

View check run for this annotation

Claude / Claude Code Review

updateFolder: \!doc null guard fires after doc properties are already accessed

The `\!doc` null guard in `updateFolder`'s transaction body fires after `doc.name`, `doc.envId`, `doc.version`, `doc.id`, and `doc.description` are already accessed in `folderVersionDAL.create` and `folderCommitService.createCommit`. If `folderDAL.update` matches no rows and `doc` is `undefined`, a `TypeError` is thrown instead of the intended `NotFoundError`. This is a pre-existing bug that this PR did not introduce or worsen — the guard was already misplaced before the PR and remains so after.
Comment thread
sheensantoscapadngan marked this conversation as resolved.
const foldersWithFullPaths = await folderDAL.findSecretPathByFolderIds(projectId, [newFolder.id, folder.id]);
// Read the folder paths inside the transaction to avoid replication lag issues
// when using read replicas. The transaction ensures we read from the primary.
const foldersWithFullPaths = await folderDAL.findSecretPathByFolderIds(projectId, [doc.id, folder.id], tx);

const newFolderWithFullPath = foldersWithFullPaths.find((f) => f?.id === newFolder.id);
if (!newFolderWithFullPath) {
throw new NotFoundError({
message: `Failed to retrieve path for folder with ID '${newFolder.id}'`
});
}
const newFolderWithFullPath = foldersWithFullPaths.find((f) => f?.id === doc.id);
if (!newFolderWithFullPath) {
throw new NotFoundError({
message: `Failed to retrieve path for folder with ID '${doc.id}'`
});
}

const folderWithFullPath = foldersWithFullPaths.find((f) => f?.id === folder.id);
if (!folderWithFullPath) {
throw new NotFoundError({
message: `Failed to retrieve path for folder with ID '${folder.id}'`
});
}
const folderWithFullPath = foldersWithFullPaths.find((f) => f?.id === folder.id);
if (!folderWithFullPath) {
throw new NotFoundError({
message: `Failed to retrieve path for folder with ID '${folder.id}'`
});
}

return { newFolder: doc, newFolderPath: newFolderWithFullPath.path, oldFolderPath: folderWithFullPath.path };
});

await snapshotService.performSnapshot(newFolder.parentId as string);
await secretV2BridgeDAL.invalidateSecretCacheByProjectId(projectId);
return {
folder: { ...newFolder, path: newFolderWithFullPath.path },
old: { ...folder, path: folderWithFullPath.path }
folder: { ...newFolder, path: newFolderPath },
old: { ...folder, path: oldFolderPath }
};

Check warning on line 528 in backend/src/services/secret-folder/secret-folder-service.ts

View check run for this annotation

Claude / Claude Code Review

updateFolder: oldFolderPath always equals newFolderPath after rename

In `updateFolder`, `doc.id === folder.id` because a SQL UPDATE preserves the row's primary key — only `name`/`description` change. The PR calls `findSecretPathByFolderIds(projectId, [doc.id, folder.id], tx)` with two identical IDs inside the same transaction (after the UPDATE), so both lookups resolve to the same post-rename path, making `oldFolderPath === newFolderPath` whenever a folder is renamed. Current route handlers only use `old.name` for audit logging (not `old.path`), so there is no im
Comment thread
sheensantoscapadngan marked this conversation as resolved.
Outdated
};

const $checkFolderPolicy = async ({
Expand Down
Loading