Problem/Motivation

When add a redirect using the form, the "to" URL is url encoded before being saved. This is problem if pasting in a URL that is already encoded, as it double encodes it, breaking the redirect.

Steps to reproduce

  1. Visit redirect form to add a new redirect
  2. For the "from" path, enter "test123"
  3. For the "to" path, enter "/sites/default/files/some%20file%20with%20spaces.txt"
  4. Save
  5. In redirect listing table, observe the redirect was saved as "/sites/default/files/some%2520file%2520with%2520spaces.txt" - it was URL encoded again. Visiting the redirect doesn't work.

This scenario is common when a user is creating a redirect by copying and pasting a URL that they want to redirect to on their site, and that URL has some encodings in it already to handle things like spaces.

Note that the Redirect module is not explicitly performing the URL encoding. Drupal's internal URL processing is. Redirect module uses a Link field internally to handle storage of the "to" URL. Internally, it stores the "to" URL as this: "internal:/sites/default/files/some%20file%20with%20spaces.txt". When the RedurectRequestSubscriber asks the redirect entity for the URL to redirect to, the Link field returns it as a Url object using Url::fromUri with argument "internal:/sites/default/files/some%20file%20with%20spaces.txt". Drupal url encodes this in UnroutedUrlAssembler's buildLocalUrl method.

Proposed resolution

Redirect module could run the user input through rawurldecode before saving it. It should assume that URLs entered may already be encoded. If the URL that's entered is NOT encoded, I don't think there is any harm in running it through a decode.

What's also interesting here is that the Url object doesn't return an encoded URL if it thinks it's an external URL. I think the logic behind this is that if a user entered an external URL, it's likely already encoded and shouldn't be re-encoded. But if it's an internal URL reference, it needs to be encoded.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#21 3194561-redirect-double-encoding-21.patch3.58 KBmauriciopieper
#10 3194561-redirect-double-encoding.patch1.29 KBjumpsuitgreen
#9 redirect-encoded.patch722 byteschrisjlock
#4 redirect_double_encoded-3194561-4.patch985 bytesigork96
#3 redirect-doubleEncoded-3194561-3.patch773 bytesjjtoyas

Issue fork redirect-3194561

Command icon 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:

About issue forks

Comments

bkosborne created an issue. See original summary.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Issue summary: View changes
jjtoyas’s picture

jjtoyas commented
Status: Active » Needs review
StatusFileSize
new redirect-doubleEncoded-3194561-3.patch773 bytes

After checking related issues to this. I found on https://www.drupal.org/project/redirect/issues/2869055 a problem really similar, but the conclusion is that the "To" column is not depending on the module, instead, It is depending on the core.

On https://www.drupal.org/project/drupal/issues/2904853, there is one patch for the core which solves the problem on the column "To".

But even with this patch, when the URL is redirected is encoded again giving the same problem, fo that I made a patch following the same strategy that was used for the core patch.

igork96’s picture

igork96 commented
StatusFileSize
new redirect_double_encoded-3194561-4.patch985 bytes

I tested the patch from #4 and it didn't solve the issue, I tried different solutions and decided to replace the '%20' manually.

andre.bonon’s picture

andre.bonon
Primary language Portuguese, Brazil
commented
Status: Needs review » Reviewed & tested by the community

This patch works for me. (Redirect 1.8.0)
For the buggy scenario I have, it fixes the "To" field in the redirect form.
The other patch I'm also using is this one https://www.drupal.org/project/redirect/issues/2884630. It fixes the "from" field on the form save.
I'm using both patches which fixed the issue with %20 beings encoded to %2520.

dpagini’s picture

dpagini commented

This patch fixes a very narrow use case of a space being encoded to %20, but does not address all encoded characters. For example, a ( or ) get encoded to %28 and %29. That makes me think there should be a more global solution to handle this...?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Yes, maybe this should do a urldecode() like the hash issue, and should have a test.

The code can also be simplifed to use the entity api properly, you almost never want to use getValue() and instead loop over the items.

Plus, it doesn't really belong in save() but buildEntity(). Wondering if this shouldn't be handled in the core link widget, how does that behave in such a case?

meri_atanasovska’s picture

meri_atanasovska commented

Patch #3 looks like a great solution, patch works perfectly.

chrisjlock’s picture

chrisjlock commented
StatusFileSize
new redirect-encoded.patch722 bytes

Option to check additional hash instead of changing value saved to db.

jumpsuitgreen’s picture

jumpsuitgreen commented
StatusFileSize
new 3194561-redirect-double-encoding.patch1.29 KB

I created this patch because our client was copy-pasting the encoded URL into the To field which caused the encoded URL to be saved to the database. To remedy this, I added a method to the RedirectForm class called isEncoded() then called that method in the save() method where—if the value in the $form_state was already encoded, the value is decoded and used to set the redirect_redirect value in the $entity prior to saving. This saves the unencoded URL to the database so that it can be properly encoded later. Hopefully this helps.

wylbur made their first commit to this issue’s fork.

wylbur opened merge request !142

wylbur’s picture

wylbur commented
Status: Needs work » Needs review

The merge request !142 applies the patch from https://www.drupal.org/project/redirect/issues/3194561#comment-15985112, and applies against the latest dev release.

Please test!

tlo405’s picture

tlo405 commented

I originally was using the patch in #3 on 8.x-1.11, however it no longer applies when upgrading to 8.x-1.12. Looks like the approach here is changing a bit though. I tried the latest MR above on the latest dev release and it unfortunately also no longer applies.

maheshv’s picture

maheshv commented

I'm also facing this issue not able to add the patch https://www.drupal.org/files/issues/2021-01-29/redirect-doubleEncoded-31... after upgrading the module to 8.x-1.12.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
maheshv’s picture

maheshv commented

Any update on this patch issue?

nitesh624’s picture

nitesh624
he/him
Primary language English
Location Ranchi, India
commented

yes patch in #10 has slightly change in approach

chamilsanjeewa made their first commit to this issue’s fork.

nitesh624’s picture

nitesh624
he/him
Primary language English
Location Ranchi, India
commented
Status: Needs work » Needs review
mauriciopieper’s picture

mauriciopieper commented
StatusFileSize
new 3194561-redirect-double-encoding-21.patch3.58 KB

Uploading the MR diff as a patch to use in composer.