fix(plugins): Skip scan now actually skips — no more silent gate-blocking#58
Merged
Merged
Conversation
…king Davidson found the install wizard's "Skip scan" checkbox didn't do anything useful: marking it sent verdict=null upward, scanGatePassed evaluated to false, and "Confirmar instalação" stayed disabled / silently dropped the request. The skipReason textarea was tagged "(required)" but had no minimum, no validation feedback, and was never sent to the backend. What changed - Added 'SKIPPED' to ScanVerdict union and emit it from handleSkipToggle when the checkbox is checked. - scanGatePassed now treats SKIPPED as a pass (same as APPROVE) — admin already made the call; backend audit-logs the skip regardless. - Skip reason textarea: placeholder rewritten as "Motivo (opcional, será registrado no audit log)". No length requirement. No client-side validation. The textarea exists for the audit trail, nothing else. - New optional onSkipReason callback flows the textarea text up to the install body so the backend's existing skip_reason audit field gets populated. - Install request now sends body.skip_scan = true + body.skip_reason when the verdict is SKIPPED. Backend (routes/plugins.py:670+) already accepts these — only the frontend was failing to emit them. - UpdatePreviewModal updated to also accept 'SKIPPED' (it relied on the legacy null === skip pattern; now both paths work). Tested - tsc --noEmit clean - Manual: marking Skip scan now allows "Confirmar instalação" to fire immediately without typing anything; with text, that text reaches the backend audit log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideFrontend now treats skipping the security scan as an explicit passing verdict, propagates an optional skip reason to the backend, and updates scan gate logic across install/update modals to recognize the new SKIPPED verdict while cleaning up the skip UX. Sequence diagram for skipping security scan during plugin installsequenceDiagram
actor Admin
participant PluginInstallModal
participant SecurityScanSection
participant ApiClient
participant Backend
Admin->>PluginInstallModal: Open plugin install modal
PluginInstallModal->>SecurityScanSection: Render with onVerdict, onSkipReason
Admin->>SecurityScanSection: Check Skip scan checkbox
SecurityScanSection->>SecurityScanSection: handleSkipToggle(true)
SecurityScanSection->>PluginInstallModal: onVerdict(SKIPPED, null)
SecurityScanSection-->>Admin: Show optional skip reason textarea
Admin->>SecurityScanSection: Type optional skip reason
SecurityScanSection->>SecurityScanSection: setSkipReason(text)
SecurityScanSection->>PluginInstallModal: onSkipReason(text)
PluginInstallModal->>PluginInstallModal: setSkipReason(text)
PluginInstallModal->>PluginInstallModal: scanGatePassed = true (verdict SKIPPED)
Admin->>PluginInstallModal: Click Confirmar instalação
PluginInstallModal->>PluginInstallModal: Build request body
PluginInstallModal->>PluginInstallModal: body.skip_scan = true
PluginInstallModal->>PluginInstallModal: body.skip_reason = trimmed skipReason (if non-empty)
PluginInstallModal->>ApiClient: POST /plugins/install body
ApiClient->>Backend: POST /plugins/install body
Backend-->>ApiClient: 200 OK (install result, audit logs skip)
ApiClient-->>PluginInstallModal: Install result
PluginInstallModal-->>Admin: Close modal, show installed plugin
Updated class diagram for scan verdict and plugin install componentsclassDiagram
class ScanVerdict {
<<union>>
APPROVE
WARN
BLOCK
SKIPPED
}
class ScanResult {
<<type>>
+status string
+findings ScanFinding[]
}
class ScanFinding {
+category string
+severity string
+message string
}
class SecurityScanSection {
+sourceUrl string
+authToken string
+onVerdict(verdict ScanVerdict, result ScanResult) void
+onVerdict(verdict null, result null) void
+onOverride(reason string) void
+onSkipReason(reason string) void
-skipScan boolean
-skipReason string
-result ScanResult | null
-error string | null
-scanning boolean
-hasRun Ref<boolean>
+handleSkipToggle(checked boolean) void
}
class PluginInstallModal {
-sourceUrl string
-uploadedPath string | null
-authToken string
-scanVerdict ScanVerdict | null
-scanResult ScanResult | null
-overrideReason string
-warnConfirmed boolean
-skipReason string
+scanGatePassed boolean
+handleScanVerdict(verdict ScanVerdict | null, result ScanResult | null) void
+handleOverride(reason string) void
+handleInstall() Promise~void~
}
class UpdatePreviewModal {
-scanVerdict ScanVerdict | null
-overrideReason string
-confirmed boolean
+scanGatePassed boolean
}
SecurityScanSection --> ScanResult
ScanResult --> ScanFinding
PluginInstallModal o-- SecurityScanSection : renders
PluginInstallModal --> ScanVerdict : uses
PluginInstallModal --> ScanResult : stores
UpdatePreviewModal --> ScanVerdict : uses
SecurityScanSection --> PluginInstallModal : onVerdict, onSkipReason callbacks
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 1 issue, and left some high level feedback:
- In
UpdatePreviewModal, thescanGatePassedlogic still treatsBLOCKas passing on any truthyoverrideReason, while the comments (andPluginInstallModal) require ≥20 chars; consider aligning the condition to avoid inconsistent override behavior. - Now that
ScanVerdictincludes'SKIPPED'and the skip path callsonVerdict('SKIPPED', null), you might want to revisit thescanVerdict === nullbranch inUpdatePreviewModal(currently documented as a skip) to more clearly distinguish between “still scanning” and “legacy skip” states.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UpdatePreviewModal`, the `scanGatePassed` logic still treats `BLOCK` as passing on any truthy `overrideReason`, while the comments (and `PluginInstallModal`) require ≥20 chars; consider aligning the condition to avoid inconsistent override behavior.
- Now that `ScanVerdict` includes `'SKIPPED'` and the skip path calls `onVerdict('SKIPPED', null)`, you might want to revisit the `scanVerdict === null` branch in `UpdatePreviewModal` (currently documented as a skip) to more clearly distinguish between “still scanning” and “legacy skip” states.
## Individual Comments
### Comment 1
<location path="dashboard/frontend/src/components/PluginInstallModal.tsx" line_range="73" />
<code_context>
+ // WARN = pass when checkbox confirmed
+ // BLOCK = pass when admin types a ≥20-char override reason
+ // SKIPPED = pass immediately (admin chose to bypass; audit logs the action)
const scanGatePassed =
scanVerdict === null
? false // still scanning
</code_context>
<issue_to_address>
**issue (bug_risk):** BLOCK override rule here uses a ≥20-char requirement, but UpdatePreviewModal only checks for non-empty text.
This modal enforces `overrideReason.trim().length >= 20` for BLOCK, but `UpdatePreviewModal` only checks `(scanVerdict === 'BLOCK' && !!overrideReason)`, so any non-empty string is accepted. Since the comment states BLOCK should require a ≥20-char reason, these flows are out of sync. Please either apply the same length check in `UpdatePreviewModal` or centralize this validation so both modals share it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // WARN = pass when checkbox confirmed | ||
| // BLOCK = pass when admin types a ≥20-char override reason | ||
| // SKIPPED = pass immediately (admin chose to bypass; audit logs the action) | ||
| const scanGatePassed = |
There was a problem hiding this comment.
issue (bug_risk): BLOCK override rule here uses a ≥20-char requirement, but UpdatePreviewModal only checks for non-empty text.
This modal enforces overrideReason.trim().length >= 20 for BLOCK, but UpdatePreviewModal only checks (scanVerdict === 'BLOCK' && !!overrideReason), so any non-empty string is accepted. Since the comment states BLOCK should require a ≥20-char reason, these flows are out of sync. Please either apply the same length check in UpdatePreviewModal or centralize this validation so both modals share it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found by Davidson: the plugin install wizard's Skip scan checkbox didn't do anything useful. Marking it sent `verdict=null` upward, `scanGatePassed` evaluated to `false`, and Confirmar instalação stayed disabled / silently dropped the request. The skip-reason textarea was tagged (required) but had no minimum, no validation feedback, and was never sent to the backend.
What changed
Why no required-reason
Davidson explicitly said justifying every skip is bobagem. The audit trail captures the action (admin id, timestamp, plugin slug, IP, optional reason). Refusing to act unless the admin types 20 characters of justification is friction that doesn't add security — it just teaches admins to type "ja ta ok"-equivalent boilerplate.
Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Allow plugin installations and updates to explicitly bypass the security scan while still recording the action and optional reason for auditing.
Bug Fixes:
Enhancements: