Skip to content

security: redact username and password in Request::toArray()#37

Open
LukasZemanNiceCZ wants to merge 1 commit intomasterfrom
security/redact-connection-credentials
Open

security: redact username and password in Request::toArray()#37
LukasZemanNiceCZ wants to merge 1 commit intomasterfrom
security/redact-connection-credentials

Conversation

@LukasZemanNiceCZ
Copy link
Copy Markdown

@LukasZemanNiceCZ LukasZemanNiceCZ commented Apr 9, 2026

Description: Redact username and password with *** in Request::toArray() to prevent credentials from appearing in logs
Possible impact: Logging output, any code that relies on toArray() or toString() returning real credentials


Problem

Request::toArray() includes the full connection params, including username and password in plaintext. This method is called in two places inside Client::_log():

  1. ConnectionException path — logged as error: $context->getRequest()->toArray()
  2. Normal request path — logged as debug: $context->toArray()

Additionally, platform-backend's custom slow query logger also calls toArray() when logging slow ES requests. This causes ES credentials to appear in application logs (e.g. Kibana) for every slow query.

Decision: always redact, no opt-out parameter

We considered adding a $hideSensitiveData = true parameter to allow the ConnectionException path to log credentials (potentially useful for debugging which connection failed). This was rejected because:

  • Anyone who can read error logs in production has the same access as anyone reading slow query logs — the security boundary is the same
  • password should never appear in logs regardless of context
  • Keeping username visible in ConnectionException logs (while redacting password) would add complexity for minimal gain
  • A simpler, consistent rule is easier to reason about

Change

In Request::toArray(), replace username and password in the connection params with ***:

$connectionParams = $this->_connection->getParams();
foreach (['username', 'password'] as $sensitiveKey) {
    if (isset($connectionParams[$sensitiveKey])) {
        $connectionParams[$sensitiveKey] = '***';
    }
}
$data['connection'] = $connectionParams;

Notes

  • No functional change — only affects the serialized representation used in logging/debugging
  • If a caller genuinely needs real credentials from a Request object, they should call getConnection()->getParams() directly
  • Submitting as draft for architecture review

🤖 Generated with Claude Code

@LukasZemanNiceCZ LukasZemanNiceCZ marked this pull request as ready for review April 9, 2026 14:02
Copilot AI review requested due to automatic review settings April 9, 2026 14:02
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens logging/debug serialization of Elastica\Request by redacting credentials from Request::toArray() so that ES auth details don’t leak into application logs (including slow-query logs that serialize requests).

Changes:

  • Redacts username and password fields from the connection parameters returned by Request::toArray().
  • Ensures Request::toString() / __toString() inherit the same redacted output via toArray().
Comments suppressed due to low confidence (1)

lib/Elastica/Request.php:213

  • Add a unit test to lock in the new behavior (e.g., in test/Elastica/RequestTest.php) asserting that when a Connection has username/password set, Request::toArray() (and toString()) return *** for those fields while leaving non-sensitive connection fields intact. This helps prevent accidental regressions that could reintroduce credential logging.
    public function toArray()
    {
        $data = $this->getParams();
        if ($this->_connection) {
            $connectionParams = $this->_connection->getParams();
            foreach (['username', 'password'] as $sensitiveKey) {
                if (isset($connectionParams[$sensitiveKey])) {
                    $connectionParams[$sensitiveKey] = '***';
                }
            }
            $data['connection'] = $connectionParams;
        }

        return $data;
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/Elastica/Request.php
Comment on lines +203 to +209
$connectionParams = $this->_connection->getParams();
foreach (['username', 'password'] as $sensitiveKey) {
if (isset($connectionParams[$sensitiveKey])) {
$connectionParams[$sensitiveKey] = '***';
}
}
$data['connection'] = $connectionParams;
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request::toArray() still returns the full connection params array including config (e.g., config['url'], config['headers'], and config['curl']). If config['url'] contains userinfo (e.g. https://user:pass@host) or headers/curl options contain credentials/tokens, those will still be logged even after redacting username/password. Consider redacting sensitive values inside config as well (at least strip userinfo from config['url'] and redact common auth headers/options).

Copilot uses AI. Check for mistakes.
Comment thread lib/Elastica/Request.php
Comment on lines +204 to +208
foreach (['username', 'password'] as $sensitiveKey) {
if (isset($connectionParams[$sensitiveKey])) {
$connectionParams[$sensitiveKey] = '***';
}
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This redaction list omits other secret connection params supported by the library, such as aws_secret_access_key / aws_session_token used by Transport\AwsAuthV4 (see lib/Elastica/Transport/AwsAuthV4.php). If those are set directly on the Connection, they will still appear in logs via Request::toArray(). Consider extending the sensitive key list (or using a centralized sanitizer) to cover these keys too.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants