Uh oh!
There was an error while loading. Please reload this page.
fix(legacy): preserve Windows CA store semantics - #146
Merged
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up improvement to the Windows legacy-PHP trust bootstrap: it refines how Windows system roots are exported into the PHP CA bundle so that Windows-specific EKU constraints are preserved, and it fixes a cache-write optimization that could miss same-size CA bundle rotations.
Changes:
- Filter Windows ROOT-store certificates by their effective EKUs (including store-only EKU properties) so roots not valid for TLS server auth aren’t exported as unconstrained PEM anchors.
- Replace the CA-bundle cache “probably matches” write optimization with a full-content comparison to detect same-size changes.
- Simplify Windows cert-store test fixtures by removing an unused PEM field and add a unit test around Windows EKU semantics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/legacy/php_manager_windows.go | Switch CA bundle write path to full-content change detection. |
| internal/legacy/cert_store_windows_test.go | Remove unused PEM representation from the Windows cert-store fixture. |
| internal/legacy/ca_bundle_windows.go | Export only store roots that effectively allow TLS server authentication (EKU-aware). |
| internal/legacy/ca_bundle_windows_test.go | Add coverage for Windows EKU behavior when determining TLS-root suitability. |
| internal/file/file.go | Add WriteIfChanged helper for full-content equality checks before writing. |
| internal/file/file_test.go | Add regression test ensuring WriteIfChanged checks the whole file and avoids rewrites on identical content. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
WriteIfChanged read the whole destination file to compare it, even when its size already showed the contents had changed. It now compares the size first, so only a file which could still match is read. The test for the purposes Windows reports for a certificate now covers the EKU extension as well as the store property, and the two together. Windows reports the purposes on both lists, so a property can withdraw the server authentication an extension grants, which is what decides whether a root belongs in the bundle. Setting the property outright rather than adding to it keeps the combined cases unambiguous. The test for leaving an unchanged file alone compared modification times a few milliseconds apart, which the clock can be too coarse to distinguish, so a rewrite could have gone unnoticed. It now backdates the file and checks that the older time survives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bojanzforce-pushed
the
fix/windows-ca-store-followups
branch
from
August 10, 2026 13:56
3a2ec31 to
521c9f7CompareReading the purposes Windows reports can fail for one certificate in the store, and that error stopped the whole enumeration. The bundle then held nothing but the shipped certificates, so a single unreadable root cost the machine every root its organization had added, leaving the CLI unable to reach a server behind TLS inspection. Such a certificate is now left out on its own, and its fingerprint and the reason are reported, so the bundle keeps every other root in the store. The bundle is now assembled whether or not all of the store could be read, which is what lets a partial failure keep the roots it did read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bojanzforce-pushed
the
fix/windows-ca-store-followups
branch
from
August 10, 2026 14:04
521c9f7 to
c2c18b5CompareUh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Summary
Follow-up to #142.
Tests