feat(modules): bridge fingerprint modules into framework detection#359
feat(modules): bridge fingerprint modules into framework detection#359TBX3D wants to merge 8 commits into
Conversation
github-actions
Bot
added
size/xl
pr summary14 files changed (+1194 -37)
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
=======================================
Coverage ? 65.05%
=======================================
Files ? 90
Lines ? 8029
Branches ? 0
=======================================
Hits ? 5223
Misses ? 2406
Partials ? 400 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e2ba979 to
76ae304
Compare
vmfunc
left a comment
vmfunc
left a comment
There was a problem hiding this comment.
the fingerprint module type itself is solid. scorer parity with MatchSignatures is pinned properly, and the zero-weight remap divergence + strict-threshold-at-0.5 edges are exactly right.
but the bridge doesn't run. BridgeFingerprints has no caller, grep it, only the definition and the tests reference it. at runtime nothing ever lands in the framework registry, so as landed this is inert scaffolding, not what the title says.
and even once you wire it, it doesn't dedup. the fingerprint module still executes in the module engine and reports its own finding (your bridge comment says as much), and registering it as a framework detector just adds a second surface, the framework engine already collapses to one best. so the tech shows up in both the module output and the framework output regardless. bridging never removes the native report. if the plan is to drop bridged fingerprint modules from the module run, that logic isn't in here.
so before this goes in: is the wiring a follow-up, and where does the native finding get suppressed? one of those has to exist for this to do what it claims. and fix the description, it states the dedup as done.
minor: bridged name is def.ID and frameworks.Register clobbers by name, so a module id colliding with a builtin ("WordPress", "Django"...) silently overwrites the builtin detector. skip-or-warn on an existing key.
76ae304 to
b84b14c
Compare
|
you are right on both counts and the description was the worst of it, sorry, it wiring: BridgeFingerprints is now called from Run before scanAllTargets. it has dedup: BridgeFingerprints returns the ids it bridged and an implicit module run collision: added frameworks.RegisterIfAbsent, check and insert under one lock, PR description rewritten to describe what the code does. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Bridges default-confidence root-scoped fingerprint modules into the framework detector registry to avoid double-reporting the same technology, while keeping the legacy signatures/ path working (now with a deprecation notice).
Changes:
- Add
fingerprintYAML module type with weighted scoring, optional version extraction, and result confidence propagation. - Bridge eligible fingerprint modules into
internal/scan/frameworksdetectors and skip bridged modules during implicit module runs to dedupe results. - Centralize per-user
~/.config/sif/...(and Windows equivalent) path resolution viainternal/sifpath, update loaders, add extensive tests, and document fingerprint modules.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sif.go | Load modules once per run; bridge fingerprint modules into framework detection; skip bridged modules in implicit module execution |
| internal/sifpath/sifpath.go | New helper to resolve per-user sif subdirectories consistently |
| internal/sifpath/sifpath_test.go | Tests for expected per-user directory layout and sibling relationships |
| internal/scan/frameworks/detector.go | Add RegisterIfAbsent to prevent user-supplied name collisions from clobbering built-ins |
| internal/scan/frameworks/detect.go | Use shared httpx.ReadCappedBody for consistent body size limiting |
| internal/scan/frameworks/custom.go | Switch signatures dir resolution to sifpath; emit deprecation notice for legacy custom signatures |
| internal/scan/frameworks/custom_backcompat_test.go | Backcompat test ensuring legacy custom signatures still parse/score |
| internal/scan/frameworks/custom_deprecation_test.go | Test that deprecation notice is logged while still loading legacy signatures |
| internal/modules/yaml.go | Parse/validate/execute new fingerprint module type; expose wrapper definition to in-package bridge |
| internal/modules/module.go | Add TypeFingerprint; add Finding.Confidence for fingerprint findings |
| internal/modules/loader.go | Use sifpath for user modules directory resolution |
| internal/modules/executor.go | Use shared httpx.ReadCappedBody cap for HTTP modules |
| internal/modules/fingerprint.go | New fingerprint module implementation: validation, scoring, version extraction |
| internal/modules/bridge.go | New bridge to register eligible fingerprint modules as framework detectors and report bridged IDs |
| internal/modules/fingerprint_type_test.go | Tests for fingerprint scoring and confidence threshold behavior |
| internal/modules/fingerprint_parse_test.go | Round-trip parse/validate/execute tests + validation/default-confidence behavior |
| internal/modules/fingerprint_equivalence_test.go | Tests pinning scorer equivalence (and known divergence boundaries) |
| internal/modules/fingerprint_bridge_test.go | Tests for bridging rules, collision safety, and framework participation |
| internal/httpx/httpx.go | Add shared MaxBodySize and ReadCappedBody helper |
| docs/modules.md | Document fingerprint module type and YAML schema |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (app *App) loadModules() { | ||
| if app.modulesLoaded { | ||
| return | ||
| } | ||
| loader, err := modules.NewLoader() | ||
| if err != nil { | ||
| log.Warnf("Failed to create module loader: %v", err) | ||
| return | ||
| } | ||
| if err := loader.LoadAll(); err != nil { | ||
| log.Warnf("Failed to load modules: %v", err) | ||
| } | ||
| builtin.Register() | ||
| app.modulesLoaded = true | ||
| } |
| keep := func(ms []modules.Module) []modules.Module { | ||
| if len(app.bridgedFingerprints) == 0 { | ||
| return ms | ||
| } | ||
| out := ms[:0] | ||
| for _, m := range ms { | ||
| if id := m.Info().ID; app.bridgedFingerprints[id] { | ||
| log.Debugf("Skipping %s: covered by framework detection", id) | ||
| continue | ||
| } | ||
| out = append(out, m) | ||
| } | ||
| return out | ||
| } |
| // UserSubdir returns the per-user sif configuration subdirectory for name (for | ||
| // example "modules" or "signatures"). It preserves sif's historical layout: | ||
| // ~/.config/sif/<name> on unix-like systems and %LOCALAPPDATA%\sif\<name> on | ||
| // windows. | ||
| func UserSubdir(name string) (string, error) { | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if runtime.GOOS == "windows" { | ||
| return filepath.Join(home, "AppData", "Local", "sif", name), nil | ||
| } | ||
| return filepath.Join(home, ".config", "sif", name), nil | ||
| } |
| output.Module("FRAMEWORK").Info("Loaded %d custom signatures", len(detectors)) | ||
| output.Module("FRAMEWORK").Info("~/.config/sif/signatures is deprecated; write type: fingerprint modules under ~/.config/sif/modules instead") | ||
| } |
| func captureStdout(t *testing.T, fn func()) string { | ||
| t.Helper() | ||
|
|
||
| r, w, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatalf("pipe: %v", err) | ||
| } | ||
| saved := os.Stdout | ||
| os.Stdout = w | ||
| output.SetSilent(false) | ||
|
|
||
| ch := make(chan string, 1) | ||
| go func() { | ||
| data, _ := io.ReadAll(r) | ||
| ch <- string(data) | ||
| }() | ||
|
|
||
| fn() | ||
|
|
||
| os.Stdout = saved | ||
| output.SetSilent(false) | ||
| w.Close() | ||
| return <-ch | ||
| } |
b84b14c to
762d3d0
Compare
|
follow-up on a regression I introduced in the wiring commit. folding the two module-load sites into one helper made loadModules swallow a loadModules now returns an error: -list-modules propagates it, a scan still also stopped the bridged-module filter from reusing the input slice's backing |
762d3d0 to
dd2c870
Compare
|
rebased onto current main now that #355 has landed. the two shared-helper heads up that CI on this branch (and a few others) is currently red for a reason |
a `fingerprint` module identifies a technology by weighted body/header signatures scored into a confidence, with an optional version regex, rather than a boolean match. it fires one finding carrying the score once it reaches the threshold (default 0.5). this is the framework detectors' scoring in the module format, so a custom tech fingerprint lives alongside other modules. validated at load (signatures present, non-empty patterns, finite weights, confidence in [0,1], version regex compiles) and covered by yaml round-trip, validation, header, version and default-threshold tests. documented in docs/modules.md. shares the response body cap via httpx.ReadCappedBody.
…orting BridgeFingerprints had no caller. nothing ever reached the framework registry at runtime, so the bridge was scaffolding rather than the feature the title claims. call it before the target loop, gated on -framework. it has to sit up there: the per-target framework scan runs long before the module loader would otherwise be touched, so bridging any later has no effect on the detection it is meant to feed. bridging alone would then name a technology twice, once from the module engine and once from the framework engine, since the module keeps running natively. BridgeFingerprints now returns the ids it bridged and an implicit module run (-all-modules, -module-tags) skips them. naming a module with -modules is a direct request and still runs it. the two engines disagree on one boundary, the module engine fires at score >= confidence and the framework engine at score > threshold, so a bridged module sitting exactly at 0.5 is reported by neither in an implicit run. that is the residual bridgeableToFramework already documents. also refuse to bridge onto a name that is already registered. frameworks.Register clobbers by name and the bridged name is the module id, so a module called "WordPress" silently replaced the builtin detector. add frameworks.RegisterIfAbsent for the check and the insert under one lock. module loading is now behind app.loadModules, which loads once per run rather than once per target.
dd2c870 to
7c75c6e
Compare
a fingerprint module and a framework detector can independently identify the same technology. this registers default-confidence, root-scoped fingerprint modules into the framework detector registry so framework detection covers yaml-defined technologies, and keeps the two engines from naming the same thing twice.
BridgeFingerprints is called from Run before scanAllTargets. it has to sit there: the per-target framework scan runs inside the -concurrency worker pool, so bridging any later would not reach it, and loading up front keeps the pool off the shared registry write path. the module set is now loaded once per run rather than once per target.
BridgeFingerprints returns the ids it bridged, and an implicit module run (-all-modules, -module-tags) skips them, so the technology is reported by the framework engine alone. naming a module with -modules is a direct request and still runs it. the two engines disagree on one boundary, the module engine fires at score >= confidence and the framework engine at score > threshold, so a bridged module sitting exactly at 0.5 is reported by neither in an implicit run. that is the residual bridgeableToFramework documents.
the bridged name is the module id and frameworks.Register clobbers by name, so a module called "WordPress" would have replaced the builtin detector. frameworks.RegisterIfAbsent does the check and the insert under one lock and the bridge refuses the name instead.
also marks the legacy custom-signature (signatures dir) path as deprecated in favour of fingerprint modules, without removing it.
stacked on #355 (shared sifpath/httpx helpers) and #356 (fingerprint module type), both open. this branch includes their commits so it builds standalone; rebasing after they merge should drop those as already-applied.