Skip to content

Support connection keyword aliases and PDO DSN passwords - #1684

Open
Jahnvi Thakkar (jahnvi480) wants to merge 4 commits into
devfrom
jahnvi/connection-keyword-aliases
Open

Jahnvi Thakkar (jahnvi480) wants to merge 4 commits into
devfrom
jahnvi/connection-keyword-aliases

Conversation

@jahnvi480

@jahnvi480 Jahnvi Thakkar (jahnvi480) commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Accept ConnectTimeout, FailoverPartner, and WorkstationID alongside LoginTimeout, Failover_Partner, and WSID in both SQLSRV and PDO_SQLSRV. Reuse the existing option IDs, ODBC keywords, and connection attributes, preserving compatibility with ODBC 17 and older ODBC 18 releases. MultipleActiveResultSets is already supported and is unchanged.
  • Accept Password as an alias for PWD in SQLSRV connection options. Add PWD and Password to PDO_SQLSRV DSNs. Password support is an additional PHP feature requested alongside the ODBC keyword alignment; it is not claimed as a documented ODBC 18.7 addition.
  • Preserve non-null PDO constructor passwords, including empty strings. Use the DSN password only when the constructor password is null/omitted. The username remains a constructor argument. Aliases share last-processed-value precedence.
  • Retain credential authentication restrictions and safe ownership through connection creation. Validate SQLSRV password types/references and reject embedded NULs in SQLSRV password values.
  • Address the secure-erasure review finding by routing parsed PDO passwords directly into a non-copyable, factory-owned buffer, before ordinary Zend-string/hash allocation. Wipe that owned allocation (including its terminator) with SecureZeroMemory/explicit_bzero before release or replacement, covering parser errors, missing server, constructor override, and connection success/failure. Reuse the same secure-erasure primitive for existing token cleanup.
  • Retain the reviewer's required focused native erasure regression under the existing test/tools/ directory: its PHP harness, C++ before-free observer, and Bash build runner exercise 22 cases including real connection success/failure. The Linux PR job runs it; production modules contain no observer API. No new test directory is introduced.
  • Bound shared connection-option brace validation to the supplied length. Valgrind exposed a read past a quoted value's terminator while exercising the new credential tests.
  • Add six PHPT regressions for alias acceptance, constructor/alias precedence, real password authentication and workstation values, malformed values, and brace boundaries. Update both access-token tests for the revised credential-conflict message and document the behavior in the changelog.
  • Fix macOS dependency setup by trusting the specific Microsoft Homebrew tap before brew tap evaluates its formulae. Install both packages using fully qualified names, and require successful, nonempty listing results so partial output cannot hide installation verification failures. No global trust bypass is used.
  • Fix the Homebrew 7.0.1 UID check failure by renaming the pipeline variable uid to sqlUser and updating all 23 references in azure-pipelines.yml. Azure otherwise exports UID=sa, shadowing Bash's real-user-ID variable. SQL authentication values, PHP/ODBC UID keywords, and existing test environment names are unchanged. No elevated execution or disabled Homebrew safety checks are introduced.
  • Scope cleanup removes the standalone macOS Python test suite and its requirements file. The net PR contains 22 files: driver code, changelog, existing pipeline adjustments, six PHPTs/two expectation updates, and the explicitly requested native security regression. No generated SDKs, build outputs, investigation files, or unrelated README changes are included.
  • Reproduce and fix the new alias test's Windows LocalDB cold-start failure: the first PWD connection exceeded its five-second timeout under coverage while later aliases passed. The two live alias tests now allow 30 seconds (20/30 for both timeout-name orderings); they are acceptance tests, not elapsed-time assertions. No retry, failure suppression, or skip was added. Connection failures print only SQLSTATE/native error codes.

Validation

  • Built pristine and patched SQLSRV and PDO_SQLSRV from source on Linux x64, PHP 8.4.16 NTS.
  • Demonstrated the original alias regressions failing before the implementation and passing afterward.
  • Relevant 22-test connection suite: 22/22 passed with ODBC 17.10.6.1, and 22/22 passed with ODBC 18.7.1.1; zero skips or warnings. These are targeted connection-suite results, not the entire functional suite.
  • The local ODBC matrix used an isolated test configuration that explicitly selected each driver; it does not imply that the committed test configuration automatically switches versions based on MSSQL_DRIVER_NAME.
  • Four no-server credential/brace PHPTs pass under Valgrind without reported errors.
  • Native secure-erasure regression: the original owned-password paths failed before the fix; 22/22 cleanup cases passed afterward, including parser errors, overwritten aliases in both orders, empty/one-byte values, constructor precedence, successful SQL connections, and authentication failures. These assertions inspect bytes before release rather than relying on leak checks.
  • PHP syntax validation passed for all 16 changed test code/skip sections. Changed production files passed Cppcheck 2.7; staged whitespace checks passed.
  • Native artifacts verified as ELF x64 shared libraries with get_module exported and runtime dependencies resolved.
  • Full prospective-PR security, correctness, end-to-end, maintainability, and adversarial reviews completed locally.
  • The removed macOS Python regression was retained outside the repository for local validation: 16/16 passed against the retained YAML. It is not a committed test dependency. YAML parsing, Bash/PHP syntax, diagnostics, and whitespace checks passed.
  • Built both extensions natively on Windows x64 for PHP 8.5.10 NTS, and tested using the checksum-verified official PHP 8.5.10 runtime with ODBC 18.5.2.1 and OpenCppCoverage 0.9.9.0. PE x64 architecture, get_module, and absence of observer exports verified.
  • 23/23 relevant Windows connection PHPTs passed under coverage, including the existing Azure Key Vault keyword test; zero skips or warnings. Original SQLSRV alias test failed with the exact CI diff from a stopped LocalDB; both updated live alias PHPTs passed independently from a stopped LocalDB.
  • Relocated native erasure test: 22/22 passed from the exact staged Linux source, including live SQL Server connections, after relocation into test/tools/.
  • Azure run 174790 on e3b29086: Linux and macOS succeeded. Windows compiled successfully but failed the cold-start alias test and an existing Azure Key Vault test with 0xC0000005. The latter has not reproduced locally with PHP 8.5.10, LocalDB SQL authentication, and the same coverage tool. No speculative production AKV change or skip was made.
  • Azure run 175208 on 60f29456: Windows and Linux succeeded. macOS updated Homebrew to 7.0.1, then failed before driver build/tests because the inherited SQL username variable became UID=sa.
  • UID regression reproduced from the actual YAML's Azure-style exported variables using Homebrew 7.0.1's exact UID comparison and Bash -pu: old configuration fails, renamed configuration passes. All 23 username references are present; parsed YAML is identical after reversing only the rename. All 34 Unix pipeline scripts pass Bash syntax checks. This follow-up changes only azure-pipelines.yml, with no new files or dependencies.

Known validation limitations

  • Fresh macOS CI must confirm Colima/Homebrew setup with the renamed pipeline variable. The UID failure is reproduced and corrected locally; native macOS package installation cannot be run from this Windows workstation. Windows passed run 175208, including the previously failing cases; no separate production fix is claimed for the older AKV access violation. Local Windows ODBC is 18.5.2.1; CI's installed ODBC version and OS/compiler build differ.
  • Whole-source Cppcheck 2.7 reports an unchanged zerodivcond warning in the existing formatting implementation. The same finding reproduces on the pristine base; no suppression was added.
  • Live-connection Valgrind reports within ODBC 18.7 reproduce using identical legacy-input probes on both pristine and patched PHP drivers (9 errors / 5 contexts in each). This PR does not claim a clean whole-driver live-connection Valgrind run.
  • PHP core can truncate a DSN at an embedded NUL before passing it to the extension; the new factory-level check does not provide end-to-end DSN NUL rejection.
  • Password-bearing DSNs may be exposed by application/exception-trace logging. Constructor credentials remain preferable when DSNs are logged or shared.
  • Secure erasure is scoped to the new driver-owned parsed-password storage on ordinary parser/factory return and C++ exception paths. It does not erase PDO's original DSN/constructor strings or existing narrow ODBC connection buffers, and does not promise cleanup after process termination or Zend bailout that skips C++ destructors.
  • Failover alias recognition does not add database-mirroring support on unsupported platforms. Actual mirrored failover and timeout-duration assertions need their appropriate integration environments; fast alias tests do not claim those measurements.

Requested PR validation

  • Run both extensions' functional suites on ODBC 18.7.1.1 (or a later supported 18.x release) and retain ODBC 17 compatibility coverage.
  • Explicitly select the intended driver and log each extension's actual connected DriverVer (sqlsrv_client_info() / PDO::ATTR_CLIENT_VERSION), without logging credentials. The locally observed ODBC 18 version was 18.07.0001.
  • Confirm all six new regressions execute, rather than being skipped.
  • Confirm the Linux Verify native PDO password erasure step runs the relocated test/tools/test_pdo_password_cleanup.sh live cases; its disposable test module must not be published as a release artifact.
  • Do not infer 18.7 coverage solely from an ODBC Driver 18 registration: the checked-in Azure Windows installer link currently identifies an older 18.6 release, Linux/macOS package installs are not minor-version asserted, and AppVeyor explicitly installs 17.8.1.1. This PR fixes macOS tap trust ordering and verification; it does not introduce a driver-version matrix. Verify the actual selected driver rather than relying on an environment variable that the test configuration may not use.

Draft pending target-platform validation.

@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.18182% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.89%. Comparing base (4b912c6) to head (60f2945).

Files with missing lines Patch % Lines
source/pdo_sqlsrv/pdo_dbh.cpp 90.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1684      +/-   ##
==========================================
+ Coverage   85.86%   85.89%   +0.03%     
==========================================
  Files          23       23              
  Lines        7251     7283      +32     
==========================================
+ Hits         6226     6256      +30     
- Misses       1025     1027       +2     
Files with missing lines Coverage Δ
source/pdo_sqlsrv/pdo_parser.cpp 96.91% <100.00%> (+0.11%) ⬆️
source/pdo_sqlsrv/pdo_util.cpp 93.40% <ø> (ø)
source/pdo_sqlsrv/php_pdo_sqlsrv_int.h 100.00% <100.00%> (ø)
source/shared/core_conn.cpp 88.40% <100.00%> (-0.49%) ⬇️
source/shared/core_sqlsrv.h 90.25% <ø> (-0.29%) ⬇️
source/shared/core_util.cpp 89.58% <100.00%> (+0.07%) ⬆️
source/sqlsrv/conn.cpp 84.71% <100.00%> (+1.06%) ⬆️
source/sqlsrv/util.cpp 85.41% <ø> (ø)
source/pdo_sqlsrv/pdo_dbh.cpp 91.26% <90.00%> (-0.29%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@David-Engel David Engel (David-Engel) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Reviewed commit 600020e. The alias mappings and bounded brace validation look correct. Requesting changes for the new PDO-owned password string: its lifetime is bounded, but releasing it does not securely erase its contents. See the inline finding.

Existing password handling is partial: the shared connection code explicitly zeroes the UTF-16 connection buffer, but only clears the narrow std::string. PHP core also retains the original DSN and constructor password until handle destruction and frees them without wiping. Those are pre-existing limitations, not additional blockers introduced by this PR. The SQLSRV Password alias follows the existing borrowed-string PWD path.

The documented DSN trace risk is confirmed: with zend.exception_ignore_args=0, a generated DSN password appeared in exception arguments, while the constructor password was protected by SensitiveParameterValue. Constructor credentials remain preferable.

Validation

  • Built both extensions from the reviewed revision on Linux with PHP 8.4.18.
  • All four no-server credential/brace PHPTs passed, with no skips.
  • No additional defects found in the reviewed diff. Live SQL tests and Valgrind were not run locally.
  • CI Linux and Windows passed. macOS failed before tests because Homebrew rejected the Microsoft tap as untrusted; that validation remains outstanding.

Comment thread source/pdo_sqlsrv/pdo_dbh.cpp Outdated
Comment thread test/native/pdo_password_cleanup.php Fixed
Comment thread test/native/pdo_password_cleanup_observer.cpp Fixed
@jahnvi480
Jahnvi Thakkar (jahnvi480) marked this pull request as ready for review September 15, 2026 03:30
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.

3 participants