feat: keep secrets out of MCP client context (path guard, content and shape redaction) - #79
Open
flow-systems wants to merge 7 commits into
Open
feat: keep secrets out of MCP client context (path guard, content and shape redaction)#79flow-systems wants to merge 7 commits into
flow-systems wants to merge 7 commits into
Conversation
Field-name based redaction (DOKPLOY_REDACT_ENV) cannot protect the tools that return raw file contents. docker-readContainerFile, dockerVolume-readVolumeFile and settings-readTraefikFile hand back the file as an opaque string, so a request for /app/.env delivers every variable to the model with redaction fully enabled. Guard the request side instead: before a tool that takes a filesystem path runs, normalize that path (resolving traversal segments and percent-encoding) and match it against a deny-list of secret-bearing globs. On a match the call is refused with an actionable error; every other use of the tool is unaffected, so reading /app/logs/error.log still works. The guard keys off the presence of a string 'path' argument rather than a hardcoded tool-name list. Exactly ten of the 597 generated tools take one, all of them genuine file operations, so newly added file tools are covered without maintenance. Configurable via DOKPLOY_BLOCK_SECRET_PATHS (default true) and DOKPLOY_SECRET_PATH_PATTERNS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 71b802082b45dc7d14945633f01cba561984d57c)
The path deny-list only covers the locations someone anticipated. When a file-access tool legitimately returns contents, secrets sitting inside that string are still invisible to field-name redaction, because the field wrapping them is called something neutral like 'content'. Scan those contents for KEY=value assignments and mask the value side whenever the key names a secret. The decision reuses DOKPLOY_REDACT_FIELDS rather than introducing a second list, so operators keep one place to configure what counts as a secret: DB_PASSWORD is masked, PORT is not. Screaming-snake keys need one extra angle. The suffix list is written for camelCase JSON fields, so SECRET_KEY collapses to 'secretkey' and ends in neither 'secret' nor any listed *Key entry. Each underscore-delimited word is therefore compared as well, by exact match to keep the widening narrow. The pass runs only on responses from tools that take a path argument, so ordinary payloads are untouched, and it follows DOKPLOY_REDACT_ENV. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 7c6658fdb6fc143e570a20f10116b6738c0df362)
The unit tests exercise the path matcher and the assignment masker in isolation; nothing proved they are actually wired into the request path. Drive createHandler directly with a mocked API client and config to assert the behaviour operators rely on: a dotenv read is refused before any request goes out, a traversal that resolves onto a secret is refused too, ordinary files still pass, and both guards honour their off switches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 0af6c8fe0f183f405a84ffeba1a01b681ece23fc)
The deny-list was built against credential files — dotenv, keys, token stores. It missed the data itself. Relational engines keep row data unencrypted on disk, so docker-readContainerFile on /var/lib/postgresql/data/base/16384/2619 returns table contents in the clear: no SQL, no password, not even a running server. For a payroll or customer database that is a larger exposure than the credentials the list already covered. Add the storage locations of Postgres, MySQL, MongoDB, Redis and SQLite. Entries are anchored to absolute paths or to distinctive directory names (pgdata, postgresql/data) rather than bare names, so that an unrelated node_modules/mysql or src/postgresql/ is not blocked by accident; both cases are covered by tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 438e8ec66e6dc667029d9d73e1c6b689abf01304)
DOKPLOY_REDACT_FIELDS and DOKPLOY_SECRET_PATH_PATTERNS replace the built-in defaults wholesale. That puts the common intent — "the defaults, plus this one entry" — on the dangerous path: the operator writes out the full list from memory or from an older README, and silently loses protections they never meant to touch. The resulting config looks more careful while being materially weaker, and nothing reports it. Add DOKPLOY_EXTRA_REDACT_FIELDS and DOKPLOY_EXTRA_SECRET_PATHS, which append to whatever is active and deduplicate. Full overrides still work but now log a warning naming the EXTRA_ variable, so that weakening a security default is at least visible in the startup output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 819e32e575e66a2e67f2540c3ba9437756d39add)
Log tools return prose. deployment-readLogs, application-readLogs and compose-readLogs hand their output straight to the model, and neither existing pass can help there: field-name redaction needs a name, and the assignment pass needs a KEY=value line. A build script that echoes a connection string leaves it fully readable. What remains is the shape of the secret. Scan every response for formats distinctive enough to recognise on sight: credentials inside a URI, Authorization headers, PEM blocks, JWTs, and the token formats of GitHub, GitLab, AWS, OpenAI, Slack and Stripe. Connection strings keep scheme, user and host so the line still helps during debugging. Applied to all responses rather than only the log tools, because a PEM block is a secret whichever endpoint returned it, and gated behind the existing DOKPLOY_REDACT_ENV rather than a new switch. The rule list is deliberately short and carries no entropy heuristics: a log mangled by false positives is useless exactly when it is needed. The limitation is explicit in a test — a password written as prose is not detected, and no pattern work would change that safely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 2ebacad4f3f74e95ca1c570c498ebd3f71320982)
The container file APIs return a file base64-encoded, not as plain text. Both content passes therefore did nothing for the tools they were written for: the assignment pass looks for KEY=value lines and the shape pass for recognisable formats, and a base64 blob contains neither. A secret in a file the path guard did not cover travelled through fully intact, merely encoded — which is no obstacle to a model that can decode it. Decode before masking and re-encode afterwards. A payload is only treated as text when it round-trips back to the exact same base64, which rejects both coincidental alphabet matches and binary files, whose bytes do not survive a UTF-8 conversion. When nothing was masked the original string is returned byte-identical, so untouched responses keep their exact encoding rather than being silently rewritten. Worth recording how this was missed: the unit tests were written against plain text and all passed, while the layer was inert in production. It only surfaced when the guards were tried against the live API. The new handler tests drive the encoded shape the API actually returns. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit e1cae88ea155e5dd4cd566435805badfdfa39228)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DOKPLOY_REDACT_ENVmasks secret-bearing fields by matching the field name ofan API response. That covers structured payloads well, but three classes of
response slip past it entirely:
1. Raw file contents.
docker-readContainerFile,dockerVolume-readVolumeFileandsettings-readTraefikFilereturn a file as anopaque string. The field wrapping it is called something neutral, so name-based
matching never fires. With redaction fully enabled, asking for
/app/.envreturns every variable in it.
2. Database storage. Relational engines keep row data unencrypted on disk.
Reading
/var/lib/postgresql/data/base/16384/2619returns the contents of atable in the clear — no SQL, no password, not even a running server. For a
payroll or customer database that is a larger exposure than the credentials the
field list already covers.
3. Log output.
deployment-readLogs,application-readLogsandcompose-readLogsreturn prose. There is no field name and no path to key off,so a build script that echoes a connection string hands it over intact.
Change
Three layers, all gated behind the existing
DOKPLOY_REDACT_ENVrather than newmaster switches.
Path guard (
DOKPLOY_BLOCK_SECRET_PATHS, defaulttrue). Before a tool thattakes a filesystem
pathruns, that path is normalised — traversal segments andpercent-encoding are resolved — and matched against a deny-list of globs:
dotenv files,
/run/secrets/**, private keys, credential stores, and the storagedirectories of Postgres, MySQL, MongoDB, Redis and SQLite. On a match the call is
refused with an actionable error. Everything else about the tool keeps working,
so
/app/logs/error.logstill reads fine.The guard keys off the presence of a string
pathargument rather than ahardcoded tool-name list. Exactly ten of the generated tools take one, all of
them genuine file operations, so tools added later are covered without
maintenance.
Content redaction. When a file-access tool does return contents, those
contents are scanned for
KEY=valueassignments and the value is masked when thekey names a secret — reusing
DOKPLOY_REDACT_FIELDSrather than introducing asecond list.
DB_PASSWORD=hunter2is masked,PORT=3000stays readable.Shape redaction. Every response is additionally scanned for secret formats:
credentials inside a URI,
Authorizationheaders, PEM blocks, JWTs, and thetoken formats of GitHub, GitLab, AWS, OpenAI, Slack and Stripe. A connection
string keeps everything but its password, so
postgres://appuser:hunter2@db:5432/appbecomespostgres://appuser:[REDACTED]@db:5432/appand remains useful for debugging.Also included:
DOKPLOY_EXTRA_REDACT_FIELDSandDOKPLOY_EXTRA_SECRET_PATHS, which appendto the defaults. The existing variables replace them wholesale, which puts the
common intent — "the defaults, plus this one entry" — on the dangerous path:
the operator writes out the full list from an older copy and silently loses
protections. A full override still works but now logs a warning.
content passes decode before masking and re-encode afterwards. A payload is
only treated as text when it round-trips back to identical base64, which
rejects binary files and coincidental alphabet matches. Untouched responses are
returned byte-identical.
Behaviour change to be aware of
The path guard is on by default, so a caller that reads
.envfiles throughdocker-readContainerFiletoday will start getting refusals. That default is adeliberate proposal, not an oversight — the failure mode of the other direction
is silent — but it is the obvious thing to push back on, and
DOKPLOY_BLOCK_SECRET_PATHS=falserestores the previous behaviour exactly.Scope
This guards what reaches the MCP client. It is not a permission boundary:
anyone holding the same
DOKPLOY_API_KEYcan call the Dokploy API directly. Italso cannot detect a password written as prose (
Connecting as admin with password hunter2), and no entropy-based guessing is attempted — a log shreddedby false positives is useless exactly when it is needed. Both limitations are
documented and one is pinned by a test.
Notes
pnpm type-checkclean.pnpm lintreports one pre-existing formatting error insrc/utils/redactSensitive.test.ts; this branch adds tests to that file butdoes not touch the offending lines.
opt-in first.
Disclosure: this was written by Claude (Anthropic) in a pair-programming
session — design, code, tests and this description. Every commit carries a
Co-Authored-Bytrailer. Two things worth knowing about that:The tests are AI-written as well, so they are not independent evidence that the
code is correct. One concrete example of why that matters: the content redaction
layer shipped in my own deployment for several hours doing nothing at all,
because the tests were written against plain text while the API returns base64.
Every test passed. It surfaced only when the guards were exercised against the
live API, and the fix is the last commit on this branch.
What I can vouch for is usage, not review: this runs against five Dokploy
instances plus one HTTP deployment, and the path guard was verified against a
real container —
.env, a traversal, a percent-encoded variant, an SSH key, aservice private key and a Postgres heap file were all refused, while ordinary
files kept reading normally. Please review it as unreviewed code from a stranger,
because in the ways that matter that is what it is.