diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index a898848..ef918f0 100644 --- a/.pre-commit-config.yaml +++ b/.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/ diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..9fb90c2 --- /dev/null +++ b/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. diff --git a/specs/003-security-fixes/checklists/requirements.md b/specs/003-security-fixes/checklists/requirements.md new file mode 100644 index 0000000..dfd0ebf --- /dev/null +++ b/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. diff --git a/specs/003-security-fixes/plan.md b/specs/003-security-fixes/plan.md new file mode 100644 index 0000000..2e91758 --- /dev/null +++ b/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. diff --git a/specs/003-security-fixes/quickstart.md b/specs/003-security-fixes/quickstart.md new file mode 100644 index 0000000..ceaebbb --- /dev/null +++ b/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
` (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 ` (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). diff --git a/specs/003-security-fixes/research.md b/specs/003-security-fixes/research.md new file mode 100644 index 0000000..bfdbed5 --- /dev/null +++ b/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. diff --git a/specs/003-security-fixes/spec.md b/specs/003-security-fixes/spec.md new file mode 100644 index 0000000..0d9e11d --- /dev/null +++ b/specs/003-security-fixes/spec.md @@ -0,0 +1,213 @@ +# Feature Specification: Remediate four security findings (#740–#743) + +**Feature Branch**: `security-fixes-740-743` + +**Created**: 2026-07-09 + +**Status**: Draft + +**Input**: User description: "Fix four security findings in the YouTrack CLI (#740 path traversal on article attachment download, #741 cleartext HTTP base URL, #742 tutorial executor shell=True hardening, #743 add SECURITY.md). Threat model: single-user local install — remote/network threats in scope, local-user file-permission threats out of scope." + +## Clarifications + +### Session 2026-07-09 + +- Q: When a user configures an `http://` (non-TLS) base URL, how should the CLI + behave to prevent silently sending the token in cleartext (#741)? → A: Warn + only, still proceed — print a clear cleartext warning to stderr and continue + the request. No blocking, no opt-in flag. The point is that transmission is no + longer *silent*; the user is informed each time. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Safe attachment download (Priority: P1) + +A user downloads an attachment from a YouTrack article. The attachment's +filename is chosen by whoever uploaded it — potentially an attacker or a +compromised server. The user expects the file to land in their working +directory under a safe name, never outside it. + +**Why this priority**: #740 is the highest-impact finding — the untrusted input +(server-supplied filename) reaches a file-write sink and can escape the intended +directory (`../../.bashrc`, absolute paths). It is remotely influenced, squarely +in the threat model. + +**Independent Test**: Simulate a download where the server returns a filename +containing path-traversal sequences; confirm the file is written only inside the +intended directory under a sanitized base name, and a traversal attempt is +refused or neutralized. + +**Acceptance Scenarios**: + +1. **Given** a server response whose attachment filename is `../../evil.sh`, + **When** the user runs `yt articles attach download` without `--output`, + **Then** the file is written inside the working directory under the bare + name `evil.sh` (no directory escape). +2. **Given** a server filename that is an absolute path (`/tmp/evil` or + `/etc/cron.d/x`), **When** downloaded, **Then** the write stays within the + intended directory and does not target the absolute location. +3. **Given** a normal filename (`report.pdf`), **When** downloaded, **Then** + behavior is unchanged — the file is saved as `report.pdf`. + +--- + +### User Story 2 - No silent cleartext credentials (Priority: P2) + +A user configures the CLI with a base URL. If they supply an insecure +`http://` URL, the API token would otherwise be transmitted in cleartext and be +capturable by a network attacker. The user should be stopped or clearly warned +before that happens. + +**Why this priority**: #741 protects the credential in transit against a +network (MITM) attacker — in scope for the threat model. Slightly lower than +#740 because it requires a network-position attacker and an insecure URL choice. + +**Independent Test**: Attempt login/verification with an `http://` base URL and +confirm the CLI surfaces the insecurity (blocks by default, or requires an +explicit opt-in) rather than silently proceeding. + +**Acceptance Scenarios**: + +1. **Given** a user provides an `http://` base URL, **When** they log in or + verify credentials, **Then** the CLI prints a clear cleartext-transmission + warning before sending the token (warn-only; the request still proceeds). +2. **Given** a user provides an `https://` base URL, **When** they log in, + **Then** behavior is unchanged (no new friction). +3. **Given** a user provides a bare host with no scheme, **When** they log in, + **Then** it continues to default to `https://` as today. + +--- + +### User Story 3 - Tutorial runs without a shell (Priority: P3) + +A user runs an interactive tutorial that executes example CLI commands. Those +commands should run without invoking a system shell, so shell metacharacters +carry no meaning and the executor is not a latent injection sink. + +**Why this priority**: #742 is defense-in-depth. The commands are currently +hardcoded, so it is not externally exploitable today, but removing `shell=True` +and the bypassable whitelist prevents a future regression from becoming an RCE. + +**Independent Test**: Run a tutorial step that executes its example command and +confirm it still works; confirm the executor no longer invokes a shell and that +shell metacharacters in a command string are not interpreted. + +**Acceptance Scenarios**: + +1. **Given** a tutorial step with a normal example command, **When** the user + chooses to execute it, **Then** the command runs and its output is shown, as + before. +2. **Given** a command string containing shell metacharacters (`;`, `&&`, + `$(...)`), **When** executed by the tutorial executor, **Then** the + metacharacters are treated as literal arguments, not shell operators — no + secondary command runs. +3. **Given** the previously dead shell-executing code path, **When** the change + is complete, **Then** it no longer invokes a system shell (removed or + converted). + +--- + +### User Story 4 - Clear vulnerability reporting path (Priority: P4) + +A security researcher who finds a vulnerability needs a documented, private way +to report it instead of opening a public issue. + +**Why this priority**: #743 is a process gap, lowest severity, but cheap and +valuable for responsible disclosure. + +**Independent Test**: Confirm a discoverable policy file exists describing a +private reporting channel and expectations. + +**Acceptance Scenarios**: + +1. **Given** a researcher looks for how to report a vulnerability, **When** they + check the repository, **Then** they find a `SECURITY.md` describing a private + channel, response expectations, and supported versions. + +--- + +### Edge Cases + +- Server filename that is empty, `.`, `..`, or all path separators → must + resolve to a safe, non-empty local name and never escape the directory. +- Server filename containing a mix of separators or URL-encoded traversal → the + sanitized name must still be confined. +- `--output` explicitly provided by the user → this is the user's own choice on + their single-user machine and remains honored (out-of-scope for traversal + hardening, which targets the *server-supplied* name). +- `http://localhost` / `http://127.0.0.1` for local dev → the insecure-URL + handling should still make the user aware, though this is a common legitimate + case (the reject-vs-warn decision governs behavior here). + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: When saving a downloaded article attachment using the + server-supplied filename, the system MUST strip any directory components and + write only within the intended target directory; a filename containing + traversal sequences or absolute paths MUST NOT cause a write outside that + directory. +- **FR-002**: Normal attachment filenames MUST continue to download with + unchanged behavior and naming. +- **FR-003**: When a user configures or authenticates against an `http://` + (non-TLS) base URL, the system MUST emit a clear cleartext-transmission + warning (to stderr) before proceeding, so the token is never sent silently. + The request still proceeds (warn-only; no blocking, no opt-in flag). +- **FR-004**: `https://` base URLs and bare hosts (defaulted to `https://`) + MUST continue to work without added friction. +- **FR-005**: The tutorial command executor MUST execute example commands + without invoking a system shell, so that shell metacharacters are not + interpreted. +- **FR-006**: The bypassable prefix-based command allowlist MUST no longer be + relied upon as the safety mechanism; any dead shell-executing code path MUST + be removed or converted to non-shell execution. +- **FR-007**: Executing a normal tutorial example command MUST continue to + produce the same user-visible result as before. +- **FR-008**: The repository MUST include a discoverable security policy + (`SECURITY.md`) documenting a private vulnerability-reporting channel, + response expectations, and supported versions. +- **FR-009**: Each behavioral fix (FR-001, FR-003, FR-005) MUST be covered by an + automated regression test that fails before the fix and passes after. + +### Key Entities + +- **Server-supplied attachment filename**: Untrusted string from a YouTrack API + response used to name a downloaded file. Must be treated as hostile input. +- **Base URL**: The configured YouTrack instance address; its scheme + (`http`/`https`) determines whether credentials travel in cleartext. +- **Tutorial example command**: A hardcoded command string a tutorial step can + execute on the user's behalf. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: 100% of attachment downloads whose server-supplied filename + contains traversal sequences or absolute paths are written inside the intended + directory (0 escapes), verified by regression tests. +- **SC-002**: Every configuration or authentication against an `http://` base + URL emits a visible cleartext warning before the token is sent (0 silent + cleartext transmissions; warning shown 100% of the time). +- **SC-003**: The tutorial executor invokes no system shell; a command string + with shell metacharacters produces 0 secondary command executions. +- **SC-004**: A `SECURITY.md` disclosure policy is present and discoverable. +- **SC-005**: All existing tests continue to pass and new regression tests for + the three behavioral fixes pass, with no regression in normal (safe-input) + behavior. + +## Assumptions + +- Threat model is a single-user local install: remote/network-influenced inputs + (server filenames, cleartext transport) are in scope; local-user filesystem + permission isolation is explicitly out of scope (per the deleted #739). See + the project threat-model memory. +- The intended target directory for a downloaded attachment (when `--output` is + not given) is the current working directory, matching today's behavior. +- The user's explicit `--output` path is trusted (their own choice on their own + machine) and is not subject to the traversal hardening. +- The `http://` behavior is resolved (see Clarifications): warn-only, the + request still proceeds. This intentionally keeps `http://localhost` and + on-prem plain-HTTP instances working while making each cleartext send visible. +- The tutorial executor already parses commands into an argument list, which the + non-shell execution can reuse. diff --git a/specs/003-security-fixes/tasks.md b/specs/003-security-fixes/tasks.md new file mode 100644 index 0000000..1abe4e0 --- /dev/null +++ b/specs/003-security-fixes/tasks.md @@ -0,0 +1,109 @@ +# Tasks: Remediate four security findings (#740–#743) + +**Feature**: 003-security-fixes | **Branch**: `security-fixes-740-743` + +**Input**: [plan.md](./plan.md), [spec.md](./spec.md), [research.md](./research.md), [quickstart.md](./quickstart.md) + +**Note**: The four user stories are independent (different files) and can be done +in any order or in parallel. Tests are REQUIRED (constitution II): each +behavioral fix ships a regression test that fails before and passes after. + +## Phase 1: Setup + +- [X] T001 Confirm branch `security-fixes-740-743`; read the current code at the three sites to anchor edits: `youtrack_cli/commands/articles.py` (download, ~L1391-1405), `youtrack_cli/auth.py` (`verify_credentials`, ~L375-423), `youtrack_cli/tutorial/executor.py` (~L87-89, L157-183) and `youtrack_cli/tutorial/core.py` (`_execute_command`, ~L357-384). + +## Phase 2: Foundational + +_None._ No shared prerequisite; each story is self-contained. + +--- + +## Phase 3: User Story 1 — Safe attachment download (#740, Priority: P1) 🎯 MVP + +**Goal**: Sanitize the server-supplied attachment filename so a malicious +filename cannot write outside the working directory. + +**Independent test**: Download with a server filename of `../../pwned`; file +lands at `/pwned`, nothing written outside. + +- [X] T002 [P] [US1] Add a regression test in `tests/test_articles.py` (new test class/functions) that invokes the article `attach download` command with the remote boundary (`ArticleManager.download_attachment`) returning `filename="../../pwned"` and no `--output`, using `tmp_path` as cwd; assert the file is written to `tmp_path/pwned` and NOT to `tmp_path.parent`. Add a second case for an absolute filename (`/tmp/evil`) and a normal filename (`report.pdf`, unchanged). Test must FAIL before T003. +- [X] T003 [US1] In `youtrack_cli/commands/articles.py` download command, replace the no-`--output` branch `output_path = Path(filename)` with a sanitized name: `safe = Path(filename).name; output_path = Path(safe) if safe and safe not in (".", "..") else Path(f"attachment_{attachment_id}")`. Leave the explicit `--output` branch unchanged. Re-run T002 → passes. + +**Checkpoint**: #740 fixed (SC-001; FR-001, FR-002). + +--- + +## Phase 4: User Story 2 — Cleartext base URL warning (#741, Priority: P2) + +**Goal**: Warn (stderr) before sending the token to an `http://` base URL; +proceed anyway (warn-only). + +**Independent test**: `warn_if_insecure_url("http://x")` emits a warning; +`https://x` emits none. + +- [X] T004 [P] [US2] Add a regression test in `tests/test_auth.py` that calls the new `warn_if_insecure_url` helper (or `verify_credentials` path) with an `http://` base URL and asserts a cleartext warning is emitted (capture console/stderr), and asserts NO warning for an `https://` URL. Test must FAIL before T005. +- [X] T005 [US2] In `youtrack_cli/auth.py`, add `warn_if_insecure_url(base_url: str) -> None` that uses `urllib.parse.urlparse` and, when scheme == "http", prints a Rich-styled warning to stderr that the API token will be sent in cleartext. Call it near the start of `verify_credentials()` (before the request). Warn-only; do not block. Re-run T004 → passes. + +**Checkpoint**: #741 fixed (SC-002; FR-003, FR-004). + +--- + +## Phase 5: User Story 3 — Tutorial executor without a shell (#742, Priority: P3) + +**Goal**: Execute tutorial example commands via `create_subprocess_exec` (argv), +not `create_subprocess_shell(shell=True)`; remove the dead shell path. + +**Independent test**: Executing a command containing `;`/`&&` produces no +secondary effect (sentinel file not created). + +- [X] T006 [P] [US3] Add a regression test in `tests/tutorial/` (e.g. `test_executor.py`) that runs `ClickCommandExecutor` against a command whose string contains a shell metacharacter payload attempting to create a sentinel file in `tmp_path` (e.g. `yt --version; touch pwned`), and asserts the sentinel file is NOT created (no shell interpretation). Test must FAIL before T007 (with shell=True the sentinel would appear). +- [X] T007 [US3] In `youtrack_cli/tutorial/executor.py`, change `_execute_via_subprocess` to build argv via the existing `parse_command()` and call `asyncio.create_subprocess_exec("yt", *args, stdout=PIPE, stderr=PIPE, env=env)` instead of `create_subprocess_shell(..., shell=True)`. Keep `is_command_allowed` as a UX filter (add a comment that it is no longer the security boundary). Re-run T006 → passes. +- [X] T008 [US3] In `youtrack_cli/tutorial/core.py`, verify `_execute_command` (the `shell=True` path, ~L357-384) is unused (`grep -rn "_execute_command" youtrack_cli/`); if unused, delete it; if used, convert it to `create_subprocess_exec` the same way. Ensure no remaining `shell=True` in `youtrack_cli/tutorial/`. + +**Checkpoint**: #742 fixed (SC-003; FR-005, FR-006, FR-007). + +--- + +## Phase 6: User Story 4 — Security disclosure policy (#743, Priority: P4) + +**Goal**: Add a discoverable vulnerability disclosure policy. + +**Independent test**: `SECURITY.md` exists at repo root with a private reporting +channel, response expectations, and supported versions. + +- [X] T009 [P] [US4] Create `SECURITY.md` at repo root: private reporting channel (GitHub Private Vulnerability Reporting and/or maintainer contact), expected response time and coordinated-disclosure window, and a Supported Versions statement. Reference enabling GitHub Private Vulnerability Reporting in repo settings. + +**Checkpoint**: #743 fixed (SC-004; FR-008). + +--- + +## Phase 7: Polish & Validation + +- [X] T010 Run `uv run pytest -q` (all green incl. new tests), `uv run ruff check .`, `uv run ruff format --check .`, `uv run ty` (constitution gates). Fix any lint/type issues introduced. +- [X] T011 Review `docs/` for any article-download or auth pages that should mention the sanitized filename behavior or the cleartext warning; update if present (constitution: docs updated with change). If none apply, note "no doc change needed". +- [X] T012 Run `git grep -n "shell=True" youtrack_cli/` to confirm no residual shell execution remains in the tutorial package. + +--- + +## Dependencies + +- **T001** before all. +- Within each story, the test precedes its implementation (TDD): T002→T003, + T004→T005, T006→T007→T008. +- **US1–US4 are mutually independent** (different files) and may proceed in + parallel. +- **Phase 7 (T010–T012)** after all stories. + +## Parallel opportunities + +- The four test-writing tasks (T002, T004, T006, T009) touch different files and + are all `[P]` — can be written together. +- The four stories can each be implemented independently; only Phase 7 + validation must wait for all. + +## Implementation strategy + +- **MVP = User Story 1 (#740)**: the highest-severity, remotely-influenced + file-write fix. Delivers the most security value alone. +- **Full delivery**: all four stories + Phase 7. Each maps 1:1 to a GitHub + issue (#740–#743) for clean traceability in the PR. diff --git a/tests/test_security_fixes.py b/tests/test_security_fixes.py new file mode 100644 index 0000000..21874bb --- /dev/null +++ b/tests/test_security_fixes.py @@ -0,0 +1,96 @@ +"""Regression tests for security findings #740, #741, #742. + +Each test fails against the pre-fix code and passes after the fix: +- #740: article attachment download must not honor traversal in the + server-supplied filename. +- #741: an ``http://`` base URL must emit a cleartext warning. +- #742: the tutorial executor must not run commands through a shell. +""" + +import asyncio +import os +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +from click.testing import CliRunner + +from youtrack_cli.auth import warn_if_insecure_url +from youtrack_cli.commands.articles import download as articles_download +from youtrack_cli.tutorial.executor import ClickCommandExecutor + + +class TestAttachmentPathTraversal: + """#740 — server-supplied attachment filename must be sanitized.""" + + def _run_download(self, filename: str, tmp_path, monkeypatch): + # Run with cwd = tmp_path so relative writes land there and persist for + # the assertions (unlike isolated_filesystem, which deletes on exit). + monkeypatch.chdir(tmp_path) + runner = CliRunner() + mock_manager = MagicMock() + mock_manager.download_attachment = AsyncMock( + return_value={ + "status": "success", + "data": { + "content": b"payload", + "filename": filename, + "metadata": {"mimeType": "text/plain"}, + }, + } + ) + with patch("youtrack_cli.articles.ArticleManager", return_value=mock_manager): + return runner.invoke(articles_download, ["ART-1", "att-1"], obj={"config": {}}) + + def test_relative_traversal_stays_in_cwd(self, tmp_path, monkeypatch): + result = self._run_download("../../pwned", tmp_path, monkeypatch) + assert result.exit_code == 0, result.output + # Written under the bare name inside the working directory... + assert (tmp_path / "pwned").is_file() + # ...and NOT two directories up, where the raw name would have landed. + assert not (tmp_path.parent.parent / "pwned").exists() + + def test_absolute_path_is_neutralized(self, tmp_path, monkeypatch): + # An absolute server filename must not be written to that absolute path. + target = Path("/tmp/yt_cli_pwned_sentinel") + if target.exists(): + target.unlink() + result = self._run_download("/tmp/yt_cli_pwned_sentinel", tmp_path, monkeypatch) + assert result.exit_code == 0, result.output + assert not target.exists() + assert (tmp_path / "yt_cli_pwned_sentinel").is_file() + + def test_normal_filename_unchanged(self, tmp_path, monkeypatch): + result = self._run_download("report.pdf", tmp_path, monkeypatch) + assert result.exit_code == 0, result.output + assert (tmp_path / "report.pdf").is_file() + + +class TestCleartextUrlWarning: + """#741 — http:// base URL must warn before the token is sent.""" + + def test_http_warns(self): + with patch("youtrack_cli.auth.get_error_console") as mock_console: + warn_if_insecure_url("http://youtrack.example.com") + mock_console.return_value.print.assert_called_once() + message = mock_console.return_value.print.call_args[0][0] + assert "cleartext" in message.lower() + + def test_https_does_not_warn(self): + with patch("youtrack_cli.auth.get_error_console") as mock_console: + warn_if_insecure_url("https://youtrack.example.com") + mock_console.return_value.print.assert_not_called() + + +class TestTutorialExecutorNoShell: + """#742 — executor must not interpret shell metacharacters.""" + + def test_metacharacters_do_not_spawn_secondary_command(self, tmp_path): + sentinel = tmp_path / "pwned_sentinel" + # Allowed prefix ("yt --version") followed by a shell injection attempt. + payload = f"yt --version; touch {sentinel}" + executor = ClickCommandExecutor() + # With shell=True this would run `touch `; with exec it is an + # inert argument to `yt`, so the sentinel must never be created. + asyncio.run(executor.execute_command(payload, require_confirmation=False)) + assert not sentinel.exists() + assert not os.path.exists(sentinel) diff --git a/tests/test_tutorial.py b/tests/test_tutorial.py index 865031c..1e828dd 100644 --- a/tests/test_tutorial.py +++ b/tests/test_tutorial.py @@ -306,27 +306,9 @@ def test_step_with_command_example(self): assert step.command_example == "echo 'hello world'" - @pytest.mark.asyncio - async def test_command_execution_success(self): - """Test successful command execution.""" - with tempfile.TemporaryDirectory() as temp_dir: - tracker = ProgressTracker(config_dir=temp_dir) - engine = TutorialEngine(tracker) - - # Test simple echo command - await engine._execute_command("echo 'test success'") - # If no exception is raised, the test passes - - @pytest.mark.asyncio - async def test_command_execution_failure(self): - """Test command execution with failure.""" - with tempfile.TemporaryDirectory() as temp_dir: - tracker = ProgressTracker(config_dir=temp_dir) - engine = TutorialEngine(tracker) - - # Test command that should fail - await engine._execute_command("nonexistent_command_that_should_fail") - # If no exception is raised, the test passes (failure is handled gracefully) + # NOTE: tests for TutorialEngine._execute_command were removed with that dead + # shell=True method (#742). Live command execution runs through + # ClickCommandExecutor (no shell); see tests/test_security_fixes.py. class TestTutorialEngineDisplay: diff --git a/youtrack_cli/auth.py b/youtrack_cli/auth.py index cafb43c..8ee2ec6 100644 --- a/youtrack_cli/auth.py +++ b/youtrack_cli/auth.py @@ -15,6 +15,7 @@ import os from datetime import datetime from pathlib import Path +from urllib.parse import urlparse import httpx from dotenv import load_dotenv @@ -22,11 +23,30 @@ from .client import reset_client_manager_sync from .config import ConfigManager -from .console import get_console +from .console import get_console, get_error_console from .models import CredentialVerificationResult from .security import CredentialManager, SecurityConfig, TokenManager -__all__ = ["AuthConfig", "AuthManager"] +__all__ = ["AuthConfig", "AuthManager", "warn_if_insecure_url"] + + +def warn_if_insecure_url(base_url: str) -> None: + """Warn (to stderr) when a base URL would send the token over cleartext HTTP. + + The YouTrack instance URL determines whether credentials travel encrypted. + An ``http://`` scheme means the API token is transmitted in cleartext and is + capturable by a network (MITM) attacker. This is warn-only: the caller still + proceeds, but the transmission is no longer silent (see issue #741). + + Args: + base_url: The configured YouTrack instance URL. + """ + if urlparse(base_url).scheme == "http": + get_error_console().print( + "[yellow]⚠ Insecure connection: the base URL uses http://, so your API " + "token will be sent in cleartext and can be intercepted. Use https:// " + "if your YouTrack instance supports it.[/yellow]" + ) class AuthConfig(BaseModel): @@ -388,6 +408,9 @@ async def verify_credentials( Raises: httpx.HTTPError: If request fails """ + # Warn before the token leaves the process over a cleartext connection. + warn_if_insecure_url(base_url) + headers = {"Authorization": f"Bearer {token}", "Accept": "application/json"} # Configure SSL verification diff --git a/youtrack_cli/commands/articles.py b/youtrack_cli/commands/articles.py index 08b221e..af96f8b 100644 --- a/youtrack_cli/commands/articles.py +++ b/youtrack_cli/commands/articles.py @@ -1392,9 +1392,17 @@ def download(ctx: click.Context, article_id: str, attachment_id: str, output: st # Determine output path if output: + # User-supplied path is their own trusted choice. output_path = Path(output) else: - output_path = Path(filename) + # The filename comes from the server and is untrusted: strip any + # directory components so a hostile name (../../x, /etc/x) cannot + # escape the working directory. Fall back to a safe default when + # the basename is empty or a traversal token. + safe_name = Path(filename).name + if not safe_name or safe_name in (".", ".."): + safe_name = f"attachment_{attachment_id}" + output_path = Path(safe_name) # Check if file exists and handle overwrite if output_path.exists() and not overwrite: diff --git a/youtrack_cli/tutorial/core.py b/youtrack_cli/tutorial/core.py index 4290425..5157221 100644 --- a/youtrack_cli/tutorial/core.py +++ b/youtrack_cli/tutorial/core.py @@ -1,7 +1,5 @@ """Core tutorial engine and base classes.""" -import asyncio -import asyncio.subprocess from abc import ABC, abstractmethod from collections.abc import Callable from dataclasses import dataclass @@ -9,7 +7,6 @@ from rich import box from rich.panel import Panel from rich.prompt import Confirm, Prompt -from rich.syntax import Syntax from rich.table import Table from rich.text import Text @@ -354,49 +351,6 @@ def display_completion(self, module: TutorialModule) -> None: self.console.print("\n") self.console.print(completion_panel) - async def _execute_command(self, command: str) -> None: - """Execute a command and display its output. - - Args: - command: The command to execute. - """ - self.console.print(f"\n[blue]⚡ Executing command:[/blue] [green]{command}[/green]") - - try: - # Show the command in a syntax panel - syntax = Syntax(command, "bash", theme="monokai", line_numbers=False) - self.console.print(Panel(syntax, title="Command", border_style="blue")) - - # Execute the command - process = await asyncio.create_subprocess_shell( - command, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, shell=True - ) - - stdout, stderr = await process.communicate() - - # Display output - if stdout: - output_text = stdout.decode().strip() - if output_text: - self.console.print( - Panel(output_text, title="[green]Output[/green]", border_style="green", expand=False) - ) - - if stderr: - error_text = stderr.decode().strip() - if error_text: - self.console.print( - Panel(error_text, title="[red]Error Output[/red]", border_style="red", expand=False) - ) - - if process.returncode == 0: - self.console.print("[green]✓ Command executed successfully![/green]\n") - else: - self.console.print(f"[red]✗ Command failed with exit code {process.returncode}[/red]\n") - - except Exception as e: - self.console.print(f"[red]✗ Failed to execute command: {e}[/red]\n") - def display_welcome(self) -> None: """Display welcome message for tutorial system.""" welcome_text = Text() diff --git a/youtrack_cli/tutorial/executor.py b/youtrack_cli/tutorial/executor.py index da0bc2d..3f88e41 100644 --- a/youtrack_cli/tutorial/executor.py +++ b/youtrack_cli/tutorial/executor.py @@ -177,9 +177,15 @@ async def _execute_via_subprocess(self, command: str) -> bool: if hasattr(self.config_manager, "get_env_vars"): env.update(self.config_manager.get_env_vars()) - # Execute the command - process = await asyncio.create_subprocess_shell( - command, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, shell=True, env=env + # Execute the command WITHOUT a shell: parse into an argv list and + # invoke the `yt` entrypoint directly. This makes shell + # metacharacters (`;`, `&&`, `$(...)`, backticks) inert, so the + # command string can never spawn a secondary process. The exec is the + # security boundary here — `is_command_allowed()` remains only a + # tutorial-scope UX filter, not the safety mechanism (issue #742). + args = self.parse_command(command) + process = await asyncio.create_subprocess_exec( + "yt", *args, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, env=env ) stdout, stderr = await process.communicate() diff --git a/youtrack_cli/utils.py b/youtrack_cli/utils.py index a88f615..f32fe0c 100644 --- a/youtrack_cli/utils.py +++ b/youtrack_cli/utils.py @@ -56,13 +56,15 @@ def loads_lenient(text: str) -> Any: - """Parse JSON, tolerating two malformations occasionally present in YouTrack - field values (most often issue descriptions): + r"""Parse JSON, tolerating malformations occasionally present in YouTrack values. + + Handles two malformations most often seen in YouTrack field values (issue + descriptions especially): 1. Literal control characters inside strings (raw newlines/tabs) — allowed by ``strict=False``. - 2. Invalid backslash escapes — e.g. a Windows path ``C:\\Users`` or a regex - ``\\d+`` serialized with single backslashes. Any backslash that is not part + 2. Invalid backslash escapes — e.g. a Windows path ``C:\Users`` or a regex + ``\d+`` serialized with single backslashes. Any backslash that is not part of a valid JSON escape sequence is doubled before re-parsing. Strict parsing is tried first, so well-formed responses are unaffected. Raises