Skip to content

Ten CodeQL alerts in source have no owner: one is a real codegen defect, the rest need a disposition #465

Description

@50rayn

CodeQL's first run produced 17 alerts. #454 took the seven actions/missing-workflow-permissions ones. These are the other ten, and nothing covers them.

Nine are in packages we publish; one is in an example. Five are rated high.

# sev rule where
14, 15 high js/incomplete-sanitization poveste/src/node/vite.ts:409, :413
16 high js/incomplete-sanitization plugin-vue/src/client/codegen.ts:172
8 high js/polynomial-redos plugin-nuxt/src/index.ts:49
9 high js/polynomial-redos plugin-vue/src/client/codegen.ts:379
10, 12, 13 medium js/bad-code-sanitization virtual/resolved-markdown-files.ts:5, virtual/resolved-support-plugins-collect.ts:8, plugin-vue/src/index.node.ts:47
11 medium js/bad-code-sanitization virtual/resolved-support-plugins-client.ts:8
17 medium js/clear-text-cookie examples/sveltekit/src/hooks.server.ts:15

The severities are the rule's, not this codebase's. Read individually they sort into three groups, and the middle group is the interesting one.

One is a real defect, and it is not a security defect

#16, codegen.ts:172. Copyable-source generation escapes a function's own text:

let code = cleanupExpression(value.toString().replace(/'/g, '\\\'').replace(/"/g, '\''))

The result is interpolated into an HTML-style attribute a few lines down — ${directive}${arg}="${serialized[0]}" — so the second replace exists to stop a double quote closing that attribute, while the first is a JavaScript string escape. Two different escaping jobs in one expression, and neither is complete: backslashes are never escaped, and they are escaped after nothing rather than before the quotes, which is the ordering CodeQL is naming.

Nothing here is exploitable — it is the user's own book rendering the user's own handler. But a handler containing a regex literal, a Windows path or an escaped quote produces copy-source that is wrong, in the feature whose entire purpose is being copied. Worth tracing to a concrete wrong output before rewriting the escaping, because the right fix depends on which of the two jobs the string is actually doing at that point.

#11, resolved-support-plugins-client.ts:8 is a smaller one of the same kind, and visible in a single line:

`'${p.id}': () => import(${JSON.stringify(require.resolve(...))})`

The import path is escaped properly. The key beside it is not — p.id goes in raw between two quotes. resolved-support-plugins-collect.ts:8 has the identical line. Plugin ids are ours today (vue3, svelte5), so nothing is broken; the inconsistency is one character of fix and removes two alerts.

Two are the pattern with no reachable input

#14, #15 (vite.ts:409, :413) build a RegExp from name.replace(/\//g, '\\/'), where name comes from a seven-entry array literal in the same file — floating-vue, @iconify/vue, pinia and friends. CodeQL is right that the escaping is incomplete and wrong that it matters: the inputs are constants. Either escape properly or stop building these aliases with a regex at all, since a literal string alias would do.

#9 (codegen.ts:379) runs /([^/]+)\.vue$/ over vnode.type.__file. Polynomial backtracking on a path with many dots — a filename, from the developer's own project.

One processes generated input, which is why it is the pick of the ReDoS pair

#8, plugin-nuxt/src/index.ts:49 — the tolerant-boot wrapper (#277):

source.replace(/export default (\[[\s\S]*?\])/, 'export default $1.map(__povesteTolerant)')

source is Nuxt's generated plugins.client.mjs, so unlike every other alert here the input is machine-produced and scales with the number of Nuxt plugins and modules a project has. Still not attacker-controlled, and still the one where the input is neither a constant nor something a human typed.

One is example code, which is the argument for fixing it

#17 is the stock SvelteKit demo hook, setting a userid cookie with httpOnly and no secure. It ships in no package. It does sit in a directory people copy from, and the fix is one property.

What to do with them

The disposition matters more than the fixes, and this is the same trap as #307: an alert list nobody rules on becomes a list nobody reads, and the next real finding arrives into a queue of seventeen. Every one of these should end as either a commit or a dismissal with a reason — used in tests, false positive, won't fix — so the queue reads as zero and the next alert is visible.

Suggested split:

#10 and #13 both construct code through JSON.stringify, which is the correct tool for the job; if they are dismissed, false positive is the honest reason rather than won't fix.

Acceptance

  • The code scanning queue reads zero open, with every alert either fixed or dismissed and a reason recorded
  • chore(toolchain): bump to Node 26 and pnpm 11 #16 has a before/after showing generated source that was wrong and now is not
  • No alert is dismissed as false positive without a sentence saying why in the dismissal comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    a:nodeCollector, builder, config, CLIa:pluginsPlugin API and official pluginssprint:2-harnessMake the test harness trustworthy; precondition for grid perf

    Type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions