Fix #1065: stop mangling "+" aliases in the signup email address - #1259
Fix #1065: stop mangling "+" aliases in the signup email address#1259Miraeld wants to merge 2 commits into
Conversation
The signup modal concatenated the raw address straight into the AJAX query
string:
'action=imagify_signup&email=' + inputValue
A literal "+" in a query string decodes to a space, so PHP received
"hanna may21@wp-media.me". sanitize_email() then stripped that space, because a
space is not valid in a local part, leaving "hannamay21@wp-media.me" - a
perfectly valid address that passed is_email(). So an account was created for an
address the user never typed, the confirmation mail went to a mailbox they
cannot read, and the UI still showed "Congratulations". That is exactly the
reported symptom: success message, no account, no email.
Two changes:
1. assets/js/notices.js encodes the address with encodeURIComponent() (and
trims it, so a pasted address with stray spaces works too). "+" now arrives
as "%2B" and survives intact.
2. The server no longer accepts an address that sanitization had to alter. The
handler compares the sanitized address against the trimmed input and rejects
any mismatch, so a mangled address fails loudly instead of silently becoming
a different account - whatever the client sends. Trimming still happens, so
pasted whitespace is not a rejection.
Also adds a .fail() handler to the signup request: without one a transport
error left the promise unsettled and the modal spinning forever.
Verified end to end: with the old code "email=hanna+may21@..." reaches the
handler as "hanna may21@..." and sanitizes to "hannamay21@..."; with the fix it
arrives and leaves as "hanna+may21@...".
Note the dots/underscores case in the issue is NOT this bug: those characters
are valid in a local part and are unaffected by both the query-string decoding
and sanitize_email(). The new tests prove the plugin transmits them unchanged,
so that half is API-side and is tracked separately.
Not up to standards ⛔🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | -2 |
🔴 Coverage 0.00% diff coverage
Metric Results Coverage variation Report missing for 07216be1 Diff coverage ❌ 0.00% diff coverage (50.00%) Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (07216be) Report Missing Report Missing Report Missing Head commit (f7c7c30) 20487 1622 7.92% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#1259) 4 0 0.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@Miraeld I cannot push to this branch because it lives in wp-media and my account has no write access here. The fix for all four review comments is ready as one commit on my fork: faisalahammad@1c907a31 To apply it: Or grant me write access to the branch and I will push directly. Thanks. |
- Drop unused $useApi property - Mock HTTP with HttpRequestTrait from wp-media/phpunit - Extend AjaxTestCase and drive the request through callAjaxAction() - Replace five test methods with one configTestData provider + fixture Addresses PR feedback. Refs #1259
|
All four review points are addressed in f7c7c30. Credit where it's due: @faisalahammad wrote this rework. He replied "Done" on each thread but has no push access to this branch, so the work was sitting on his fork. I cherry-picked his commit rather than reimplementing it, so git authorship stays with him — I reviewed and verified it rather than taking "Done" on trust. |
4 participants
Description
Fixes #1065
Signing up with a
+alias (hanna+may21@wp-media.me) showed "Congratulations!" but created no account and sent no email. It now works.What was really happening is worse than "signup fails": an account was created, just for a different address than the one typed, so the API key email went to a mailbox the user cannot read.
Supersedes #1075 by @faisalahammad, who found the
encodeURIComponentfix. Opened separately because that PR's branch is on a fork withmaintainerCanModify: false, so it could not be updated.Type of change
Detailed scenario
What was tested
Automated - 5 new integration tests (
Tests/Integration/inc/classes/ImagifyAdminAjaxPost/imagifySignupCallback.php). They interceptpre_http_requestand assert the address that actually leaves the plugin, which is the thing the bug corrupted:hanna+may21@wp-media.mefilm.simleu_21@gmail.comhanna may21@wp-media.me(a mangled address)tzinkeh@yahoo.nonot-an-emailVerified by mutation: removing the
$email !== $raw_emailguard makes the mangled-address test fail.Mechanism verified end to end outside the test suite too:
Suites: 153 integration / 401 assertions, 528 unit / 1528 assertions, 0 failures. PHPCS, PHPStan and ESLint clean.
notices.min.jsrebuilt withgrunt uglify:all(onlynotices.min.jschanged).Not covered: the browser leg was not exercised manually - the local environment was not logged in and I do not enter credentials. The JS change is one expression, ESLint-clean, and confirmed present in the minified bundle; the round-trip above proves the behaviour. Worth a click-through on staging before release.
How to test
+alias, e.g.you+imagify1@yourdomain.com.imagify_signuprequest must showemail=you%2Bimagify1%40yourdomain.com(encoded), not a bare+.you@yourdomain.com) still works, and one with stray leading/trailing spaces is accepted rather than rejected.Affected Features & Quality Assurance Scope
assets/js/notices.js).Imagify_Admin_Ajax_Post::imagify_signup_callback()- the only consumer of the new validation.signupErrorRequestFailed.Technical description
Documentation
The failure had two independent causes and both are fixed.
Client.
'action=imagify_signup&email=' + inputValueput the raw address in a query string. Perapplication/x-www-form-urlencoded,+there means a space, so PHP's$_GETparser decoded it to one.encodeURIComponent()sends%2Binstead.Server.
sanitize_email()does not reject an address with a space - its local-part filter (wp-includes/formatting.php:33) strips characters that are invalid there. Sohanna may21@wp-media.mesilently becamehannamay21@wp-media.me, whichis_email()then happily accepted. That is why the UI reported success: from the plugin's point of view the signup genuinely succeeded, just for the wrong address.Fixing only the client would leave that trap in place for any other caller, so the handler now refuses any address sanitization had to alter:
sanitize_email()does not change case, so this does not reject mixed-case addresses.On the dots/underscores case in the issue: that is a separate problem.
.and_are valid local-part characters, untouched by both the query-string decoding andsanitize_email(). The new tests prove the plugin sends such addresses through unchanged, which localises that half to the API or mail service rather than the plugin. Tracked separately so this fix is not held up by it.New dependencies
None.
Risks
user@example.com.(trailing dot) is now rejected. Technically legal, effectively never typed, and a clear error beats a wrong account.notices.min.js.Mandatory Checklist
Code validation
Code style
Unticked items justification
The browser leg of the acceptance criteria was verified by the round-trip proof and the integration tests rather than by a manual click-through - see "Not covered" above.
Additional Checks