diff --git a/.agents/skills/dg-assess-security-advisories/SKILL.md b/.agents/skills/dg-assess-security-advisories/SKILL.md new file mode 100644 index 000000000..09f5b5d19 --- /dev/null +++ b/.agents/skills/dg-assess-security-advisories/SKILL.md @@ -0,0 +1,31 @@ +--- +name: dg-assess-security-advisories +description: Assess Discourse Graphs Dependabot alerts for actual exposure, produce evidence-backed reports, and carry out approved dismissal or remediation decisions. Use for individual advisories or review batches. +--- + +# Assess security advisories + +Explain whether an advisory affects our system, what an attacker would need, and what we should do. Package severity and installation alone do not establish exposure. + +Use `$dg-engineering-writing-style` for explanations and `$dg-create-engineering-ticket` when a ticket is requested. Preserve the user's scope: a request for a count, ticket, explanation, or dry run does not authorize a full assessment or GitHub mutations. + +## Assess + +1. For a batch, snapshot authenticated GitHub alerts and record the selection rule. Exclude prior reviewed alert numbers when the user asks for the next batch. Preserve duplicate advisories across manifests as separate entries. +2. Pin evidence to the reviewed main commit. Follow the vulnerable function from its workspace dependency path to actual callers, inputs, and outputs. Read [assessment.md](references/assessment.md) for the evidence checklist and parent-package investigation. +3. Classify the result as unused vulnerable code, exposure proposed for risk acceptance, routine remediation, product remediation, or unresolved. State confidence and what has not been tested. Do not convert uncertainty into a dismissal. +4. Produce the requested explanation or report. For batches, use [templates.md](references/templates.md) and the [HTML template](assets/report.html). Save artifacts under the repository's `local/` directory, not an operating-system `/local` directory. + +## Decide and act + +Present a concrete recommendation before requesting a decision. Honor approval already given for the same action and batch; do not ask twice. Do not infer approval for a different alert from a previous dismissal. + +Before an approved dismissal, remediation PR, or closure, read [decisions.md](references/decisions.md). It covers rechecking evidence, choosing the dismissal reason, recording comments, verifying results, and closing the assessment without misrepresenting deferred risks as fixed. + +The default is an assessment, not an automatic upgrade or dismissal. Michael has accepted some tooling risk, but that historical decision is not blanket approval. A dependency override also has maintenance cost; explain why it is needed and when it can be removed. + +## Examples and validation + +Read [cases.md](references/cases.md) when an example helps explain a decision or when validating changes to this skill. Its three historical cases cover an unused vulnerable function, accepted tooling risk, and shared content reaching a parser. They are fixtures, not current security determinations. + +A dry run produces local recommendations only. It must not post comments, dismiss alerts, change dependencies, or open tickets/PRs. Validate the reasoning against the evidence, not by matching exact wording. diff --git a/.agents/skills/dg-assess-security-advisories/agents/openai.yaml b/.agents/skills/dg-assess-security-advisories/agents/openai.yaml new file mode 100644 index 000000000..741236b02 --- /dev/null +++ b/.agents/skills/dg-assess-security-advisories/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Assess DG security advisories" + short_description: "Assess actual exposure and record approved decisions" + default_prompt: "Use $dg-assess-security-advisories to assess the selected Dependabot alerts and recommend actions based on our actual usage." diff --git a/.agents/skills/dg-assess-security-advisories/assets/report.html b/.agents/skills/dg-assess-security-advisories/assets/report.html new file mode 100644 index 000000000..52e10cbad --- /dev/null +++ b/.agents/skills/dg-assess-security-advisories/assets/report.html @@ -0,0 +1,187 @@ + + + + + + Security advisory assessment + + + +
+

Security advisory assessment

+

Repository: REPLACE_REPOSITORY. Reviewed main: REPLACE_SHA.

+

Assessment snapshot: REPLACE_TIME. Selection: REPLACE_SELECTION.

+

+ Open repository totals: REPLACE_SEVERITY_COUNTS. Batch counts and + recommendations: REPLACE_BATCH_SUMMARY. +

+

+ Decision record: REPLACE_TICKET_LINK. Last outcome verification: + REPLACE_VERIFICATION_TIME. +

+
+ + +

+
+ +
+

REPLACE_ALERT_NUMBER: REPLACE_PACKAGE

+

REPLACE_PLAIN_LANGUAGE_IMPACT

+
+
Advisory and snapshot state
+
REPLACE_GHSA_SEVERITY_STATE_AND_HTTPS_LINK
+
Versions and manifest
+
REPLACE_INSTALLED_AFFECTED_PATCHED_MANIFEST
+
Dependency path and environment
+
REPLACE_WORKSPACE_PARENTS_AND_USAGE
+
Call, input, and attacker prerequisites
+
REPLACE_SOURCE_LINKS_ARGUMENTS_AND_USER_ACTION
+
Evidence and limitations
+
REPLACE_OBSERVATION_CONFIDENCE_AND_NOT_TESTED
+
Parent packages and options
+
REPLACE_LATEST_PARENT_PIN_RANGE_AND_OPTIONS
+
Recommendation
+
REPLACE_ACTION_AND_REASON
+
Proposed dismissal
+
+ REPLACE_REASON_OR_NOT_APPLICABLE +
REPLACE_PROPOSED_COMMENT
+
+
Owner decision and execution
+
+ REPLACE_PENDING_OR_APPROVED_DECISION_ACTUAL_STATE_AND_VERIFICATION +
+
Follow-up and revisit condition
+
REPLACE_LINK_AND_CONDITION
+
+ Back to top +
+
+ + + diff --git a/.agents/skills/dg-assess-security-advisories/references/assessment.md b/.agents/skills/dg-assess-security-advisories/references/assessment.md new file mode 100644 index 000000000..00a75924d --- /dev/null +++ b/.agents/skills/dg-assess-security-advisories/references/assessment.md @@ -0,0 +1,68 @@ +# Assessment procedure + +## Snapshot and selection + +Use an authenticated GitHub connector or CLI for the requested repository. Paginate all results; do not count only the first page. Record open totals by severity, snapshot time, and exact selected IDs. + +```sh +gh api --paginate --slurp 'repos/DiscourseGraphs/discourse-graph/dependabot/alerts?state=open&per_page=100' +``` + +`--slurp` produces an array of pages. Flatten before filtering. Some gh versions do not combine `--slurp` with `--jq`; sort locally when needed. On Windows, use explicit UTF-8 when saving JSON rather than relying on shell redirection defaults. + +Use the user's requested ordering. If none is supplied, the previous batch convention is descending alert number within severity. Say this is a batch order, not an exploitability ranking. Exclude prior reviewed numbers, including reviewed alerts still open, when selecting the next batch. + +Record each alert's number, URL, state, GHSA, package, manifest, vulnerable range, and first patched version. Resolve installed versions from the lockfile. Preserve duplicate entries across manifests and major versions, while sharing evidence where justified. + +## Reachability evidence + +Record the main commit SHA and source links pinned to it. Distinguish the checked-out branch from main and report relevant differences. Never silently reuse a stale local installation as evidence of main's dependency graph. + +For each alert, answer: + +| Question | Evidence to collect | +| ----------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Why is the package installed? | Workspace root, direct parent, complete transitive path, dependency category, and resolved versions. Include auto-installed peers; `pnpm why` alone may miss them. | +| What is vulnerable? | Advisory mechanism, affected function/options, input requirements, and first patched version. Read the affected implementation where needed. | +| Do we execute that function? | App/build entry points, callers and arguments, imports, feature configuration, and relevant dependency source. An import or devDependency label alone is insufficient. | +| Who controls the input? | Customer, group member, repository contributor, build configuration, downloaded archive source, or fixed code values. Follow input through transformations to the vulnerable call and relevant output sink. | +| What could happen? | Concrete confidentiality, integrity, or availability impact in that process. Do not infer data theft from a CPU-exhaustion advisory. | +| What remains uncertain? | Missing source, unavailable environment, assumptions, confidence, and the smallest verification step that would change the decision. | + +Check host-provided globals and build externalization. For example, upgrading a package does not replace `window.CryptoJS` if the Roam build maps imports to that global. Distinguish package code from a host-loaded implementation. + +Separate product runtime, developer/build/install tooling, and unused paths. Tooling can still process hostile input; it is not automatically safe. For archive extraction, identify the archive source and extraction method. For glob denial of service, distinguish a malicious pattern from a filename matched against a fixed pattern. + +For shared content, record membership controls and recipient actions. Admin-approved membership narrows access but does not prevent a malicious or compromised member from submitting executable content. Keep a local parser reproduction separate from a complete cross-user exploit in the host app. + +Use harmless, bounded probes in a local scratch directory. Avoid exhaustion payloads, production mutations, or testing against other users. State exactly what the probe proves. + +## Parent versions and alternatives + +Inspect both the installed parent's package manifest and current published metadata. Check whether the latest parent release actually changes or removes the affected dependency; a newer parent is not proof of a fix. + +```sh +npm view @latest version dependencies optionalDependencies engines --json +``` + +Follow optional/platform packages when a packaging change may move the dependency. Determine whether we use the parent itself, just a different feature of the parent, or neither. + +Compare these options: + +1. Refresh targeted lockfile resolutions if existing parent ranges allow the patched version. +2. Upgrade a used parent, with validation of its affected workflows. +3. Remove an unused direct dependency if its consumers and scope justify that change. +4. Use a version-scoped override only when necessary and authorized. +5. Recommend no action or explicit acceptance of residual risk with evidence. + +Exact pins and compatible ranges require different explanations. An override installs a published replacement package; it is not a source patch. Scope it to affected versions within an existing major when compatible. Record the parent, reason, and removal check: remove the override, refresh relevant lockfile entries, confirm affected versions do not return, and validate. Broad parent ranges can keep matching an override and hold the selected version; it does not clean itself up. + +Avoid global major overrides, speculative parent upgrades, and unrelated lockfile churn. A grouped tooling PR is reasonable only when the changes are compatible, useful, and approved together. + +## Report checks + +For batch reports, include counts, the reviewed SHA, all selected alerts, source evidence, limitations, options, proposed comments, and separate final decision/state fields. A recommendation is not an executed action. + +Copy `assets/report.html` to repository `local/`, replace its example card, and create one card per selected alert. Escape untrusted advisory/source text and URLs before insertion. Use only intended HTTPS links and escaped text; never embed executable input from an advisory or note. The report itself must remain self-contained. + +Verify counts against the snapshot, search/filter/reset behavior, internal links, HTTPS link targets, browser errors, and mobile overflow. Keep raw authenticated API responses local. Put the durable rationale in the ticket so future work does not depend on ignored local files. diff --git a/.agents/skills/dg-assess-security-advisories/references/cases.md b/.agents/skills/dg-assess-security-advisories/references/cases.md new file mode 100644 index 000000000..2223b4f18 --- /dev/null +++ b/.agents/skills/dg-assess-security-advisories/references/cases.md @@ -0,0 +1,50 @@ +# Historical cases and dry-run fixtures + +These sanitized cases capture evidence from the September 2026 review at main `090ef105a1623fd16e419eeb8fee814f6d4e14d6`. They do not establish current exposure or authorize new mutations. Recheck code, dependencies, metadata, and decisions for live work. + +## Raw fixtures + +### Case A: CryptoJS #562 + +The advisory concerns weak randomness in crypto-js 3.1.9-1. The lockfile path is Roam -> roamjs-components 0.90.0 -> crypto-js. The inspected dependency consumer is ExternalLogin.js, which calls AES.decrypt. That component and its OAuth wrappers are not referenced by the app. App searches found no CryptoJS secret-generation calls. The Roam build maps crypto-js imports to window.CryptoJS. + +### Case B: tar #515 + +The advisory concerns archive decompression/parse denial of service in tar <=7.5.18. The lockfile contains tar through Supabase CLI, Vercel's @vercel/fun, and @mapbox/node-pre-gyp. Supabase's installer extracts downloaded versioned release archives. Vercel's runtime tooling can extract downloaded runtime archives. No product feature accepting customer archives was found. Michael later explicitly accepted this tooling risk and requested closure of the override PR. + +### Case C: shared Obsidian import + +The importer fetches full.text for a selected shared node and calls matter(content) before validating nodeTypeId. Its installed gray-matter 4.0.3 uses js-yaml 3.14.1 by default. A bounded local probe showed increasing CPU cost for !!omap YAML. A separate local probe returned calculated: 42 from: + +```markdown +---javascript +({ nodeTypeId: "review-probe", calculated: 6 * 7 }) +--- + +Test body +``` + +The parser's JavaScript engine evaluates the expression. A complete cross-user attack inside Obsidian was not tested. Michael stated that imports require group membership and an admin must add members. V0 ATJSON tickets preserve native Markdown imports; the v1 draft retains frontmatter and Markdown fallback. + +## Dry-run exercise + +Use only these fixtures and the skill to draft one evidence record per case, a recommendation, and any proposed dismissal text. Treat Case B's recorded approval as historical context, not permission to mutate a live alert. Produce local output only. A reviewer can give an evaluator the raw fixtures above without the criteria below to avoid supplying the expected conclusions. + +Check the resulting reasoning against these criteria: + +| Case | Required distinction | +| ---- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| A | The vulnerable randomness path was not found; AES decryption alone is not that mechanism. A peer upgrade does not replace Roam's global. A proposed not_used dismissal must be tied to the inspected path and baseline. | +| B | Archive extraction really occurs, so not_used is unsupported. Historical acceptance supports tolerable_risk for that decision, not a claim that the code is safe or fixed. | +| C | Shared content reaches parsing, with membership and import-selection prerequisites. Separate YAML CPU exhaustion from JavaScript execution. Confirmed local evaluation is not proof of filesystem access, data theft, or a complete host exploit. V0 storage does not remove the path; v1 criteria must cover fallback parsing. | + +Also check that a request to draft or assess produces no external writes, and that proposed comments fit the current API limit. Record limitations rather than filling evidence gaps with assumptions. + +## Decision references + +- [ENG-2235](https://linear.app/discourse-graphs/issue/ENG-2235): assessment and decisions. Twenty unused-path dismissals were followed by five explicitly accepted tooling-risk dismissals. +- [ENG-2236](https://linear.app/discourse-graphs/issue/ENG-2236) and [PR #1409](https://github.com/DiscourseGraphs/discourse-graph/pull/1409): routine overrides passed CI, but the PR closed without merging after Michael weighed their maintenance cost. +- [ENG-1925](https://linear.app/discourse-graphs/issue/ENG-1925): v1 import follow-up requires rejection of executable frontmatter, including legacy Markdown fallback. The separate js-yaml 3.x alert #567 remained open when the assessment closed. +- [ENG-2237](https://linear.app/discourse-graphs/issue/ENG-2237): next batch, excluding prior reviewed IDs. + +The original report and probes were local-only artifacts. These fixtures intentionally preserve the decision-relevant evidence without requiring those files or embedding raw authenticated API responses. diff --git a/.agents/skills/dg-assess-security-advisories/references/decisions.md b/.agents/skills/dg-assess-security-advisories/references/decisions.md new file mode 100644 index 000000000..4a42db0db --- /dev/null +++ b/.agents/skills/dg-assess-security-advisories/references/decisions.md @@ -0,0 +1,57 @@ +# Decisions and execution + +## Choose the action + +| Finding | Recommendation | GitHub dismissal reason, if approved | +| ------------------------------------------------------------------------- | --------------------------------------------------- | ----------------------------------------------------------------- | +| Evidence shows the vulnerable function or required invocation is not used | Dismiss with the actual call-path evidence | `not_used` | +| A path exists, but the owner accepts the remaining exposure | Record the prerequisites, impact, and accepted risk | `tolerable_risk` | +| A useful tooling fix is justified | Schedule or implement the scoped update | Leave open until fixed or explicitly accepted | +| Untrusted product content reaches the vulnerable behavior | Propose a fix or mitigation and verification | Leave open unless the owner explicitly accepts this specific risk | +| Evidence is incomplete | Explain the uncertainty and next check | Do not infer dismissal | + +Low risk, unused code, fixed code, and a deferred fix are different states. Preserve the distinction in GitHub, Linear, and the report. + +## Approved dismissals + +Before mutating, confirm authorization covers the exact alerts and action. Re-read alert state/identity, GHSA, affected dependency/range, and the reviewed source baseline. If main or dependency metadata changed, determine whether the evidence still holds. Reassess material changes before applying a stale decision. + +Use an individual comment containing the input/call-path rationale, relevant assumptions, and ticket reference. GitHub accepted a maximum of 280 characters during the September 2026 review; confirm the current API constraint and validate each comment before submission. Keep the full evidence in the ticket/report. + +Example request body, only after approval: + +```json +{ + "state": "dismissed", + "dismissed_reason": "not_used", + "dismissed_comment": "No caller supplies a variable generation size; inspected calls use default/fixed positive lengths. Main . ENG-." +} +``` + +Submit sequentially through the authenticated connector or `PATCH /repos/{owner}/{repo}/dependabot/alerts/{number}`. Stop on an unexpected error rather than continuing an unverified batch. For an ambiguous response, read the alert before retrying. Never assume a failed response means no mutation occurred. + +Verify each persisted state, reason, and exact comment with fresh reads. Record successes and failures individually. Return at least one direct alert link so the user can inspect its activity history. Use the dismissal's comment field; do not promise a separate GitHub advisory discussion comment. + +## Approved remediation + +Use the engineering ticket skill to create a scoped child issue when requested. Record affected alerts, target versions, parent constraints, and excluded adjacent work. Use its generated branch name and the repository's PR conventions. + +Run the required frozen installation and repository validation. If validation fails, investigate whether the change caused it; reproduce a suspected baseline failure rather than labeling it pre-existing without evidence. Keep material validation limitations clear and wait for required GitHub checks after opening/updating the authorized PR. + +Do not broaden a small dependency update into unrelated fixes. Explain override maintenance and removal criteria before proposing it. Successful tests do not establish that a low-value override is worth maintaining. + +## Acceptance, deferral, and closure + +If the user chooses risk acceptance after a PR exists, close it without merging only when authorized. Record the decision and rationale on the original assessment issue. Close the abandoned implementation ticket using an appropriate status such as Won't Fix, without presenting it as shipped remediation. + +Mark the assessment Done when its review and decisions are complete. A deferred vulnerability may remain open with an explicit linked owner/work item. Update report decisions, comments, counts, and timestamps after execution; keep the original assessment snapshot distinct from current state. + +For deferred work, find a matching existing ticket before proposing a new one. Check its actual scope and fallback paths. Adding ATJSON storage does not remove existing Markdown parsing; acceptance criteria must cover executable frontmatter and legacy import paths explicitly. + +Create a next-batch ticket only when requested. Exclude the previous batch, state the ordering, and set the requested assignee/date in the user's timezone. A due date is not proof that a reminder notification was scheduled; report which action the available tool actually performed. + +## Operational notes + +Never expose credentials in reports, commands, or examples. Use structured connector inputs or UTF-8 request/body files for multiline text. JSON serialization is not shell escaping. In PowerShell, convert file content to a plain string before serializing it; file-content objects can carry metadata. + +If a gh PR-edit command fails on a deprecated GraphQL field, an authenticated REST update is an alternative. Verify current state before retrying any ambiguous write. Tool names and API constraints can change; use currently available documentation instead of depending on a historical CLI error. diff --git a/.agents/skills/dg-assess-security-advisories/references/templates.md b/.agents/skills/dg-assess-security-advisories/references/templates.md new file mode 100644 index 000000000..ec619adec --- /dev/null +++ b/.agents/skills/dg-assess-security-advisories/references/templates.md @@ -0,0 +1,105 @@ +# Reusable templates + +Replace angle-bracket fields with the current batch's evidence. These are drafting templates, not payloads ready to submit. Use the engineering ticket skill for current team/project/status/label selection. + +## Assessment ticket + +```markdown +## Problem + +- Assess Dependabot alerts for actual exposure. Severity alone does not establish risk to our applications. + +## Solution + +- Trace dependency paths, vulnerable calls, inputs, and current parent-package options against main . +- Save a reviewable HTML report under repository local/ with recommendations and proposed comments. + +## Done When + +- Every selected alert has evidence, prerequisites, impact, confidence, limitations, and a concrete recommended action. +- The owner has a reviewable result before any action requiring approval; no unapproved mutation is performed. + +## Out of Scope + +- + +## Notes + +- Snapshot: