morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #3440] [CLOSED] fix(security): pin CI actions, protect Hopper xauth and downloads, flag old apktool #5743

New issue

Closed

opened 2026-10-31 01:09:06 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:06 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3440
Author: @GovinduSasanka
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: security/hardening-bridges-downloads-ci

📝 Commits (9)

afda2a9 ci: pin remaining workflow actions and guard against tag-only references
1b5fac5 fix(hopper): pass the private X authority cookie to xauth on stdin
585b003 fix(hopper): refuse vendor responses that redirect off HTTPS
4559ab8 fix(apktool): flag launchers older than 2.9.2 (CVE-2024-21633)
4bb22ee docs(install): show how to run a reviewed, tag-pinned installer
74695e8 fix(hopper): check every redirect hop for HTTPS before requesting it
ba0bbc5 fix(apktool): compare the CVE-2024-21633 cutoff with semver
dedd752 test(ci): parse workflow YAML when checking action pinning
e051ec6 Merge remote-tracking branch 'origin/main' into sweep/3440

📊 Changes

9 files changed (+283 additions, -25 deletions)

View changed files

📝 .github/workflows/real-cutter.yml (+3 -3)
📝 docs/installation.md (+10 -0)
📝 scripts/hopper-demo-x11.py (+5 -9)
📝 src/apktool/ApktoolConfiguration.test.ts (+23 -0)
📝 src/apktool/ApktoolConfiguration.ts (+30 -5)
📝 src/apktool/ApktoolProvider.ts (+7 -1)
➕ src/application/HopperDownload.test.ts (+73 -0)
📝 src/application/HopperDownload.ts (+53 -7)
➕ tests/boundary/filesystem/workflowActionPinning.test.ts (+79 -0)

📄 Description

This PR fixes three low-severity security problems and one documentation gap, and stops a CI regression from coming back. I found them while reviewing REA; this branch is rebased on a32b4b6.

CI: real-cutter.yml reintroduced tag-pinned actions after #1901.
Hopper (Linux): the private X display cookie shows up in ps.
Hopper downloads: a redirect can quietly move a download off HTTPS.
apktool: REA gives no warning when the apktool in use still has CVE-2024-21633.

Each fix is a separate commit with tests where the code can be tested, and none of them changes a tool contract.

Fixes #3434, fixes #3436, fixes #3437, fixes #3438, fixes #3439

(#3435, the Ghidra loopback accept, was already fixed on main by acceptAuthenticatedLoopbackClient in d4bd65c, so I dropped my commit for it.)

How it works

Action pinning (#3434)
- #1901 pinned every external action, but real-cutter.yml later added actions/upload-artifact@v4 (twice) and actions/download-artifact@v4.
- Those three now use the same SHAs as the rest of the repo (ea165f8… # v4.6.2, d3f86a1… # v4.3.0).
- The new tests/boundary/filesystem/workflowActionPinning.test.ts parses every .github/**/*.yml (workflows and composite actions) and fails on any external uses value that isn't owner/repo@<40-hex SHA>, so this can't regress again without a failing test.
xauth cookie (#3436)
- authorize_display now runs xauth -q -f <authority> source - and sends add :N . <cookie> on stdin, so the cookie never appears in the command line.
Hopper downloads (#3437)
- Redirects are followed manually (at most 5 hops), and each hop must be https: before it is requested.
- An HTTPS → HTTP → HTTPS chain is refused without ever contacting the HTTP hop. A missing or unparseable Location is refused too.
- The existing length, SHA-1 and codesign checks still run on accepted downloads.
apktool CVE-2024-21633 (#3438)
- The new apktoolVersionLimitations(version) returns a limitation when semver.lt(version, "2.9.2"). It uses the same -dirty→+dirty normalization as the existing documented-version floor (shared comparableApktoolVersion), so 2.9.2-rc.1 is flagged. Unparseable versions return nothing.
- Both inspect_apktool_client and decode_android_resources include it in their existing limitations output.
Installer docs (#3439)
- docs/installation.md now explains how to download install.sh from a rea-agents-<version> tag, read it, and run it with --version.

Decisions

apktool: warning instead of refusal.
- docs/apktool-resource-analysis.md documents verification with Debian's 2.7.0-dirty. Refusing old launchers would break that and other distribution packages.
- A limitation keeps them usable and still shows the risk in output that agents and users read.
Hopper: HTTPS only, not "Hopper's domain only".
- I couldn't confirm whether hopperapp.com redirects downloads to a CDN, so restricting the final host could break installs.
- Requiring HTTPS closes the downgrade without that risk, and the checksum checks still apply.
- Pinning the macOS codesign Team ID is noted in #3437 but not done here; it needs the vendor's designated requirement.
Pinning: reuse the SHAs already used across the repo rather than resolving new ones, so real-cutter runs the same action code as every other lane.
Pinning guard: parse YAML rather than match lines. Greptile pointed out that a line regex misses flow-style steps and rejects quoted SHAs. It covers composite actions and reusable workflow calls, and skips local ./ and docker:// references.
Hopper redirects: check each hop, not the final URL. Greptile showed that a final-URL check still lets fetch request an attacker-controllable plain-HTTP hop, so the check moved in front of every request.
Things I checked and left alone:
- start_frida_session's non-destructive annotation is deliberate and covered by an existing test exception (spawned targets stay suspended until resume_frida_session).
- The packaged skill already replaces rea-agents@latest with the shipped version at build time (generatedSkill.test.ts).
Contribution scope: these are fixes within existing boundaries, so no maintainer scope decision is needed. There are no CLI/MCP contract or catalog changes, and no generated files are committed.

Expected effects:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Hardening of existing behavior only: CI pinning, xauth argv, HTTPS check on download redirects, output limitation, docs.

Verifications

All of these ran locally on Windows 11 with Node 24.13.1, on the rebased branch, after npm ci --ignore-scripts:

npm run check:fast passed (typecheck and lint).
npx tsc --noEmit passed.
npx oxfmt --check on all 8 changed source and test files passed.
python -I -m py_compile scripts/hopper-demo-x11.py passed.
vitest run passed: 4 files, 38 tests.
- src/apktool/ApktoolConfiguration.test.ts
- src/application/HopperDownload.test.ts
- tests/boundary/filesystem/workflowActionPinning.test.ts
- tests/conformance/providers/ghidra/realGhidraWindowsWorkflow.test.ts
Hopper download tests now cover HTTPS→HTTPS (accepted), HTTPS→HTTP→HTTPS (refused, and the HTTP hop is never requested), a missing Location, and a redirect loop.
The apktool tests now include 2.9.2-rc.1 (flagged) and 2.10.0-dirty (not flagged).
Regression check: before the real-cutter.yml fix, workflowActionPinning.test.ts lists exactly the three @v4 references; afterwards it passes.

What I could not run:

The apktool provider tests and hopper-demo-x11.py only run on POSIX/Linux, so CI will be their first real run.
LinuxHopper.test.ts and MacHopper.test.ts fail on a Windows host with and without this branch, so I'm relying on CI for them. HopperDownload.ts is their shared download path.

Commit summary

Commit	Summary
`afda2a9`	ci: pin the remaining `real-cutter.yml` actions, plus a test that rejects tag-only references
`1b5fac5`	fix(hopper): pass the private X authority cookie to xauth on stdin
`585b003`	fix(hopper): refuse vendor responses that redirect off HTTPS
`4559ab8`	fix(apktool): flag launchers older than 2.9.2 (CVE-2024-21633)
`4bb22ee`	docs(install): show how to run a reviewed, tag-pinned installer
`74695e8`	fix(hopper): check every redirect hop for HTTPS before requesting it (review)
`ba0bbc5`	fix(apktool): compare the CVE-2024-21633 cutoff with semver (review)
`dedd752`	test(ci): parse workflow YAML when checking action pinning (review)

File changes

File	Change
`.github/workflows/real-cutter.yml`	3 action references pinned to commit SHAs
`tests/boundary/filesystem/workflowActionPinning.test.ts`	New: YAML-parsed guard plus fixtures (block, flow, quoted, local, reusable, docker)
`scripts/hopper-demo-x11.py`	The xauth cookie is sent on stdin through `source -`
`src/application/HopperDownload.ts`	Manual redirect loop with an HTTPS check on every hop; injectable `fetch` for tests
`src/application/HopperDownload.test.ts`	New: URL scheme checks, plus redirect chains (HTTPS ok, HTTP hop refused, missing Location, loop)
`src/apktool/ApktoolConfiguration.ts`	`apktoolVersionLimitations` using semver with a 2.9.2 cutoff; shared `comparableApktoolVersion`
`src/apktool/ApktoolProvider.ts`	Version limitation added to client and decode results
`src/apktool/ApktoolConfiguration.test.ts`	Warns for 2.7.0-dirty, 2.9.1 and v1.5.2; 2.9.2-rc.1 is flagged; no warning for 2.9.2 and later, 2.10.0-dirty, or unparseable versions
`docs/installation.md`	Reviewed, tag-pinned installer steps

Totals: 9 files, +286 / −26.

Author readiness

Human owner: @GovinduSasanka

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Other findings from the same review were reported privately through GitHub's vulnerability reporting, as SECURITY.md asks.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3440 **Author:** [@GovinduSasanka](https://github.com/GovinduSasanka) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `security/hardening-bridges-downloads-ci` --- ### 📝 Commits (9) - [`afda2a9`](https://github.com/morluto/rea/commit/afda2a96b4bdf6ffd70d7f68ca7d21f7b39c7c0f) ci: pin remaining workflow actions and guard against tag-only references - [`1b5fac5`](https://github.com/morluto/rea/commit/1b5fac5b4f8fed6a537d702b94f23605a4ddcad7) fix(hopper): pass the private X authority cookie to xauth on stdin - [`585b003`](https://github.com/morluto/rea/commit/585b0039ce822d392fa7ecb6539805f8fb548652) fix(hopper): refuse vendor responses that redirect off HTTPS - [`4559ab8`](https://github.com/morluto/rea/commit/4559ab87798b5ce00c59eebbd35b59a233628709) fix(apktool): flag launchers older than 2.9.2 (CVE-2024-21633) - [`4bb22ee`](https://github.com/morluto/rea/commit/4bb22eeed502c9d25a28cb68524054b91b516cc9) docs(install): show how to run a reviewed, tag-pinned installer - [`74695e8`](https://github.com/morluto/rea/commit/74695e8f299e2493c8b47665c1ad87fa73a5c0dd) fix(hopper): check every redirect hop for HTTPS before requesting it - [`ba0bbc5`](https://github.com/morluto/rea/commit/ba0bbc5783f34cbc84a27533cfb65c1073c27636) fix(apktool): compare the CVE-2024-21633 cutoff with semver - [`dedd752`](https://github.com/morluto/rea/commit/dedd7520c4fd6d965ee0cf655413527f315aff14) test(ci): parse workflow YAML when checking action pinning - [`e051ec6`](https://github.com/morluto/rea/commit/e051ec64c5b4f59cafafeec2529f2ff94e958e4f) Merge remote-tracking branch 'origin/main' into sweep/3440 ### 📊 Changes **9 files changed** (+283 additions, -25 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/real-cutter.yml` (+3 -3) 📝 `docs/installation.md` (+10 -0) 📝 `scripts/hopper-demo-x11.py` (+5 -9) 📝 `src/apktool/ApktoolConfiguration.test.ts` (+23 -0) 📝 `src/apktool/ApktoolConfiguration.ts` (+30 -5) 📝 `src/apktool/ApktoolProvider.ts` (+7 -1) ➕ `src/application/HopperDownload.test.ts` (+73 -0) 📝 `src/application/HopperDownload.ts` (+53 -7) ➕ `tests/boundary/filesystem/workflowActionPinning.test.ts` (+79 -0) </details> ### 📄 Description This PR fixes three low-severity security problems and one documentation gap, and stops a CI regression from coming back. I found them while reviewing REA; this branch is rebased on `a32b4b6`. - **CI:** `real-cutter.yml` reintroduced tag-pinned actions after #1901. - **Hopper (Linux):** the private X display cookie shows up in `ps`. - **Hopper downloads:** a redirect can quietly move a download off HTTPS. - **apktool:** REA gives no warning when the apktool in use still has CVE-2024-21633. Each fix is a separate commit with tests where the code can be tested, and none of them changes a tool contract. Fixes #3434, fixes #3436, fixes #3437, fixes #3438, fixes #3439 (#3435, the Ghidra loopback accept, was already fixed on `main` by `acceptAuthenticatedLoopbackClient` in d4bd65c, so I dropped my commit for it.) ## How it works - **Action pinning (#3434)** - #1901 pinned every external action, but `real-cutter.yml` later added `actions/upload-artifact@v4` (twice) and `actions/download-artifact@v4`. - Those three now use the same SHAs as the rest of the repo (`ea165f8… # v4.6.2`, `d3f86a1… # v4.3.0`). - The new `tests/boundary/filesystem/workflowActionPinning.test.ts` parses every `.github/**/*.yml` (workflows and composite actions) and fails on any external `uses` value that isn't `owner/repo@<40-hex SHA>`, so this can't regress again without a failing test. - **xauth cookie (#3436)** - `authorize_display` now runs `xauth -q -f <authority> source -` and sends `add :N . <cookie>` on stdin, so the cookie never appears in the command line. - **Hopper downloads (#3437)** - Redirects are followed manually (at most 5 hops), and each hop must be `https:` *before* it is requested. - An HTTPS → HTTP → HTTPS chain is refused without ever contacting the HTTP hop. A missing or unparseable `Location` is refused too. - The existing length, SHA-1 and codesign checks still run on accepted downloads. - **apktool CVE-2024-21633 (#3438)** - The new `apktoolVersionLimitations(version)` returns a limitation when `semver.lt(version, "2.9.2")`. It uses the same `-dirty`→`+dirty` normalization as the existing documented-version floor (shared `comparableApktoolVersion`), so `2.9.2-rc.1` is flagged. Unparseable versions return nothing. - Both `inspect_apktool_client` and `decode_android_resources` include it in their existing `limitations` output. - **Installer docs (#3439)** - `docs/installation.md` now explains how to download `install.sh` from a `rea-agents-<version>` tag, read it, and run it with `--version`. ## Decisions - **apktool: warning instead of refusal.** - `docs/apktool-resource-analysis.md` documents verification with Debian's `2.7.0-dirty`. Refusing old launchers would break that and other distribution packages. - A limitation keeps them usable and still shows the risk in output that agents and users read. - **Hopper: HTTPS only, not "Hopper's domain only".** - I couldn't confirm whether hopperapp.com redirects downloads to a CDN, so restricting the final host could break installs. - Requiring HTTPS closes the downgrade without that risk, and the checksum checks still apply. - Pinning the macOS codesign Team ID is noted in #3437 but not done here; it needs the vendor's designated requirement. - **Pinning: reuse the SHAs already used across the repo** rather than resolving new ones, so `real-cutter` runs the same action code as every other lane. - **Pinning guard: parse YAML rather than match lines.** Greptile pointed out that a line regex misses flow-style steps and rejects quoted SHAs. It covers composite actions and reusable workflow calls, and skips local `./` and `docker://` references. - **Hopper redirects: check each hop, not the final URL.** Greptile showed that a final-URL check still lets fetch request an attacker-controllable plain-HTTP hop, so the check moved in front of every request. - **Things I checked and left alone:** - `start_frida_session`'s non-destructive annotation is deliberate and covered by an existing test exception (spawned targets stay suspended until `resume_frida_session`). - The packaged skill already replaces `rea-agents@latest` with the shipped version at build time (`generatedSkill.test.ts`). - **Contribution scope:** these are fixes within existing boundaries, so no maintainer scope decision is needed. There are no CLI/MCP contract or catalog changes, and no generated files are committed. Expected effects: - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [x] Hardening of existing behavior only: CI pinning, xauth argv, HTTPS check on download redirects, output limitation, docs. ## Verifications All of these ran locally on Windows 11 with Node 24.13.1, on the rebased branch, after `npm ci --ignore-scripts`: - `npm run check:fast` passed (typecheck and lint). - `npx tsc --noEmit` passed. - `npx oxfmt --check` on all 8 changed source and test files passed. - `python -I -m py_compile scripts/hopper-demo-x11.py` passed. - `vitest run` passed: 4 files, 38 tests. - `src/apktool/ApktoolConfiguration.test.ts` - `src/application/HopperDownload.test.ts` - `tests/boundary/filesystem/workflowActionPinning.test.ts` - `tests/conformance/providers/ghidra/realGhidraWindowsWorkflow.test.ts` - Hopper download tests now cover HTTPS→HTTPS (accepted), HTTPS→HTTP→HTTPS (refused, and the HTTP hop is never requested), a missing Location, and a redirect loop. - The apktool tests now include `2.9.2-rc.1` (flagged) and `2.10.0-dirty` (not flagged). - **Regression check:** before the `real-cutter.yml` fix, `workflowActionPinning.test.ts` lists exactly the three `@v4` references; afterwards it passes. What I could not run: - The apktool provider tests and `hopper-demo-x11.py` only run on POSIX/Linux, so CI will be their first real run. - `LinuxHopper.test.ts` and `MacHopper.test.ts` fail on a Windows host with and without this branch, so I'm relying on CI for them. `HopperDownload.ts` is their shared download path. ## Commit summary | Commit | Summary | |---|---| | `afda2a9` | **ci:** pin the remaining `real-cutter.yml` actions, plus a test that rejects tag-only references | | `1b5fac5` | **fix(hopper):** pass the private X authority cookie to xauth on stdin | | `585b003` | **fix(hopper):** refuse vendor responses that redirect off HTTPS | | `4559ab8` | **fix(apktool):** flag launchers older than 2.9.2 (CVE-2024-21633) | | `4bb22ee` | **docs(install):** show how to run a reviewed, tag-pinned installer | | `74695e8` | **fix(hopper):** check every redirect hop for HTTPS before requesting it (review) | | `ba0bbc5` | **fix(apktool):** compare the CVE-2024-21633 cutoff with semver (review) | | `dedd752` | **test(ci):** parse workflow YAML when checking action pinning (review) | ## File changes | File | Change | |---|---| | `.github/workflows/real-cutter.yml` | 3 action references pinned to commit SHAs | | `tests/boundary/filesystem/workflowActionPinning.test.ts` | **New:** YAML-parsed guard plus fixtures (block, flow, quoted, local, reusable, docker) | | `scripts/hopper-demo-x11.py` | The xauth cookie is sent on stdin through `source -` | | `src/application/HopperDownload.ts` | Manual redirect loop with an HTTPS check on every hop; injectable `fetch` for tests | | `src/application/HopperDownload.test.ts` | **New:** URL scheme checks, plus redirect chains (HTTPS ok, HTTP hop refused, missing Location, loop) | | `src/apktool/ApktoolConfiguration.ts` | `apktoolVersionLimitations` using semver with a 2.9.2 cutoff; shared `comparableApktoolVersion` | | `src/apktool/ApktoolProvider.ts` | Version limitation added to client and decode results | | `src/apktool/ApktoolConfiguration.test.ts` | Warns for 2.7.0-dirty, 2.9.1 and v1.5.2; 2.9.2-rc.1 is flagged; no warning for 2.9.2 and later, 2.10.0-dirty, or unparseable versions | | `docs/installation.md` | Reviewed, tag-pinned installer steps | Totals: 9 files, +286 / −26. ## Patch links - Full PR: [.patch](https://github.com/morluto/rea/pull/3440.patch) · [.diff](https://github.com/morluto/rea/pull/3440.diff) - [`afda2a9` ci: pin actions + guard](https://github.com/GovinduSasanka/rea/commit/afda2a96b4bdf6ffd70d7f68ca7d21f7b39c7c0f) ([.patch](https://github.com/GovinduSasanka/rea/commit/afda2a96b4bdf6ffd70d7f68ca7d21f7b39c7c0f.patch)) - [`1b5fac5` fix(hopper): xauth](https://github.com/GovinduSasanka/rea/commit/1b5fac5b4f8fed6a537d702b94f23605a4ddcad7) ([.patch](https://github.com/GovinduSasanka/rea/commit/1b5fac5b4f8fed6a537d702b94f23605a4ddcad7.patch)) - [`585b003` fix(hopper): HTTPS](https://github.com/GovinduSasanka/rea/commit/585b0039ce822d392fa7ecb6539805f8fb548652) ([.patch](https://github.com/GovinduSasanka/rea/commit/585b0039ce822d392fa7ecb6539805f8fb548652.patch)) - [`4559ab8` fix(apktool)](https://github.com/GovinduSasanka/rea/commit/4559ab87798b5ce00c59eebbd35b59a233628709) ([.patch](https://github.com/GovinduSasanka/rea/commit/4559ab87798b5ce00c59eebbd35b59a233628709.patch)) - [`4bb22ee` docs(install)](https://github.com/GovinduSasanka/rea/commit/4bb22eeed502c9d25a28cb68524054b91b516cc9) ([.patch](https://github.com/GovinduSasanka/rea/commit/4bb22eeed502c9d25a28cb68524054b91b516cc9.patch)) - [`74695e8` fix(hopper): per-hop HTTPS](https://github.com/GovinduSasanka/rea/commit/74695e8f299e2493c8b47665c1ad87fa73a5c0dd) ([.patch](https://github.com/GovinduSasanka/rea/commit/74695e8f299e2493c8b47665c1ad87fa73a5c0dd.patch)) - [`ba0bbc5` fix(apktool): semver](https://github.com/GovinduSasanka/rea/commit/ba0bbc5783f34cbc84a27533cfb65c1073c27636) ([.patch](https://github.com/GovinduSasanka/rea/commit/ba0bbc5783f34cbc84a27533cfb65c1073c27636.patch)) - [`dedd752` test(ci): YAML guard](https://github.com/GovinduSasanka/rea/commit/dedd7520c4fd6d965ee0cf655413527f315aff14) ([.patch](https://github.com/GovinduSasanka/rea/commit/dedd7520c4fd6d965ee0cf655413527f315aff14.patch)) ## Author readiness  Human owner: @GovinduSasanka - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. Other findings from the same review were reported privately through GitHub's vulnerability reporting, as SECURITY.md asks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:09:06 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:46:00 +00:00

[PR #6180] [CLOSED] docs(errors): correct stale capability-unknown and comparison-rationale rows #7188

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

morluto/rea#5743

No description provided.

Rows
Columns