Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
There is an issue with the image captcha refresh link. Though its "href" parameter is correctly set with a starting slash, clicking on it will make a JS call with double slashes in the destination url. This might introduce issues in installations where htaccess rules to avoid these are not present. It would be nice to avoid adding any extra slash when creating the url.
Proposed resolution
Clean host and path from having slashes at their respective end/start and just add one in-between.
Remaining tasks
Upload patch.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3109771-6-image-reload-href.patch | 708 bytes | Nick Dewitte |
| #2 | 3109771-2-image-reload-href.patch | 629 bytes | dpavon |
Issue fork captcha-3109771
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
dpavon CreditAttribution: dpavon commentedHere's a possible patch as a suggestion, though the JS specifics can be altered as long as we get a similar result.
Comment #3
dpavon CreditAttribution: dpavon commentedComment #4
asmita26 CreditAttribution: asmita26 as a volunteer and at QED42 commentedComment #5
asmita26 CreditAttribution: asmita26 as a volunteer and at QED42 commentedComment #6
Nick Dewitte CreditAttribution: Nick Dewitte commentedWithout the patch, the XHR request made by the script is redirected to the correct URL.
This can cause issues when the requests are cached on the server, resulting in an eternal redirect loop, when the caching system ignores this double slash.
Re-rolled the patch since it wouldn't apply anymore.
Comment #7
spookyisland CreditAttribution: spookyisland commentedUnfortunately my server doesn't redirect to the correct URL. This patch fixes the issue for me. Will you include it in the next version of the module?
Comment #8
AnybodyCould we please have a test to prove this?
Comment #9
Anybody@Nick Dewitte thanks, the patch in #6 looks good and correct. Could you create a MR from the patch and add tests to show it works as expected for cases where slashes are present or not?
Comment #10
AnybodyFor tests.
Comment #11
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedComment #12
Anybody@Grevil would you be so kind to add a little JavaScript enabled test for that link to reproduce the issue? With the change, it should be fixed. That way we can make safe it doesn't break again in the future.
You may also decide no test is needed and commit this as-is after manual testing. You decide.
Comment #13
Anybody@Grevil: @thomas.frobieter just found a further bug with the image captcha reload functionality, so this generally needs tests.
Comment #14
AnybodyCreated a MR from #6 by @Nick Dewitte
Please review and add a test for the reload functionality.
Comment #16
AnybodyComment #17
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedTested it manually, and it works great!
Added the test requirements in #3324399: Add more CAPTCHA test coverage, for when we have more time.
Comment #19
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedDone.
Comment #20
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedSorry, I don't know what I tested there yesterday, but this definitely breaks the image captcha refresh functionality...
I apologize.
Comment #21
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedComment #23
Anybodyconst baseUrl = document.location.origin;was missing. No idea where that hat gone since the patch.Please retest @Grevil! Already committed to 2.x
Comment #24
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedYep, fixed, thanks @Anybody!
Comment #25
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented