chore(security): enforce secret gates and add E2E secret-filter QA#39
Conversation
Reviewer's GuideAdds configurable secret-scanning and suspicious-file exclusion across file filtering and analysis, wires gitleaks staged scanning into local tooling and hooks, and extends automated tests and E2E screenshot QA to cover the new secret-filtering behavior and config toggles. Sequence diagram for secret filtering from config toggles to file treesequenceDiagram
actor User
participant ConfigTab
participant LocalStorage
participant SourceTab
participant RendererFileService as RendererFileService
participant ElectronAPI as ElectronAPI_getDirectoryTree
participant SecretScanner
User->>ConfigTab: Open Config tab
ConfigTab->>LocalStorage: Load existing configContent
User->>ConfigTab: Uncheck enable_secret_scanning and exclude_suspicious_files
User->>ConfigTab: Click Save config
ConfigTab->>ConfigTab: saveConfig()
ConfigTab->>LocalStorage: setItem(configContent)
User->>SourceTab: Switch to Source tab
User->>SourceTab: Click Refresh file list
SourceTab->>RendererFileService: refreshFiles()
RendererFileService->>LocalStorage: getItem(configContent)
RendererFileService->>ElectronAPI: getDirectoryTree(rootPath, configContent)
ElectronAPI->>ElectronAPI: determine excludeSensitiveFiles flag
ElectronAPI->>SecretScanner: isSensitiveFilePath(itemPath) for each file
alt excludeSensitiveFiles is false
SecretScanner-->>ElectronAPI: false for secret files
ElectronAPI-->>RendererFileService: directory tree including secret files
else excludeSensitiveFiles is true
SecretScanner-->>ElectronAPI: true for secret files
ElectronAPI-->>RendererFileService: directory tree without secret files
end
RendererFileService-->>SourceTab: Rendered file tree
SourceTab-->>User: Secret files visible or hidden depending on config
Class diagram for secret scanning utilities and config integrationclassDiagram
class ConfigObject {
+boolean~optional~ use_custom_excludes
+boolean~optional~ use_custom_includes
+boolean~optional~ use_gitignore
+boolean~optional~ enable_secret_scanning
+boolean~optional~ exclude_suspicious_files
+boolean~optional~ include_tree_view
+boolean~optional~ show_token_count
}
class SecretMatch {
+string id
+string description
}
class SecretScanResult {
+boolean isSuspicious
+SecretMatch[] matches
}
class SecretScanner {
+shouldExcludeSuspiciousFiles(config: ConfigObject) boolean
+isSensitiveFilePath(filePath: string) boolean
+scanContentForSecrets(content: string) SecretScanResult
+scanFileForSecrets(filePath: string) SecretScanResult
}
class FileAnalyzer {
-ConfigObject config
+analyzeFile(filePath: string) number
}
class FilterUtils {
+shouldExclude(itemPath: string, isDirectory: boolean, config: ConfigObject, includePatterns: string[], excludePatterns: string[]) boolean
}
class ConfigTab {
-boolean useCustomExcludes
-boolean useCustomIncludes
-boolean useGitignore
-boolean enableSecretScanning
-boolean excludeSuspiciousFiles
-boolean includeTreeView
-boolean showTokenCount
-string fileExtensions
+saveConfig() void
+onConfigChange(configContent: string) void
}
SecretScanner <.. ConfigObject : uses
FileAnalyzer --> ConfigObject : has
FileAnalyzer ..> SecretScanner : uses
FilterUtils ..> SecretScanner : uses
FilterUtils --> ConfigObject : uses
ConfigTab --> ConfigObject : builds
ConfigTab ..> SecretScanner : controls via flags
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughAdds a secret-scanning subsystem (rules, file/content scanners, path heuristics), config flags and UI toggles, integrates scanning into file analysis and filtering, adds staged gitleaks pre-commit invocation and CLI wiring, updates QA mocks, and introduces unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as "ConfigTab (UI)"
participant Analyzer as "FileAnalyzer"
participant Scanner as "SecretScanner"
participant Filter as "FilterUtils"
participant FS as "Filesystem"
participant CLI as "gitleaks (scripts/lib/security)"
User->>UI: toggle enable_secret_scanning / exclude_suspicious_files
UI->>UI: persist config (enable_secret_scanning, exclude_suspicious_files)
Note over Analyzer,Scanner: File analysis flow
Analyzer->>FS: read file content
FS-->>Analyzer: content
Analyzer->>Scanner: scanContentForSecrets(content, config)
Scanner-->>Analyzer: SecretScanResult {isSuspicious, matches}
alt isSuspicious and exclude_suspicious_files enabled
Analyzer->>Filter: shouldExclude(itemPath, config)
Filter-->>Analyzer: true
Analyzer-->>Analyzer: return null (skip further analysis)
else not suspicious or exclusion disabled
Analyzer-->>Analyzer: continue tokenization/analysis
Analyzer-->>User: analysis result
end
Note over CLI,Scanner: pre-commit staged check
CLI->>Scanner: runGitleaksStaged() (protect --staged --redact --no-banner --verbose --exit-code 1)
Scanner-->>CLI: exit code / report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Review Summary by QodoAdd secret scanning and suspicious file filtering with E2E QA verification
WalkthroughsDescription• Add secret scanning and suspicious file filtering with configurable toggles • Implement pattern-based secret detection (GitHub tokens, AWS keys, private keys, etc.) • Integrate secret filtering into file analysis and directory tree filtering • Add E2E tests verifying secret files hidden by default and visible when disabled • Enforce staged secret scanning in pre-commit hooks (gitleaks) on all platforms Diagramflowchart LR
A["Config UI"] -->|enable_secret_scanning| B["Secret Scanner"]
A -->|exclude_suspicious_files| B
B -->|scanContentForSecrets| C["Pattern Detection"]
B -->|isSensitiveFilePath| D["Path Analysis"]
C -->|matches found| E["Skip File"]
D -->|sensitive detected| E
E -->|filtered tree| F["File Analyzer"]
G["Pre-commit Hook"] -->|gitleaks:staged| H["Staged Files Check"]
H -->|blocks secrets| I["Commit Prevention"]
File Changes1. src/types/ipc.ts
|
Summary of ChangesHello @Mehdi-Bl, 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 bolsters the application's security posture by integrating robust secret detection directly into the development workflow via pre-commit hooks and enhancing the user interface with configurable secret filtering options. It also introduces new end-to-end tests to ensure these security features function as expected, preventing accidental exposure of sensitive information and improving overall code integrity. 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. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The sensitive-file detection logic is duplicated (and slightly divergent) between
scripts/capture-ui-screenshot.js(bothisSensitiveMockPath/cloneAndFilterMockTreeand the inlinecloneAndFilteringetDirectoryTree) andsrc/utils/secret-scanner.ts; consider reusing the shared helpers fromsecret-scanneror at least a single helper in the script to avoid future drift. - The decision logic for when to exclude suspicious files now lives in multiple places (
shouldExcludeSuspiciousFiles+isSensitiveFilePathinfilter-utilsand content scanning infile-analyzer); it may be worth centralizing the policy (path vs content, and how the two toggles interact) intosecret-scannerto keep behavior consistent as rules evolve.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The sensitive-file detection logic is duplicated (and slightly divergent) between `scripts/capture-ui-screenshot.js` (both `isSensitiveMockPath`/`cloneAndFilterMockTree` and the inline `cloneAndFilter` in `getDirectoryTree`) and `src/utils/secret-scanner.ts`; consider reusing the shared helpers from `secret-scanner` or at least a single helper in the script to avoid future drift.
- The decision logic for when to exclude suspicious files now lives in multiple places (`shouldExcludeSuspiciousFiles` + `isSensitiveFilePath` in `filter-utils` and content scanning in `file-analyzer`); it may be worth centralizing the policy (path vs content, and how the two toggles interact) into `secret-scanner` to keep behavior consistent as rules evolve.
## Individual Comments
### Comment 1
<location> `scripts/capture-ui-screenshot.js:346-355` </location>
<code_context>
+ getDirectoryTree: async (_dirPath, configContent) => {
</code_context>
<issue_to_address>
**suggestion:** Secret filtering logic is duplicated and slightly reimplemented inside `getDirectoryTree`.
By reintroducing `cloneAndFilter` here with its own “is this sensitive?” checks, this logic can easily drift from `isSensitiveMockPath` / `cloneAndFilterMockTree` and from what the renderer uses. Please refactor to use a single shared filtering implementation (or delegate to the renderer’s logic) so the mock behavior stays consistent and easier to maintain.
Suggested implementation:
```javascript
window.electronAPI = {
getDefaultConfig: async () => mockConfig,
selectDirectory: async () => mockRootPath,
getDirectoryTree: async (_dirPath, configContent) => {
const activeConfig =
typeof configContent === 'string' && configContent.trim()
? configContent
: localStorage.getItem('configContent') || '';
const excludeSensitiveFiles =
!/(^|\n)\s*enable_secret_scanning\s*:\s*false\b/i.test(activeConfig) &&
!/(^|\n)\s*exclude_suspicious_files\s*:\s*false\b/i.test(activeConfig);
// Delegate to the shared mock tree filtering logic so behavior matches the renderer.
return cloneAndFilterMockTree({
excludeSensitiveFiles,
configContent: activeConfig,
});
```
I’m assuming there is an existing shared helper like `cloneAndFilterMockTree` (and likely `isSensitiveMockPath`) used elsewhere in this file or in related test helpers. If the name or signature differs, adjust the call accordingly. If that helper does not yet exist in this file, you’ll need to:
1. Extract the common “is this sensitive?” logic from wherever the renderer does its filtering into a reusable function (e.g. `isSensitiveMockPath`).
2. Implement `cloneAndFilterMockTree` (or equivalent) to traverse the mock directory tree and apply `isSensitiveMockPath` based on `excludeSensitiveFiles`, and then import or define it in `scripts/capture-ui-screenshot.js`.
3. Ensure other mock usages (if any) also use this shared helper instead of their own inline filtering logic so behavior stays consistent.
</issue_to_address>
### Comment 2
<location> `tests/unit/file-analyzer.test.ts:409-416` </location>
<code_context>
expect(result).toBeNull();
});
+
+ test('should skip suspicious files when secret scanning is enabled', () => {
+ isBinaryFile.mockReturnValue(false);
+ fs.readFileSync.mockReturnValue(`const token = "${FAKE_GITHUB_TOKEN}";`);
+
+ const result = fileAnalyzer.analyzeFile('src/secrets.ts');
+
+ expect(result).toBeNull();
+ expect(mockTokenCounter.countTokens).not.toHaveBeenCalled();
+ });
});
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for FileAnalyzer behavior when secret scanning is disabled
Please also add tests for the inverse cases: construct `FileAnalyzer` with (a) `enable_secret_scanning: false` and (b) `exclude_suspicious_files: false`, then assert that a file with the same token content is analyzed (i.e., `countTokens` is called and a non-null result is returned). This will complete coverage of the new configuration combinations.
Suggested implementation:
```typescript
test('should skip suspicious files when secret scanning is enabled', () => {
isBinaryFile.mockReturnValue(false);
fs.readFileSync.mockReturnValue(`const token = "${FAKE_GITHUB_TOKEN}";`);
const result = fileAnalyzer.analyzeFile('src/secrets.ts');
expect(result).toBeNull();
expect(mockTokenCounter.countTokens).not.toHaveBeenCalled();
});
test('should analyze suspicious files when secret scanning is disabled', () => {
isBinaryFile.mockReturnValue(false);
fs.readFileSync.mockReturnValue(`const token = "${FAKE_GITHUB_TOKEN}";`);
const analyzer = new FileAnalyzer(mockTokenCounter, {
enable_secret_scanning: false,
exclude_suspicious_files: true,
});
const result = analyzer.analyzeFile('src/secrets.ts');
expect(result).not.toBeNull();
expect(mockTokenCounter.countTokens).toHaveBeenCalled();
});
test('should analyze suspicious files when excluding suspicious files is disabled', () => {
isBinaryFile.mockReturnValue(false);
fs.readFileSync.mockReturnValue(`const token = "${FAKE_GITHUB_TOKEN}";`);
const analyzer = new FileAnalyzer(mockTokenCounter, {
enable_secret_scanning: true,
exclude_suspicious_files: false,
});
const result = analyzer.analyzeFile('src/secrets.ts');
expect(result).not.toBeNull();
expect(mockTokenCounter.countTokens).toHaveBeenCalled();
});
```
1. Ensure that `mockTokenCounter` is defined in the test file scope (it likely already is, since it is used in the existing tests). If not, define it as a Jest mock and pass it into the default `fileAnalyzer` instance.
2. Confirm that the `FileAnalyzer` constructor signature matches `new FileAnalyzer(tokenCounter, options)`. If your implementation differs, adjust the `new FileAnalyzer(...)` calls accordingly.
3. If the configuration option names differ from `enable_secret_scanning` and `exclude_suspicious_files` in the implementation, update the option keys in the two new tests to match the actual configuration interface.
</issue_to_address>
### Comment 3
<location> `tests/unit/components/config-tab.test.tsx:74-75` </location>
<code_context>
expect(screen.getByLabelText('Filter by file extensions')).toBeChecked();
expect(screen.getByLabelText('Use exclude patterns')).toBeChecked();
expect(screen.getByLabelText('Apply .gitignore rules')).toBeChecked();
+ expect(screen.getByLabelText('Scan content for secrets')).toBeChecked();
+ expect(screen.getByLabelText('Exclude suspicious files')).toBeChecked();
// Check textareas
</code_context>
<issue_to_address>
**suggestion (testing):** Test that ConfigTab correctly persists secret-scanning toggles to the saved config
These assertions only cover the default UI state. Please add a test that toggles one or both checkboxes, triggers the save flow, and verifies the resulting config (via `onConfigChange` or YAML) reflects `enable_secret_scanning: false` / `exclude_suspicious_files: false` so we know the toggles actually persist into the saved config.
Suggested implementation:
```typescript
describe('ConfigTab', () => {
it('persists secret scanning toggles into saved config', async () => {
const onConfigChange = vi.fn(); // Use jest.fn() instead if the rest of the file uses Jest
// Use the same render helper / props pattern as other tests in this file.
// Example if existing tests do something like:
// const { user } = renderConfigTab({ onConfigChange, initialConfig: getDefaultConfig() });
const { user } = render(
<ConfigTab
// Spread whatever default/required props the other tests provide
onConfigChange={onConfigChange}
/>,
);
// Secret scanning checkboxes should be checked by default
const scanSecretsCheckbox = screen.getByLabelText('Scan content for secrets');
const excludeSuspiciousCheckbox = screen.getByLabelText('Exclude suspicious files');
expect(scanSecretsCheckbox).toBeChecked();
expect(excludeSuspiciousCheckbox).toBeChecked();
// Toggle them off
await user.click(scanSecretsCheckbox);
await user.click(excludeSuspiciousCheckbox);
expect(scanSecretsCheckbox).not.toBeChecked();
expect(excludeSuspiciousCheckbox).not.toBeChecked();
// Trigger the same "save" flow used in other tests
// (e.g. clicking a "Save" button or equivalent).
const saveButton = screen.getByRole('button', { name: /save/i });
await user.click(saveButton);
// Assert that the saved config persists the toggled values.
// If ConfigTab calls onConfigChange with a full config object:
expect(onConfigChange).toHaveBeenCalled();
const lastConfigArg = onConfigChange.mock.calls.at(-1)[0];
expect(lastConfigArg).toMatchObject({
enable_secret_scanning: false,
exclude_suspicious_files: false,
});
// If instead the config is stored as YAML in a textarea, replace the above
// with something like:
//
// const yamlTextarea = screen.getByLabelText(/configuration yaml/i);
// expect(yamlTextarea).toHaveValue(
// expect.stringContaining('enable_secret_scanning: false'),
// );
// expect(yamlTextarea).toHaveValue(
// expect.stringContaining('exclude_suspicious_files: false'),
// );
});
```
To wire this up cleanly you’ll need to:
1. **Align the render call with existing tests**
- If the file already has a `renderConfigTab` or similar helper, replace the direct `render(<ConfigTab ... />)` call with that helper and pass `onConfigChange` through in the same way other tests do (including any `initialConfig` / `defaultConfig` props).
2. **Match the test framework**
- If the rest of the file uses Jest, replace `vi.fn()` with `jest.fn()` and `onConfigChange.mock.calls` access with the appropriate Jest mock typing if needed.
3. **Reuse the same "save" interaction used elsewhere**
- If other tests trigger persistence through a different element (e.g. a “Generate config” button, tab switch, or blur event), replace the `saveButton` click with that same interaction so you’re exercising the real save path.
4. **Use the same assertion style for config persistence**
- If other tests already assert against emitted config via `onConfigChange`, reuse that pattern (e.g. maybe they inspect the first call instead of the last).
- If there is an existing YAML textarea that represents the saved config and other tests assert on its value, you can drop the `onConfigChange` assertions and instead assert that the YAML includes `enable_secret_scanning: false` and `exclude_suspicious_files: false`, using the same label/placeholder and matchers as the other tests.
</issue_to_address>
### Comment 4
<location> `tests/unit/utils/secret-scanner.test.ts:47-71` </location>
<code_context>
+ });
+ });
+
+ describe('scanContentForSecrets', () => {
+ test('should detect known secret patterns', () => {
+ const content = `
+ const token = "${FAKE_GITHUB_TOKEN}";
+ AWS_SECRET_ACCESS_KEY="${FAKE_AWS_SECRET_ACCESS_KEY}"
+ `;
+
+ const result = scanContentForSecrets(content);
+ expect(result.isSuspicious).toBe(true);
+ expect(result.matches.length).toBeGreaterThan(0);
+ });
+
+ test('should return clean result for normal content', () => {
+ const content = `
+ export const sum = (a, b) => a + b;
+ console.log("hello");
+ `;
+
+ const result = scanContentForSecrets(content);
+ expect(result).toEqual({
+ isSuspicious: false,
+ matches: [],
</code_context>
<issue_to_address>
**suggestion (testing):** Add targeted tests to cover all secret detection rules
The current tests only cover GitHub and AWS patterns. Please add small, rule-specific tests for the other patterns (Slack tokens, Stripe secret keys, JWT-like tokens, and the generic credential-assignment regex) so each rule is explicitly validated and future regex changes are caught by targeted failures.
```suggestion
describe('scanContentForSecrets', () => {
test('should detect known secret patterns', () => {
const content = `
const token = "${FAKE_GITHUB_TOKEN}";
AWS_SECRET_ACCESS_KEY="${FAKE_AWS_SECRET_ACCESS_KEY}"
`;
const result = scanContentForSecrets(content);
expect(result.isSuspicious).toBe(true);
expect(result.matches.length).toBeGreaterThan(0);
});
test('should detect Slack tokens', () => {
const content = `
const slackToken = "xoxb-123456789012-1234567890123-abcdefghijklmnopqrstuvwx";
`;
const result = scanContentForSecrets(content);
expect(result.isSuspicious).toBe(true);
expect(result.matches.length).toBeGreaterThan(0);
});
test('should detect Stripe secret keys', () => {
const content = `
const stripeSecretKey = "sk_live_51H8aBcDeFgHiJkLmNoPqRsTuVwXyZaBcDeFgHiJkLmNoPqRs";
`;
const result = scanContentForSecrets(content);
expect(result.isSuspicious).toBe(true);
expect(result.matches.length).toBeGreaterThan(0);
});
test('should detect JWT-like tokens', () => {
const content = `
const jwt = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c";
`;
const result = scanContentForSecrets(content);
expect(result.isSuspicious).toBe(true);
expect(result.matches.length).toBeGreaterThan(0);
});
test('should detect generic credential assignments', () => {
const content = `
const password = "super-secret-password";
api_key = "abc123-very-secret-key";
`;
const result = scanContentForSecrets(content);
expect(result.isSuspicious).toBe(true);
expect(result.matches.length).toBeGreaterThan(0);
});
test('should return clean result for normal content', () => {
const content = `
export const sum = (a, b) => a + b;
console.log("hello");
`;
const result = scanContentForSecrets(content);
expect(result).toEqual({
isSuspicious: false,
matches: [],
});
});
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be6b456e9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| const SENSITIVE_PATH_SEGMENTS = [ | ||
| '/.aws/credentials', | ||
| '/.npmrc', | ||
| '/.pypirc', | ||
| '/.docker/config.json', |
There was a problem hiding this comment.
The new sensitive-path filter hard-codes segments with a leading /, but FileAnalyzer.shouldProcessFile calls shouldExclude with a relative path (see getRelativePath usage in src/main/index.ts). For repo-root files like .npmrc or .aws/credentials, the relative path is .npmrc/.aws/credentials and will never include /.npmrc or /.aws/credentials, so isSensitiveFilePath returns false and those files will be processed even with secret filtering enabled (and content scanning may miss unquoted tokens). This means secret filtering is ineffective for some sensitive dotfiles in the analysis path.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively strengthens the project's security posture by integrating gitleaks for staged secret scanning in pre-commit hooks and by adding secret filtering capabilities to the UI. The changes are well-implemented across the board, from the shell scripts to the React components and backend utilities. The addition of E2E tests for the new secret filtering behavior is a great way to ensure its correctness.
My main feedback is regarding some duplicated logic in the E2E test script (scripts/capture-ui-screenshot.js), where secret detection rules are reimplemented instead of being reused from the production code. Addressing this will improve maintainability and prevent the tests from becoming out of sync with the application's behavior.
Code Review by Qodo
1. Windows gitleaks path blocked
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/secret-scanner.ts`:
- Around line 117-127: The current scanFileForSecrets function returns {
isSuspicious: false } on fs.readFileSync errors which allows files to bypass
scanning; change the error fallback to a fail-closed behavior by returning {
isSuspicious: true, matches: [], error: <error message> } or rethrowing the
error so callers must handle it; update scanFileForSecrets to include the caught
error details (or an error property) and/or make the behavior configurable
(e.g., a parameter or environment flag) while keeping the normal successful path
calling scanContentForSecrets(content).
🧹 Nitpick comments (9)
scripts/lib/security.js (1)
178-202: Consider extracting shared gitleaks resolution logic.
runGitleaksStagedduplicates the gitleaks resolution, validation, and spawn pattern fromrunGitleaks(lines 146–176). A small helper likeresolveGitleaksOrThrow()returning{ gitleaksPath, commandName, env }would eliminate this duplication across both functions.♻️ Sketch of shared helper
+function resolveGitleaksOrThrow() { + const gitleaksPath = resolveCommand('gitleaks', [ + path.join('bin', 'gitleaks'), + path.join('bin', 'gitleaks.exe'), + ]); + if (!gitleaksPath) { + throw new Error('gitleaks not found in PATH or ./bin (install gitleaks first)'); + } + assertAllowedExecutable(gitleaksPath); + const commandName = process.platform === 'win32' ? 'gitleaks.exe' : 'gitleaks'; + const env = withExecutablePath({ ...process.env }, gitleaksPath); + return { commandName, env }; +}scripts/capture-ui-screenshot.js (1)
346-383: Duplicated sensitive-file detection logic insideaddInitScriptcallback.The inline
isSensitivecheck (lines 360–366) duplicatesisSensitiveMockPath(lines 268–281). This is unavoidable due to Playwright'saddInitScriptserialization boundary — functions from the outer scope can't be referenced inside the callback. Consider adding a brief comment explaining why, so future maintainers don't attempt to refactor one without updating the other.📝 Suggested comment
+ // NOTE: This duplicates isSensitiveMockPath() because addInitScript + // serialises the callback — outer functions are not available here. const cloneAndFilter = (items) => {.pre-commit-config.yaml (1)
34-37: Redundant gitleaks execution between Husky and pre-commit framework.Both the Husky hook (invoking
npm run gitleaks:staged) and the official gitleaks pre-commit hook (added here) scan staged changes usingprotect --staged. If both hook systems are active, gitleaks will scan staged files twice per commit.Consider whether both systems are needed for gitleaks, or if one can serve as the sole enforcement gate.
src/utils/secret-scanner.ts (2)
66-80: Consider adding.env.local,.env.*.local, and common keyring/vault paths.The sensitive file name patterns cover
.envvariants well, but you might also want to match paths like.ssh/directory contents,*.tfvars(Terraform variables often contain secrets), andhtpasswdfiles.
82-82:normalizeFilePathlowercases the entire path — may over-match on case-sensitive filesystems.On Linux (case-sensitive FS),
/Project/.Envand/project/.envare distinct paths. Lowercasing makes matching more aggressive, which is acceptable for a security filter (better to over-exclude than under-exclude), but worth noting in a comment for future maintainers.tests/unit/components/config-tab.test.tsx (1)
20-21: New config options are mocked and asserted — good coverage of the render path.Consider adding a toggle test for the new checkboxes (similar to the existing
'calls onConfigChange when checkbox changes'test for exclude patterns) to verify the debounced callback fires for secret-scanning toggles too. This can be deferred.Also applies to: 74-75
tests/integration/main-process/handlers.test.ts (1)
330-354: Test relies on implicit default config for secret scanning — consider making it explicit.The test works because
shouldExcludeSuspiciousFilesreturnstruewhen config keys areundefined. However, theyaml.parsemock inbeforeEach(line 158) returns a config withoutenable_secret_scanning/exclude_suspicious_files, so the test silently depends on this default. Adding the keys explicitly to the config would make the test's intent clearer and more resilient to future default changes.Suggested improvement
const configContent = ` use_custom_excludes: true use_gitignore: true include_extensions: - .js exclude_patterns: - "**/node_modules/**" + enable_secret_scanning: true + exclude_suspicious_files: true `;tests/unit/utils/secret-scanner.test.ts (1)
47-57: The AWS secret key pattern isn't tested in isolation.The
scanContentForSecretstest at lines 49-52 includes bothFAKE_GITHUB_TOKENandFAKE_AWS_SECRET_ACCESS_KEYin the same content string. If the AWS pattern doesn't actually match'A'.repeat(40), the test still passes because the GitHub token triggersisSuspicious: true. Consider adding a separate test case with only the AWS key to ensure that pattern also matches independently.Suggested addition
+ test('should detect AWS secret key pattern independently', () => { + const content = `AWS_SECRET_ACCESS_KEY="${FAKE_AWS_SECRET_ACCESS_KEY}"`; + const result = scanContentForSecrets(content); + expect(result.isSuspicious).toBe(true); + expect(result.matches.length).toBeGreaterThan(0); + });src/renderer/components/ConfigTab.tsx (1)
77-83: The two flags are coupled inshouldExcludeSuspiciousFiles— consider a UI hint.
shouldExcludeSuspiciousFilesreturnstrueonly when bothenable_secret_scanningandexclude_suspicious_filesare notfalse. Unchecking "Scan content for secrets" also disables path-based file exclusion (.env, credentials, etc.), which users might not expect. A small helper text or disabling the "Exclude suspicious files" checkbox when scanning is off would clarify this dependency.
1 participant
Summary
gitleaks:stagedcommand wiring (gitleaks protect --staged).pre-commit-config.yamlcoverage (ts/tsx+ gitleaks hook)qa:screenshotfor secret filtering behavior:tests/catalog.mdwith the new screenshot-gate use caseValidation
npm run lintnpm test -- --runInBandnpm run qa:screenshotmake sonar(local scan succeeded)Notes
Gitleaks Secret Scanon default branchSummary by CodeRabbit
New Features
Bug Fixes
Tests