feat: harden download and install security

2026-03-21 20:48:53 +00:00 · 2026-03-21 20:48:53 +00:00 · af13e98eb3
commit af13e98eb3
parent f8ffb95376
33 changed files with 1517 additions and 46 deletions
--- a/.plans/012-security-hardening-config-and-integrity/2026-03-21-security-hardening-config-and-integrity-design.md
+++ b/.plans/012-security-hardening-config-and-integrity/2026-03-21-security-hardening-config-and-integrity-design.md
@ -0,0 +1,171 @@
+# Security Hardening Config And Integrity Design
+
+## Summary
+
+This change hardens the `aim` install pipeline around transport trust, desktop-entry generation, provider integrity checks, and path safety. The approved shape is narrow by design: add `allow_http` to `config.toml` with a default of `false`, apply it only to user-supplied HTTP sources, keep AppImageHub provider-returned downloads HTTPS-only for now, track the broader AppImageHub host-trust issue in `.architecture/security-issues.md`, sanitize launcher display names before writing `.desktop` files, enforce AppImageHub checksum verification semantics, and harden stable-ID-derived install paths.
+
+## Goals
+
+- Add `allow_http` to the user-facing runtime config with a secure default of `false`.
+- Apply `allow_http` only to user-supplied source URLs and explicit HTTP provider inputs.
+- Keep AppImageHub provider-returned download URLs on a stricter HTTPS-only policy.
+- Record the unresolved AppImageHub host-trust issue in `.architecture/security-issues.md`.
+- Prevent newline and control-character injection in generated desktop entries.
+- Enforce AppImageHub checksum validation rather than silently dropping provider checksum metadata.
+- Explicitly reject or contain dangerous stable IDs such as `..`.
+- Add targeted adversarial regression coverage for the new hardening paths.
+- Add lightweight audit logging around external helper execution.
+
+## Non-Goals
+
+- No global provider trust framework beyond the minimum AppImageHub note and HTTPS enforcement in this slice.
+- No redesign of the theme config loader split unless it becomes necessary to thread runtime config.
+- No cryptographic reclassification of AppImageHub MD5 into the existing trusted SHA-512 path.
+- No broader provider security audit implementation outside the issues already approved here.
+- No new CLI flags unless the implementation later proves config-only is insufficient.
+
+## Approaches
+
+### Option 1: Global `allow_http`
+
+This would make a single config flag disable HTTPS enforcement everywhere, including provider-returned URLs. It is easy to wire, but it weakens the trust boundary too broadly and makes one local preference affect third-party provider behavior in ways that are hard to reason about.
+
+### Option 2: User-Input-Only `allow_http`
+
+This is the approved design. `allow_http` applies only to user-supplied direct URLs and explicit HTTP provider inputs such as legacy SourceForge HTTP forms. Provider-returned download URLs remain subject to provider-specific policy. This keeps the config narrow and predictable while still giving advanced users an escape hatch for manual HTTP sources.
+
+### Option 3: Split Security Flags Per Source Class
+
+This would introduce separate toggles for direct URLs, provider URLs, and possibly per-provider policy. It is the most explicit shape, but it creates unnecessary configuration surface for the current repo.
+
+## Approved Design
+
+### Config Model
+
+Add `allow_http = false` to the existing runtime `config.toml` model used by `crates/aim-cli/src/config.rs`. This config is already loaded in `main.rs` for rendering behavior, but the current dispatch path does not receive it. The implementation should thread the loaded runtime config into dispatch rather than adding an unrelated second config lookup.
+
+The existing theme-only loader under `crates/aim-cli/src/cli/config.rs` is not the place for this setting. This change should preserve that split unless unification becomes necessary later.
+
+### Transport Policy
+
+#### User-Supplied Sources
+
+User-supplied HTTP sources are:
+
+- raw `http://...` direct URLs
+- explicit HTTP provider forms that the source parsing layer currently accepts, such as SourceForge HTTP URLs
+
+These should be rejected by default when `allow_http = false` and allowed only when `allow_http = true`.
+
+#### Provider-Returned URLs
+
+Provider-returned URLs are not covered by `allow_http` in this slice. In particular, AppImageHub download URLs returned by the provider transport should be enforced as HTTPS-only regardless of user config.
+
+This distinction preserves the user’s ability to opt into an insecure source they typed deliberately without silently expanding trust for provider-sourced URLs.
+
+### AppImageHub Security Handling
+
+AppImageHub gets two separate treatments:
+
+1. **Immediate enforcement now**
+   - reject non-HTTPS AppImageHub download URLs
+   - fail resolution or add-plan construction with a provider-specific error
+
+2. **Deferred broader issue**
+   - document the missing host-trust / domain allowlist model in `.architecture/security-issues.md`
+   - do not try to solve the broader provider trust framework in this slice
+
+This satisfies the approved direction: the HTTPS rule halves the immediate risk, while the architectural gap remains visible and explicit.
+
+### Desktop Entry Sanitization
+
+The `.desktop` renderer should sanitize display names before interpolation. The key requirement is to prevent field injection, so the sanitation policy should at minimum:
+
+- strip or reject `\n` and `\r`
+- collapse other control characters
+- preserve normal display names for legitimate apps
+
+The sanitation should happen close to desktop-entry generation rather than mutating the stored display name globally.
+
+### AppImageHub Checksum Enforcement
+
+The current AppImageHub path stores MD5 metadata but drops it before installation. This slice should stop silently ignoring it.
+
+Because the existing `trusted_checksum` path is a SHA-512 base64 trust mechanism, AppImageHub MD5 should not be forced into that same contract. Instead, add a provider-specific integrity verification path that:
+
+- computes MD5 for the staged payload when AppImageHub provides one
+- fails installation on mismatch
+- treats MD5 as weaker integrity metadata, not as equivalent to the existing trusted checksum model
+
+This preserves conceptual clarity: strong trusted checksums remain one mechanism, while provider-specific MD5 integrity checks are a separate, weaker guardrail.
+
+### Stable ID Path Hardening
+
+Stable IDs are interpolated into payload, desktop, and icon paths. The normalizer currently preserves `.` characters, so `..` survives.
+
+The approved change is:
+
+- reject normalized IDs containing `..`
+- add a final containment assertion or validation on managed paths before installation proceeds
+
+This is defense in depth even if current command paths do not expose an easy exploit.
+
+### External Command Audit Logging
+
+The desktop integration refresh path should log helper execution at debug level, including command name, args, and exit status when available. This is low-priority observability work, but it belongs in the same hardening tranche because it improves forensic clarity.
+
+## Error Handling
+
+- Rejected HTTP user inputs should fail with a clear security-oriented message explaining that HTTP is disabled unless `allow_http = true` is set.
+- Rejected AppImageHub download URLs should fail with a provider-specific security message rather than a generic parse failure.
+- Desktop-entry sanitation should be non-disruptive where possible; reject only if the sanitized output would become invalid or empty.
+- AppImageHub checksum mismatch should fail install before commit, matching the spirit of existing checksum enforcement.
+- Stable-ID hardening should fail deterministically with an explicit invalid-identity error rather than produce a malformed path.
+
+## Testing Strategy
+
+### Config And Transport Tests
+
+Add tests for:
+
+- `allow_http` defaulting to `false`
+- config override with `allow_http = true`
+- direct `http://` URL rejection by default
+- direct `http://` URL success when config enables it
+- SourceForge HTTP behavior matching the same policy
+- AppImageHub provider-returned `http://` URL rejection even when `allow_http = true`
+
+### Desktop Entry Tests
+
+Add tests for:
+
+- newline-bearing display names being sanitized before render
+- control characters not appearing in generated desktop entries
+- ordinary display names remaining unchanged
+
+### Integrity Tests
+
+Add tests for:
+
+- valid AppImageHub MD5 succeeds
+- invalid AppImageHub MD5 fails before commit
+- missing AppImageHub MD5 continues with the current provider behavior
+
+### Path Hardening Tests
+
+Add tests for:
+
+- normalized identifiers containing `..` being rejected
+- installation path validation refusing escape outside managed roots
+
+### Observability Tests
+
+If practical, add tests around helper invocation logging or at least unit-test the formatting / branch behavior around helper execution outcomes.
+
+## Delivery Notes
+
+- Do not conflate AppImageHub MD5 with the existing trusted SHA-512 checksum contract.
+- Keep `allow_http` policy narrow and explicit.
+- Prefer plumbing the already-loaded runtime config into dispatch rather than inventing another config read path.
+- Track deferred provider host trust in `.architecture/security-issues.md`, not hidden in TODOs.
+- This slice is security-hardening-first; avoid mixing in unrelated product work.
--- a/.plans/012-security-hardening-config-and-integrity/2026-03-21-security-hardening-config-and-integrity-implementation-plan.md
+++ b/.plans/012-security-hardening-config-and-integrity/2026-03-21-security-hardening-config-and-integrity-implementation-plan.md
@ -0,0 +1,391 @@
+# Security Hardening Config And Integrity Implementation Plan
+
+> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.
+
+**Goal:** Add secure-by-default HTTP policy controls, enforce AppImageHub HTTPS and checksum handling, sanitize desktop entries, harden stable-ID path usage, and document the remaining AppImageHub trust issue.
+
+**Architecture:** Extend the existing runtime `CliConfig` with `allow_http`, thread that config into dispatch and add/install planning, keep provider-returned AppImageHub URLs on a stricter HTTPS-only path, add a provider-specific MD5 integrity check distinct from the existing trusted checksum mechanism, and tighten install-time path and desktop-entry generation at the boundary where files are written.
+
+**Tech Stack:** Rust workspace, Cargo tests, TOML config loading, existing install pipeline, fixture-backed provider tests.
+
+---
+
+### Task 1: Record the approved security shape in repo docs
+
+**Files:**
+- Create: `.architecture/security-issues.md`
+- Modify: `README.md`
+- Reference: `.audits/2026-03-21T20-08-04Z-post-appimagehub-security-audit.md`
+
+**Step 1: Write the security issues note**
+
+Create `.architecture/security-issues.md` with:
+
+- a short description of the AppImageHub host-trust gap
+- current mitigation: AppImageHub downloads must be HTTPS
+- deferred work: domain allowlist / provider trust policy
+- status label such as `open`
+
+**Step 2: Update the README security/config section**
+
+Document:
+
+- `allow_http = false` default
+- `allow_http = true` only affects user-supplied HTTP sources
+- provider-returned AppImageHub URLs remain HTTPS-only
+
+**Step 3: Verify docs exist and read clearly**
+
+Run: `rg -n "allow_http|AppImageHub|security" README.md .architecture/security-issues.md`
+Expected: matching lines in both files
+
+**Step 4: Commit**
+
+```bash
+git add .architecture/security-issues.md README.md
+git commit -m "docs: record download security policy"
+```
+
+### Task 2: Add `allow_http` to runtime config and thread it into dispatch
+
+**Files:**
+- Modify: `crates/aim-cli/src/config.rs`
+- Modify: `crates/aim-cli/src/main.rs`
+- Modify: `crates/aim-cli/src/lib.rs`
+- Test: `crates/aim-cli/tests/config_loading.rs`
+
+**Step 1: Write the failing config tests**
+
+Add tests covering:
+
+- default config has `allow_http == false`
+- config file with `allow_http = true` parses and loads correctly
+
+**Step 2: Run the focused tests to verify failure**
+
+Run: `cargo test --package aim-cli --test config_loading`
+Expected: FAIL because `allow_http` does not exist yet
+
+**Step 3: Add the config field**
+
+Update `CliConfig` with:
+
+- `allow_http: bool`
+- `#[serde(default)]`
+- default value `false`
+
+**Step 4: Thread config into dispatch**
+
+Refactor the dispatch entrypoints so the already-loaded runtime config is available during query resolution and install planning.
+
+Preferred shape:
+
+- add `dispatch_with_reporter_and_config(...)`
+- keep existing `dispatch_with_reporter(...)` delegating to default config if needed for compatibility
+- update `main.rs` to call the config-aware path
+
+**Step 5: Run the focused tests to verify pass**
+
+Run: `cargo test --package aim-cli --test config_loading`
+Expected: PASS
+
+**Step 6: Commit**
+
+```bash
+git add crates/aim-cli/src/config.rs crates/aim-cli/src/main.rs crates/aim-cli/src/lib.rs crates/aim-cli/tests/config_loading.rs
+git commit -m "feat: add allow_http runtime config"
+```
+
+### Task 3: Enforce HTTP policy for user-supplied sources only
+
+**Files:**
+- Modify: `crates/aim-core/src/source/input.rs`
+- Modify: `crates/aim-core/src/app/add.rs`
+- Modify: `crates/aim-cli/src/lib.rs`
+- Test: `crates/aim-core/tests/query_resolution.rs`
+- Test: `crates/aim-cli/tests/end_to_end_cli.rs`
+
+**Step 1: Write the failing behavior tests**
+
+Add tests covering:
+
+- direct `http://example.com/app.AppImage` fails by default
+- the same input succeeds when `allow_http = true`
+- explicit SourceForge `http://...` inputs follow the same rule
+
+**Step 2: Run the focused tests to verify failure**
+
+Run: `cargo test --package aim-cli --test end_to_end_cli`
+Expected: FAIL because HTTP is currently accepted unconditionally
+
+**Step 3: Add an explicit HTTP policy check**
+
+Implement a narrow policy helper that is evaluated only for user-supplied source inputs before add/install proceeds.
+
+Requirements:
+
+- reject insecure HTTP when config disallows it
+- preserve HTTPS behavior unchanged
+- do not let this config affect provider-returned URLs
+
+**Step 4: Surface a clear security error**
+
+Ensure the user sees a message equivalent to:
+
+- `insecure HTTP sources are disabled; set allow_http = true to permit them`
+
+**Step 5: Run the focused tests to verify pass**
+
+Run: `cargo test --package aim-cli --test end_to_end_cli`
+Expected: PASS with both rejection and opt-in cases covered
+
+**Step 6: Commit**
+
+```bash
+git add crates/aim-core/src/source/input.rs crates/aim-core/src/app/add.rs crates/aim-cli/src/lib.rs crates/aim-core/tests/query_resolution.rs crates/aim-cli/tests/end_to_end_cli.rs
+git commit -m "feat: enforce user http policy"
+```
+
+### Task 4: Enforce HTTPS for AppImageHub provider-returned downloads
+
+**Files:**
+- Modify: `crates/aim-core/src/source/appimagehub.rs`
+- Modify: `crates/aim-core/src/adapters/appimagehub.rs`
+- Modify: `crates/aim-core/src/app/add.rs`
+- Test: `crates/aim-core/tests/adapter_contract.rs`
+- Test: `crates/aim-cli/tests/end_to_end_cli.rs`
+
+**Step 1: Write the failing AppImageHub tests**
+
+Add a fixture-backed case where AppImageHub returns an `http://` download URL.
+
+Expected result:
+
+- install planning or resolution fails with a provider-specific security error
+- this remains true even when `allow_http = true`
+
+**Step 2: Run the focused tests to verify failure**
+
+Run: `cargo test --package aim-core --test adapter_contract`
+Expected: FAIL because AppImageHub URLs are currently accepted verbatim
+
+**Step 3: Add AppImageHub URL validation**
+
+Validate provider-returned AppImageHub download URLs for:
+
+- HTTPS scheme required
+- clear provider-specific error path
+
+Do not add the broader host allowlist in this task.
+
+**Step 4: Run the focused tests to verify pass**
+
+Run: `cargo test --package aim-core --test adapter_contract && cargo test --package aim-cli --test end_to_end_cli`
+Expected: PASS
+
+**Step 5: Commit**
+
+```bash
+git add crates/aim-core/src/source/appimagehub.rs crates/aim-core/src/adapters/appimagehub.rs crates/aim-core/src/app/add.rs crates/aim-core/tests/adapter_contract.rs crates/aim-cli/tests/end_to_end_cli.rs
+git commit -m "fix: require https for appimagehub downloads"
+```
+
+### Task 5: Sanitize desktop entry display names
+
+**Files:**
+- Modify: `crates/aim-core/src/app/add.rs`
+- Test: `crates/aim-core/tests/install_integration.rs`
+- Test: `crates/aim-cli/tests/end_to_end_cli.rs`
+
+**Step 1: Write the failing desktop-entry tests**
+
+Add tests covering:
+
+- display name containing `\nExec=evil` does not inject a second field
+- display name containing control characters renders safely
+- normal display names still render as expected
+
+**Step 2: Run the focused tests to verify failure**
+
+Run: `cargo test --package aim-core --test install_integration`
+Expected: FAIL because desktop entry output currently interpolates raw display names
+
+**Step 3: Implement minimal sanitation**
+
+Add a helper near desktop entry rendering that:
+
+- strips `\r` and `\n`
+- replaces other control characters with spaces or removes them
+- preserves ordinary printable text
+
+Use the sanitized value only for desktop-entry rendering, not for mutating the stored app record.
+
+**Step 4: Run the focused tests to verify pass**
+
+Run: `cargo test --package aim-core --test install_integration`
+Expected: PASS
+
+**Step 5: Commit**
+
+```bash
+git add crates/aim-core/src/app/add.rs crates/aim-core/tests/install_integration.rs crates/aim-cli/tests/end_to_end_cli.rs
+git commit -m "fix: sanitize desktop entry names"
+```
+
+### Task 6: Enforce AppImageHub MD5 integrity checks
+
+**Files:**
+- Modify: `Cargo.toml`
+- Modify: `crates/aim-core/Cargo.toml`
+- Modify: `crates/aim-core/src/domain/artifact.rs` or the existing artifact type definition file
+- Modify: `crates/aim-core/src/app/add.rs`
+- Modify: `crates/aim-core/src/integration/install.rs`
+- Test: `crates/aim-core/tests/checksum_verification.rs`
+- Test: `crates/aim-cli/tests/end_to_end_cli.rs`
+
+**Step 1: Identify the artifact checksum type location**
+
+Before editing, confirm where `ArtifactCandidate` is defined and where a provider-specific MD5 field should live.
+
+**Step 2: Write the failing integrity tests**
+
+Add tests covering:
+
+- AppImageHub install succeeds with matching MD5 fixture data
+- AppImageHub install fails before commit on MD5 mismatch
+- AppImageHub install still succeeds when no MD5 exists
+
+**Step 3: Run the focused tests to verify failure**
+
+Run: `cargo test --package aim-core --test checksum_verification`
+Expected: FAIL because AppImageHub MD5 is currently ignored
+
+**Step 4: Add a separate weak-integrity field/path**
+
+Implement a provider-specific integrity path distinct from `trusted_checksum`.
+
+Requirements:
+
+- store the provider MD5 on the artifact candidate or equivalent install request
+- verify it after staging and before commit
+- do not overload the existing trusted SHA-512 semantics
+
+**Step 5: Add any needed dependency explicitly**
+
+If an MD5 crate is required, add it at the workspace and crate level.
+
+**Step 6: Run the focused tests to verify pass**
+
+Run: `cargo test --package aim-core --test checksum_verification && cargo test --package aim-cli --test end_to_end_cli`
+Expected: PASS
+
+**Step 7: Commit**
+
+```bash
+git add Cargo.toml crates/aim-core/Cargo.toml crates/aim-core/src/app/add.rs crates/aim-core/src/integration/install.rs crates/aim-core/tests/checksum_verification.rs crates/aim-cli/tests/end_to_end_cli.rs
+git commit -m "feat: verify appimagehub md5 integrity"
+```
+
+### Task 7: Harden stable IDs and managed path containment
+
+**Files:**
+- Modify: `crates/aim-core/src/app/identity.rs`
+- Modify: `crates/aim-core/src/app/add.rs`
+- Test: `crates/aim-core/tests/identity_resolution.rs`
+- Test: `crates/aim-core/tests/install_paths.rs`
+
+**Step 1: Write the failing hardening tests**
+
+Add tests covering:
+
+- identifiers normalizing to `..` are rejected
+- managed install paths do not escape managed roots
+
+**Step 2: Run the focused tests to verify failure**
+
+Run: `cargo test --package aim-core --test identity_resolution --test install_paths`
+Expected: FAIL because `..` currently survives normalization and there is no explicit containment check
+
+**Step 3: Implement identity and path validation**
+
+Add:
+
+- explicit normalized-ID rejection for `..`
+- path containment validation before install proceeds
+
+Keep the implementation minimal and deterministic.
+
+**Step 4: Run the focused tests to verify pass**
+
+Run: `cargo test --package aim-core --test identity_resolution --test install_paths`
+Expected: PASS
+
+**Step 5: Commit**
+
+```bash
+git add crates/aim-core/src/app/identity.rs crates/aim-core/src/app/add.rs crates/aim-core/tests/identity_resolution.rs crates/aim-core/tests/install_paths.rs
+git commit -m "fix: harden stable id paths"
+```
+
+### Task 8: Add external helper audit logging and adversarial regression coverage
+
+**Files:**
+- Modify: `crates/aim-core/src/integration/refresh.rs`
+- Modify: `crates/aim-core/src/source/appimagehub.rs`
+- Test: `crates/aim-core/tests/adapter_contract.rs`
+- Test: `crates/aim-core/tests/install_integration.rs`
+- Test: `crates/aim-cli/tests/end_to_end_cli.rs`
+
+**Step 1: Write the failing or missing regression tests**
+
+Add adversarial cases for:
+
+- malformed AppImageHub XML or missing fields handled cleanly
+- malicious display names in fixture-backed install flows
+- helper execution paths producing expected warnings/loggable branches
+
+**Step 2: Implement minimal logging**
+
+Add debug-level logging around helper execution in `refresh.rs`.
+
+**Step 3: Run focused tests**
+
+Run: `cargo test --package aim-core --test adapter_contract --test install_integration && cargo test --package aim-cli --test end_to_end_cli`
+Expected: PASS
+
+**Step 4: Commit**
+
+```bash
+git add crates/aim-core/src/integration/refresh.rs crates/aim-core/src/source/appimagehub.rs crates/aim-core/tests/adapter_contract.rs crates/aim-core/tests/install_integration.rs crates/aim-cli/tests/end_to_end_cli.rs
+git commit -m "test: cover security edge cases"
+```
+
+### Task 9: Full verification and final docs pass
+
+**Files:**
+- Modify: `.plans/012-security-hardening-config-and-integrity/2026-03-21-security-hardening-config-and-integrity-design.md` if implementation drifted
+- Modify: `.plans/012-security-hardening-config-and-integrity/2026-03-21-security-hardening-config-and-integrity-implementation-plan.md` if task wording drifted
+- Modify: `.architecture/security-issues.md` if final wording needs adjustment
+
+**Step 1: Run formatting and full verification**
+
+Run:
+
+```bash
+cargo fmt --all
+cargo test --workspace
+cargo clippy --workspace --all-targets --all-features -- -D warnings
+```
+
+Expected: all commands succeed.
+
+**Step 2: Re-read the security docs**
+
+Confirm the final README and `.architecture/security-issues.md` text still matches the implementation.
+
+**Step 3: Commit**
+
+```bash
+git add .plans/012-security-hardening-config-and-integrity .architecture/security-issues.md README.md
+git commit -m "docs: record security hardening plan"
+```