Skip to content

Escape feature-file text in generated code, harden CI - #10

Merged
menjoo merged 1 commit into
mainfrom
harden-codegen-and-ci
Aug 2, 2026
Merged

Escape feature-file text in generated code, harden CI#10
menjoo merged 1 commit into
mainfrom
harden-codegen-and-ci

Conversation

@menjoo

@menjoo menjoo commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Findings from a security review of the repository and its settings. Two are defects in this
codebase; the rest are CI and repository configuration.

Generated code

FeatureTestGenerator embedded two values from a .feature file as raw code rather than through
KotlinPoet's %S: a pickle's tags and a doc string's media type, both built by hand as "$it".
Every other embedded value in that file already used %S, so this was an oversight.

Both are attacker-controlled. A tag may contain anything except whitespace, and ${...} needs
none, so @a${Runtime.getRuntime()} became a live template in the consumer's test source. A media
type is unconstrained text after the fence, so a quote closed the literal outright. Either way a
.feature file — data, reviewed as prose — became code compiled and run by ./gradlew build.

Mutation-tested: reverting either sink fails exactly the tests written for it.

The KDoc test is a guard, not a fix. A scenario name is echoed into the function's KDoc where a
bare */ would end the block early; KotlinPoet already escapes the * to *, and the test
pins that down so a version bump cannot quietly remove it.

Release workflow

inputs.version was interpolated into four run blocks. The template engine substitutes before
bash parses, so the surrounding quotes were not a defence — and the first sink sat inside the step
that validates the version, so the validation could not constrain its own input. A dispatcher with
write access could have read the signing key, which write access does not otherwise grant. It goes
through the environment now.

The job moves into a release environment, so the publishing credentials sit behind a deployment
approval instead of at repository scope where any workflow on any branch could read them.

Actions and permissions

Every action is pinned to a commit SHA. android-actions/setup-android was the one that mattered:
a mutable v3 tag in the only job holding the signing key and the Central Portal credentials. The
rest are pinned for consistency, since the repository now requires SHA pinning.

CI and upstream-drift declare contents: read. Both ran with a write token because the repository
default was write, and CI runs ./gradlew build — arbitrary build-script code from the branch
under test.

Applied outside this diff

  • Repository default GITHUB_TOKEN set to read
  • SHA pinning required for actions
  • Dependabot alerts and automated security fixes enabled
  • release environment created, required reviewer, protected branches only
  • The four publishing secrets moved to environment scope; repository copies deleted
  • required_status_checks added to the main ruleset for the Tier A and example jobs. The browser
    job is deliberately excluded, per the comment in ci.yml about not letting a Karma flake block
    the gate.

Verification

:cucumber-kmp-gradle-plugin:test, :cucumber-kmp-core:jvmTest, :cucumber-kmp-ksp:test and
examples/calculator jvmTest (real end-to-end feature generation) all pass locally.

This is also the first pull request to run through the newly required status checks.

Two values from a .feature file reached the generated Kotlin as raw code
rather than through KotlinPoet's %S: a pickle's tags and a doc string's
media type were embedded by hand-building a "$it" literal. Everything
else in the generator already used %S, so this was an oversight rather
than a decision.

Both are attacker-controlled text. A tag may contain anything except
whitespace, and ${...} needs none, so @A${Runtime.getRuntime()} became a
live template. A media type is unconstrained text after the fence, so a
quote closed the literal outright. Either way a .feature file -- data,
reviewed as prose -- became code compiled and run by ./gradlew build.

Mutation-tested: reverting either sink fails exactly the tests written
for it, and the pair of them fails three.

The KDoc test is a guard, not a fix. A scenario name is echoed into the
function's KDoc where a bare */ would end the block early; KotlinPoet
already escapes the * to *, and the test pins that down so a version
bump cannot quietly remove it.

On the CI side, inputs.version was interpolated into four run blocks in
the release workflow. The template engine substitutes before bash parses,
so the surrounding quotes were not a defence -- and the first sink was
inside the step that validates the version, which meant the validation
could not constrain its own input. A dispatcher with write access could
have read the signing key, which write access does not otherwise grant.
It goes through the environment now.

Every action is pinned to a commit SHA. android-actions/setup-android
was the one that mattered: a mutable v3 tag in the only job holding the
signing key and the Central Portal credentials. The rest are pinned for
consistency, since the repository now requires SHA pinning.

CI and upstream-drift declare contents: read. Both ran with a write
token because the repository default was write, and CI runs ./gradlew
build -- arbitrary build-script code from the branch under test.

The release job moves into a `release` environment so the publishing
secrets sit behind a deployment approval rather than at repository
scope, where any workflow on any branch can reach them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@menjoo
menjoo force-pushed the harden-codegen-and-ci branch from bd1ac05 to e3612c3 Compare August 2, 2026 08:12
@menjoo
menjoo merged commit 6f7be57 into main Aug 2, 2026
3 checks passed
@menjoo
menjoo deleted the harden-codegen-and-ci branch August 2, 2026 08:23
menjoo added a commit that referenced this pull request Aug 2, 2026
The README still says 0.1.1 now that 0.1.1 has shipped, because step 3
of `RELEASING.md` was a list
of commands someone had to remember. This makes it a workflow.

## What it does

Actions → **Post-release** → *Run workflow*, entering the version just
released. It:

1. checks the tag exists — pointing the docs at an unreleased version
would send people to a 404
2. runs `set-docs-version.py <version>` over the README's install
snippets
3. runs `set-version.sh <next>-SNAPSHOT`
4. pushes `post-<version>` and links to the *Open a pull request* page
in its summary

Simulated end to end against a clone: produces `README.md | 8 ++--` and
`gradle.properties | 2 +-`,
which is what #7 did by hand.

## Why it anchors on coordinates, not version numbers

`set-docs-version.py` matches only `id("io.github.menjoo.cucumberkmp")
version "X"` and
`io.github.menjoo.cucumberkmp:<module>:X`.

Of the fifteen version references in this repository, **eight are
historical** and must never be
rewritten:

| Where | Text |
|---|---|
| `release.yml` | "0.1.0 was lost to precisely this" |
| `verify-publishable.py` | "The 0.1.0 release failed on exactly these
rules" |
| `RELEASING.md` | "Step 5 exists because 0.1.0 was lost without it" |
| `ARCHITECTURE.md` | "**0.1.0 does not exist on Central**", "the
`v0.1.0` tag remains as a record" |
| `get-version.sh`, `RELEASING.md` | doc-comment examples |

A substitution blunt enough to catch the install snippets would erase
the record of the incident
that step 5 of the release workflow exists to prevent. Anchoring on the
coordinate cannot reach
prose at all. Verified: the KSP plugin's `version "2.3.10"` on the line
above the plugin id is
untouched, and `ARCHITECTURE.md` comes out byte-identical.

It also fails loudly if the anchors match nothing, rather than reporting
success — otherwise
"documentation is current" and "the patterns broke in a docs
restructure" look the same, which is
how a script like this rots unnoticed.

`ARCHITECTURE.md` is deliberately not rewritten. Its status lines want a
human sentence.

## Why it doesn't open the pull request

A pull request opened with `GITHUB_TOKEN` does not trigger workflow runs
— GitHub blocks that to
prevent recursion. With CI now required on `main`, a bot-opened pull
request would sit permanently
unable to satisfy its own checks. Opening it yourself is one click and
makes CI run normally.

A PAT would buy that click for a long-lived credential, which is the
trade `RELEASING.md` already
declines under "Why not one button".

## Notes

- Python rather than bash, matching `verify-publishable.py`: anchored
regex with capture groups and
  per-line reporting is awkward in shell.
- Hardening carried over from #10: `contents: write` only, inputs reach
bash through `env`, actions
  SHA-pinned.
- Also retargets the Release workflow's "Next:" summary at this
workflow.

## Ordering

Merge after #11. The first real use is 0.1.2's post-release step.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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