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
TFA does not process the destination parameter correctly when it contains query parameters.
The Zendesk module, when doing SSO with Zendesk, involves a redirect to:
user?destination=services/zendesk%3Fbrand_id%3D123456%26locale_id%3D1%26return_to%3Dhttps%253A//example.zendesk.com/agent/tickets/123
When the TFA process intervenes, and the user enters the correct code, it generates a redirect to:
services/zendesk%3Fbrand_id%3D123456%26locale_id%3D1%26return_to%3Dhttps%253A//example.zendesk.com/agent/tickets/123
That results in a 404 error, of course, because the query parameters are erroneously URL-encoded.
Proposed resolution
Fix _tfa_form_get_destination() to separate the query from the path in ?destination - in other words, make it behave a bit more like drupal_goto() in its processing of ?destination.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | tfa-2687021-17.patch | 2.32 KB | jcnventura |
| |||
| #17 | interdiff_11_17.txt | 2.34 KB | jcnventura |
Comments
Comment #2
pjcdawkins CreditAttribution: pjcdawkins commentedThis patch
drupal_goto()Comment #3
pjcdawkins CreditAttribution: pjcdawkins commentedComment #4
drummLooks like something similar happens with a
#fragment: #2800461: login (with two factor authentication) results in a "404 - Page not found" mesageComment #5
frederickjhRelated Issue
Comment #6
pjcdawkins CreditAttribution: pjcdawkins at Platform.sh commentedFWIW, we've used this patch in production for 6 months.
Comment #7
frederickjhHi pjcdawkins!
Thanks for that information about the patch that fixes this.
drumm is unwilling to patch this module on drupal.org before the patch is commited to this module. I am guessing before you do this the security team will not review the updated version of the module nor the patch.
I can understand drumm's point of view. If you think that this patch fixes this issue in the correct manner then I would urge you to commit it and release a new version of the TFA module so that the security team can review it and allow the drupal.org webmasters to update and fix the issue there.
Thanks for your help and understanding in this matter!
Frederick
Comment #8
pjcdawkins CreditAttribution: pjcdawkins at Platform.sh commentedHi Frederick, there's some misunderstanding here. I am not a TFA module maintainer; I don't have the authority nor the permission to commit my patch.
Comment #9
frederickjhMy apologies @pjcdawkins! I should have check that and not assumed you were a maintainer. Thanks for the patch and the update on how it is working.
I previous comments then about committing a patch for this issue and pushing a new release should be redirected to the module maintainers.
Comment #10
petar.kolicov CreditAttribution: petar.kolicov commentedHi @pjcdawkins,
When FTA is enable and user try to reset password, TFA redirect is not working correct because $options['query'] = $destination['query']; overwrites 'pass-reset-token' in query. I add one change to your patch ($options['query'] = array_merge($options['query'], $destination['query'])).
Comment #11
petar.kolicov CreditAttribution: petar.kolicov commentedSorry, I found one mistake in my code.
Comment #13
frederickjhHi!
I am changing this issue to major as it affects all users on drupal.org that use two-factor authentication. In the related issue drumm, one of the web admins for drupal.org says that until this code in commit and released so that the security team reviews it he is unwilling to patch drupal.org. I think that this constitutes being a major bug by the fact that it reflects badly on the drupal community and that it affects users willing to improve the security of their drupal.org account by enabling two-factor authentication.
Comment #14
pjcdawkins CreditAttribution: pjcdawkins at Platform.sh commented@petar.kolicov's modification to my earlier patch is a good one.
Setting patch #11 RTBC.
Comment #15
DamienMcKennaComment #16
DamienMcKennaComment #17
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedI've undone the $destination -> $path rename as it made it a bit harder to review the patch, and it changed a lot of code in places that really didn't need to be changed.
Comment #19
jcnventura CreditAttribution: jcnventura at 1xINTERNET commented