Skip to content

Commit 19c6dbe

Browse files
committed
fix(security): gate source extraction on deep_scan + wire deep-scan report banner
Address adversarial review findings on Spec 077 US3 (PR #789): - #1 (MUST): published-package-source extraction is now part of the opt-in deep-scan layer. server.go computes the effective fetch as IsDeepScanEnabled() && fetch_package_source (default true), and SetDeepScan(false) force-disables the resolver's fetch fallback as defense-in-depth. Deep scan OFF (default) => no npx/uvx source-fetch network egress. Added TestServiceDeepScanGatesPackageSourceFetch and updated the doc comments to match. - #2 (MUST): the deep-scan report banner was dead code — AggregatedReport had no DeepScan field, so /scan/report and /scans/{jobId}/report never emitted deep_scan. Added DeepScan *DeepScanDescriptor (json deep_scan, omitempty) to AggregatedReport and populate it in GetScanReport and GetScanReportByJobID (the live report-page path). - #3 (LOW): buildDeepScanDescriptor now inspects Pass-1 AND Pass-2 scanner statuses (variadic jobs, deduped) so heavy trivy/supply-chain scanner failures are reflected in scanners_failed/availability. Baseline verdict logic and the quarantine state machine are unchanged. Related: Spec 077
1 parent abf8050 commit 19c6dbe

5 files changed

Lines changed: 140 additions & 32 deletions

File tree

internal/security/scanner/service.go

Lines changed: 75 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,15 @@ func (s *Service) SetDeepScan(enabled bool, scanners []string) {
222222
} else {
223223
s.engine.deepScanScanners = nil
224224
}
225+
// Spec 077 US3: published-package-source extraction is part of the opt-in
226+
// deep-scan layer, so it must never run (and never cause network egress)
227+
// while deep scan is off. Force the resolver's fetch fallback off here as
228+
// defense-in-depth. When deep scan is ENABLED the server layer decides the
229+
// concrete value from deep_scan.fetch_package_source (default true) via
230+
// SetFetchPackageSource, so we deliberately do not flip it back on here.
231+
if !enabled && s.sourceResolver != nil {
232+
s.sourceResolver.SetFetchPackageSource(false)
233+
}
225234
if enabled {
226235
s.logger.Info("Deep scan enabled (security.deep_scan.enabled=true): Docker scanner " +
227236
"plugins + published-package-source extraction may run as an opt-in enrichment " +
@@ -248,30 +257,47 @@ func (s *Service) isBaselineScanner(scannerID string) bool {
248257
}
249258

250259
// buildDeepScanDescriptor assembles the informational deep-scan availability
251-
// descriptor (Spec 077 FR-008) from a job's per-scanner statuses. It reports the
252-
// opt-in layer's state SEPARATELY from the baseline verdict and MUST NOT
253-
// influence ScanSummary.Status. Returns nil when deep scan is disabled, so the
254-
// descriptor is omitted entirely and the invariant (Enabled=false ⇒ everything
255-
// empty) holds.
256-
func (s *Service) buildDeepScanDescriptor(job *ScanJob) *DeepScanDescriptor {
257-
if job == nil || !s.deepScanEnabled() {
260+
// descriptor (Spec 077 FR-008) from the per-scanner statuses of one or more
261+
// jobs. It reports the opt-in layer's state SEPARATELY from the baseline verdict
262+
// and MUST NOT influence ScanSummary.Status. Both passes are considered: Pass 1
263+
// (security scan) and Pass 2 (supply-chain audit, where the heavy trivy /
264+
// supply-chain scanners run), so a Pass-2 scanner failure is reflected too.
265+
// Returns nil when deep scan is disabled, so the descriptor is omitted entirely
266+
// and the invariant (Enabled=false ⇒ everything empty) holds.
267+
func (s *Service) buildDeepScanDescriptor(jobs ...*ScanJob) *DeepScanDescriptor {
268+
if !s.deepScanEnabled() {
258269
return nil
259270
}
271+
haveJob := false
260272
desc := &DeepScanDescriptor{Enabled: true}
261-
for _, ss := range job.ScannerStatuses {
262-
if s.isBaselineScanner(ss.ScannerID) {
263-
continue // baseline coverage drives Status, not the descriptor
264-
}
265-
switch ss.Status {
266-
case ScanJobStatusCompleted:
267-
desc.Ran = true
268-
desc.Available = true
269-
case ScanJobStatusFailed:
270-
desc.ScannersFailed = append(desc.ScannersFailed, DeepScanScannerFailure{
271-
ID: ss.ScannerID,
272-
Reason: ss.Error,
273-
})
273+
seenFailed := make(map[string]bool)
274+
for _, job := range jobs {
275+
if job == nil {
276+
continue
274277
}
278+
haveJob = true
279+
for _, ss := range job.ScannerStatuses {
280+
if s.isBaselineScanner(ss.ScannerID) {
281+
continue // baseline coverage drives Status, not the descriptor
282+
}
283+
switch ss.Status {
284+
case ScanJobStatusCompleted:
285+
desc.Ran = true
286+
desc.Available = true
287+
case ScanJobStatusFailed:
288+
if seenFailed[ss.ScannerID] {
289+
continue // dedupe the same deep scanner across passes
290+
}
291+
seenFailed[ss.ScannerID] = true
292+
desc.ScannersFailed = append(desc.ScannersFailed, DeepScanScannerFailure{
293+
ID: ss.ScannerID,
294+
Reason: ss.Error,
295+
})
296+
}
297+
}
298+
}
299+
if !haveJob {
300+
return nil
275301
}
276302
return desc
277303
}
@@ -303,8 +329,14 @@ func (s *Service) resolveIsolationMode(serverName string) string {
303329
}
304330

305331
// SetFetchPackageSource toggles whether the source resolver may fetch the
306-
// published source of package-runner servers (npx/uvx) for scanning. See
307-
// SecurityConfig.ScannerFetchPackageSource (MCP-2206). Default is enabled.
332+
// published source of package-runner servers (npx/uvx) for scanning. This is a
333+
// facet of the opt-in deep-scan layer (Spec 077 US3): the server layer only
334+
// enables it when security.deep_scan.enabled is true AND
335+
// security.deep_scan.fetch_package_source is not explicitly false (the
336+
// deprecated top-level ScannerFetchPackageSource, MCP-2206, is still honored as
337+
// a fallback). With deep scan off the effective value is always false, so
338+
// scanning an npx/uvx server performs no published-package-source network
339+
// egress by default.
308340
func (s *Service) SetFetchPackageSource(enabled bool) {
309341
if s.sourceResolver == nil {
310342
return
@@ -1219,6 +1251,12 @@ func (s *Service) GetScanReport(ctx context.Context, serverName string) (*Aggreg
12191251
agg.ScanContext = primaryJob.ScanContext
12201252
agg.ScannerStatuses = primaryJob.ScannerStatuses
12211253

1254+
// Spec 077 US3 (FR-008): mirror the opt-in deep-scan availability descriptor
1255+
// onto the report so the report page renders the informational banner. Both
1256+
// passes are considered. Informational only — never changes the verdict; nil
1257+
// (omitted) when deep scan is off.
1258+
agg.DeepScan = s.buildDeepScanDescriptor(pass1Job, pass2Job)
1259+
12221260
return agg, nil
12231261
}
12241262

@@ -1315,6 +1353,7 @@ func (s *Service) GetScanReportByJobID(ctx context.Context, jobID string) (*Aggr
13151353
// If this is a Pass 1 job, try to find and merge companion Pass 2 results.
13161354
// The companion is resolved via the lightweight scan-job index, so this does
13171355
// NOT deserialize the full per-server scan history (MCP-2205).
1356+
var companionPass2 *ScanJob
13181357
if job.ScanPass == ScanPassSecurityScan || job.ScanPass == 0 {
13191358
if companionID := s.findCompanionPass2JobID(job); companionID != "" {
13201359
pass2Reports, err := s.storage.ListScanReportsByJob(companionID)
@@ -1330,6 +1369,12 @@ func (s *Service) GetScanReportByJobID(ctx context.Context, jobID string) (*Aggr
13301369
agg.Pass1Complete = true
13311370
agg.Pass2Complete = true
13321371
}
1372+
// Load the companion Pass-2 job so its per-scanner statuses feed the
1373+
// deep-scan descriptor (Spec 077 FR-008) — the heavy trivy /
1374+
// supply-chain scanners run in Pass 2.
1375+
if cj, cerr := s.storage.GetScanJob(companionID); cerr == nil {
1376+
companionPass2 = cj
1377+
}
13331378
}
13341379

13351380
// Check if Pass 2 is running
@@ -1342,6 +1387,13 @@ func (s *Service) GetScanReportByJobID(ctx context.Context, jobID string) (*Aggr
13421387
agg.ScanContext = job.ScanContext
13431388
agg.ScannerStatuses = job.ScannerStatuses
13441389

1390+
// Spec 077 US3 (FR-008): mirror the opt-in deep-scan availability descriptor
1391+
// so the report page renders the informational banner. Considers this job and
1392+
// its companion Pass-2 job (when the requested job is a Pass-1 job).
1393+
// Informational only — never changes the verdict; nil (omitted) when deep
1394+
// scan is off.
1395+
agg.DeepScan = s.buildDeepScanDescriptor(job, companionPass2)
1396+
13451397
return agg, nil
13461398
}
13471399

@@ -1782,7 +1834,7 @@ func (s *Service) GetScanSummary(ctx context.Context, serverName string) *ScanSu
17821834
// informational dimension. This never influences Status — a failed or
17831835
// unavailable deep scanner leaves the baseline verdict untouched. Nil (and
17841836
// thus omitted) when deep scan is off.
1785-
summary.DeepScan = s.buildDeepScanDescriptor(primaryJob)
1837+
summary.DeepScan = s.buildDeepScanDescriptor(pass1Job, pass2Job)
17861838

17871839
// Check if the primary job failed
17881840
if primaryJob.Status == ScanJobStatusFailed {

internal/security/scanner/service_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2234,3 +2234,45 @@ func TestServiceDeepScanOffByDefault(t *testing.T) {
22342234
t.Errorf("non-allow-listed deep scanner must be dropped; got %v", ids)
22352235
}
22362236
}
2237+
2238+
// TestServiceDeepScanGatesPackageSourceFetch verifies Spec 077 US3: published-
2239+
// package-source extraction is a facet of the opt-in deep-scan layer, so it must
2240+
// not run (no network egress) while deep scan is off. Turning the deep-scan
2241+
// layer off must force the source resolver's fetch fallback off; the server
2242+
// layer re-enables it (honoring fetch_package_source) only when deep scan is on.
2243+
func TestServiceDeepScanGatesPackageSourceFetch(t *testing.T) {
2244+
dir := t.TempDir()
2245+
logger := zap.NewNop()
2246+
store := newMockStorage()
2247+
docker := NewDockerRunner(logger)
2248+
registry := NewRegistry(dir, logger)
2249+
2250+
svc := NewService(store, registry, docker, dir, logger)
2251+
2252+
// Simulate an operator who had opted into source fetch previously.
2253+
svc.SetFetchPackageSource(true)
2254+
if !svc.sourceResolver.fetchPackageSource {
2255+
t.Fatalf("precondition: fetch should be enabled after SetFetchPackageSource(true)")
2256+
}
2257+
2258+
// Deep scan OFF (the default) must forbid the published-package-source fetch
2259+
// so scanning an npx/uvx server performs no network egress.
2260+
svc.SetDeepScan(false, nil)
2261+
if svc.sourceResolver.fetchPackageSource {
2262+
t.Errorf("deep scan off must force source fetch off (no egress by default)")
2263+
}
2264+
2265+
// The server layer is the authority when deep scan is on: it calls
2266+
// SetFetchPackageSource with deep_scan.fetch_package_source (default true).
2267+
svc.SetDeepScan(true, nil)
2268+
svc.SetFetchPackageSource(true)
2269+
if !svc.sourceResolver.fetchPackageSource {
2270+
t.Errorf("deep scan on with fetch_package_source=true must allow source fetch")
2271+
}
2272+
2273+
// An explicit fetch_package_source=false still wins while deep scan is on.
2274+
svc.SetFetchPackageSource(false)
2275+
if svc.sourceResolver.fetchPackageSource {
2276+
t.Errorf("explicit fetch_package_source=false must disable source fetch")
2277+
}
2278+
}

internal/security/scanner/source_resolver.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,14 @@ import (
2323
type SourceResolver struct {
2424
logger *zap.Logger
2525

26-
// fetchPackageSource, when true (the default), allows the resolver to fetch
27-
// the PUBLISHED source of a package-runner server (npx/uvx) — without
28-
// executing it — as a last resort before falling back to a
29-
// tool-definitions-only scan. Air-gapped deployments can disable this via
30-
// security.deep_scan.fetch_package_source=false to forbid network egress.
26+
// fetchPackageSource, when true, allows the resolver to fetch the PUBLISHED
27+
// source of a package-runner server (npx/uvx) — without executing it — as a
28+
// last resort before falling back to a tool-definitions-only scan. This is a
29+
// facet of the opt-in deep-scan layer (Spec 077 US3): the server layer keeps
30+
// it false unless security.deep_scan.enabled is true, and even then honors
31+
// security.deep_scan.fetch_package_source (default true) so air-gapped
32+
// deployments can forbid the network egress. With deep scan off it is always
33+
// false, so no published-package-source fetch happens by default.
3134
fetchPackageSource bool
3235
}
3336

internal/security/scanner/types.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,11 @@ type AggregatedReport struct {
300300
// Scan context from the primary job (for report page display)
301301
ScanContext *ScanContext `json:"scan_context,omitempty"`
302302
ScannerStatuses []ScannerJobStatus `json:"scanner_statuses,omitempty"` // Per-scanner execution logs
303+
// DeepScan is the opt-in heavy-layer availability descriptor (Spec 077 US3),
304+
// mirrored from ScanSummary so the report page can render the informational
305+
// deep-scan banner. Informational only — a failed or unavailable deep scanner
306+
// never changes the baseline verdict. nil/omitted when deep scan is disabled.
307+
DeepScan *DeepScanDescriptor `json:"deep_scan,omitempty"`
303308
}
304309

305310
// ReportSummary provides counts by severity and threat level

internal/server/server.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2000,12 +2000,18 @@ func (s *Server) startCustomHTTPServer(ctx context.Context, streamableServer *se
20002000
im := core.NewIsolationManager(liveCfg.DockerIsolation)
20012001
return string(im.ResolveMode(sc))
20022002
})
2003-
// Published-package-source fetch is enabled by default; only an explicit
2004-
// false in config disables it (MCP-2206).
2003+
// Spec 077 US3: published-package-source extraction is part of the opt-in
2004+
// deep-scan layer, so it only runs when deep scan is enabled. When enabled
2005+
// it honors deep_scan.fetch_package_source (default true; an explicit
2006+
// false — or the deprecated top-level ScannerFetchPackageSource, MCP-2206 —
2007+
// forbids the network egress for air-gapped hosts). Deep scan OFF (the
2008+
// default) ⇒ no source fetch/egress at all.
20052009
if cfg != nil && cfg.Security != nil {
2006-
if fetch := cfg.Security.EffectiveFetchPackageSource(); fetch != nil && !*fetch {
2007-
secService.SetFetchPackageSource(false)
2010+
fetchPref := true
2011+
if fetch := cfg.Security.EffectiveFetchPackageSource(); fetch != nil {
2012+
fetchPref = *fetch
20082013
}
2014+
secService.SetFetchPackageSource(cfg.Security.IsDeepScanEnabled() && fetchPref)
20092015
}
20102016
secService.SetEmitter(s.runtime)
20112017
secService.SetServerInfoProvider(&configServerInfoProvider{

0 commit comments

Comments
 (0)