Skip to content

psgi env: fix double encoding of path parts #2239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sparrow2009
Copy link

Fix double encoding of path parts when parsing a PSGI env with a SCRIPT_NAME set.

@sparrow2009 sparrow2009 force-pushed the fix_path_for_psgi_with_script_name branch from 6ee9b93 to 670ea6d Compare April 4, 2025 09:02
@@ -2,6 +2,7 @@ use Mojo::Base -strict;

use Test::More;
use Mojo::Message::Request;
use Mojo::Util 'encode', 'url_escape';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the use Mojo::Util qw(...) style everywhere now.

@kraih kraih requested review from a team, marcusramberg, kraih, christopherraa and Grinnz April 4, 2025 12:25
Fix double encoding of path parts when parsing a PSGI env with a SCRIPT_NAME set.
Make sure path is decoded and unescaped as expected by to_string.

The test $env used in the regression test included is a stripped down version of the result of:

  use HTTP::Request::Common 'GET';
  use HTTP::Message::PSGI 'req_to_psgi';
  my $script_name = ...;
  my $path = ...;
  $env = req_to_psgi( GET( 'http://localhost:8080'.$script_name.$path ), SCRIPT_NAME => $script_name );
@sparrow2009 sparrow2009 force-pushed the fix_path_for_psgi_with_script_name branch from 670ea6d to 26d373d Compare April 4, 2025 13:06
@sparrow2009
Copy link
Author

sparrow2009 commented Apr 7, 2025

A final note: This is actually a downstream patch in order to handle a race condition that is built into Mojo::Path::to_string().

my $path = ...;
Mojo::Path->new( $path )->to_string() # expects a decoded and unescaped $path as it encodes and escapes $path
my $path_obj = Mojo::Path->new( $path );
$path_obj->parts; # expects an encoded and escaped $path as it unescapes and decodes $path
$path_obj->to_string(); # encodes and escapes path parts

Thus if one happens to call to_string() on an object passed an encoded string on construction and did not call a method that _parse()d the string before double encoding occurs.

From reading the object construction examples listed in the synopsis of Mojo::Path I get the impression (correct me if I am wrong) that passing both encoded and decoded strings on object construction is valid and actually considered a feature. This design inevitably makes it impossible however to tell apart the nature of the construction string (encoded/decoded) later (and if my reasoning is correct can be considered a design flaw).

@kraih kraih requested a review from Copilot June 16, 2025 16:21
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses the double encoding issue of path parts when parsing a PSGI env with a SCRIPT_NAME set. The key changes include:

  • Adding a test case that covers a UTF-8 path part scenario.
  • Utilizing Mojo::Util’s encode and url_escape functions in tests.
  • Invoking $path->parts in the request parser to address the encoding problem.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
t/mojo/request_cgi.t New test for UTF-8 path parts and verification of roundtripping
lib/Mojo/Message/Request.pm Adjusted environment parsing by calling $path->parts
Comments suppressed due to low confidence (1)

lib/Mojo/Message/Request.pm:200

  • Consider adding an inline comment explaining why calling $path->parts at this point fixes the double encoding issue. This will help future maintainers understand the intent behind the call.
$path->parts;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants