uri_for() corrupts query parameters hash for caller

[Byron Young-3]

Hey everybody,

I ran into an issue at $work where we keep passing the same $query_params hashref to a number of uri_for() calls successively, but if there are characters in the query params that need to be escaped they get escaped each time, leading to sequences like

?filter=Not%25252BRun

after the same $query_params have been run through uri_for a few of times (because the '%' keeps getting escaped).  The query hash was originally { filter => 'Not Run' }.

So, we patched uri_for() here at work to create a copy of $params and work with that, and that fixes the issue.  However, it seems like such a simple fix that I feel like it must have been thought of and discussed and shot down in the past, but I didn't find anything in the list archives indicating that.  Is there some reason uri_for() does things that way?

If not I'll gladly supply patch + test.

Thanks,
Byron

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Byron Young-3]

Byron Young wrote on 2009-06-12:

> Hey everybody,
>
> I ran into an issue at $work where we keep passing the same
> $query_params hashref to a number of uri_for() calls successively,
> but if there are characters in the query params that need to be
> escaped they get escaped each time, leading to sequences like
>
> ?filter=Not%25252BRun
>
> after the same $query_params have been run through uri_for a few of
> times (because the '%' keeps getting escaped).  The query hash was
> originally { filter => 'Not Run' }.
>
> So, we patched uri_for() here at work to create a copy of $params
> and work with that, and that fixes the issue.  However, it seems
> like such a simple fix that I feel like it must have been thought
> of and discussed and shot down in the past, but I didn't find
> anything in the list archives indicating that.  Is there some
> reason uri_for() does things that way?
>
> If not I'll gladly supply patch + test.
>
> Thanks,
> Byron
>
> _______________________________________________ List:
> Catalyst@... Listinfo: http://lists.scsys.co.uk/cgi-
> bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-
> archive.com/catalyst@.../ Dev site:
> http://dev.catalyst.perl.org/

I also noticed that the docs for uri_for used to warn of the destructiveness but that warning has been removed in more recent versions.  I'd like to suggest that it be added back and made more prominent if there really is a good reason for mangling the caller's data.  I can provide a doc patch in that case, too.

Byron

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Byron Young-3]

Byron Young wrote on 2009-06-12:

> Byron Young wrote on 2009-06-12:
>> Hey everybody,
>>
>> I ran into an issue at $work where we keep passing the same
>> $query_params hashref to a number of uri_for() calls successively, but
>> if there are characters in the query params that need to be escaped
>> they get escaped each time, leading to sequences like
>>
>> ?filter=Not%25252BRun
>>
>> after the same $query_params have been run through uri_for a few of
>> times (because the '%' keeps getting escaped).  The query hash was
>> originally { filter => 'Not Run' }.
>>
>> So, we patched uri_for() here at work to create a copy of $params
>> and work with that, and that fixes the issue.  However, it seems
>> like such a simple fix that I feel like it must have been thought
>> of and discussed and shot down in the past, but I didn't find
>> anything in the list archives indicating that.  Is there some
>> reason uri_for() does things that way?
>>
>> If not I'll gladly supply patch + test.
>>
>> Thanks,
>> Byron
>>
>
> I also noticed that the docs for uri_for used to warn of the
> destructiveness but that warning has been removed in more recent
> versions.  I'd like to suggest that it be added back and made more
> prominent if there really is a good reason for mangling the
> caller's data.  I can provide a doc patch in that case, too.
>
> Byron
>

Hey,

I know people have been busy (I think there were some perl conferences lately?) and I think my issue slipped through the cracks.  Just wanted to know what people thought about this and whether I should submit my patch or take a different approach.

Thanks
Byron

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Tomas Doran]

Byron Young wrote:

> I know people have been busy (I think there were some perl conferences lately?) and I think my issue slipped through the cracks.  Just wanted to know what people thought about this and whether I should submit my patch or take a different approach.
>

Sorry for dropping this on the floor.

Yes, please submit a patch :)

Cheers
t0m

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Byron Young-3]

Tomas Doran wrote on 2009-06-26:

> Byron Young wrote:
>
>> I know people have been busy (I think there were some perl
> conferences lately?) and I think my issue slipped through the
> cracks.  Just wanted to know what people thought about this and
> whether I should submit my patch or take a different approach.
>>
>
> Sorry for dropping this on the floor.
>
> Yes, please submit a patch :)
>
> Cheers
> t0m

Alrighty, here you go, patch + test are attached.  There are based off the 5.71001 svn head because that's what I have currently.  5.8's uri_for() looks the same, so it should apply there as well, but let me know if you need another one from 5.8.

(I'm not sure how tracking contributors works, or if you do, but if so this was worked on by myself and Amir Sadoughi).

Byron



_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Tomas Doran]

On 26 Jun 2009, at 23:19, Byron Young wrote:

> Alrighty, here you go, patch + test are attached.  There are based  
> off the 5.71001 svn head because that's what I have currently.  
> 5.8's uri_for() looks the same, so it should apply there as well,  
> but let me know if you need another one from 5.8.

http://dev.catalyst.perl.org/svnweb/Catalyst/revision/?rev=10736

Thanks very much for the patch, applied ok to 5.80 trunk.

I rewrote your fix to just not mangle $_, which fixes the same issue  
with less code, and avoids the unsafe each..

Unfortunately, this just missed the Catalyst 5.80006 release, so  
you'll have to wait for the next one to see it in released code, sorry!

Cheers
t0m


_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Byron Young-3]

Tomas Doran wrote on 2009-06-29:

>
> On 26 Jun 2009, at 23:19, Byron Young wrote:
>
>> Alrighty, here you go, patch + test are attached.  There are based off
>> the 5.71001 svn head because that's what I have currently. 5.8's
>> uri_for() looks the same, so it should apply there as well, but let me
>> know if you need another one from 5.8.
>  http://dev.catalyst.perl.org/svnweb/Catalyst/revision/?rev=10736
>
> Thanks very much for the patch, applied ok to 5.80 trunk.
>
> I rewrote your fix to just not mangle $_, which fixes the same issue
> with less code, and avoids the unsafe each..
>
> Unfortunately, this just missed the Catalyst 5.80006 release, so you'll
> have to wait for the next one to see it in released code, sorry!
>
> Cheers
> t0m

Oh, nice, that's a much better solution.  If you don't mind, though, can you explain what you mean about the 'unsafe each'?

Thanks!
Byron

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Tomas Doran]

On 30 Jun 2009, at 00:31, Byron Young wrote:
>  If you don't mind, though, can you explain what you mean about the  
> 'unsafe each'?

If your application code half iterates through the params hash with  
each before calling uri_for, then the param copy would only copy the  
second half of the hash (as each has an internal iterator).

Cheers
t0m


_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Byron Young-3]

Tomas Doran wrote on 2009-06-29:

> On 30 Jun 2009, at 00:31, Byron Young wrote:
>>  If you don't mind, though, can you explain what you mean about
> the
>> 'unsafe each'?
>  If your application code half iterates through the params hash with
> each before calling uri_for, then the param copy would only copy the
> second half of the hash (as each has an internal iterator).
>
> Cheers
> t0m

Ah, makes sense.  Learn something new every day!

Thanks!
Byron

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Aristotle Pagaltzis]

* Tomas Doran <bobtfish@...> [2009-06-30 02:35]:
> If your application code half iterates through the params hash
> with each before calling uri_for, then the param copy would
> only copy the second half of the hash (as each has an internal
> iterator).

FWIW you can reset the iterator using `keys`, which is cheap to
call in void or scalar context too.

Regards,
--
Aristotle Pagaltzis // <http://plasmasturm.org/>

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/