Repository navigation
fix: keep credentials out of DSN parse failures - #10
Merged
Merged
Conversation
parse_url() rejects the mistakes people actually make in a DSN (an empty host after the userinfo, a port out of range, a secret key with an unencoded "/"), and the refusal quoted the whole DSN back, user and password included. Anything that logs the message, or lets the exception escape to stderr, wrote the credentials out with it. Nothing of the input is kept: redacting only the userinfo would mean picking apart a string parse_url() has just refused, and the query can carry secrets of its own. The scheme and host refusals already quoted nothing; the regression test pins all three. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With the message clean, the refusal's stack trace still printed the DSN argument, cut to zend.exception_string_param_max_len (15 bytes by default): the scheme, the user and the start of the password. Traces carry arguments unless zend.exception_ignore_args is on, and it is off by default and in the official PHP images, which ship no php.ini. #[\SensitiveParameter] prints the argument as Object(SensitiveParameterValue) instead. PHP before 8.2 ignores the attribute, so the trace test runs from 8.2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
SensitiveParameter only takes effect from PHP 8.2, and this package still supports 8.0 and 8.1, where the refusal's trace kept printing the DSN argument. A trace prints a parameter's current value rather than the one passed, so the constructor drops $dsn once parse_url() is done with it, and the trace test now runs on every supported version. The test no longer pins how PHP renders a redacted argument (NULL before 8.2, Object(SensitiveParameterValue) from 8.2): it asserts that neither the user nor the password is printed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
abnegate
added a commit
to open-runtimes/executor
that referenced
this pull request
Sep 24, 2026
getDevice() receives the connection string with its access and secret keys in it, and a refusal leaked it three ways: - its message forwarded utopia-php/dsn's, which in 0.2.1 and earlier quotes the whole DSN back when parse_url() rejects it; - the chained DSN exception carried that same message, and its DSN::__construct() frame, into anything that prints the exception; - every exception raised beneath getDevice() printed the connection argument in its trace, cut to 15 bytes by default, which is the scheme and the start of the credentials. Traces carry arguments unless zend.exception_ignore_args is on, and it is off by default and in appwrite/utopia-base, which ships no php.ini. The message matters most: the build path copies an exception's message into the build output it returns, so a malformed OPR_EXECUTOR_CONNECTION_STORAGE would put the operator's keys in build logs. The refusal now only says the DSN could not be parsed, and chains nothing, so what it prints no longer depends on which dsn version a consumer resolves (utopia-php/dsn#10 fixes the parser's message at the source). $connection is #[\SensitiveParameter], as utopia-php/storage already does for the secret key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Sep 24, 2026
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
1 participant
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.
What leaked
new DSN($dsn)threwInvalidArgumentException("Unable to parse DSN: $dsn")wheneverparse_url()failed. That quoted the whole DSN, user and password included.parse_url()fails on the mistakes people actually make in a DSN:s3://KEY:SECRET@/bucket)/, which AWS secret keys often containAny caller that logged the message or let the exception escape to stderr wrote the credentials out with it. We saw this in edge: backup Jobs run inline PHP that builds a storage device from
BACKUP_DSN, and an unparseable DSN printedUnable to parse DSN: s3://<access key>:<secret key>@/backups?...into the pod logs.The stack trace leaked as well. Traces carry arguments unless
zend.exception_ignore_argsis on. It's off by default, and the official PHP images ship no php.ini. So the refusal's trace printedDSN->__construct('s3://KEY:SECR...'), cut to 15 bytes: the scheme, the user and the start of the password.Fix
Unable to parse DSN: malformedand quotes nothing from the input. I didn't redact just the userinfo, because that means picking apart a stringparse_url()has just rejected, and the query can carry secrets too. The exception type is stillInvalidArgumentException.$dsnparameter is#[\SensitiveParameter], and the constructor unsets$dsnonceparse_url()has read it. A trace prints a parameter's current value, so the unset covers PHP 8.0 and 8.1, which ignore the attribute. The redacted frame prints asNULLon 8.0/8.1 andObject(SensitiveParameterValue)from 8.2.I checked all three throw sites. The scheme-required and host-required refusals already quoted nothing, and no other method echoes its input.
Tests
testRefusalMessageOmitsCredentialscovers one case per throw site (unparseable, no scheme, no host). It asserts that neither the user nor the password appears in the message. The unparseable case failed before this change withFailed asserting that 'Unable to parse DSN: s3://AKIAKEY:SECRETKEY@/backups?region=us-east-1' does not contain "AKIAKEY".testUncaughtRefusalPrintsNoCredentialsturns argument capture on with the length limit lifted, then asserts that the printed exception (message plus trace) holds neither credential. With the message fix alone, it failed onDSN->__construct('s3://AKIAKEY:SECRETKEY@/backups?region=us-east-1'). Without the unset, it still fails on 8.0 and 8.1, including when PHP starts withzend.exception_ignore_args=1.Run locally with dependencies resolved per version:
composer lint(Pint, psr12) passes.After merge
This needs a
0.2.2release. Then consumers should bump: open-runtimes/executor (0.2.*, lock at0.2.1; see open-runtimes/executor#255), appwrite-labs/edge (lock at0.2.1) and appwrite.🤖 Generated with Claude Code