Fix misleading security scan status message in jf skills publish - #541
Fix misleading security scan status message in jf skills publish#541tal-qwak wants to merge 4 commits into
Conversation
The APPROVED status from the Skills Xray gate reflects Artifactory/Xray's current indexing state at the moment of the check, not necessarily a completed content scan - for freshly published skills the message could print before Xray has had a chance to fully scan the artifact. Reword the message so it reports what was actually observed (no violations found) rather than asserting the artifact 'passed security scan', which implies a guarantee the gate cannot currently make.
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Xray gate updates approved-status log messages in immediate and polling completion paths. The messages now state that no Xray violations were found for the skill and version. ChangesXray approval log wording
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change only replaces misleading security-scan success text with a pending-status message and does not alter scan gating or control flow. No actionable merge-blocking risk remains after normal checks. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Following up on the earlier wording fix: 'no violations found' alone still reads as a clean bill of health, which is the same false confidence 'passed security scan' gave. Add an explicit caveat that the scan may still be in progress, since Xray's current Skills gate status can report APPROVED before the async content scan has actually finished.
The previous wording said the same thing twice ('so far' + 'may still be
pending') across two sentences. Match the file's existing bracket-tag
convention ([VIOLATION] for the blocked case) with [SCAN PENDING] here, so
the one word that matters is skimmable at a glance instead of requiring
the full sentence to be read.
|
I have read the CLA Document and I hereby sign the CLA |
tal-qwak
changed the title
| return nil | ||
| case services.SkillXrayStatusApproved: | ||
| log.Info(fmt.Sprintf("[SUCCESS] Skill \"%s\" v%s passed security scan.", params.Slug, params.Version)) | ||
| log.Info(fmt.Sprintf("[SCAN PENDING] Skill \"%s\" v%s: no violations found yet.", params.Slug, params.Version)) |
There was a problem hiding this comment.
If jfrog-cli is doing checks to figure out what is the XRAY status via artifactory API, then changes has to be made on the API to return proper status instead of Approved. If the api is returning approved but still it means that scanning is not completed then API response should be improved.
There was a problem hiding this comment.
Agreed, and this PR is intentionally scoped as a mitigation, not the real fix. We traced this down to the actual root cause: Xray's GetArtifactState() (xray/backend/backend/service/artifact_details/artifact_details_service.go in JFROG/xray) sets ArtifactProcessed = true for PendingScanValue ("Pending Scan") and INDEXING, not just SCANNED - so the Skills gate can report APPROVED before the async content-scan (aiscanner) pipeline has actually produced a verdict. We drafted a fix scoped to PkgType == skills there, but couldn't push a branch to JFROG/xray (hit what looks like a repo-level policy blocking direct branch creation) - happy to hand off the diff or open it properly once we know the right contribution path for that repo. This PR just reduces the blast radius on the CLI side in the meantime.
There was a problem hiding this comment.
There should be an improvement again when there is a proper fix on JFROG/xray.
3 participants
Summary
jf skills publish(without--skip-scan) prints[SUCCESS] Skill "<slug>" v<version> passed security scan.as soon as the Skills Xray gate (GET .../api/skills/{repoKey}/xrayStatus) returnsAPPROVED.xray/api/v1/summary/artifactfor the same artifact showed a Critical "Malicious skill" finding (XRAY-00672) that the gate never surfaced.xray_gate.gois correct as-is (it already polls onSCAN_IN_PROGRESSuntil a terminal status). The issue is purely the wording:APPROVEDreflects Artifactory/Xray's current indexing state at the moment of the check, not a guaranteed completed content scan, so asserting the artifact "passed security scan" overclaims what the gate can currently promise.Change
Reworded both
APPROVEDlog lines (the immediate case inCheckXrayGateand the post-poll case inpollUntilDone):Matches the file's existing bracket-tag convention (
[VIOLATION]for the blocked case), and leads with the one word that matters (PENDING, notSUCCESS) so it's skimmable without reading the full line.No behavior change — same statuses handled the same way, same control flow. Confirmed
go build ./agent/skills/...andgo test ./agent/skills/...pass unchanged.Note for maintainers
This does not close the underlying gap — it only makes the CLI honest about what it doesn't know. The actual cause (why
APPROVEDcan be returned before a skill's async content scan completes) is server-side, in Xray's artifact-processed status computation, not in this CLI.jf skills installshares the same gate check and remains exposed to the same window. Happy to share full details on request.Test plan
go build ./agent/skills/...go test ./agent/skills/...Summary by CodeRabbit