Fix alt text upload URL matching#621
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #621 +/- ##
=============================================
+ Coverage 73.18% 74.00% +0.82%
- Complexity 1731 1739 +8
=============================================
Files 85 85
Lines 7473 7491 +18
=============================================
+ Hits 5469 5544 +75
+ Misses 2004 1947 -57
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens upload-URL-to-local-path mapping for alt text generation and improves test isolation in admin settings E2E runs.
Changes:
- Tighten upload URL matching logic to avoid substring (“lookalike”) matches.
- Add an integration test to ensure lookalike upload URLs are rejected.
- Clear stored credentials during admin settings E2E setup to reduce cross-test contamination.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/e2e/specs/admin/settings.spec.js | Clears credentials during E2E setup to prevent state leakage between runs. |
| tests/Integration/Includes/Abilities/Alt_Text_GenerationTest.php | Adds a regression test for rejecting lookalike upload URLs and a reflection helper for URL normalization. |
| includes/Abilities/Image/Alt_Text_Generation.php | Replaces substring check with stricter prefix/boundary check for upload base URL matching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $lookalike_url = 'https://bad' . $normalized_baseurl . '/ai-test-image.jpg'; | ||
|
|
There was a problem hiding this comment.
I verified the constructed URL against the current normalizer: normalize_upload_url() strips the scheme before this value is used, so the test URL becomes a valid different-host URL such as https://badlocalhost:8889/wp-content/uploads/ai-test-image.jpg, not https://badhttps://.... The substr( $normalized_baseurl, -3 ) fixture path is intentional because it proves the previous str_contains() behavior would have sliced the shifted URL into an existing local file path. I also reran the targeted integration test successfully (33 tests, 84 assertions, 1 skipped).
| if ( | ||
| $normalized_url !== $normalized_baseurl && | ||
| ! str_starts_with( $normalized_url, $normalized_baseurl . '/' ) | ||
| ) { |
There was a problem hiding this comment.
I checked this path and the current code already rejects the exact uploads base URL before any filesystem return: after normalization the relative path is empty, and the existing empty-relative-path guard returns null. Even beyond that, the return path is guarded by is_file(), so the uploads directory would not be returned as a valid local image file. I reran the targeted integration test successfully (33 tests, 84 assertions, 1 skipped).
| } ) => { | ||
| // Activate the request mocking plugin. | ||
| await requestUtils.activatePlugin( 'e2e-test-request-mocking' ); | ||
| await clearCredentials( requestUtils ); |
There was a problem hiding this comment.
Curious why this was needed for this particular PR? Seems unrelated to the changes made here
There was a problem hiding this comment.
This is not required by the alt-text production change itself. I added it after this PR’s first CI run exposed an existing retry-isolation issue in this settings E2E test: global setup seeds the OpenAI credential, and when Playwright retried this test in isolation, the connector field was already masked/disabled so the fill failed. Clearing credentials here makes the test self-contained and prevents that unrelated flake from blocking the PR. If you prefer keeping this PR strictly scoped to the alt-text fix, I can split/drop this E2E isolation change.
Co-authored-by: Darin Kotter <darin.kotter@gmail.com>
Co-authored-by: Darin Kotter <darin.kotter@gmail.com>
What?
Tightens local upload URL matching in alt text generation so only the configured uploads base URL, or paths under it, can resolve to local files.
Why?
The previous substring check could treat lookalike URLs containing the uploads base URL as local upload URLs.
How?
Replaces the substring check with an exact-or-prefix boundary check and adds a regression test for a lookalike uploads URL.
Use of AI Tools
AI assistance: Yes
Tool(s): Sisyphus / OhMyOpenCode
Model(s): GPT-5.5
Used for: validation.
Testing Instructions
composer lint -- includes/Abilities/Image/Alt_Text_Generation.php tests/Integration/Includes/Abilities/Alt_Text_GenerationTest.php.composer phpstan.npm run typecheck.npm run lint:js.npm run wp-env:test start -- --xdebug=coverage && npm run test:php -- --filter Alt_Text_GenerationTest && npm run wp-env:test stop.Screenshots or screencast
Not applicable; this is a PHP behavior fix covered by tests.
Changelog Entry