36 KiB
Security review findings (S1)
Review of commit ce5ae6c on feat/full-api-coverage, 2026-10-14. Scope: every path from an
HTTP request or a stdio message to a Gitea call (transports.py, auth.py, server.py,
oauth/*, gitea/*, rate_limit.py, config.py), audit and logging, setup_cmd.py, the
Docker image and CI. The threat model is in security.md. S2 fixes these
findings and adds the negative tests to tests/security/.
Severity: critical, high, medium, low. Reproduction scripts referenced below were run in a scratch directory against the code as committed; each fix lists the negative test that proves it.
Summary
| 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 handling for OAuth users, path, query and header encoding in the executor, redirect handling, response size and JSON depth bounds, stdio stdout hygiene, error detail exposure, PKCE, code and refresh replay handling, consent CSRF and clickjacking headers, the browser-binding cookie, token store encryption and atomic writes, the setup command's env file handling, and the Dockerfile (pinned digests, non-root, no secrets in layers).
Findings
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 butapplication/x-www-form-urlencodedbefore a byte is read; form parsed withmax_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_submitcallsawait request.form()), routes atcallback.py:169-173. - Description: the browser routes are custom Starlette routes. Unlike the MCP endpoint and the
SDK's
/authorize,/token,/registerand/revoke(all wrapped inRequestBodyLimitMiddleware), they have no body limit, anduvicornsets none. Starlette parsesmultipart/form-datawith up to 1000 fields of up to 1 MiB each held in memory, and file parts of unlimited size spooled to/tmp, which is a RAM-backed tmpfs in the shippeddocker-compose.yml. TheOrigincheck passes when the header is absent, and the CSRF check only runs after parsing. Reproduced: one unauthenticated POST with 500 one-megabyte multipart fields raised the server's RSS from 93 MB to 593 MB; a single 200 MB file part was consumed completely before the 400 answer. The per-IP limit (120 requests per minute) counts requests, not bytes, so a single client can exhaust memory and get the process killed. OAuth mode only. - Fix: wrap the consent and callback routes in
RequestBodyLimitMiddlewarewith a small cap (16 KiB is ample: the form has three short fields); answer 415 before reading the body when the content type is notapplication/x-www-form-urlencoded; parse withrequest.form(max_files=0, max_fields=8, max_part_size=4096). - Negative test (
tests/security/test_oauth_input_bounds.py): drive the ASGI app with a countingreceive; a 1 MiB urlencoded POST to/oauth/consentgets 413 having read at most the cap plus one chunk; a multipart POST gets 415 having read no body; a valid consent still works.
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 allSECRET_DETECTION_MODEvalues. Tests:tests/security/test_secret_echo.py. - Where:
src/aegis_gitea_mcp/gitea/executor.py:339-356and603-625(only values submitted in the request body are redacted, viasensitive_body_valuesinsrc/aegis_gitea_mcp/gitea/encoding.py:387-405). - Description: the Gitea
Hookschema includesauthorization_header, and Gitea fills it on list, get and edit. A create call redacts the header because the caller submitted it, butgitea_hook_repo_list_hooks,gitea_hook_repo_get_hookand the org, user and admin equivalents (andgitea_api_requeston hook paths) return stored headers verbatim to the model and the remote client. Reproduced with a mockedrepoListHooksanswer: the header value appears in the result. Pattern masking does not catchBasic ...or mostBearer ...values. - Fix: redact known secret-bearing response fields in
_seal, before pattern masking: a non-emptyauthorization_headerat any depth becomes[REDACTED](presence stays visible). Keep the set (_SENSITIVE_RESPONSE_KEYS) next to_SENSITIVE_BODY_KEYSso new fields are added in one place. Credential-issuing operations are unaffected (no hook operation is one). - Negative test (
tests/security/test_secret_echo.py): list, get and edit hook responses (repo, org, user, admin, and the raw tool on/repos/o/r/hooks) containingauthorization_headernever contain the value incontentorstructured_content, withSECRET_DETECTION_MODEset tooff,maskandblock.
SEC-03 (medium): audit write failures after startup do not stop later calls
- Status: fixed in
43eebd3: the write-ahead alternative: atool_call_startedentry is written before every known tool runs and the call is refused withaudit_unavailablewhen it fails. Documented indocs/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) andserver.py:509-537(write errors are logged and swallowed);src/aegis_gitea_mcp/audit.py:104-107. - Description: the fail-closed check only proves the file could be opened once. Once the singleton exists, a full disk, a revoked mount or any other write error makes every later call run unaudited, indefinitely, while AGENTS.md promises an audit event for every call. The post-execution swallow is intentional for the call that already ran, but nothing stops the next one.
- Fix: record audit health. On a write failure set
AuditLogger.healthy = False; at the start ofcall_tool, refuse withaudit_unavailablewhile unhealthy, after one probe write (a smallaudit_probesecurity event) fails. Alternatively write atool_invocation_startedentry before execution and refuse when it fails. Either way, document the choice indocs/audit-logging.md. - Negative test (
tests/security/test_audit_fail_closed.py): after one successful call, make the log file'swriteraiseOSError; the next call returnsaudit_unavailableand the mocked Gitea receives no request; when writes work again, calls resume.
SEC-04 (medium): settings read a .env file from the process working directory
- Status: fixed in
79aa4f7:./.envloads only as a regular file owned by the user with mode 0600;--env-fileandAEGIS_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
./.envfor every setting not present in the environment. Claude Code and other clients start stdio servers with the project directory as the working directory, so a cloned repository can plant a.env. Reproduced: withGITEA_URLandGITEA_TOKENin the environment, a world-readable.envin the working directory setSECRET_DETECTION_MODE=off,EXPOSE_ERROR_DETAILS=trueandAUDIT_LOG_PATH=/dev/null(all calls then go unaudited, and the startup audit check passes). A client configuration withoutGITEA_URLin the environment would also send the token to an attacker-chosenGITEA_URL. - Fix: honour
./.envonly when it is a regular file (not a symlink) owned by the current user and not readable or writable by group or others (the modesetupwrites, 0600); otherwise ignore it and print one warning to stderr. Add an explicit--env-fileoption (andAEGIS_ENV_FILE) for other locations. Document it indocs/configuration.md. - Negative test (
tests/security/test_env_file.py): a 0644.envin the working directory withSECRET_DETECTION_MODE=offandAUDIT_LOG_PATH=/dev/nullis ignored (mode staysmask, the audit path stays underDATA_DIR); a symlinked.envis ignored; a 0600.envowned by the user is honoured; environment values still win.
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: optionalOAUTH_REDIRECT_ALLOWLIST(exact URIs orhttps://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
URI and any
client_name. Gitea remembers consent for our single application, so the only thing between a phishing link and a victim's full-scope token (defaultwrite:on every category, including admin for site admins) is a click on our consent page, which shows an attacker-chosen name (for example "Claude") and the redirect URI. The design is documented in ADR 0004; this is an owner decision on residual risk. - Fix (owner to choose): add
OAUTH_REDIRECT_HOSTS, an allowlist of redirect hosts accepted at registration (default: loopback plus the hosted clients the owner uses, for exampleclaude.aiandchat.mistral.ai), with a clear registration error otherwise; on the consent page show the redirect host first and prominently and the client's registration age. Or accept the risk and record it as a waiver indocs/security.md. - Negative test (
tests/security/test_oauth_redirects.py): with the allowlist set, registeringhttps://evil.example/cbreturnsinvalid_redirect_uri; allowlisted and loopback URIs register; existing rules (http on non-loopback, fragments, credentials, custom schemes) stay rejected.
SEC-06 (medium): fork pull requests run Docker-capable jobs on the shared runner
- Status: fixed in
766968f:test-integration,test-mcpandbuild-and-pushcarry the fork guard. The Gitea setting "require approval for workflows from forks" is a manual step recorded indocs/deployment.md. Tests:tests/security/test_ci_workflows.py. - Where:
.gitea/workflows/ci.yml:22-25(pull_requesttodevandmain), jobstest-integration(ci.yml:119-144) andtest-mcp(ci.yml:153-185). - Description: a pull request from a fork runs its own workflow and test code, and these jobs use the runner's Docker daemon (building images, starting containers, joining networks). Access to the daemon is root-equivalent on the host that other tenants share. Secrets are not passed to fork runs, but the host is.
- Fix: enable "require approval for workflows from forks" (or the equivalent runner setting) in
Gitea, and guard the Docker jobs with
if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name == github.repository. Record the setting indocs/deployment.mdor the CI notes. - Negative test (
tests/security/test_ci_workflows.py): parse the workflow YAML; every job that callsdocker(or the integration and MCP smoke jobs by name) carries the fork guard.
SEC-07 (medium): CI actions pinned by mutable tags, one of them receives the registry token
- Status: fixed in
766968f: everyuses:is pinned to a full commit SHA with a version comment;docker/login-actionwas replaced bydocker login --password-stdininto a per-jobDOCKER_CONFIG;persist-credentials: falseon 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@v3receivesREGISTRY_TOKEN(ci.yml:242-248). - Description:
actions/checkout@v4,actions/setup-python@v5,astral-sh/setup-uv@v5,actions/upload-artifact@v4anddocker/login-action@v3resolve to whatever the tag points to at run time. A compromised or retagged action runs with the job's secrets and the Docker daemon.actions/checkoutalso persists the job token in.git/configfor later steps. - Fix: pin every
uses:to a full commit SHA with the version in a comment; replacedocker/login-actionwithecho "$REGISTRY_TOKEN" | docker login --username ... --password-stdin; setpersist-credentials: falseon checkout where pushing is not needed. - Negative test (
tests/security/test_ci_workflows.py): everyuses:value in.gitea/workflows/*.ymlmatches@[0-9a-f]{40}$; no step passessecrets.*to a third-party action'swith:.
SEC-08 (low): rate limiting gaps
- Status: fixed in
f57c237:/tokenand/revokerate limited; IPv6 clients keyed by /64;ForwardedHeadersMiddlewareapplies forwarded headers forFORWARDED_ALLOW_IPSpeers and logs one warning for any other peer that sendsX-Forwarded-For. Tests:tests/security/test_rate_limits.py. - Where:
src/aegis_gitea_mcp/oauth/components.py:37(RATE_LIMITED_PATHSlacks/tokenand/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. - Description:
/tokenand/revoke(client secret checks, store writes) are not limited. Each IPv6 address is its own bucket, so a client with a /64 is effectively unlimited. Behind a proxy whose address is not inFORWARDED_ALLOW_IPS(the default127.0.0.1does not match a Docker network), every request shares the proxy's bucket, and because the limit runs before authentication, any unauthenticated client can exhaust the budget of all authenticated users. - Fix: add
/tokenand/revoketoRATE_LIMITED_PATHS; key IPv6 clients by their /64; log a one-time warning when a request carriesX-Forwarded-Forfrom a peer outsideFORWARDED_ALLOW_IPS. - Negative test (
tests/security/test_rate_limits.py): limit+1 requests to/tokenget 429; two addresses in one IPv6 /64 share a bucket; a forwarded header from an untrusted peer logs the warning once.
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 thanPENDING_TTLare 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.putevicts 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). - Description: 1024
/authorizerequests (needs only a self-registered client) evict every legitimate pending sign-in; 500 registrations evict recently registered clients that have not finished signing in. Every registration writes and fsyncs the whole store on the event loop. Impact: sign-in denial of service, no data exposure. - Fix: per-IP quota on pending requests and registrations (for example 20 each); never evict a
client younger than
PENDING_TTL; runOAuthStore._writein a worker thread (anyio.to_thread.run_sync). - Negative test (
tests/security/test_oauth_input_bounds.py): 1100/authorizecalls from one IP do not evict a pending request from another IP; a client registered a minute ago survives a registration flood.
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,servewarns 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_urlvalidates the parsed URL but returns the raw string),config.py:180-201. - Description:
https://gi\ttea.example.com,https://gitea.example.com/a\nband values with spaces pass validation;urlsplitstrips tabs and newlines for the check while the raw value is used later, so the validated and the used URL can differ. A plainhttp://GITEA_URLto a non-loopback host is accepted at serve time without any warning, although the admin token, the OAuth client secret and user tokens then travel in clear text (setup warns,servedoes not). - Fix: reject any whitespace or non-printable character in both URLs; return the URL rebuilt
from its parsed parts; print a startup warning (or refuse unless an explicit
ALLOW_INSECURE_GITEA_URL=true) for http to a non-loopback host. - Negative test (
tests/security/test_base_url_validation.py): parametrized rejection of tab, newline, carriage return, space, NUL and bidi controls inGITEA_URLandPUBLIC_BASE_URL; existing rejections (scheme, credentials, query, fragment, bad port, missing host, http public base URL) kept;servewithGITEA_URL=http://gitea.internalprints the warning.
SEC-11 (low): audit log created with umask defaults; unkeyed hash chain
- Status: fixed, keyed chain waived (W-11) in
7307214: directory 0700, log openedO_NOFOLLOWwith mode 0600, an older loose log is tightened, startup exits 2 for a symlink. The optionalAUDIT_HMAC_KEYwas 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_DIRis 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 other local users can read logins, paths and timing of every call. The audit path is opened withoutO_NOFOLLOW. The SHA-256 chain has no key, so anyone with write access can rewrite it consistently, and truncating the tail is not detectable. - Fix: create the directory with 0700 and open the log with
os.open(path, O_WRONLY | O_APPEND | O_CREAT | O_NOFOLLOW, 0o600); document off-host shipping as the integrity control, optionally add anAUDIT_HMAC_KEYfor a keyed chain. - Negative test (
tests/security/test_audit_file.py): after the first call the log is 0600 and a newly createdDATA_DIRis 0700; a symlink at the audit path makes startup exit 2.
SEC-12 (low): tool audit entries do not record the client, session or source IP
- Status: fixed in
fd5e4b7: tool entries carryclient_id, a 12 charactersessionhash andclient_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 which registered client a stolen token belonged to).
- Fix: add
client_idfrom the verified access token, a short hash of the session id and the client IP (when on HTTP) to the tool invocation entry. - Negative test (
tests/security/test_audit_fail_closed.pyor a sibling): an OAuth call's entry contains the client id and no token or session id in clear.
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_DIRowned by another user, or one that stays group or world writable after the chmod attempt, only produces a warning. Whoever controls the directory can replace the store with an older copy (resurrecting revoked grants) or delete it. - Fix: refuse (raise
OAuthStoreError) when the directory's owner is not the effective user or it is still writable by group or others. - Negative test (
tests/security/test_token_store.py): with a directory reported as owned by another uid (monkeypatchedos.stat) or mode 0777 that cannot be tightened, loading or saving fails with a clear message.
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 publishreadsUV_PUBLISH_PASSWORD. Tests:tests/security/test_ci_workflows.py(fakedockeronPATH). - 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
ancestoris one of the given refs, and the build job passes the shared tags:latest,:mainand:dev; on a daemon that also runs a deployment of this image that can remove containers that are not the job's.docker logoutacts on the shared credential store when the job is not containerized.uv publish --password "${REGISTRY_TOKEN}"puts the token on the command line. - Fix: remove containers only for per-run refs (
aegis-ci-<run id>-*and the job's own:sha-tag); for shared tags only untag (docker image rmof the tag); pass the publish token throughUV_PUBLISH_PASSWORD. - Negative test (
tests/security/test_ci_workflows.py): run the cleanup script with a fakedockeronPATHthat records argv;rm -fis only issued for containers of per-run refs;publish.ymlcontains no--passwordargument.
SEC-15 (low): tests/security missing; publish not gated on the security job
- Status: fixed in
766968f:tests/securityexists (this suite) andpublish.ymlhas asecurityjob thatpublishneeds (gitleaks, security tests, pip-audit, ruffS). 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/securityexits with code 4 (path not found), so the CIsecurityjob fails today. The publish workflow ships packages without gitleaks, pip-audit or the security tests. - Fix: S2 creates the suite listed below; add a
securityjob (pip-audit, ruffS, security tests, gitleaks) topublish.ymland makepublishneed it. - Negative test: the suite itself; a CI workflow test asserting
publishneeds the security job.
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 inbuild-and-pushbefore the push and fails on fixable HIGH or CRITICAL findings (scripts/ci/scan_image.sh, waivers insecurity/trivyignore.txt);scan.ymlrepeats 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.
- Fix: add a scan step (Trivy or Grype pinned by digest) on the built image, failing on high and critical with a waiver file, and a scheduled digest refresh reminder.
- Negative test: CI only (the step runs and fails on a known vulnerable test image).
SEC-17 (low): consent POST accepts a missing Origin header
- Status: fixed in
782d416: consent POST needs anOriginequal to the public origin, with theRefererorigin as fallback; missing,nullor 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 matchingOrigin". - Description: the
SameSite=Strictdouble-submit cookie is the effective CSRF control and is sound, but the code and the ADR disagree. Browsers sendOriginon form POSTs. - Fix: require
Originto equal the public origin (fall back toRefererwhenOriginis absent), or correct the ADR. - Negative test (
tests/security/test_oauth_csrf.py): POST withoutOriginandReferer, withOrigin: null, and with a foreign origin all get 400 and no redirect.
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 asrejected_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 could carry a secret", but the audit entry records them before validation, and an unknown tool name (up to 128 characters) is written to the audit log and echoed in the error. Pattern masking still runs, so only non-pattern secrets are affected.
- Fix: record only argument names that are properties of the tool's schema (plus a count of rejected ones); record unknown tool names as a fixed marker plus length, and do not echo them.
- Negative test (
tests/security/test_redaction.py): a call with an extra argument named like a 40-hex token, and a call to a tool named like one, leave neither value in the audit file or the error.
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
oauth_callback_binding_mismatchaudit event, a false alarm that trains operators to ignore the one event that signals a confused-deputy attempt. - Fix: suffix the binding cookie name with a short hash of the Gitea
state(__Host-aegis-oauth-flow-<8 hex>), and look it up by the state on the callback. - 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 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.
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
in tests/test_oauth_helpers.py and tests/test_executor_helpers.py.
test_auth_required.py/mcpwithoutAuthorizationgets 401 withWWW-Authenticate(bearer-only app and OAuth app).- Wrong bearer gets 401: same length, shorter, longer, empty, wrong scheme (
Basic,token), trailing whitespace, the bearer of another deployment. - The static bearer is compared with
hmac.compare_digeston digests (spy). - OAuth: expired, revoked, other-resource and unknown
agm_tokens get 401; anagm_token never resolves to the admin identity (Gitea mock sees the user's token only). - Route inventory: the set of routes reachable without authentication is exactly
/,/health, the.well-knowndocuments and the OAuth routes; adding a route fails the test.
test_startup_fail_closed.py- Non-loopback binds (
0.0.0.0,::, a public IP, a host name,127.1,0) without bearer or OAuth exit 2 and never calluvicorn.run. - Half-configured OAuth, a bearer shorter than 32 characters, an http
PUBLIC_BASE_URLon a public host, an unwritable audit log and (SEC-11) a symlinked audit path all exit 2.
- Non-loopback binds (
test_redaction.py- With all secrets configured (
GITEA_TOKEN,MCP_BEARER_TOKEN,GITEA_OAUTH_CLIENT_SECRET,TOKEN_ENCRYPTION_KEY, a user's Gitea access and refresh tokens, an issuedagm_token), drive a Gitea error that echoes each of them and a success body that contains each of them: none appears in the result, the error message, the audit file, or captured logs at DEBUG. uvicorn.accessrecords for/oauth/callback?code=...&state=...and/oauth/consent?request=...contain[redacted]instead of the query.reprofSettings,Identity,SessionRecord,UpstreamTokensandPendingAuthorizationcontains no secret.- Validation errors never contain argument values (SEC-18 for names).
- With all secrets configured (
test_secret_echo.pygitea_actions_repo_list_secretsandgitea_actions_org_list_secretsreturn the names Gitea sends; the update tools never echo the submitteddata, including in a Gitea 422 error that repeats it.- SEC-02: hook list, get and edit responses never contain
authorization_headervalues, in everySECRET_DETECTION_MODE. - Submitted
password,secret,token,authorization_headerand migration credentials are redacted from results. - Credential-issuing operations return the new credential (documented exemption, W-07) but the audit file and logs do not contain it.
test_malformed_input.py- Unknown arguments, wrong types, missing required arguments rejected before Gitea is contacted.
- Path values with
..,., empty segment,\,?,#,%,%2e, NUL, CR, LF, U+2028 rejected for catalog path parameters and for the raw tool path; a raw path starting//rejected. - Query names
token,access_token,sudo,_sudoin any case rejected. sudowith CR/LF, spaces or over 255 characters rejected.- Upload: oversized base64 rejected before decoding, invalid base64, filenames with quotes or CR/LF, unknown file keys.
- Raw method outside the enum;
READ_ONLYwith a non-GET raw call. - Deeply nested JSON arguments and bodies give an error result, never an exception.
- HTTP: body over the cap gets 413, non-JSON content type 400, foreign
Host421, foreignOrigin403.
test_base_url_validation.pyGITEA_URLandPUBLIC_BASE_URL: non-http schemes, credentials, query, fragment, bad port, missing host, whitespace and control characters (SEC-10), http public base URL on a public host; trailing/api/v1normalized.MCP_PATH,FORWARDED_ALLOW_IPSandOAUTH_SCOPES(unknown scope) rejections.- Redirect URIs at registration: http on non-loopback, fragment, credentials,
javascript:, custom schemes rejected.
test_oauth_csrf.py(SEC-17, SEC-19): consent without the CSRF cookie, with a mismatched token, without or with a foreignOrigin; callback without the binding cookie, with a replayed or expired state; none of them redirects.test_oauth_input_bounds.py(SEC-01, SEC-09).test_audit_fail_closed.pyandtest_audit_file.py(SEC-03, SEC-11, SEC-12).test_env_file.py(SEC-04).test_token_store.py: file mode 0600, directory 0700, symlink refused, wrong key stops startup without wiping, store content is not plaintext, our tokens only as hashes, SEC-13.test_rate_limits.py(SEC-08).test_ci_workflows.py(SEC-06, SEC-07, SEC-14, SEC-15).
B5 checker verdict
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:
- SEC-01: the unauthenticated consent POST reads unbounded multipart bodies into memory and tmpfs. This is B5 code and reproducible with one request. Fix and its negative test are required before B5 is accepted.
Non-blocking (fix in S2, or record as waivers): SEC-05 (redirect allowlist, owner decision), SEC-09, SEC-12, SEC-13, SEC-17 (code or ADR), SEC-19.
What was checked and holds:
- Identity separation: an access token with the OAuth marker always resolves to its own session or fails; it never falls back to the admin token, even when the session ends or the upstream refresh fails. Only the static bearer (or no token on stdio and unauthenticated loopback) reaches admin-token mode; an OAuth-only deployment fails closed for it. The session login is checked against the token subject.
- Authorization server: DCR client ids are minted by the SDK; PKCE S256 is mandatory towards clients and used towards Gitea; redirect URIs are validated at registration and matched by the SDK; codes are single use, bound to the client, replay revokes the grant; refresh tokens rotate and reuse revokes the grant; revocation is limited to the owning client by the SDK; tokens are bound to the MCP resource and scope.
- Browser flow: consent CSRF double submit with a
SameSite=Strict__Host-cookie, a browser-binding cookie on the callback, single-use states, no redirect on any invalid callback,no-store,no-referrer,DENYframing and CSP on every page, HTML escaping of the client name and redirect URI, no request parameter echoed. - Upstream: confidential client with form-encoded credentials, no redirects followed, coarse error kinds only, login validated against a strict pattern, token type and length checked, unknown Gitea scopes rejected at configuration.
- Store: Fernet with an HKDF-SHA256 key, mode 0600 through
O_EXCLtemporary files and atomic rename, symlinks refused, a wrong key or corrupt file stops startup without wiping, our tokens stored only as hashes, pruning of expired tokens and abandoned sessions, bounded clients and sessions per login. - Secret handling: the user's refresh token, the store key and the client secret are redacted
from results; issued
agm_tokens are masked by pattern; access-log query strings are dropped. - Quality gates at
ce5ae6c: 157 OAuth tests and the whole unit suite pass (846 passed, 23 skipped), coverage 98.2 percent,ruff check,ruff format --checkandmypyclean. The docs drift check failed only on the staledocs/security.md, replaced in this changeset.