fix(activesync): harden auth header, Autodiscover parsing/XML escaping, and ValidateCert#74
Merged
Merged
Conversation
Strip only a leading, case-insensitive "Basic " scheme token instead of str_replace(), which removed the token anywhere in the string, and decode strictly so malformed Authorization headers yield no credentials rather than arbitrary bytes.
Locate the Autodiscover EMailAddress element by tag name instead of a fixed offset ($values[2]), which selected the wrong node (or errored) when a client reordered or added elements. The wire value is always an email address; mapping it to a backend username remains the driver's job. XML-escape every value interpolated into the Autodiscover responses (email, display name, URLs, backend host/port, and the client-supplied request/response schemas echoed back into attributes). These were previously concatenated raw, corrupting the XML for values containing metacharacters and allowing content injection via the schema values.
openssl_x509_checkpurpose() returns true, false, or -1 on error. The -1 branch tested an undefined $results variable (typo for $result) and was dead code; reorder to check the -1 error case first with a strict comparison so certificate errors are reported as unknown rather than misclassified as an invalid purpose.
🔍 CI ResultsOverall: ❌ 12/12 lanes failed TL;DR: ❌ Quality issues: PHPUnit: 36 tests failed; PHPStan: 332 unique errors in 8 lanes. Summary by PHP Version
Quality Metrics
❌ Failed Lanesphp8.0-dev
php8.0-stable
php8.1-dev
php8.1-stable
php8.2-dev
php8.2-stable
php8.3-dev
php8.3-stable
php8.4-dev
php8.4-stable
php8.5-dev
php8.5-stable
CI powered by horde-components • View full results |
ralflang
approved these changes
Jul 2, 2026
Record the PR #74 review follow-up: certificate validation should become a validator interface owned by Horde\ActiveSync with the implementation and configuration owned by Core, rather than the inline OpenSSL checks in Request_ValidateCert.
ralflang
added a commit
that referenced
this pull request
Jul 4, 2026
Release version 3.0.1 fix(activesync): treat collection lock contention as transient during PING Merge pull request #74 from horde/security/activesync-hardening docs(activesync): note ValidateCert validator extraction in H6 todo fix(activesync): correct ValidateCert OpenSSL result handling fix(activesync): harden Autodiscover parsing and escape XML responses fix(activesync): harden HTTP Basic auth header parsing
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
2 participants
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.
Summary
Credentialshandler.EMailAddressby tag name and XML-escape all values interpolated into Autodiscover responses.ValidateCertOpenSSL result handling (dead/typo'd error branch).Motivation
A read-through of the ActiveSync request path surfaced a few low-level robustness and content-injection issues in the parts of the library that consume untrusted client input (the Authorization header, the Autodiscover request body, and S/MIME certificate validation).
Changes
Credentials.php— Strip only a leading, case-insensitiveBasicscheme token (str_replace()removed the token anywhere in the string) and decode strictly (base64_decode(..., true)), so malformed Authorization headers produce no credentials instead of arbitrary bytes.Request/Autodiscover.phpEMailAddresselement by tag name instead of the fixed offset$values[2], which selected the wrong node (or errored) when a client reordered/added elements. The wire value is always an email address per the Autodiscover protocol; mapping it to a backend username stays the driver's responsibility (getUsernameFromEmail()), so this is backend-agnostic (email-as-username, AD/LDAP, plain usernames)._xmlEscape()helper (ENT_QUOTES | ENT_XML1) and apply it to every value interpolated into the mobilesync/outlook/failure responses — including the client-supplied request/response schemas echoed back into XML attributes — closing a content-injection/XML-corruption gap.Request/ValidateCert.php—openssl_x509_checkpurpose()returnstrue/false/-1. The-1branch tested an undefined$resultsvariable (typo for$result) and was dead code; reorder to check=== -1first so certificate errors are reported as unknown rather than misclassified as an invalid purpose.Test plan
php -lclean on all changed filesAutodiscoverTest(4/4) andServerTest(2/2) passValidateCertwith a valid, expired, and untrusted S/MIME certificate