fix: resolve SonarQube code smells, harden commit parsing against ReDoS, and split lint CI - #513
Merged
Merged
Conversation
Linearize the revert-commit regex (O(n²) → O(n), exported as REVERT_PATTERN with a regression test), reject NaN at input-parse time, unify paginated API option defaults via spread, build Windows paths with win32.join, and adopt dedicated matchers, it.each tables, and hook-first ordering in tests. Document the conventions in CLAUDE.md and .claude/rules/tests.md.
One job per linter (format, code, types, text, actions, secrets) under a renamed Lint workflow, with an aggregate Lint gate preserving the previous required-check name. Title Case step names, named checkout steps, and trigger/permission consistency across workflows.
Commit messages are PR-author-controlled and semver-mode defaults to conventional-commits, so super-linear parsing was an attacker-triggerable CI stall. Three measured defects addressed: - Linearize the preset revertPattern (O(n²) on whitespace runs, ~14s at 100k chars → <1ms), exported as REVERT_PATTERN with a regression test - Disable issuePrefixes: the unused issue-references regex the library builds from it took 10s on a single 2k-char line; consumed outputs are verified identical with it off - Digest-gate messages over 16k past the library's quadratic no-match stub, parsing only the header and first BREAKING CHANGE footer so oversized messages keep identical type/scope/subject/breaking results Also folds structurally identical tests into it.each tables and adds pathological-input coverage that surfaces any regression as a timeout.
virgofx
changed the title
virgofx
marked this pull request as ready for review
August 8, 2026 20:55
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability and security of the Terraform Module Releaser GitHub Action by addressing SonarQube code smells, hardening conventional-commit parsing against ReDoS vectors in PR-controlled commit messages, and restructuring CI linting into clearer per-linter jobs while documenting the conventions behind these changes.
Changes:
- Hardened
conventional-commits-parserusage and added regression tests for pathological commit-message inputs to prevent CI stalls (ReDoS). - Refactored various utilities and tests to satisfy SonarQube guidance (e.g.,
replaceAll, dedicated matchers,it.each, avoiding object-literal default params). - Split the Lint workflow into separate jobs with an aggregate gate to preserve the single required check name.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wiki.ts | Uses replaceAll and improves regex escaping implementation consistency. |
| src/utils/string.ts | Simplifies renderTemplate placeholder substitution using ?? and replaceAll. |
| src/utils/metadata.ts | Adds number-input validation to prevent NaN from propagating into config. |
| src/types/metadata.types.ts | Updates action-input metadata documentation for number parsing behavior. |
| src/terraform-module.ts | Uses a single multi-argument push() and replaceAll for normalization steps. |
| src/terraform-docs.ts | Builds Windows paths with win32.join instead of hand-escaped separators. |
| src/tags.ts | Adjusts pagination option defaults by merging overrides into { per_page: 100, page: 1 }. |
| src/releases.ts | Removes object-literal default param and merges pagination overrides over defaults. |
| src/config.ts | Uses .includes() and removes now-redundant NaN check post input-validation change. |
| src/commit-analyzer.ts | Introduces linear-time revert pattern + parse-safe message digesting to mitigate ReDoS. |
| CLAUDE.md | Codifies code-quality conventions used to resolve SonarQube findings and prevent regressions. |
| .github/workflows/test.yml | Aligns triggers/permissions and normalizes step naming/IDs for clarity. |
| .github/workflows/release.yml | Normalizes job/step naming for consistency and clearer workflow UI. |
| .github/workflows/release-start.yml | Normalizes job/step naming and improves clarity of step intent. |
| .github/workflows/lint.yml | Splits linting into per-linter jobs plus an aggregate “Lint” gate job. |
| .github/workflows/check-dist.yml | Reorders step fields (name/id/if) without changing behavior. |
| .claude/rules/tests.md | Adds explicit testing conventions (matchers, it.each, hook ordering). |
| tests/wiki.test.ts | Moves hooks above tests and uses dedicated matchers (toHaveLength). |
| tests/utils/string.test.ts | Refactors renderTemplate tests into an it.each table. |
| tests/utils/file.test.ts | Replaces boolean-style assertions with dedicated matchers. |
| tests/terraform-module.test.ts | Converts repeated release-version tests into a parameterized table. |
| tests/tags.test.ts | Updates assertions to dedicated matchers and length matchers. |
| tests/releases.test.ts | Updates assertions to dedicated matchers and length matchers. |
| tests/pull-request.test.ts | Replaces instanceof boolean assertions with toBeInstanceOf matchers. |
| tests/devcontainer.test.ts | Uses not.toBeNaN() matcher for clearer failures. |
| tests/config.test.ts | Updates expected error to reflect new input-parse-time rejection behavior. |
| tests/commit-analyzer.test.ts | Adds pathological-input tests to enforce linear-time parsing behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…e complexity SonarQube flagged createConfigFromInputs at 18 (limit 15) on the PR after the NaN validation landed inside the nested type chain. Move the type-specific branches into a getInputValue helper with early returns; the loop now only maps inputs to config keys and wraps errors. Behavior and error messages are unchanged and covered by the existing config/metadata tests.
parseInt accepted partially numeric values ('123abc' → 123, '1.5' → 1,
'1O' → 1), silently misconfiguring the action while the metadata docs
claimed non-numeric values were rejected. Validate against /^[+-]?\d+$/
before parsing so malformed values fail loudly at startup with the input
name in the error. getInput() already trims, so no explicit trim is
needed; a leading '+' remains accepted for parity with parseInt.
📋 Release Plan
📝 Changelog
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Clears every open SonarQube code smell, fixes two measured ReDoS defects reachable through PR commit messages, splits the Lint workflow into per-linter jobs, and codifies the underlying conventions in
CLAUDE.md/.claude/rules/tests.mdso these classes of findings don't recur.ReDoS hardening (
src/commit-analyzer.ts)Commit messages are PR-author-controlled and
semver-modedefaults toconventional-commits, so super-linear parsing was an attacker-triggerable CI stall for any repo using this action. Three measured defects:revertPatternbacktracks on whitespace runs ([\s\S]+?"?\s*ambiguity)REVERT_PATTERN([\s\S]*?[^"\s]) — <1ms @ 100k, verified behavior-identical on realistic revert messagesconventional-commits-parserissue-references regex ((?:.*?)??\s*([\w-.\/]*?)??(#)…), built from the library-defaultissuePrefixes: ['#']and run on every lineissuePrefixes: undefined— the references output is unused by this action; consumed outputs (type/scope/subject/breaking) verified byte-identical across a corpus/(?!.*)/is itself O(n²)BREAKING CHANGE:footer line) before parsing — preserves all consumed outputs, unlike naive truncation which could silently drop a late breaking footer (a missed major bump)Five new pathological-input tests turn any regression into a test timeout. Filing an upstream issue against
conventional-commits-parseris recommended follow-up (their stub fix is/(?!)/).SonarQube findings
toHaveLength,toBeInstanceOf/.not.toBeInstanceOf,toBeNaN(plus a repo-wide sweep of the same patterns, e.g.Array.isArray(x)).toBe(true))commit-analyzer/terraform-module/stringfolded intoit.eachtables (full 21-caserenderTemplatetable), plus the adjacent non-conventional-message clusterwiki.test.ts.includes('*.tf')over.some()equality (config.ts); single multi-argpush()(terraform-module.ts);??over ternary (string.ts);replaceAllover global-regexreplace(wiki.ts,string.ts,terraform-module.ts)releases.ts): nowoptions?spread over inline defaults — also fixes the real pitfall wheregetAllReleases({ page: 2 })silently droppedper_page: 100;getAllTagsunified on the same idiomterraform-docs.ts):win32.joininstead of hand-escaped separators — eliminates the flagged\\escapes rather than prettifying themmetadata.ts): number inputs now fail fast at parse time with the input name in the error; the downstreamNumber.isNaNcheck was masking this for the single current number input but any future one would have letNaNthrough silentlyCI
lint.ymlrenamed Lint and split into per-linter jobs — Format, Code, Types, Text, Actions, Secrets — for a cleaner check listingLintgate job (needs:all six, fails on failed/cancelled/skipped) preserves the previous single required-check name —Lint, review it; the gate can be dropped in favor of requiring the granular jobstest.ymljob key/permissions/trigger alignment, stale "master" comment removed; PR-facing job display names (Check dist,TypeScript Tests) deliberately untouched to avoid stranding required checksConventions
New Code quality section in
CLAUDE.md(linear regexes, preferred idioms, no object-literal defaults, boundary validation) and matcher/it.each/hook rules in.claude/rules/tests.md.Verification
Biome,
tsc --noEmit, textlint, and Prettier all clean; 797 tests pass (+8 new)