Clear the 6 open dependency advisories, and decide what the audit job gates #81

Closed
opened 2026-08-31 03:13:10 +00:00 by claude-bot · 5 comments
Contributor

Two related problems surfaced on 2026-08-30 while unblocking the Renovate queue.

1. Six open advisories on main

From the Dependency audit job on run #7625 (43d3a32) — 6 vulnerabilities (2 moderate, 4 high):

Package Severity Route Fix
brace-expansion high x3 transitive (@typescript-eslint/typescript-estree, root) in-range
js-yaml 4.0.0-4.3.0 high transitive via @eslint/eslintrc in-range
nanoid <=3.3.17 high x2 transitive in-range
postcss <=8.5.22 high x2 transitive in-range
react-router 6.0.0-7.17.0 moderate x2 direct (react-router-dom ^6.26.2) needs v7 major

pip-audit reports nothing — this is entirely npm-side.

Remediation route

The four high transitive ones are all in-range, so #39 (lock file maintenance) and #38 (npm non-major) clear them once rebased onto current main.

react-router is the only one needing judgement. The advisory range covers all of 6.x, so the non-major bump already in #38 does not fix it — it needs react-router-dom v7. That major was gated behind dependencyDashboardApproval in renovate.json; approved on the dashboard (#34) on 2026-08-30, along with a rebase-all.

The migration should be small. Total router usage is 4 files and 5 symbols, all still exported by react-router-dom v7:

  • src/App.tsxBrowserRouter, Route, Routes
  • src/components/layout/Header.tsxLink, useLocation
  • src/pages/AuditLogPage.tsxLink
  • src/pages/SettingsPage.tsxLink

No data routers, no loaders/actions, no SSR — and one of the two advisories (deserializeErrors() SSR hydration) does not apply to this app at all.

  • #39 / #38 land, clearing brace-expansion, js-yaml, nanoid, postcss
  • react-router-dom v7 lands, clearing both moderates
  • npm audit --audit-level=high exits clean

2. continue-on-error: true does not do what the workflow intends

security-audit is marked continue-on-error: true and commented "Non-blocking for now (advisory)". On this Forgejo runner that does not hold — a failing continue-on-error job still marks the whole run failure.

Evidence: run #4506 (PR #36) had Dependency audit as the only failing job — every other job including the full E2E passed — and the run is recorded as failure. Same on #7642 (b3ce3a2), where all five other jobs are green.

Consequences:

  • Runs are red for reasons unrelated to the change under test, which trains everyone to ignore the badge.
  • This directly blocks #56 (require green CI before merge). Turning on branch protection today would block every merge until the advisories are cleared, and would re-block spontaneously whenever a new CVE is published against a transitive dep.

Decision needed (not taken here)

  • (a) Keep it a real gate. Drop the misleading continue-on-error, keep dependencies clean, accept that a newly published CVE can block unrelated merges until triaged.
  • (b) Make it genuinely advisory. || true on the audit steps so the job passes and findings stay in the log. Honest about being non-blocking, but nobody reads a green log.
  • (c) Split it out. Move the audit to its own scheduled (e.g. weekly) workflow so it reports independently and never gates a PR. Keeps the signal, removes the coupling.

Inclination is (c), with (a) reasonable if the dependency surface stays small. Whichever is chosen should land before #56.

  • Decide and implement
  • Remove the now-inaccurate "Non-blocking for now (advisory)" comment either way

Blocks: #56.

Two related problems surfaced on 2026-08-30 while unblocking the Renovate queue. ## 1. Six open advisories on `main` From the `Dependency audit` job on run #7625 (`43d3a32`) — **6 vulnerabilities (2 moderate, 4 high)**: | Package | Severity | Route | Fix | |---|---|---|---| | `brace-expansion` | high x3 | transitive (`@typescript-eslint/typescript-estree`, root) | in-range | | `js-yaml` 4.0.0-4.3.0 | high | transitive via `@eslint/eslintrc` | in-range | | `nanoid` <=3.3.17 | high x2 | transitive | in-range | | `postcss` <=8.5.22 | high x2 | transitive | in-range | | `react-router` 6.0.0-7.17.0 | moderate x2 | **direct** (`react-router-dom ^6.26.2`) | **needs v7 major** | `pip-audit` reports nothing — this is entirely npm-side. ### Remediation route The four high transitive ones are all in-range, so #39 (lock file maintenance) and #38 (npm non-major) clear them once rebased onto current `main`. `react-router` is the only one needing judgement. The advisory range covers **all of 6.x**, so the non-major bump already in #38 does not fix it — it needs `react-router-dom` v7. That major was gated behind `dependencyDashboardApproval` in `renovate.json`; **approved on the dashboard (#34) on 2026-08-30**, along with a rebase-all. The migration should be small. Total router usage is 4 files and 5 symbols, all still exported by `react-router-dom` v7: - `src/App.tsx` — `BrowserRouter`, `Route`, `Routes` - `src/components/layout/Header.tsx` — `Link`, `useLocation` - `src/pages/AuditLogPage.tsx` — `Link` - `src/pages/SettingsPage.tsx` — `Link` No data routers, no loaders/actions, no SSR — and one of the two advisories (`deserializeErrors()` SSR hydration) does not apply to this app at all. - [ ] #39 / #38 land, clearing brace-expansion, js-yaml, nanoid, postcss - [ ] react-router-dom v7 lands, clearing both moderates - [ ] `npm audit --audit-level=high` exits clean ## 2. `continue-on-error: true` does not do what the workflow intends `security-audit` is marked `continue-on-error: true` and commented *"Non-blocking for now (advisory)"*. **On this Forgejo runner that does not hold** — a failing `continue-on-error` job still marks the whole run `failure`. Evidence: run #4506 (PR #36) had `Dependency audit` as the **only** failing job — every other job including the full E2E passed — and the run is recorded as `failure`. Same on #7642 (`b3ce3a2`), where all five other jobs are green. Consequences: - Runs are red for reasons unrelated to the change under test, which trains everyone to ignore the badge. - **This directly blocks #56** (require green CI before merge). Turning on branch protection today would block *every* merge until the advisories are cleared, and would re-block spontaneously whenever a new CVE is published against a transitive dep. ### Decision needed (not taken here) - **(a) Keep it a real gate.** Drop the misleading `continue-on-error`, keep dependencies clean, accept that a newly published CVE can block unrelated merges until triaged. - **(b) Make it genuinely advisory.** `|| true` on the audit steps so the job passes and findings stay in the log. Honest about being non-blocking, but nobody reads a green log. - **(c) Split it out.** Move the audit to its own scheduled (e.g. weekly) workflow so it reports independently and never gates a PR. Keeps the signal, removes the coupling. Inclination is **(c)**, with **(a)** reasonable if the dependency surface stays small. Whichever is chosen should land **before** #56. - [ ] Decide and implement - [ ] Remove the now-inaccurate "Non-blocking for now (advisory)" comment either way Blocks: #56.
claude-bot added this to the v1.0.0 milestone 2026-08-31 03:13:20 +00:00
Author
Contributor

Part 2 resolved: option (c), split it out — 124a8c4

New .forgejo/workflows/audit.yml runs pip-audit + npm audit on a weekly schedule (0 9 * * 1) plus workflow_dispatch. The security-audit job is gone from ci.yml, which is now five jobs with no continue-on-error anywhere.

Details worth recording:

  • No continue-on-error in the new workflow either. The flag was the problem — it promised something this runner does not honour. Standalone, the audit can just fail honestly; failing gates nothing because nothing depends on it.
  • Schedule is Mondays 09:00 UTC, deliberately after Renovate's before 6am on monday window, so each report reflects the state once that week's dependency PRs exist rather than the state just before them.
  • Both toolchains are set up before either audit runs, and the npm step carries if: always(). In the old job a pip-audit failure short-circuited the steps after it, so backend findings hid frontend ones. Now one run reports both stacks.
  • Forgejo runs scheduled workflows from the default branch, so this only takes effect from main — noted in the file.

#56 is unblocked. Branch protection can now require green CI without a transitive CVE blocking unrelated merges.

Part 1 (the six advisories) is still open and routed through Renovate: rebase-all and the react-router v7 approval are ticked on #34, awaiting Renovate's next run.

## Part 2 resolved: option (c), split it out — `124a8c4` New `.forgejo/workflows/audit.yml` runs `pip-audit` + `npm audit` on a weekly schedule (`0 9 * * 1`) plus `workflow_dispatch`. The `security-audit` job is gone from `ci.yml`, which is now five jobs with **no `continue-on-error` anywhere**. Details worth recording: - **No `continue-on-error` in the new workflow either.** The flag was the problem — it promised something this runner does not honour. Standalone, the audit can just fail honestly; failing gates nothing because nothing depends on it. - **Schedule is Mondays 09:00 UTC**, deliberately after Renovate's `before 6am on monday` window, so each report reflects the state once that week's dependency PRs exist rather than the state just before them. - **Both toolchains are set up before either audit runs**, and the npm step carries `if: always()`. In the old job a `pip-audit` failure short-circuited the steps after it, so backend findings hid frontend ones. Now one run reports both stacks. - Forgejo runs scheduled workflows from the default branch, so this only takes effect from `main` — noted in the file. **#56 is unblocked.** Branch protection can now require green CI without a transitive CVE blocking unrelated merges. Part 1 (the six advisories) is still open and routed through Renovate: rebase-all and the react-router v7 approval are ticked on #34, awaiting Renovate's next run.
Author
Contributor

Advisory progress

Two of the five packages are cleared:

Package Severity Status
postcss high ×2 cleared by #38 (deb3b26)
react-router moderate ×2 cleared by #87react-router-dom v7
brace-expansion high ×3 needs #39 (lock file maintenance)
js-yaml high needs #39
nanoid high ×2 needs #39

pip-audit remains clean; this was always npm-side only.

On the react-router major

It needed no source changes. The whole router surface is BrowserRouter/Route/Routes, Link and useLocation across four files, all still exported by v7 — matching the assessment in this issue when the major was approved. The full pipeline passed, including the E2E specs that exercise routing, so the upgrade is verified rather than assumed.

Worth noting one of the two advisories (GHSA-337j-9hxr-rhxg, arbitrary constructor injection via deserializeErrors() in SSR hydration) never applied to this app at all — Iris has no SSR. It is cleared regardless.

What remains

All three outstanding highs are transitive and clear together via #39 (lock file maintenance), which is currently green. It cannot merge yet only because it rewrites the whole lockfile and #35 / #36 are also queued against the same file — they have to land one at a time with a Renovate regeneration between each.

On part 2 of this issue

The audit split (124a8c4) is working as designed. The scheduled Dependency audit workflow fired on its own at 09:00 Monday (run #7729) and failed — correctly, since the advisories are real — without gating a single merge. That is precisely the behaviour this issue asked for, and it is what made #56 (branch protection) safe to enable.

## Advisory progress Two of the five packages are cleared: | Package | Severity | Status | |---|---|---| | `postcss` | high ×2 | ✅ cleared by #38 (`deb3b26`) | | `react-router` | moderate ×2 | ✅ cleared by #87 — `react-router-dom` v7 | | `brace-expansion` | high ×3 | ⏳ needs #39 (lock file maintenance) | | `js-yaml` | high | ⏳ needs #39 | | `nanoid` | high ×2 | ⏳ needs #39 | `pip-audit` remains clean; this was always npm-side only. ### On the react-router major It needed **no source changes**. The whole router surface is `BrowserRouter`/`Route`/`Routes`, `Link` and `useLocation` across four files, all still exported by v7 — matching the assessment in this issue when the major was approved. The full pipeline passed, including the E2E specs that exercise routing, so the upgrade is verified rather than assumed. Worth noting one of the two advisories (`GHSA-337j-9hxr-rhxg`, arbitrary constructor injection via `deserializeErrors()` in SSR hydration) never applied to this app at all — Iris has no SSR. It is cleared regardless. ### What remains All three outstanding highs are transitive and clear together via **#39** (lock file maintenance), which is currently green. It cannot merge yet only because it rewrites the whole lockfile and #35 / #36 are also queued against the same file — they have to land one at a time with a Renovate regeneration between each. ### On part 2 of this issue The audit split (`124a8c4`) is working as designed. The scheduled `Dependency audit` workflow fired on its own at **09:00 Monday** (run #7729) and failed — correctly, since the advisories are real — **without gating a single merge**. That is precisely the behaviour this issue asked for, and it is what made #56 (branch protection) safe to enable.
Author
Contributor

Audited 2026-09-01 by running the audit rather than reading the last report. 6 advisories → 2.

npm audit --audit-level=high against the current main lockfile, in a clean node:22 container:

brace-expansion  high ×3   (GHSA-3jxr-9vmj-r5cp, GHSA-mh99-v99m-4gvg, GHSA-rgw5-rvv9-x895)
js-yaml 4.0.0-4.3.0  high  (GHSA-5p4m-2wfm-xmqj)

2 high severity vulnerabilities
fix available via `npm audit fix`

Part 1 — advisories

  • #39 / #38 landed — both PRs are gone from the open list, and nanoid and postcss are clear.
  • react-router-dom v7 landedfrontend/package.json now pins ^7.0.0, so both moderates are cleared. This was the only one needing judgement and it's done.
  • npm audit --audit-level=high exits clean — not yet: brace-expansion (×3) and js-yaml remain.

Both survivors are transitive and in-range, npm reports a fix is available, and both arrive through the ESLint tree — js-yaml via @eslint/eslintrc, brace-expansion via @typescript-eslint/typescript-estree plus a root copy. #35 (chore(deps): update eslint) is open and mergeable and should clear both. Worth merging that and re-running before doing anything manual; npm audit fix by hand would just fight Renovate.

Part 2 — what the audit job gates

  • Decided and implemented: option (c), split it out. audit.yml now runs pip-audit + npm audit --audit-level=high weekly (Mondays 09:00 UTC, after Renovate's window) plus workflow_dispatch, with no continue-on-error anywhere — a red audit is a real signal that gates nothing.
  • The misleading "Non-blocking for now (advisory)" comment is gone — the security-audit job was removed from ci.yml entirely, and audit.yml's header documents why continue-on-error could not express "advisory" on this runner.

This issue is one merge away from closing. #56 is no longer blocked by the gating half — that's already resolved — only by the two remaining advisories if you want a clean audit before turning on branch protection.

**Audited 2026-09-01 by running the audit rather than reading the last report. 6 advisories → 2.** `npm audit --audit-level=high` against the current `main` lockfile, in a clean `node:22` container: ``` brace-expansion high ×3 (GHSA-3jxr-9vmj-r5cp, GHSA-mh99-v99m-4gvg, GHSA-rgw5-rvv9-x895) js-yaml 4.0.0-4.3.0 high (GHSA-5p4m-2wfm-xmqj) 2 high severity vulnerabilities fix available via `npm audit fix` ``` ### Part 1 — advisories - [x] **#39 / #38 landed** — both PRs are gone from the open list, and `nanoid` and `postcss` are clear. - [x] **`react-router-dom` v7 landed** — `frontend/package.json` now pins `^7.0.0`, so **both moderates are cleared**. This was the only one needing judgement and it's done. - [ ] **`npm audit --audit-level=high` exits clean** — not yet: `brace-expansion` (×3) and `js-yaml` remain. Both survivors are transitive and **in-range**, npm reports a fix is available, and both arrive through the ESLint tree — `js-yaml` via `@eslint/eslintrc`, `brace-expansion` via `@typescript-eslint/typescript-estree` plus a root copy. **#35 (`chore(deps): update eslint`) is open and mergeable and should clear both.** Worth merging that and re-running before doing anything manual; `npm audit fix` by hand would just fight Renovate. ### Part 2 — what the audit job gates - [x] **Decided and implemented: option (c), split it out.** `audit.yml` now runs `pip-audit` + `npm audit --audit-level=high` weekly (Mondays 09:00 UTC, after Renovate's window) plus `workflow_dispatch`, with no `continue-on-error` anywhere — a red audit is a real signal that gates nothing. - [x] **The misleading "Non-blocking for now (advisory)" comment is gone** — the `security-audit` job was removed from `ci.yml` entirely, and `audit.yml`'s header documents *why* `continue-on-error` could not express "advisory" on this runner. **This issue is one merge away from closing.** #56 is no longer blocked by the gating half — that's already resolved — only by the two remaining advisories if you want a clean audit before turning on branch protection.
Author
Contributor

Correcting my previous comment: #35 is merged, and it does not clear the remaining two. I said this issue was "one merge away" — that was wrong, and I only found out by test-merging #35 onto current main and re-running the audit rather than assuming.

npm audit --audit-level=high with #35 applied — still 2 high. But the picture underneath changed, and the dependency paths explain why:

$ npm ls brace-expansion
iris-frontend@0.1.0
+-- eslint@9.39.5
| `-- minimatch@3.1.5
|   `-- brace-expansion@1.1.14      <-- affected (<=1.1.17)
`-- typescript-eslint@8.68.0
  `-- @typescript-eslint/typescript-estree@8.68.0
    `-- minimatch@10.2.6
      `-- brace-expansion@5.0.9      <-- clean, fixed by #35

$ npm ls js-yaml
iris-frontend@0.1.0
`-- eslint@9.39.5
  `-- @eslint/eslintrc@3.3.6
    `-- js-yaml@4.3.0                <-- affected (4.0.0 - 4.3.0)

So #35 did fix the typescript-estree half of brace-expansion. What survives is entirely inside eslint's own dependency tree, at the latest published eslint (9.39.5): an old minimatch@3 and an @eslint/eslintrc that pins js-yaml@4.3.0.

That's the important correction: Renovate cannot fix these. There is no newer eslint to bump to. Waiting for the dependency bot is waiting for upstream eslint to drop minimatch@3 and bump @eslint/eslintrc — indefinite.

What would actually close this

Patched versions exist and are in-range for their parents:

  • brace-expansion@1.1.18 (advisory is <=1.1.17; minimatch@3 wants ^1.1.7, so 1.1.18 satisfies it)
  • js-yaml@4.3.2 (advisory is 4.0.0 - 4.3.0)

So an npm overrides block in frontend/package.json, scoped by version so it doesn't clobber the healthy brace-expansion@5:

"overrides": {
  "brace-expansion@1": "1.1.18",
  "js-yaml@4": "4.3.2"
}

I have not done this — it's a judgement call that belongs to whoever owns the dependency policy, not a mechanical fix:

  • It pins transitive deps of a package we don't control, and overrides are invisible in npm ls output, so they're easy to forget.
  • They need removing once eslint ships the fix upstream, or they'll silently hold back a legitimate bump.
  • Renovate does not manage overrides, so they go stale with no PR to remind anyone.

The alternative is to accept two DoS-class advisories in dev-only tooling (eslint is not shipped to the browser) and let them clear whenever eslint updates. Given the attack surface is a linter running on our own source, that's defensible — but it means npm audit --audit-level=high stays red, which matters for #56 if you want a clean audit before turning on branch protection.

Recommendation: add the overrides with a comment naming the advisories and the condition for removal. It's the only route that gets a clean audit this side of upstream.

Updated checklist for part 1:

  • #39 / #38 landed — nanoid, postcss clear
  • react-router-dom v7 landed — both moderates clear
  • #35 landed — clears the typescript-estree route to brace-expansion
  • npm audit --audit-level=high exits clean — needs the overrides decision above; not reachable via Renovate
**Correcting my previous comment: #35 is merged, and it does *not* clear the remaining two.** I said this issue was "one merge away" — that was wrong, and I only found out by test-merging #35 onto current `main` and re-running the audit rather than assuming. `npm audit --audit-level=high` with #35 applied — still 2 high. But the picture underneath changed, and the dependency paths explain why: ``` $ npm ls brace-expansion iris-frontend@0.1.0 +-- eslint@9.39.5 | `-- minimatch@3.1.5 | `-- brace-expansion@1.1.14 <-- affected (<=1.1.17) `-- typescript-eslint@8.68.0 `-- @typescript-eslint/typescript-estree@8.68.0 `-- minimatch@10.2.6 `-- brace-expansion@5.0.9 <-- clean, fixed by #35 $ npm ls js-yaml iris-frontend@0.1.0 `-- eslint@9.39.5 `-- @eslint/eslintrc@3.3.6 `-- js-yaml@4.3.0 <-- affected (4.0.0 - 4.3.0) ``` So #35 **did** fix the `typescript-estree` half of `brace-expansion`. What survives is entirely **inside `eslint`'s own dependency tree, at the latest published eslint (9.39.5)**: an old `minimatch@3` and an `@eslint/eslintrc` that pins `js-yaml@4.3.0`. That's the important correction: **Renovate cannot fix these.** There is no newer eslint to bump to. Waiting for the dependency bot is waiting for upstream eslint to drop `minimatch@3` and bump `@eslint/eslintrc` — indefinite. ### What would actually close this Patched versions exist and are in-range for their parents: - `brace-expansion@1.1.18` (advisory is `<=1.1.17`; `minimatch@3` wants `^1.1.7`, so 1.1.18 satisfies it) - `js-yaml@4.3.2` (advisory is `4.0.0 - 4.3.0`) So an npm `overrides` block in `frontend/package.json`, scoped by version so it doesn't clobber the healthy `brace-expansion@5`: ```json "overrides": { "brace-expansion@1": "1.1.18", "js-yaml@4": "4.3.2" } ``` I have **not** done this — it's a judgement call that belongs to whoever owns the dependency policy, not a mechanical fix: - It pins transitive deps of a package we don't control, and overrides are invisible in `npm ls` output, so they're easy to forget. - They need removing once eslint ships the fix upstream, or they'll silently hold back a legitimate bump. - Renovate does not manage `overrides`, so they go stale with no PR to remind anyone. The alternative is to accept two DoS-class advisories in **dev-only tooling** (eslint is not shipped to the browser) and let them clear whenever eslint updates. Given the attack surface is a linter running on our own source, that's defensible — but it means `npm audit --audit-level=high` stays red, which matters for #56 if you want a clean audit before turning on branch protection. **Recommendation:** add the overrides with a comment naming the advisories and the condition for removal. It's the only route that gets a clean audit this side of upstream. Updated checklist for part 1: - [x] #39 / #38 landed — `nanoid`, `postcss` clear - [x] `react-router-dom` v7 landed — both moderates clear - [x] #35 landed — clears the `typescript-estree` route to `brace-expansion` - [ ] `npm audit --audit-level=high` exits clean — needs the overrides decision above; **not reachable via Renovate**
Author
Contributor

Done — #102 merged. npm audit --audit-level=high reports found 0 vulnerabilities.

Final state of both halves:

Part 1 — the six advisories

  • #39 / #38 landed — nanoid, postcss clear
  • react-router-dom v7 landed — both moderates clear
  • #35 landed — cleared the typescript-estree route to brace-expansion
  • npm audit --audit-level=high exits clean — the last two needed overrides (#102), since they lived inside eslint's own tree at the latest eslint and no dependency PR could reach them

Part 2 — what the audit job gates

  • Decided and implemented: option (c), split into audit.yml — weekly + workflow_dispatch, no continue-on-error, gates nothing
  • The misleading "Non-blocking for now (advisory)" comment is gone with the job it described

#56 is now unblocked on both counts — the gating coupling is removed and the audit is clean, so requiring green CI won't be blocked by pre-existing advisories.

Carrying forward: #101 tracks removing the two pins once eslint fixes them upstream. Renovate doesn't manage overrides, so that issue and the CONTRIBUTING.md note are the only things that will remind anyone.

Closing.

**Done — #102 merged. `npm audit --audit-level=high` reports `found 0 vulnerabilities`.** Final state of both halves: **Part 1 — the six advisories** - [x] #39 / #38 landed — `nanoid`, `postcss` clear - [x] `react-router-dom` v7 landed — both moderates clear - [x] #35 landed — cleared the `typescript-estree` route to `brace-expansion` - [x] `npm audit --audit-level=high` exits clean — the last two needed `overrides` (#102), since they lived inside eslint's own tree at the latest eslint and no dependency PR could reach them **Part 2 — what the audit job gates** - [x] Decided and implemented: option **(c)**, split into `audit.yml` — weekly + `workflow_dispatch`, no `continue-on-error`, gates nothing - [x] The misleading "Non-blocking for now (advisory)" comment is gone with the job it described **#56 is now unblocked on both counts** — the gating coupling is removed *and* the audit is clean, so requiring green CI won't be blocked by pre-existing advisories. Carrying forward: **#101** tracks removing the two pins once eslint fixes them upstream. Renovate doesn't manage `overrides`, so that issue and the `CONTRIBUTING.md` note are the only things that will remind anyone. Closing.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rbrooks/Iris-WLED#81
No description provided.