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 74.50% 74.50%
- Complexity 1740 1741 +1
==========================================
Files 85 85
Lines 7522 7523 +1
==========================================
+ Hits 5604 5605 +1
Misses 1918 1918
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>
|
@1d0u Finally getting back to this PR and see there's some merge conflicts here that need addressed. In addition, the testing instructions in the PR description don't provide any real details, they only talk about our automated testing tools (which run automatically on each PR so no need to mention those there). My main concern here is I'm trying to understand what this PR is actually fixing or more plainly, what scenarios currently lead to problems that this PR addresses? I'm all for tightening our code where it makes sense but ideally we have some reproduction steps of the actual problem being solved here |
|
@1d0u heads up that I'm punting this to the next release in hopes you'll be able to update the testing instructions and resolve merge conflicts to be considered then, thanks! |
|
It's been multiple weeks waiting for a response on this one so going to close this out for now. But happy to re-open if there's still a desire to try and get this across the finish line |
github-project-automation
Bot
moved this from In progress
to Done
in WordPress AI Roadmap
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