[PB-6505] Minimize secret data we store#2020
Conversation
Deploying drive-web with
|
| Latest commit: |
b9325b2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b21b7aef.drive-web.pages.dev |
| Branch Preview URL: | https://minimize-stored-b2b.drive-web.pages.dev |
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR replaces the single JSON-serialized ChangesB2B Workspace Storage Refactor
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant UI as DriveView / TrashService / SdkFactory
participant Store as workspacesStore
participant LocalStorage as localStorageService
UI->>LocalStorage: setB2BWorkspace(workspaceId, workspaceMnemonic)
Store->>LocalStorage: get(B2BworkspaceId)
LocalStorage-->>Store: workspaceId
alt unselect workspace
Store->>LocalStorage: clearB2BWorkspace()
else fetch encryption key
UI->>LocalStorage: getB2BWorkspaceMnemonic()
LocalStorage-->>UI: mnemonic
end
Related PRs: None identified. Suggested labels: refactor, storage, workspaces Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/app/drive/services/network.service/getEnvironmentConfig.test.ts (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale helper name after refactor.
createMockWorkspacenow returns a plain mnemonic string ('workspace-key'), not a workspace object. Consider renaming tocreateMockWorkspaceMnemonicfor clarity, since the old name is misleading post-refactor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/drive/services/network.service/getEnvironmentConfig.test.ts` at line 42, The helper name is stale after the refactor: `createMockWorkspace` now returns a mnemonic string rather than a workspace object, so rename it to `createMockWorkspaceMnemonic` in `getEnvironmentConfig.test.ts` and update any nearby references to match the new meaning for clarity.src/services/local-storage.service.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor API asymmetry: no
getB2BWorkspaceId()accessor.
getB2BWorkspaceMnemonic()is exposed as a dedicated method, but callers needing the workspace id (SdkFactory, workspacesStore, getTrash.service) must importLocalStorageItemand call the genericget(LocalStorageItem.B2BworkspaceId)directly. Adding a matchinggetB2BWorkspaceId()would keep the service's encapsulation consistent for both halves of the same conceptual value.♻️ Suggested addition
function getB2BWorkspaceMnemonic(): string | null { return get(LocalStorageItem.B2BworkspaceMnemonic); } + +function getB2BWorkspaceId(): string | null { + return get(LocalStorageItem.B2BworkspaceId); +}Also applies to: 64-76, 115-117, 135-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/local-storage.service.ts` at line 2, Add a dedicated B2B workspace ID accessor to LocalStorageService to match the existing getB2BWorkspaceMnemonic() helper and avoid leaking LocalStorageItem usage to callers. Update the LocalStorageService class by introducing getB2BWorkspaceId() that reads the same underlying B2B workspace ID entry currently accessed via LocalStorageItem.B2BworkspaceId, and then switch the affected call sites in SdkFactory, workspacesStore, and getTrash.service to use the new method instead of calling get(...) directly. Keep the naming and behavior consistent with the existing mnemonic accessor so both parts of the workspace credentials API are exposed symmetrically.src/services/local-storage.service.test.ts (1)
127-127: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSeed
B2BworkspaceIdwith a plain string
LocalStorageItem.B2BworkspaceIdis stored as a raw workspace id, soJSON.stringify(mockWorkspaceData)is misleading here. Use a simple id fixture like'test-workspace-id', or drop the seed if nothing reads it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/local-storage.service.test.ts` at line 127, The test setup is seeding LocalStorageItem.B2BworkspaceId with a JSON string, but this key is supposed to hold a raw workspace id. Update the localStorage fixture in local-storage.service.test.ts to use a plain string value such as a simple workspace id, or remove the seed entirely if LocalStorageItem.B2BworkspaceId is not consumed by the test; keep the change focused around the localStorage.setItem setup and any related mockWorkspaceData/stringifyWorkspaceData usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/store/slices/workspaces/workspacesStore.ts`:
- Around line 74-76: Delay the local workspace restore in
checkAndSetLocalWorkspace until fetchWorkspaces has finished populating state,
since the current B2BworkspaceId branch can run before
state.workspaces.selectedWorkspace and state.workspaces.workspaces are ready and
dispatch null. Update the login flow around
fetchWorkspaces/checkAndSetLocalWorkspace so restoration happens after the fetch
completes, or trigger the restore from the fetch completion path using the
existing workspacesStore logic and the LocalStorageItem.B2BworkspaceId check.
In `@src/views/Trash/services/getTrash.service.ts`:
- Around line 87-89: The workspaceId handling in getTrash.service.ts is unsafe
because localStorageService.get can return null and the current workspaceId as
string cast hides the missing-value case. Update getTrashItems call-site in the
getTrash service to explicitly guard for a missing B2BworkspaceId before calling
workspacesService.getTrashItems, and handle that branch appropriately instead of
forcing the nullable value through a cast. Use the existing workspaceId,
trashType, and getTrashItems symbols to keep the fix localized.
---
Nitpick comments:
In `@src/app/drive/services/network.service/getEnvironmentConfig.test.ts`:
- Line 42: The helper name is stale after the refactor: `createMockWorkspace`
now returns a mnemonic string rather than a workspace object, so rename it to
`createMockWorkspaceMnemonic` in `getEnvironmentConfig.test.ts` and update any
nearby references to match the new meaning for clarity.
In `@src/services/local-storage.service.test.ts`:
- Line 127: The test setup is seeding LocalStorageItem.B2BworkspaceId with a
JSON string, but this key is supposed to hold a raw workspace id. Update the
localStorage fixture in local-storage.service.test.ts to use a plain string
value such as a simple workspace id, or remove the seed entirely if
LocalStorageItem.B2BworkspaceId is not consumed by the test; keep the change
focused around the localStorage.setItem setup and any related
mockWorkspaceData/stringifyWorkspaceData usage.
In `@src/services/local-storage.service.ts`:
- Line 2: Add a dedicated B2B workspace ID accessor to LocalStorageService to
match the existing getB2BWorkspaceMnemonic() helper and avoid leaking
LocalStorageItem usage to callers. Update the LocalStorageService class by
introducing getB2BWorkspaceId() that reads the same underlying B2B workspace ID
entry currently accessed via LocalStorageItem.B2BworkspaceId, and then switch
the affected call sites in SdkFactory, workspacesStore, and getTrash.service to
use the new method instead of calling get(...) directly. Keep the naming and
behavior consistent with the existing mnemonic accessor so both parts of the
workspace credentials API are exposed symmetrically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 460b80a9-71b4-45de-a165-640e5896e037
📒 Files selected for processing (13)
src/app/core/factory/sdk/index.test.tssrc/app/core/factory/sdk/index.tssrc/app/core/types.tssrc/app/drive/services/network.service/getEnvironmentConfig.test.tssrc/app/drive/services/network.service/getEnvironmentConfig.tssrc/app/store/slices/workspaces/workspacesStore.test.tssrc/app/store/slices/workspaces/workspacesStore.tssrc/services/local-storage.service.test.tssrc/services/local-storage.service.tssrc/services/navigation.service.tssrc/views/Drive/DriveView.tsxsrc/views/Trash/services/getTrash.service.test.tssrc/views/Trash/services/getTrash.service.ts
| const b2bWorkspaceId = localStorageService.get(LocalStorageItem.B2BworkspaceId); | ||
| if (b2bWorkspaceId) { | ||
| const workspaceId = b2bWorkspaceId; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "checkAndSetLocalWorkspace|fetchWorkspaces\(" --type=ts -C3 -g '!*.test.ts'Repository: internxt/drive-web
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the workspace store file and nearby symbols.
git ls-files 'src/app/store/slices/workspaces/*' 'src/app/**' | sed -n '1,200p'
echo '---'
ast-grep outline src/app/store/slices/workspaces/workspacesStore.ts --view expanded || true
echo '---'
sed -n '1,220p' src/app/store/slices/workspaces/workspacesStore.ts
echo '---SEARCH---'
rg -n "checkAndSetLocalWorkspace|setSelectedWorkspace|fetchWorkspaces|selectedWorkspace|workspaces\.workspaces|LocalStorageItem\.B2Bworkspace" src/app -g '!*.test.ts'Repository: internxt/drive-web
Length of output: 37876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'src/app/store/slices/workspaces/*' 'src/app/**' | sed -n '1,200p'
echo '---'
ast-grep outline src/app/store/slices/workspaces/workspacesStore.ts --view expanded || true
echo '---'
sed -n '1,220p' src/app/store/slices/workspaces/workspacesStore.ts
echo '---SEARCH---'
rg -n "checkAndSetLocalWorkspace|setSelectedWorkspace|fetchWorkspaces|selectedWorkspace|workspaces\.workspaces|LocalStorageItem\.B2Bworkspace" src/app -g '!*.test.ts'Repository: internxt/drive-web
Length of output: 37876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '---REDUCERS---'
sed -n '235,390p' src/app/store/slices/workspaces/workspacesStore.ts
echo '---BOOTSTRAP SEARCH---'
rg -n "workspaceThunks\.(checkAndSetLocalWorkspace|fetchWorkspaces)|checkAndSetLocalWorkspace\(|fetchWorkspaces\(" src/app src -g '!*.test.ts'
echo '---INIT FILES CANDIDATES---'
git ls-files 'src/app/**/index.ts' 'src/app/**/index.tsx' 'src/app/**/App*' 'src/app/**/bootstrap*' 'src/app/**/main*' 'src/app/**/store*' | sed -n '1,200p'Repository: internxt/drive-web
Length of output: 8442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '---GLOBAL SEARCH---'
rg -n "checkAndSetLocalWorkspace|fetchWorkspaces|workspaceThunks" . -g '!*.test.ts' -g '!node_modules/**'
echo '---FILES WITH APP INIT/BOOT---'
fd -H -a 'index.ts|index.tsx|main.ts|main.tsx|App.tsx|bootstrap.ts|bootstrap.tsx' src app . 2>/dev/null | sed -n '1,200p' || trueRepository: internxt/drive-web
Length of output: 15589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '---GLOBAL SEARCH---'
rg -n "checkAndSetLocalWorkspace|fetchWorkspaces|workspaceThunks" . -g '!*.test.ts' -g '!node_modules/**'
echo '---BOOT FILES---'
fd -H -a 'index.ts|index.tsx|main.ts|main.tsx|App.tsx|bootstrap.ts|bootstrap.tsx' src . 2>/dev/null | sed -n '1,200p' || trueRepository: internxt/drive-web
Length of output: 15575
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '---AUTH INIT SECTION---'
sed -n '560,620p' src/services/auth.service.ts
echo '---APP INIT SECTION---'
sed -n '120,150p' src/App.tsx
echo '---NAVIGATION SERVICE SECTION---'
sed -n '60,95p' src/services/navigation.service.tsRepository: internxt/drive-web
Length of output: 4363
Delay local-workspace restoration until fetchWorkspaces() has populated state
checkAndSetLocalWorkspace() is dispatched immediately after fetchWorkspaces() in the login flow, so this branch can run while state.workspaces.selectedWorkspace and state.workspaces.workspaces are still empty. It then dispatches null, and the saved B2B workspace is lost. Await the fetch before restoring, or re-run this from the fetch completion path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/store/slices/workspaces/workspacesStore.ts` around lines 74 - 76,
Delay the local workspace restore in checkAndSetLocalWorkspace until
fetchWorkspaces has finished populating state, since the current B2BworkspaceId
branch can run before state.workspaces.selectedWorkspace and
state.workspaces.workspaces are ready and dispatch null. Update the login flow
around fetchWorkspaces/checkAndSetLocalWorkspace so restoration happens after
the fetch completes, or trigger the restore from the fetch completion path using
the existing workspacesStore logic and the LocalStorageItem.B2BworkspaceId
check.
|
| dispatch(workspacesActions.setSelectedWorkspace(null)); | ||
| dispatch(workspacesActions.setCredentials(null)); | ||
| localStorageService.set(LocalStorageItem.WorkspaceCredentials, 'null'); | ||
| } else if (isSelectedWorkspace) { |
There was a problem hiding this comment.
I think that here mabye we should check if the workspace exists isSelectedWorkspace && isSelectedWorkspace && selectedWorkspace?.workspace.id === workspaceId to avoid unexpected errores, because before the workspace was already stored in local storage and now comes from redux and http calls.
Add an await or move these lines into a thunk that handles the situation to wait the workspaces fetch here
await dispatch(workspaceThunks.fetchWorkspaces());
dispatch(workspaceThunks.checkAndSetLocalWorkspace());
2 participants
Description
This PR minimizes the data we store. We used to store the entire workspace (type WorkspaceData), but mostly used it for ID and mnemonic. Unfortunately, WorkspaceData has phoneNumber, email, and address - all sensitive fields we never use.
It got highlighted by Git Security once before, but it got fooled by the switch from setItem to set.
Related Issues
Relates to PB-6505
Related Pull Requests
Checklist
Testing Process
Additional Notes
The only line I'm not fully sure about is line 199 of src/app/store/slices/workspaces/workspacesStore.ts:
dispatch(workspacesActions.setSelectedWorkspace(selectedWorkspace));Summary by CodeRabbit