Harden cleanup handling and packaged renderer loading#5
Conversation
|
@codex review |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRenderer loading is now conditional on packaging state; cleanup operations are validated against the latest scan snapshot before removal; path containment checks were tightened with normalization and Windows case-insensitivity; a CSP meta tag was added; tests updated/added for these behaviors and visual tolerance. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Client/Test
participant Service as CleanupService
participant ScanStore as LatestScanFindings
participant FS as FileSystem
participant Settings as SettingsStore
Test->>Service: call apply(requestedFindings)
Service->>ScanStore: resolveAuthorizedFinding(requestedFinding.id)
ScanStore-->>Service: scannedFinding (or null)
alt scannedFinding matches requestedFinding
Service->>FS: removeArtifact(authorizedFinding)
FS-->>Service: removal success
Service->>Settings: recordCleanup({ packageName, artifactCount })
Settings-->>Service: ack
Service-->>Test: return removed=[authorizedFinding], failed=[]
else no match / missing
Service-->>Test: return removed=[], failed=[{ id, reason: "latest scan results" }]
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the application by addressing two critical areas: file cleanup authorization and packaged Electron renderer loading. It resolves a security vulnerability in the cleanup service that could allow tampering with file removal targets and hardens the renderer process in packaged builds by enforcing local content loading and implementing a Content Security Policy. These changes aim to prevent potential exploits and improve the overall integrity of the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/visual.packaged.spec.ts (1)
73-76: Consider extracting the repeated empty-state assertions into a helper.Lines 73-76, Line 97-100, and Line 120-123 duplicate the same checks; a helper would keep these assertions consistent across viewport tests.
♻️ Optional refactor
+async function expectSleepingEmptyState(page: Page) { + await expect(page.getByText('WSA is currently sleeping')).toBeVisible() + await expect(page.getByRole('button', { name: 'Wake WSA' })).toBeVisible() + await expect(page.getByRole('button', { name: 'Open Wizard' })).toBeVisible() + await expect(page.getByTestId('queue-empty-state')).toBeVisible() +} await app.page.getByTestId('setup-close-button').click() - await expect(app.page.getByText('WSA is currently sleeping')).toBeVisible() - await expect(app.page.getByRole('button', { name: 'Wake WSA' })).toBeVisible() - await expect(app.page.getByRole('button', { name: 'Open Wizard' })).toBeVisible() - await expect(app.page.getByTestId('queue-empty-state')).toBeVisible() + await expectSleepingEmptyState(app.page) await closeSetupWizardIfVisible(app.page) - await expect(app.page.getByText('WSA is currently sleeping')).toBeVisible() - await expect(app.page.getByRole('button', { name: 'Wake WSA' })).toBeVisible() - await expect(app.page.getByRole('button', { name: 'Open Wizard' })).toBeVisible() - await expect(app.page.getByTestId('queue-empty-state')).toBeVisible() + await expectSleepingEmptyState(app.page) await closeSetupWizardIfVisible(app.page) - await expect(app.page.getByText('WSA is currently sleeping')).toBeVisible() - await expect(app.page.getByRole('button', { name: 'Wake WSA' })).toBeVisible() - await expect(app.page.getByRole('button', { name: 'Open Wizard' })).toBeVisible() - await expect(app.page.getByTestId('queue-empty-state')).toBeVisible() + await expectSleepingEmptyState(app.page)Also applies to: 97-100, 120-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/visual.packaged.spec.ts` around lines 73 - 76, Extract the four repeated empty-state assertions into a reusable helper (e.g., expectEmptyStateVisible or assertEmptyStateVisible) and replace the duplicate blocks in tests/e2e/visual.packaged.spec.ts (the occurrences around the three viewport sections) with a single call to that helper; the helper should accept the Playwright Page or app object and run the four checks: getByText('WSA is currently sleeping'), getByRole('button', { name: 'Wake WSA' }), getByRole('button', { name: 'Open Wizard' }), and getByTestId('queue-empty-state') so all assertions remain consistent.tests/main/cleanup-service.test.ts (1)
84-107: Consider adding edge case tests.The helper functions are well-designed. Consider adding tests for:
apply()called without priorscan()(emptylatestScanFindings)- Finding with matching
targetbut differentidThese would strengthen confidence in the validation logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/cleanup-service.test.ts` around lines 84 - 107, Add two edge-case tests: one that constructs the service via createService and calls CleanupService.apply() without calling scan() first (so latestScanFindings is empty) and asserts the expected no-op or error behavior; and another that uses createDesktopFinding to create two findings with the same target but different id values and verifies the validation/matching logic (i.e., that matching is done by id not target) when calling scan()/apply() on the service. Use the existing helpers (createService, createDesktopFinding) and reference CleanupService.apply and CleanupService.scan in your test names/assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/visual.packaged.spec.ts`:
- Around line 73-76: Extract the four repeated empty-state assertions into a
reusable helper (e.g., expectEmptyStateVisible or assertEmptyStateVisible) and
replace the duplicate blocks in tests/e2e/visual.packaged.spec.ts (the
occurrences around the three viewport sections) with a single call to that
helper; the helper should accept the Playwright Page or app object and run the
four checks: getByText('WSA is currently sleeping'), getByRole('button', { name:
'Wake WSA' }), getByRole('button', { name: 'Open Wizard' }), and
getByTestId('queue-empty-state') so all assertions remain consistent.
In `@tests/main/cleanup-service.test.ts`:
- Around line 84-107: Add two edge-case tests: one that constructs the service
via createService and calls CleanupService.apply() without calling scan() first
(so latestScanFindings is empty) and asserts the expected no-op or error
behavior; and another that uses createDesktopFinding to create two findings with
the same target but different id values and verifies the validation/matching
logic (i.e., that matching is done by id not target) when calling scan()/apply()
on the service. Use the existing helpers (createService, createDesktopFinding)
and reference CleanupService.apply and CleanupService.scan in your test
names/assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7b6da3c-3a60-47a2-9056-0559e04aae69
📒 Files selected for processing (7)
src/main/index.tssrc/main/services/cleanup/CleanupService.tssrc/main/services/cleanup/helpers.tssrc/renderer/index.htmltests/e2e/visual.packaged.spec.tstests/main/cleanup-helpers.test.tstests/main/cleanup-service.test.ts
chrisjcthomas
force-pushed
the
codex/security-scan-fixes
branch
from
ecc7749 to
f314a19
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| private rememberScannedFindings(findings: CleanupFinding[]): void { | ||
| this.latestScanFindings = new Map(findings.map((finding) => [finding.id, finding])) | ||
| } |
There was a problem hiding this comment.
🔴 Scoped scan in uninstallApp replaces entire authorization cache, silently invalidating unrelated cleanup findings
rememberScannedFindings at src/main/services/cleanup/CleanupService.ts:289-291 replaces the entire latestScanFindings map on every scan() call. When uninstallApp triggers a package-scoped scan(packageName) (src/main/ipc/registerIpc.ts:48), only that single package's findings are generated (due to the filter at CleanupService.ts:62-63), and then they replace the full map. This silently destroys the authorization snapshot for all other packages' findings from a prior full scan.
User-facing impact: A user performs a full cleanup scan, selects findings across multiple packages, then uninstalls an app from the Installed Apps page. The uninstallApp handler's implicit scoped scan wipes the cache. When the user returns to apply their selected cleanup findings, every finding for other packages fails with "Cleanup item no longer matches the latest scan results." Similarly, if ADB becomes unavailable during the scoped scan, rememberScannedFindings([]) at line 47 clears the entire cache.
Prompt for agents
In src/main/services/cleanup/CleanupService.ts, the rememberScannedFindings method (line 289-291) replaces the entire latestScanFindings map every time scan() is called. When scan(packageName) is called with a specific package (as happens in the uninstallApp IPC handler at src/main/ipc/registerIpc.ts:48), this destroys all previously cached findings for other packages.
Fix: When scan() is called with a packageName argument (a scoped scan), rememberScannedFindings should merge the new findings into the existing map rather than replacing it. One approach:
1. Add a parameter to rememberScannedFindings indicating whether this is a scoped scan.
2. For scoped scans: first remove all existing entries for the scanned packageName, then add the new findings.
3. For full scans (no packageName): replace the entire map as currently done.
4. Also fix the early-return on ADB failure (line 47): when a scoped scan fails, only clear that package's entries rather than the whole map.
Alternatively, you could have the uninstallApp handler call a different method that doesn't mutate the authorization cache.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
ELECTRON_RENDERER_URL, and the renderer shell had no CSP.ELECTRON_RENDERER_URLand load the local renderer shell.Validation
npm run validatenpm run smoke:packagednpm run test:visualnpm run devrelease/win-unpacked/WSA Manager.exerelease/WSA.Manager.Setup.<version>.exeCodex review
@codex reviewrequested on this PR, or automatic reviews are enabledUI evidence
tests/e2e/__screenshots__/visual.packaged.spec.ts/1400x920,1060x740,920x640Notes
sandbox: truewas evaluated but breaks the packaged preload bridge in the current app architecture, so this PR keepssandbox: falseand focuses on the confirmed exploit path plus packaged renderer hardening.unsafe-evalbecause the stricter variant broke packaged runtime; it still blocks remote scripts and tightens object/base/form usage.Summary by CodeRabbit
Bug Fixes
Security
Startup
Tests