Skip to content

fix(security): address the CodeQL and Scorecard findings - #32

Open
ExtraToast wants to merge 2 commits into
mainfrom
fix/security-alerts
Open

fix(security): address the CodeQL and Scorecard findings#32
ExtraToast wants to merge 2 commits into
mainfrom
fix/security-alerts

Conversation

@ExtraToast

Copy link
Copy Markdown
Contributor

Grouped by what the alerts actually were, rather than by severity badge.

Noise: 34 alerts scanning code we do not own

Every java/useless-tostring-call was in kotlin/build/generated. The Kotlin analysis compiles the client to produce a database, that compilation generates the whole client, and CodeQL then scanned it — style notes about code openapi-generator wrote. None of it is committed; it exists only for the duration of the analysis.

A CodeQL config now excludes build output, typescript/dist and typescript/src/generated.

Real: two medium findings, fixed rather than dismissed

js/file-access-to-http and js/http-to-file-access flagged that specs/surface.json names the URL the nightly job fetches, and the fetched body is written back into the repository. CodeQL was right — a one-line edit to a config file decided where CI made an outbound request.

  • the URL must now be https on a host in an allow-list held in code, not in the file it constrains (a list living beside the value it guards guards nothing)
  • specFile / lockFile must be bare filenames, so a write cannot escape specs/

Verified rejecting http://, an unknown host, a non-URL, ../../etc/pwn.json and sub/dir.json, and still accepting the real config.

js/unused-local-variable was a genuinely unused binding. Removed.

Posture

  • TokenPermissionsspec-sync.yml granted contents: write at the top level, inherited by any job it ever grows. Moved to the single job that pushes a branch. release.yml's job-level write stays: release-please cannot create a release or merge its pull request without it.
  • BinaryArtifacts — the checked-in gradle-wrapper.jar, whose contents Scorecard cannot judge. Wrapper checksum validation is now explicit in CI, so a swapped jar fails the build rather than executing.

Not fixable here

CodeReview (0/19 approved), BranchProtection (no required approvers), SAST and CITests (11/26 and 19/26 of commits) are all consequences of the unattended release chain and the bot-PR workaround. They resolve with an AUTOMATION_TOKEN. Maintained is "repo created within the last 90 days" and resolves with time; Fuzzing and CIIBestPractices do not apply to a generated API client.

46 open alerts on brevo-client, 16 on discord-client. Grouped by what they
actually were:

34 java/useless-tostring-call, all in kotlin/build/generated. The Kotlin
analysis has to compile the client to produce a database, and that compilation
generates the entire client, which CodeQL then scanned -- style notes about
code openapi-generator wrote and nobody here can edit. None of it is even
committed. A CodeQL config now excludes build output, dist, and the generated
TypeScript tree.

Two medium findings were real and are fixed rather than dismissed.
js/file-access-to-http and js/http-to-file-access flagged that surface.json
names the URL the nightly job fetches, and the fetched body is then written
into the repository -- so a one-line config edit decided where CI made an
outbound request. The URL is now validated against an https-only host
allow-list held in code, not in the file it constrains, and the spec and lock
file names must be bare filenames so a write cannot escape specs/. Both guards
were verified to reject http, an unknown host, a non-URL, and ../ traversal.

js/unused-local-variable was a genuinely unused destructured binding. Removed.

TokenPermissions: spec-sync.yml granted contents: write at the top level, so
every job it ever grows would inherit it. Moved to the one job that pushes a
branch. release.yml's job-level write stays -- release-please cannot create a
release or merge its pull request without it.

BinaryArtifacts flags the checked-in gradle-wrapper.jar, which it cannot judge
the contents of. Wrapper checksum validation is now explicit in CI, so a
swapped jar fails the build instead of executing.
Already the default in gradle/actions/setup-gradle; stated here so a future
change of that default cannot quietly drop it. The previous attempt anchored on
the action's commit SHA, which Dependabot had since rewritten, so it silently
changed nothing.
@ExtraToast

Copy link
Copy Markdown
Contributor Author

Correction

The description says wrapper checksum validation "is now explicit in CI", implying it was previously absent. It was not: validate-wrappers already defaults to true in gradle/actions/setup-gradle, so the wrapper jar has been verified on every run all along.

It is now pinned explicitly so a future change of that default cannot quietly drop it, but the BinaryArtifacts risk was already mitigated — that alert can be dismissed on those grounds rather than treated as newly fixed.

(My first attempt at this also anchored on the action's commit SHA, which Dependabot had since rewritten, so it matched nothing and changed nothing while reporting success.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant