Hiddenden/AegisGitea-MCP
5
0 Fork 0
Code Issues 2 Pull Requests Actions Packages Projects 1 Releases 1 Wiki Activity

docs: record S2 results, mark findings fixed or waived, update the security model

Browse Source
This commit is contained in:
Latte committed 2026-10-14 17:55:59 +02:00
1 parent d7cac91175
commit f86b9b529c
2 files changed
+89 -40
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

docs/security-findings.md
+53 -23
View File
@@ -12,27 +12,34 @@ it.
## Summary
| Id | Severity | Title |
|---|---|---|
| SEC-01 | high | Unbounded request body on the unauthenticated consent POST |
| SEC-02 | medium | Stored webhook `authorization_header` returned by hook read tools |
| SEC-03 | medium | Audit write failures after startup do not stop later calls |
| SEC-04 | medium | Settings read a `.env` file from the process working directory |
| SEC-05 | medium | Any https redirect URI can be registered; the consent page is the only barrier |
| SEC-06 | medium | Fork pull requests run Docker-capable jobs on the shared runner |
| SEC-07 | medium | CI actions pinned by mutable tags, one of them receives the registry token |
| SEC-08 | low | Rate limiting gaps (`/token`, `/revoke`, IPv6, proxy address) |
| SEC-09 | low | OAuth pre-sign-in state can be flushed by unauthenticated floods |
| SEC-10 | low | `GITEA_URL` and `PUBLIC_BASE_URL` accept whitespace and control characters; plain http Gitea is silent |
| SEC-11 | low | Audit log created with umask defaults; unkeyed hash chain |
| SEC-12 | low | Tool audit entries do not record the client, session or source IP |
| SEC-13 | low | Token store directory ownership is not verified |
| SEC-14 | low | CI cleanup and credential handling on the shared Docker daemon |
| SEC-15 | low | `tests/security` missing, so the CI `security` job fails; publish is not gated on it |
| SEC-16 | low | No container image vulnerability scan |
| SEC-17 | low | Consent POST accepts a missing `Origin` header, unlike ADR 0004 says |
| SEC-18 | low | Unvalidated argument names and unknown tool names reach the audit log |
| SEC-19 | low | One fixed cookie name for concurrent sign-ins in the same browser |
| Id | Severity | Title | Status (S2) |
|---|---|---|---|
| SEC-01 | high | Unbounded request body on the unauthenticated consent POST | fixed |
| SEC-02 | medium | Stored webhook `authorization_header` returned by hook read tools | fixed |
| SEC-03 | medium | Audit write failures after startup do not stop later calls | fixed |
| SEC-04 | medium | Settings read a `.env` file from the process working directory | fixed |
| SEC-05 | medium | Any https redirect URI can be registered; the consent page is the only barrier | fixed, default waived (W-10) |
| SEC-06 | medium | Fork pull requests run Docker-capable jobs on the shared runner | fixed |
| SEC-07 | medium | CI actions pinned by mutable tags, one of them receives the registry token | fixed |
| SEC-08 | low | Rate limiting gaps (`/token`, `/revoke`, IPv6, proxy address) | fixed |
| SEC-09 | low | OAuth pre-sign-in state can be flushed by unauthenticated floods | fixed |
| SEC-10 | low | `GITEA_URL` and `PUBLIC_BASE_URL` accept whitespace and control characters; plain http Gitea is silent | fixed |
| SEC-11 | low | Audit log created with umask defaults; unkeyed hash chain | fixed, keyed chain waived (W-11) |
| SEC-12 | low | Tool audit entries do not record the client, session or source IP | fixed |
| SEC-13 | low | Token store directory ownership is not verified | fixed |
| SEC-14 | low | CI cleanup and credential handling on the shared Docker daemon | fixed |
| SEC-15 | low | `tests/security` missing, so the CI `security` job fails; publish is not gated on it | fixed |
| SEC-16 | low | No container image vulnerability scan | fixed |
| SEC-17 | low | Consent POST accepts a missing `Origin` header, unlike ADR 0004 says | fixed |
| SEC-18 | low | Unvalidated argument names and unknown tool names reach the audit log | fixed |
| SEC-19 | low | One fixed cookie name for concurrent sign-ins in the same browser | fixed |
All 19 findings were fixed or waived in S2 (status column and per-finding status lines below); the
negative tests live in `tests/security/`. The fixes found four further defects that are fixed in
the same changeset: secrets inside JSON object keys were not pattern masked (`fef80a5`), a
trailing newline passed the `sudo` pattern (`984673e`), a `data` field on an Action secret
response was not redacted (`421a650`), and configuration errors for `OAUTH_SCOPES` and
`TOOLSETS` echoed the offending values (`0f5f5cd`).
No critical findings. Verified as sound (no finding): static bearer comparison, the identity
split between static bearer, OAuth and no-auth (including OAuth-only deployments), Sudo
@@ -46,6 +53,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-01 (high): unbounded request body on the unauthenticated consent POST
- Status: fixed in `d5ca530`: consent and callback routes wrapped in a 16 KiB body limit; 415 for any content type but `application/x-www-form-urlencoded` before a byte is read; form parsed with `max_files=0, max_fields=8, max_part_size=4096`. Tests: `tests/security/test_oauth_input_bounds.py`.
- Where: `src/aegis_gitea_mcp/oauth/callback.py:137-155` (`consent_submit` calls
`await request.form()`), routes at `callback.py:169-173`.
- Description: the browser routes are custom Starlette routes. Unlike the MCP endpoint and the
@@ -69,6 +77,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-02 (medium): stored webhook `authorization_header` returned by hook read tools
- Status: fixed in `5b622e6`: `SENSITIVE_RESPONSE_KEYS` (the request-side key list) redacts non-empty string values by key name at any depth in every result except credential-issuing operations, in all `SECRET_DETECTION_MODE` values. Tests: `tests/security/test_secret_echo.py`.
- Where: `src/aegis_gitea_mcp/gitea/executor.py:339-356` and `603-625` (only values submitted
in the request body are redacted, via `sensitive_body_values` in
`src/aegis_gitea_mcp/gitea/encoding.py:387-405`).
@@ -90,6 +99,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-03 (medium): audit write failures after startup do not stop later calls
- Status: fixed in `43eebd3`: the write-ahead alternative: a `tool_call_started` entry is written before every known tool runs and the call is refused with `audit_unavailable` when it fails. Documented in `docs/audit-logging.md`. Tests: `tests/security/test_audit_fail_closed.py`.
- Where: `src/aegis_gitea_mcp/server.py:414-426` (only obtaining the logger is checked) and
`server.py:509-537` (write errors are logged and swallowed);
`src/aegis_gitea_mcp/audit.py:104-107`.
@@ -109,6 +119,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-04 (medium): settings read a `.env` file from the process working directory
- Status: fixed in `79aa4f7`: `./.env` loads only as a regular file owned by the user with mode 0600; `--env-file` and `AEGIS_ENV_FILE`; documented in README, configuration and troubleshooting. Tests: `tests/security/test_env_file.py`.
- Where: `src/aegis_gitea_mcp/config.py:109-114` (`env_file=".env"`).
- Description: pydantic-settings loads `./.env` for every setting not present in the
environment. Claude Code and other clients start stdio servers with the project directory as
@@ -129,6 +140,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-05 (medium): any https redirect URI can be registered; the consent page is the only barrier
- Status: fixed, default waived (W-10) in `8be8592`: owner decision: optional `OAUTH_REDIRECT_ALLOWLIST` (exact URIs or `https://host/` prefixes, checked at registration and `/authorize`); the open default is waived with the consent page as mitigation, and the allowlist is recommended for public deployments. Tests: `tests/security/test_oauth_redirects.py`.
- Where: `src/aegis_gitea_mcp/oauth/provider.py:99-115` (`redirect_uri_problem`),
`provider.py:306-343` (`register_client`), `src/aegis_gitea_mcp/oauth/callback.py:109-135`.
- Description: dynamic client registration is open to anyone and accepts any https redirect
@@ -149,6 +161,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-06 (medium): fork pull requests run Docker-capable jobs on the shared runner
- Status: fixed in `766968f`: `test-integration`, `test-mcp` and `build-and-push` carry the fork guard. The Gitea setting "require approval for workflows from forks" is a manual step recorded in `docs/deployment.md`. Tests: `tests/security/test_ci_workflows.py`.
- Where: `.gitea/workflows/ci.yml:22-25` (`pull_request` to `dev` and `main`), jobs
`test-integration` (`ci.yml:119-144`) and `test-mcp` (`ci.yml:153-185`).
- Description: a pull request from a fork runs its own workflow and test code, and these jobs
@@ -164,6 +177,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-07 (medium): CI actions pinned by mutable tags, one of them receives the registry token
- Status: fixed in `766968f`: every `uses:` is pinned to a full commit SHA with a version comment; `docker/login-action` was replaced by `docker login --password-stdin` into a per-job `DOCKER_CONFIG`; `persist-credentials: false` on every checkout. Tests: `tests/security/test_ci_workflows.py`.
- Where: `.gitea/workflows/ci.yml:40, 63-69, 90-108, 124-130, 160-166, 206, 242, 299-305,
324-330`; `.gitea/workflows/publish.yml:22-28, 47-53, 77-85, 138`. `docker/login-action@v3`
receives `REGISTRY_TOKEN` (`ci.yml:242-248`).
@@ -180,6 +194,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-08 (low): rate limiting gaps
- Status: fixed in `f57c237`: `/token` and `/revoke` rate limited; IPv6 clients keyed by /64; `ForwardedHeadersMiddleware` applies forwarded headers for `FORWARDED_ALLOW_IPS` peers and logs one warning for any other peer that sends `X-Forwarded-For`. Tests: `tests/security/test_rate_limits.py`.
- Where: `src/aegis_gitea_mcp/oauth/components.py:37` (`RATE_LIMITED_PATHS` lacks `/token` and
`/revoke`), `src/aegis_gitea_mcp/rate_limit.py:142-163` (per-IP key is the full address),
`src/aegis_gitea_mcp/transports.py:168-188`, `transports.py:400-419`.
@@ -197,6 +212,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-09 (low): OAuth pre-sign-in state can be flushed by unauthenticated floods
- Status: fixed in `87f048c`: per client address at most 20 pending sign-ins and 20 registrations per hour; clients younger than `PENDING_TTL` are never evicted; the store write runs in a worker thread. Tests: `tests/security/test_oauth_input_bounds.py`.
- Where: `src/aegis_gitea_mcp/oauth/provider.py:139-159` (`_ExpiringMap.put` evicts the oldest
entry), `provider.py:80-85`, `provider.py:333-356` (client eviction),
`src/aegis_gitea_mcp/oauth/store.py:250-275` (synchronous fsync on the event loop).
@@ -213,6 +229,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-10 (low): base URLs accept whitespace and control characters; plain http Gitea is silent
- Status: fixed in `d96c3ac`: whitespace, backslash and control characters rejected in both URLs, the URL is rebuilt from its parsed parts, `serve` warns for plain http to a non-loopback Gitea (a warning rather than a refusal, so existing lab setups keep working). Tests: `tests/security/test_base_url_validation.py`.
- Where: `src/aegis_gitea_mcp/config.py:87-103` (`_normalize_http_url` validates the parsed URL
but returns the raw string), `config.py:180-201`.
- Description: `https://gi\ttea.example.com`, `https://gitea.example.com/a\nb` and values with
@@ -230,6 +247,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-11 (low): audit log created with umask defaults; unkeyed hash chain
- Status: fixed, keyed chain waived (W-11) in `7307214`: directory 0700, log opened `O_NOFOLLOW` with mode 0600, an older loose log is tightened, startup exits 2 for a symlink. The optional `AUDIT_HMAC_KEY` was not added: off-host shipping is the documented integrity control. Tests: `tests/security/test_audit_file.py`.
- Where: `src/aegis_gitea_mcp/audit.py:36-39`, `audit.py:64-69`.
- Description: `DATA_DIR` is created with the default mode (0755 under a 022 umask) and the
audit log with 0644; only the OAuth store later tightens the directory, so in admin-token mode
@@ -244,6 +262,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-12 (low): tool audit entries do not record the client, session or source IP
- Status: fixed in `fd5e4b7`: tool entries carry `client_id`, a 12 character `session` hash and `client_ip`. Tests: `tests/security/test_audit_fail_closed.py`.
- Where: `src/aegis_gitea_mcp/server.py:509-537`.
- Description: in OAuth mode an entry names the Gitea login but not which MCP client or grant
made the call, nor the source address, which limits incident response (for example finding
@@ -255,6 +274,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-13 (low): token store directory ownership is not verified
- Status: fixed in `6f7a3c0`: the store directory must be owned by the effective user and must not stay group or world writable; checked on load and on every save. Tests: `tests/security/test_token_store.py`.
- Where: `src/aegis_gitea_mcp/oauth/store.py:133-143`.
- Description: an existing `DATA_DIR` owned by another user, or one that stays group or world
writable after the chmod attempt, only produces a warning. Whoever controls the directory can
@@ -267,6 +287,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-14 (low): CI cleanup and credential handling on the shared Docker daemon
- Status: fixed in `766968f`: cleanup removes containers only for per-run refs (`aegis-ci-*`, `:sha-*`) and only untags shared tags; the credential directory is per job; `uv publish` reads `UV_PUBLISH_PASSWORD`. Tests: `tests/security/test_ci_workflows.py` (fake `docker` on `PATH`).
- Where: `scripts/ci/cleanup_job_resources.sh:43-48`, `.gitea/workflows/ci.yml:281-290`,
`.gitea/workflows/publish.yml:150-157`.
- Description: the cleanup removes every container whose `ancestor` is one of the given refs,
@@ -283,6 +304,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-15 (low): `tests/security` missing; publish not gated on the security job
- Status: fixed in `766968f`: `tests/security` exists (this suite) and `publish.yml` has a `security` job that `publish` needs (gitleaks, security tests, pip-audit, ruff `S`). Tests: `tests/security/test_ci_workflows.py`.
- Where: `.gitea/workflows/ci.yml:337`, `.gitea/workflows/publish.yml` (needs only lint and
test).
- Description: `pytest tests/security` exits with code 4 (path not found), so the CI `security`
@@ -295,6 +317,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-16 (low): no container image vulnerability scan
- Status: fixed in `766968f`: Trivy 0.76.0 pinned with sha256 (`scripts/ci/install_trivy.sh`) scans the built image in `build-and-push` before the push and fails on fixable HIGH or CRITICAL findings (`scripts/ci/scan_image.sh`, waivers in `security/trivyignore.txt`); `scan.yml` repeats it weekly as the digest refresh reminder. Verified locally against the built image (clean).
- Where: `docker/Dockerfile:6-7`, `.gitea/workflows/ci.yml` (`build-and-push`).
- Description: base image digests are pinned and refreshed by hand; OS package advisories in
the image are never checked.
@@ -304,6 +327,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-17 (low): consent POST accepts a missing `Origin` header
- Status: fixed in `782d416`: consent POST needs an `Origin` equal to the public origin, with the `Referer` origin as fallback; missing, `null` or foreign values are refused. Tests: `tests/security/test_oauth_csrf.py`.
- Where: `src/aegis_gitea_mcp/oauth/callback.py:138-140`; ADR 0004 says approval needs "a
matching `Origin`".
- Description: the `SameSite=Strict` double-submit cookie is the effective CSRF control and is
@@ -315,6 +339,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-18 (low): unvalidated argument names and unknown tool names reach the audit log
- Status: fixed in `fd5e4b7`: only argument names that are schema properties are recorded (rest counted as `rejected_argument_count`); unknown tool names are recorded as `(unknown tool, N chars)` and not echoed. Tests: `tests/security/test_audit_fail_closed.py`.
- Where: `src/aegis_gitea_mcp/server.py:412-413` (argument names recorded before validation),
`server.py:428-431` (unknown tool name echoed and audited).
- Description: validation messages carefully avoid caller-chosen argument names because "they
@@ -329,6 +354,7 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
### SEC-19 (low): one fixed cookie name for concurrent sign-ins in the same browser
- Status: fixed in `782d416`: the binding cookie is named per flow (`aegis-oauth-flow-<8 hex of the state hash>`). Tests: `tests/security/test_oauth_csrf.py`.
- Where: `src/aegis_gitea_mcp/oauth/callback.py:56-60`, `callback.py:150-155`.
- Description: a second sign-in started in the same browser overwrites the first flow's binding
cookie. The first callback then fails and writes a high-severity
@@ -339,7 +365,10 @@ the Dockerfile (pinned digests, non-root, no secrets in layers).
- Negative test (`tests/security/test_oauth_csrf.py`): two interleaved flows in one browser
both complete; a callback without its own cookie still fails and is audited.
## Negative tests S2 must add to `tests/security/`
## Negative tests S2 added to `tests/security/`
All of the files below exist now (the list is kept as the specification they were written
from). Results are in the "Verification results" section of [security.md](security.md).
Some of these duplicate existing unit tests on purpose: the security suite is the one CI and
reviewers read, and it must stay green independently of refactors elsewhere. Reuse the helpers
@@ -415,7 +444,8 @@ in `tests/test_oauth_helpers.py` and `tests/test_executor_helpers.py`.
## B5 checker verdict
Verdict: CHANGES REQUIRED.
Verdict: CHANGES REQUIRED (at `ce5ae6c`; the blocking item was fixed in S2, `d5ca530`, and
every non-blocking item below was fixed or waived, see the status column above).
Blocking: