🔒 fix(security): remediate findings #740–#743

.pre-commit-config.yaml

@@ -76,7 +76,10 @@ repos:
       - id: ty
         name: Type checking with ty
         description: Static type checking for Python code
-        entry: uv run ty check --ignore call-non-callable --ignore unresolved-attribute --ignore invalid-assignment --ignore invalid-argument-type --ignore unresolved-import youtrack_cli
+        # Pin ty to the same version the tox `type` gate uses so local pre-commit
+        # matches CI. Without the pin, `uv run` picks up whatever ty is in the
+        # venv (a newer release), which flags pre-existing warnings CI does not.
+        entry: uv run --with ty==0.0.1a14 ty check --ignore call-non-callable --ignore unresolved-attribute --ignore invalid-assignment --ignore invalid-argument-type --ignore unresolved-import --ignore invalid-method-override youtrack_cli
         language: system
         types: [python]
         exclude: ^tests/

SECURITY.md

@@ -0,0 +1,42 @@
+# Security Policy
+
+## Reporting a Vulnerability
+
+Please report security vulnerabilities **privately** — do not open a public
+GitHub issue for them.
+
+Preferred channel: use GitHub's **Private vulnerability reporting** for this
+repository (the **Report a vulnerability** button under the *Security* tab). If
+that is unavailable, contact the maintainer directly at **rcheley@gmail.com**.
+
+Please include:
+
+- A description of the vulnerability and its impact.
+- Steps to reproduce (or a proof of concept).
+- The affected version (`yt --version`) and platform.
+
+## What to expect
+
+- **Acknowledgement:** within 7 days of your report.
+- **Assessment & fix:** we aim to have a fix or mitigation plan for confirmed
+  issues within 30 days, prioritized by severity.
+- **Coordinated disclosure:** we will agree a disclosure timeline with you and
+  credit you in the release notes unless you prefer to remain anonymous.
+
+## Supported Versions
+
+Security fixes are applied to the **latest released version**. Please upgrade to
+the latest version before reporting, in case the issue is already resolved.
+
+| Version | Supported |
+| ------- | --------- |
+| Latest release | ✅ |
+| Older releases | ❌ (please upgrade) |
+
+## Scope
+
+YouTrack CLI is a single-user, local command-line tool. In-scope concerns
+include handling of remote/network-influenced input (e.g. YouTrack API
+responses), transport security of credentials, and command execution. Threats
+that require another malicious local user on the same machine (e.g. local file
+permissions) are outside the tool's threat model.

specs/003-security-fixes/checklists/requirements.md

@@ -0,0 +1,37 @@
+# Specification Quality Checklist: Remediate four security findings (#740–#743)
+
+**Purpose**: Validate specification completeness and quality before proceeding to planning
+**Created**: 2026-07-09
+**Feature**: [spec.md](../spec.md)
+
+## Content Quality
+
+- [x] No implementation details (languages, frameworks, APIs)
+- [x] Focused on user value and business needs
+- [x] Written for non-technical stakeholders
+- [x] All mandatory sections completed
+
+## Requirement Completeness
+
+- [x] No [NEEDS CLARIFICATION] markers remain
+- [x] Requirements are testable and unambiguous
+- [x] Success criteria are measurable
+- [x] Success criteria are technology-agnostic (no implementation details)
+- [x] All acceptance scenarios are defined
+- [x] Edge cases are identified
+- [x] Scope is clearly bounded
+- [x] Dependencies and assumptions identified
+
+## Feature Readiness
+
+- [x] All functional requirements have clear acceptance criteria
+- [x] User scenarios cover primary flows
+- [x] Feature meets measurable outcomes defined in Success Criteria
+- [x] No implementation details leak into specification
+
+## Notes
+
+- The spec names file paths and the `http`/`https` distinction because they are
+  the domain nouns of these security findings, not leaked implementation choices.
+- One small UX decision (hard-refuse vs. warn-and-opt-in for `http://`) is
+  intentionally deferred to `/speckit-clarify`; both options satisfy FR-003.

specs/003-security-fixes/plan.md

@@ -0,0 +1,103 @@
+# Implementation Plan: Remediate four security findings (#740–#743)
+
+**Branch**: `security-fixes-740-743` | **Date**: 2026-07-09 | **Spec**: [spec.md](./spec.md)
+
+**Input**: Feature specification from `specs/003-security-fixes/spec.md`
+
+## Summary
+
+Four independent, low-risk fixes to existing YouTrack CLI security findings:
+- **#740** (path traversal): sanitize the server-supplied attachment filename in
+  the article download command so writes stay in the working directory.
+- **#741** (cleartext transport): warn (stderr) when authenticating against an
+  `http://` base URL before the token is sent; still proceed (warn-only).
+- **#742** (shell hardening): run tutorial example commands via
+  `create_subprocess_exec` with a parsed argv list instead of
+  `create_subprocess_shell(shell=True)`; remove the dead shell path.
+- **#743** (process): add `SECURITY.md` disclosure policy.
+
+Three behavioral fixes ship with regression tests (constitution: NON-NEGOTIABLE).
+
+## Technical Context
+
+**Language/Version**: Python (project targets 3.10–3.14; run via `uv`)
+
+**Primary Dependencies**: `click`, `rich`, `httpx`, `pydantic` (all existing —
+no new dependencies)
+
+**Storage**: N/A (config `.env` / keyring already exist; untouched here)
+
+**Testing**: `pytest` with real objects (constitution II); `tox` for the matrix.
+New tests colocated under `tests/`.
+
+**Target Platform**: Local CLI on developer machines (single-user install)
+
+**Project Type**: Single-project Python CLI
+
+**Performance Goals**: No performance impact; changes are on error/edge paths.
+
+**Constraints**: Threat model = single-user local install; remote/network
+inputs in scope, local-user file-permission isolation out of scope. No new
+dependencies. No CLI-surface breakage for the happy path.
+
+**Scale/Scope**: 3 source files touched + 1 new doc + 3 test files (or additions
+to existing test modules). ~small diffs each.
+
+## Constitution Check
+
+*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
+
+- **I. Code Quality**: `ruff` + `ty` clean; changes are typed; deps via `uv`;
+  docs updated (SECURITY.md is itself doc; review `docs/` for any auth/download
+  pages needing a note). ✅ PASS.
+- **II. Testing Standards (NON-NEGOTIABLE)**: Each behavioral fix (FR-001,
+  FR-003, FR-005) gets a regression test that fails before and passes after,
+  using real objects (drive the command/executor/helper directly; the download
+  test injects a crafted filename via the manager result — mock only the remote
+  boundary, justified). ✅ PASS.
+- **III. UX Consistency**: Warnings go to stderr; error/exit behavior unchanged;
+  the `http://` warning is actionable ("token sent in cleartext"). Happy-path
+  output unchanged. ✅ PASS.
+- **IV. Performance**: Edge/error-path only; no latency change. ✅ PASS.
+- **Workflow**: Feature branch `security-fixes-740-743`; issues #740–#743;
+  squash-merge after green. ✅ PASS.
+
+No violations. Complexity Tracking not required.
+
+## Project Structure
+
+### Documentation (this feature)
+
+```text
+specs/003-security-fixes/
+├── plan.md              # This file
+├── spec.md              # Spec (+ Clarifications)
+├── research.md          # Phase 0 — approach decisions per finding
+├── quickstart.md        # Phase 1 — validation guide
+├── checklists/
+│   └── requirements.md  # Spec quality checklist
+└── tasks.md             # Phase 2 (/speckit-tasks)
+```
+
+Not generated (justified): `data-model.md` — the "entities" (filename, base URL)
+are inputs, not persisted models. `contracts/` — no new/changed external
+interface; behavior changes only.
+
+### Source Code (repository root)
+
+```text
+youtrack_cli/commands/articles.py      # #740 sanitize server filename
+youtrack_cli/auth.py                   # #741 warn on http:// (verify path)
+youtrack_cli/tutorial/executor.py      # #742 exec instead of shell
+youtrack_cli/tutorial/core.py          # #742 remove/convert dead shell path
+SECURITY.md                            # #743 new file
+tests/…                                # regression tests for #740/#741/#742
+```
+
+**Structure Decision**: Four independent edits, one per finding, each mappable
+to its own user story. No shared refactor needed. Order by severity: #740 →
+#741 → #742 → #743. See research.md for the specific approach per finding.
+
+## Complexity Tracking
+
+No constitution violations — section intentionally empty.

specs/003-security-fixes/quickstart.md

@@ -0,0 +1,51 @@
+# Quickstart / Validation Guide: Security fixes #740–#743
+
+**Feature**: 003-security-fixes
+
+## Prerequisites
+
+- `uv` installed; on branch `security-fixes-740-743`.
+
+## Automated validation
+
+```sh
+# Full suite incl. new regression tests for #740/#741/#742
+uv run pytest -q
+
+# Lint + types (constitution gates)
+uv run ruff check .
+uv run ruff format --check .
+uv run ty
+```
+
+Expected: all green, including the three new regression tests. Each new test
+fails on `main` (pre-fix) and passes on this branch.
+
+## Manual spot-checks
+
+### #740 — attachment path traversal
+- With a server/attachment whose filename contains `../`, run
+  `yt articles attach download <article> <attachment>` (no `--output`).
+- Expected: file saved in the current directory under the bare name; nothing
+  written to a parent directory. A normal filename (`report.pdf`) is unchanged.
+
+### #741 — cleartext base URL warning
+- `yt auth login --base-url http://localhost:8080 --token <t>` (or verify).
+- Expected: a visible warning on stderr that the token will be sent over
+  cleartext HTTP; the command still proceeds. `https://` shows no warning.
+
+### #742 — tutorial executor no shell
+- Run a tutorial and execute a step's example command.
+- Expected: command runs and output shows, as before. Internally the executor
+  uses `create_subprocess_exec` (no shell); shell metacharacters in a command
+  string are treated as literal args (no secondary command runs).
+
+### #743 — security policy
+- Confirm `SECURITY.md` exists at repo root and documents a private reporting
+  channel, response expectations, and supported versions. On GitHub it appears
+  under the Security tab / "Report a vulnerability".
+
+## Reference
+
+Approach and rationale per finding: [research.md](./research.md). Requirements
+and acceptance scenarios: [spec.md](./spec.md).

specs/003-security-fixes/research.md

@@ -0,0 +1,100 @@
+# Phase 0 Research: Security fixes #740–#743
+
+**Feature**: 003-security-fixes | **Date**: 2026-07-09
+
+## R1. #740 — Sanitize server-supplied attachment filename
+
+**Decision**: In `commands/articles.py download`, when `--output` is not given,
+derive the local name with `Path(filename).name` (strips all directory
+components). If that yields an empty/unsafe name (`""`, `.`, `..`), fall back to
+`f"attachment_{attachment_id}"`. Keep the explicit `--output` path as-is (user's
+own trusted choice).
+
+**Rationale**: `Path(filename).name` neutralizes both relative traversal
+(`../../x` → `x`) and absolute paths (`/etc/x` → `x`) in one stdlib call — the
+minimal, correct fix. Confining to CWD matches today's behavior. This mirrors
+the already-safe pattern used by `yt issues attach download`
+(`attachment_{attachment_id}` default).
+
+**Alternatives considered**: `os.path.basename` (equivalent; `Path.name` is the
+project idiom). Full `resolve()`-and-prefix-check (more code; `.name` already
+guarantees no separators so a prefix check is redundant for the no-`--output`
+path).
+
+## R2. #741 — Warn on cleartext (`http://`) base URL
+
+**Decision**: Add a small helper in `auth.py`, e.g.
+`warn_if_insecure_url(base_url)`, that prints a Rich-styled warning to stderr
+when the URL scheme is `http`. Call it from `verify_credentials()` (the
+chokepoint hit during interactive `yt auth login` and explicit verification)
+before the request is made. Warn-only; the request proceeds (per Clarifications).
+
+**Rationale**: `verify_credentials` is the single point every interactive login
+and verification passes through, so one call covers the "authenticate" paths
+without touching every request. Warn-only keeps `http://localhost` and on-prem
+plain-HTTP working (single-user threat model), while making each cleartext send
+visible. Scheme parsed with `urllib.parse.urlparse` (stdlib) rather than a
+fragile `startswith`.
+
+**Alternatives considered**:
+- Warn inside the HTTP client on every request — highest coverage but noisy
+  (repeats per call). Rejected for warn-fatigue.
+- Warn only in the `main.py` login command — misses the programmatic
+  `verify_credentials` path. Rejected.
+- Hard-reject / opt-in flag — rejected by the user in Clarifications (warn-only).
+
+## R3. #742 — Remove `shell=True` from the tutorial executor
+
+**Decision**: In `executor.py`, change `_execute_via_subprocess` to build an
+argv list via the existing `parse_command()` (which already uses `shlex.split`)
+and run `asyncio.create_subprocess_exec("yt", *args, …)` instead of
+`create_subprocess_shell(command, shell=True)`. Keep `is_command_allowed()` as a
+tutorial-scope UX filter but note it is no longer the security boundary — with
+`exec`, shell metacharacters are inert. Remove (or convert) the dead
+`_execute_command` in `core.py` that also uses `shell=True`.
+
+**Rationale**: `create_subprocess_exec` passes arguments directly to the process
+without a shell, so `;`, `&&`, `$(...)`, backticks carry no meaning — this is the
+real fix, and the code already parses commands into args, so it's a small
+change. Invoking the installed `yt` entrypoint with the parsed args preserves
+current behavior.
+
+**Alternatives considered**: Tightening the whitelist to exact-match — still
+leaves `shell=True` as a latent sink; rejected. Executing Click in-process
+(the file's own TODO) — larger change than warranted for a hardening fix;
+deferred.
+
+**To verify during implement**: confirm `core.py::_execute_command` is truly
+uncalled before deleting (grep showed only its definition; the live path is
+`executor.execute_command`).
+
+## R4. #743 — SECURITY.md
+
+**Decision**: Add `SECURITY.md` at repo root describing: private reporting
+channel (GitHub Private Vulnerability Reporting and/or maintainer email),
+response-time expectation, coordinated-disclosure window, and supported
+versions.
+
+**Rationale**: Standard, discoverable location; GitHub surfaces it in the
+Security tab and the "Report a vulnerability" UI. Lowest-effort closure of the
+process gap.
+
+**Alternatives considered**: `.github/SECURITY.md` (equivalent; root is fine and
+more visible). Enabling GitHub Private Vulnerability Reporting is a repo setting
+the maintainer can toggle; the file will reference it.
+
+## R5. Testing approach (FR-009)
+
+**Decision**: One regression test per behavioral fix, real-object style:
+- **#740**: invoke the download command (via Click `CliRunner` or the manager)
+  with the remote boundary returning `filename="../../pwned"`; assert the file
+  lands at `tmp_path/pwned` and nothing is written outside `tmp_path`. Mock only
+  the network `download_attachment` result (justified boundary mock).
+- **#741**: call `warn_if_insecure_url("http://example")` and assert a warning is
+  emitted; assert no warning for `https://`. Pure real-object.
+- **#742**: drive the executor with a metacharacter command and assert no
+  secondary effect (e.g. a sentinel file is NOT created), and/or assert
+  `create_subprocess_exec` is used. Prefer the observable-side-effect assertion.
+
+**Rationale**: Matches constitution II (real objects; mock only the un-driveable
+remote boundary). Each test fails before its fix and passes after.