feat(secrets): JIT Snyk Learn lessons for Secrets findings [EDU-4754]#1265
feat(secrets): JIT Snyk Learn lessons for Secrets findings [EDU-4754]#1265SnykOleg wants to merge 25 commits into
Conversation
Add a new case in lessonsLookupParams for types.SecretsIssue so the lookup returns a non-nil LessonLookupParams when invoked from the getLearnLesson command (or from any future eager wiring). Secrets findings carry no Learn-tagged rule (Phase 0b probe of /v1/learn/lessons confirmed rules=[] on the CWE-798 'Hardcoded secrets' lesson) and ship with empty Ecosystem (convert.go does not set it). Pass an empty Rule and let the existing cache fall-through (rule -> ecosystem -> all-lessons) plus filterForCWEs CWE intersection resolve to the published CWE-798 lesson. No wire-protocol change: the appended SecretsIssue ordinal (5) was already in the iota block. LS_PROTOCOL_VERSION is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
… [EDU-4754] Inject learn.Service into secrets.Scanner (mirroring how it is injected into code.Scanner) and call a new enhanceWithLearnLesson helper after the issue-aggregation loop in Scan and before the cache mutations, matching the order in code.go:254-257. The helper iterates the produced []types.Issue, calls learnService.GetLesson with the issue's ecosystem, ID, CWEs, CVEs and IssueType, and writes lesson.Url into the issue via SetLessonUrl on success. Errors are logged and swallowed so a Learn cache miss never blocks the scan. Sentry instrumentation around the lookup is intentionally omitted: secrets.Scanner has no errorReporter field today and the rest of the package logs transient errors via its zerolog logger only. Adding error_reporting.ErrorReporter just for one non-actionable Learn-cache miss is disproportionate; revisit alongside any future Sentry pass on the secrets package. Application wiring updated in di/init.go to pass the existing learnService into secrets.New. Co-authored-by: Cursor <cursoragent@cursor.com>
When a secret issue carries a LessonUrl (set during scan by the enhanceWithLearnLesson helper added in the previous commit), surface it in the details panel as a 'Learn about this issue type' link -- identical markup to the code template at infrastructure/code/template/details.html:66-74 so the two products share the same UI affordance. The required CSS rules (.sn-learn, .lesson-link.styled-link.is-external, .lesson-icon, .is-external-icon) are already present in infrastructure/secrets/template/styles.css; no stylesheet port is needed. The template renderer is updated to pass the new data-map keys (LessonUrl, LessonIcon, ExternalIcon). Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This comment has been minimized.
This comment has been minimized.
|
/describe |
|
PR Description updated to latest commit (448e4c2) |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
This comment has been minimized.
This comment has been minimized.
…U-4754] Make learn.Service available to every JSON-RPC handler via the request-context dependencies map, mirroring how authenticationService, ldxSyncService, and inlineValueProvider are already plumbed through withContext. Scanners can now consume the learn service from ctx instead of carrying it as a struct field, satisfying the convention in .cursor/rules/server-dependencies.mdc. The DepLearnService key already existed in internal/context/context.go and is read today by infrastructure/oss/unified_converter.go. Wiring it through withContext means every scanner downstream of the LSP handlers sees the same instance without per-scanner enrichContext boilerplate. Pre-existing pattern unchanged: dependencies are added to the deps map only when non-nil, so tests that pass nil for unrelated dependencies keep working. Response to PR #1265 review comment from @bastiandoetsch: #1265 (comment)
…earn lookup into FindingsConverter [EDU-4754] Stop carrying learn.Service as a field on secrets.Scanner. The production withContext middleware (server.go, previous commit) now injects the service into every request context under DepLearnService; the converter reads it back with the same lookup pattern used in infrastructure/oss/unified_converter.go. This change responds to two PR #1265 review comments from @bastiandoetsch and aligns the secrets scanner with the convention in .cursor/rules/server-dependencies.mdc. 1. Constructor surface (review comment 1) secrets.Scanner.learnService field removed; secrets.New() reverts to its pre-PR 8-parameter signature. application/di/init.go no longer threads learnService through the secrets constructor. Production wiring runs entirely through the request context. 2. Loop fusion (review comment 2) The standalone Scanner.enhanceWithLearnLesson loop after the scan aggregation is removed. The lesson lookup now lives inside FindingsConverter.findingToIssues so each issue is enriched in the single pass that converts it. ToIssues takes a context.Context and extracts the learn.Service via learnServiceFromContext (local helper mirroring oss/unified_converter.go:getLearnServiceAndErrorReporter). A nil learnService, a lookup error, or a nil/empty lesson all leave LessonUrl untouched - Learn cache misses must never fail the scan. 3. Tests - All 16 secrets.New() call sites in secrets_test.go dropped the learnService argument. The 13 unrelated-to-Learn tests that previously used a no-op mock_learn stub now exercise the graceful-degradation path (nil service in ctx, no lookup, no LessonUrl - same behaviour the production code follows when the middleware has nothing to inject). - The 3 lesson-specific tests (PopulatesLessonUrl, LessonUrlEmptyOnLearnError, LessonUrlEmptyOnNilLesson) inject the mock via a new withLearnServiceInContext test helper that mirrors the production withContext plumbing path. - convert_test.go ToIssues calls take t.Context() to match the new signature. 4. Side-effect fix This also unbreaks two pre-existing helpers in secrets_test.go (seedScannerCache, scanWithEngineError) that were calling the old 8-arg New() signature and would not compile against the prior 9-arg form. make lint is now 0 issues across the repo. Review threads: - #1265 (comment) - #1265 (comment)
This comment has been minimized.
This comment has been minimized.
@bastiandoetsch Done, re-signed 🙏 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey @bastiandoetsch I hope this PR is good to go 🙏 |
This comment has been minimized.
This comment has been minimized.
Bring branch up to date with main before merge of PR #1265.
This comment has been minimized.
This comment has been minimized.
| cwes, | ||
| cves, | ||
| } | ||
| case types.SecretsIssue: |
There was a problem hiding this comment.
Should fix — a Secrets finding with no CWE gets an arbitrary, unrelated lesson. This case passes an empty rule and relies on "CWE intersection through the all-lessons fall-through." That works only when the finding actually has a CWE. When it doesn't (extractProblems returns cwes=nil whenever there's no cwe problem), the lookup goes: empty rule + empty ecosystem → all-lessons branch → filterForCWEs with an empty CWE list, which filterLessonWithComparatorFunc treats as a no-op and returns the full list → GetLesson picks lessons[0]. So the "Learn how to remediate Secrets securely" link points at an arbitrary, unrelated lesson (and the selection is non-deterministic across runs, since GetAll() iterates a map).
Two reviewers flagged this independently. Suggested fix: in the Secrets path (or in enrichWithLearnLesson) skip the lookup / return no lesson when there is no CWE match, so a CWE-less secret gets no link rather than a wrong one. Add a real-service (non-mock) test asserting LessonUrl is empty for a no-CWE secret with a non-empty Learn cache — the current tests mock GetLesson and never exercise this path.
— AI review
| AdditionalData: additionalData, | ||
| }) | ||
| } | ||
| c.enrichWithLearnLesson(issue, learnService) |
There was a problem hiding this comment.
Suggestion — redundant per-location Learn lookups. enrichWithLearnLesson is called inside the per-location loop, so a finding with N locations in a file emits N issues and issues N identical GetLesson calls (same ID/CWEs/CVEs/ecosystem). Since the Secrets path always misses the rule/ecosystem caches and falls through to a full lesson-catalog scan, each of those N calls scans the whole catalog. Resolve the lesson once per finding before the location loop and reuse the URL for every emitted issue.
Minor, non-blocking, related: learnServiceFromContext here duplicates the same DepLearnService context-read used in oss/unified_converter.go / oss/cli_scanner.go — worth a shared helper so the three copies can't drift.
— AI review
Reviewed What's solid: HTML rendering is safe — Should fix (see inline on Suggestions:
CI is red. Reviewers could not reproduce a failure in the touched packages locally ( — AI review |
PR Reviewer Guide 🔍
|
5 participants
Why
Secrets findings in the IDE had no Snyk Learn JIT link on the issue card (unlike SAST/OSS). This adds CWE-798 lesson lookup during scan and product-aligned panel copy so IDE parity matches the web inventory work in EDU-4753 / app-ui#5425.
Jira: EDU-4754. Learn code actions (editor lightbulb) are out of scope.
Product review (Waleed / Jakob)
What you're reviewing: the Secrets issue details panel in the IDE (HTML from snyk-ls when you open a finding from the tree). Not the web inventory drawer (app-ui#5425 / #5426).
What changed for users
N LOCATIONS IN THIS FILE.Previews: LS HTML with simulated VS Code Dark+ theme. Final spacing/fonts depend on the IDE webview.
Visuals
IDE vs web (EDU-4753)
Out of scope
Product sign-off checklist
Optional live check
Engineering details (for snyk-ls reviewers)
Summary
CWE-keyed lookup resolves the published "Hardcoded secrets" lesson for findings tagged with CWE-798 (first CWE only, same as SAST/OSS).
learn.Service.GetLessonfortypes.SecretsIssueinlessonsLookupParams.Issue.LessonUrlinFindingsConverter.findingToIssues(single conversion pass).secrets.Scanner.enrichContextso autosave / background scans getLessonUrl, not onlyworkspace/executeCommandscans.SecretsIssue = 5in README forsnyk.getLearnLesson/snyk.openLearnLesson.LocationsCountfor the multi-location banner.Net diff vs
main:infrastructure/learn/*,infrastructure/secrets/*,domain/ide/command/*_learn_lesson_test.go,application/di/init.go,README.md,docs/ui-rendering.md.Fail-soft: Learn lookup errors, nil lessons, or missing learn service leave
LessonUrlempty. Scans never fail because of Learn.Companion work
Notes for the reviewer
FindingsConverter.findingToIssues.Test plan
make lint— 0 issuesmake test— pass (infrastructure/learn,infrastructure/secrets,domain/ide/command)lessonsLookupParamsforSecretsIssue—infrastructure/learn/service_test.goconvert_test.go,secrets_test.go,secrets_html_test.gogetLearnLesson/openLearnLessonacceptissueType=5docs/ui-rendering.md(Secrets Issue Details Panel section)Backwards compatibility
types.SecretsIssueordinal is5(already on main).LS_PROTOCOL_VERSIONunchanged.Checklist
make generate)make lint)SecretsIssue = 5)docs/ui-rendering.md)