Loading ...
Sorry, an error occurred while loading the content.
 

Re: [mp2] [patch] CGI::Cookie does not work prior to response phase

Expand Messages
  • Nick Tonkin
    On Fri, 11 Apr 2003, Stas Bekman wrote:Nick Tonkin wrote: The patch looks good (though I haven t tested it). One thing: [...] + my $r; +
    Message 1 of 16 , Apr 10 9:46 PM
      On Fri, 11 Apr 2003, Stas Bekman wrote:

      > Nick Tonkin wrote:
      >
      > The patch looks good (though I haven't tested it). One thing:
      >
      > [...]
      > > + my $r;
      > > + $r = (shift || Apache->request()) if $MOD_PERL;
      > > + if (defined $r) {
      > > + $raw_cookie = $r->headers_in->{'Cookie'};
      > > + } else {
      > > + if (exists $ENV{PATH_INFO}) {
      > > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
      > > + } else {
      > > + die '%ENV has not been set. Turn SetupEnv on or ' .
      > > + 'run $r->subprocess_env()';
      > > + }
      > > + }
      >
      >
      > the only situation when CGI env vars might be unavailable is under mp2, but
      > since you already do:
      >
      > $r = (shift || Apache->request()) if $MOD_PERL;
      >
      > this extra die is not needed. You also don't need defined($r). All you need is:
      >
      > + my $r;
      > + $r = (shift || Apache->request()) if $MOD_PERL;
      > + if ($r) {
      > + $raw_cookie = $r->headers_in->{'Cookie'};
      > + } else {
      > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
      > + }
      >
      > however your logic could be made even more user-friendly if you did this:
      >
      > + my $r;
      > + $r = shift if $MOD_PERL;
      > + $r ||= eval { Apache->request() } if $MOD_PERL;
      > + if ($r) {
      > + $raw_cookie = $r->headers_in->{'Cookie'};
      > + } else {
      > + if (exists $ENV{PATH_INFO}) {
      > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
      > + } else {
      > + die '%ENV has not been set. Turn SetupEnv on or ' .
      > + 'run $r->subprocess_env()';
      > + }
      > + }
      >
      > Here, we use Apache->request only if it's available, and if $r was neither
      > passed as an argument nor it's available through Apache->request, we try to
      > use the third fallback solution and if even that not available, we die. Looks
      > like the best solution to me.

      Agreed.

      >
      > > It seemd more logical to test for $ENV{PATH_INFO} than
      > > $ENV{QUERY_STRING}, dunno if you agree.
      >
      > I believe that they all are all always set no matter what, need to check the
      > actual implementation. So I don't really care which one is used.

      Hm. So $ENV{QUERY_STRING} gets set (to what?) when there is no query string?
      Whatever. I don't care either, I just think it's more intuitive to assume that
      there's always going to be a PATH_INFO ...


      So, attached is (yet) another diff, and then someone should test it on mp1, and then
      you'll send it to Lincoln?

      - nick

      --

      ~~~~~~~~~~~~~~~~~~~~
      Nick Tonkin {|8^)>
    • Nick Tonkin
      On Fri, 11 Apr 2003, Stas Bekman wrote: It seemd more logical to test for $ENV{PATH_INFO} than $ENV{QUERY_STRING}, dunno if you agree.
      Message 2 of 16 , Apr 10 9:48 PM
        On Fri, 11 Apr 2003, Stas Bekman wrote:

        >
        > > > It seemd more logical to test for $ENV{PATH_INFO} than
        > > > $ENV{QUERY_STRING}, dunno if you agree.
        > >
        > > I believe that they all are all always set no matter what, need to check
        > > the actual implementation. So I don't really care which one is used.
        >
        > It can be QUERY_STRING, but not PATH_INFO, since the former is always set,
        > whereas the latter is not:
        >
        > apache-1.3/src/main/util_script.c:381: ap_table_setn(e, "QUERY_STRING",
        > r->args ? r->args : "");
        >
        > httpd-2.0/server/util_script.c:370: apr_table_setn(e, "QUERY_STRING",
        > r->args ? r->args : "");
        >
        > We can choose from: SERVER_PROTOCOL, REQUEST_METHOD, QUERY_STRING and REQUEST_URI.
        >
        > __________________________________________________________________
        > Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
        > http://stason.org/ mod_perl Guide ---> http://perl.apache.org
        > mailto:stas@... http://use.perl.org http://apacheweek.com
        > http://modperlbook.org http://apache.org http://ticketmaster.com
        >

        Heh. Missed you by a sec. Here's a new patch.

        - nick

        --

        ~~~~~~~~~~~~~~~~~~~~
        Nick Tonkin {|8^)>
      • Nick Tonkin
        ... But then does nothing if it s not defined. No graceful handling in other words. Or do I miss something? - nick -- ~~~~~~~~~~~~~~~~~~~~ Nick Tonkin {|8^)
        Message 3 of 16 , Apr 10 9:50 PM
          On Thu, 10 Apr 2003, Nick Tonkin wrote:

          > On Fri, 11 Apr 2003, Stas Bekman wrote:
          >
          > > Nick Tonkin wrote:
          > > [...]
          > >
          > > Your diff is the other way around, - should say what was removed and + what
          > > was added, you just need to swap the order of files while doing diff-- the
          > > newer file last...
          > >
          > > > 1) is the use of Apache->request correct? For mp1 and mp2?
          > >
          > > Yes
          > >
          > > > 2) the code to check for mod_perl does not appear to do any error
          > > > checking, even when evalling. (Again I note that I copied it from
          > > > CGI.pm)
          > >
          > > It's correct, since it checks 'defined $mod_perl::VERSION'

          But then does nothing if it's not defined. No graceful handling in other words.
          Or do I miss something?

          - nick

          --

          ~~~~~~~~~~~~~~~~~~~~
          Nick Tonkin {|8^)>
        • Stas Bekman
          ... Yup, I knew that this will happen. It misses this comment. I ve submitted a patch to Lincoln to add this comment. if (exists $ENV{MOD_PERL}) { eval
          Message 4 of 16 , Apr 10 9:58 PM
            Nick Tonkin wrote:
            > On Thu, 10 Apr 2003, Nick Tonkin wrote:
            >
            >
            >>On Fri, 11 Apr 2003, Stas Bekman wrote:
            >>
            >>
            >>>Nick Tonkin wrote:
            >>>[...]
            >>>
            >>>Your diff is the other way around, - should say what was removed and + what
            >>>was added, you just need to swap the order of files while doing diff-- the
            >>>newer file last...
            >>>
            >>>
            >>>>1) is the use of Apache->request correct? For mp1 and mp2?
            >>>
            >>>Yes
            >>>
            >>>
            >>>>2) the code to check for mod_perl does not appear to do any error
            >>>> checking, even when evalling. (Again I note that I copied it from
            >>>> CGI.pm)
            >>>
            >>>It's correct, since it checks 'defined $mod_perl::VERSION'
            >>
            >
            > But then does nothing if it's not defined. No graceful handling in other words.
            > Or do I miss something?

            Yup, I knew that this will happen. It misses this comment. I've submitted a
            patch to Lincoln to add this comment.

            if (exists $ENV{MOD_PERL}) {
            eval "require mod_perl";
            # mod_perl handlers may run system() on scripts using CGI.pm
            # - make sure so we don't get fooled by inherited $ENV{MOD_PERL}
            if (defined $mod_perl::VERSION) {
            if ($mod_perl::VERSION >= 1.99) {

            it talks about system() calls made from mod_perl scripts. When this happens
            $ENV{MOD_PERL} exists, but eval fails, $mod_perl::VERSION undefined == we
            don't run under mod_perl.

            __________________________________________________________________
            Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
            http://stason.org/ mod_perl Guide ---> http://perl.apache.org
            mailto:stas@... http://use.perl.org http://apacheweek.com
            http://modperlbook.org http://apache.org http://ticketmaster.com
          • Stas Bekman
            ... Functionalty seems to be alright, I d just do one last tiny change: + my $raw_cookie; + my $r = shift; + $r ||= eval { Apache- request() } if
            Message 5 of 16 , Apr 10 10:04 PM
              > +# Fetch a list of cookies from the environment or the incoming headers and
              > +# return as a hash. The cookies are parsed as normal escaped URL data.
              > sub fetch {
              > my $class = shift;
              > - my $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
              > + my $raw_cookie;
              > + my $r;
              > + $r = shift if $MOD_PERL;
              > + $r ||= eval { Apache->request() } if $MOD_PERL;
              > + if ($r) {
              > + $raw_cookie = $r->headers_in->{'Cookie'};
              > + } else {
              > + if (exists $ENV{REQUEST_URI}) {
              > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
              > + } else {
              > + die '%ENV has not been set. Turn SetupEnv on or ' .
              > + 'run $r->subprocess_env()';
              > + }
              > + }
              > return () unless $raw_cookie;
              > return $class->parse($raw_cookie);
              > }

              Functionalty seems to be alright, I'd just do one last tiny change:

              + my $raw_cookie;
              + my $r = shift;
              + $r ||= eval { Apache->request() } if $MOD_PERL;
              + if ($r) {
              + $raw_cookie = $r->headers_in->{'Cookie'};
              + } else {
              + if (exists $ENV{REQUEST_URI}) {
              + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
              + } else {
              + die '%ENV has not been set. Turn SetupEnv on or ' .
              + 'run $r->subprocess_env()';
              + }
              + }

              but yours is just fine (though if you decide to go with yours pack the two if
              $MOD_PERL together).

              __________________________________________________________________
              Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
              http://stason.org/ mod_perl Guide ---> http://perl.apache.org
              mailto:stas@... http://use.perl.org http://apacheweek.com
              http://modperlbook.org http://apache.org http://ticketmaster.com
            • Nick Tonkin
              On Fri, 11 Apr 2003, Stas Bekman wrote: +# Fetch a list of cookies from the environment or the incoming headers and +# return as a hash. The cookies
              Message 6 of 16 , Apr 10 10:05 PM
                On Fri, 11 Apr 2003, Stas Bekman wrote:

                >
                > > +# Fetch a list of cookies from the environment or the incoming headers and
                > > +# return as a hash. The cookies are parsed as normal escaped URL data.
                > > sub fetch {
                > > my $class = shift;
                > > - my $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
                > > + my $raw_cookie;
                > > + my $r;
                > > + $r = shift if $MOD_PERL;
                > > + $r ||= eval { Apache->request() } if $MOD_PERL;
                > > + if ($r) {
                > > + $raw_cookie = $r->headers_in->{'Cookie'};
                > > + } else {
                > > + if (exists $ENV{REQUEST_URI}) {
                > > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
                > > + } else {
                > > + die '%ENV has not been set. Turn SetupEnv on or ' .
                > > + 'run $r->subprocess_env()';
                > > + }
                > > + }
                > > return () unless $raw_cookie;
                > > return $class->parse($raw_cookie);
                > > }
                >
                > Functionalty seems to be alright, I'd just do one last tiny change:

                Done. Patch attached.

                >
                > + my $raw_cookie;
                > + my $r = shift;
                > + $r ||= eval { Apache->request() } if $MOD_PERL;
                > + if ($r) {
                > + $raw_cookie = $r->headers_in->{'Cookie'};
                > + } else {
                > + if (exists $ENV{REQUEST_URI}) {
                > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
                > + } else {
                > + die '%ENV has not been set. Turn SetupEnv on or ' .
                > + 'run $r->subprocess_env()';
                > + }
                > + }
                >
                > but yours is just fine (though if you decide to go with yours pack the two if
                > $MOD_PERL together).
                >
                > __________________________________________________________________
                > Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
                > http://stason.org/ mod_perl Guide ---> http://perl.apache.org
                > mailto:stas@... http://use.perl.org http://apacheweek.com
                > http://modperlbook.org http://apache.org http://ticketmaster.com
                >


                - nick

                --

                ~~~~~~~~~~~~~~~~~~~~
                Nick Tonkin {|8^)>
              • Stas Bekman
                ... it s set to . Notice that we check: exists $ENV{QUERY_STRING} may be using REQUEST_METHOD is better. ... which is a wrong assumption I suppose, otherwise
                Message 7 of 16 , Apr 10 10:07 PM
                  > Hm. So $ENV{QUERY_STRING} gets set (to what?) when there is no query string?

                  it's set to "". Notice that we check:

                  exists $ENV{QUERY_STRING}

                  may be using REQUEST_METHOD is better.

                  > Whatever. I don't care either, I just think it's more intuitive to assume that
                  > there's always going to be a PATH_INFO ...

                  which is a wrong assumption I suppose, otherwise it's would be always set in
                  C. You are probably confusing it with REQUEST_URI, which is always there and
                  also an intuitive choice.

                  __________________________________________________________________
                  Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
                  http://stason.org/ mod_perl Guide ---> http://perl.apache.org
                  mailto:stas@... http://use.perl.org http://apacheweek.com
                  http://modperlbook.org http://apache.org http://ticketmaster.com
                • Stas Bekman
                  ... Great! mp1 has a test which uses CGI::Cookie, so you probably want to test that it works. Also porting the test to mp2 s test suite would be great too.
                  Message 8 of 16 , Apr 10 10:14 PM
                    > Done. Patch attached.

                    Great!

                    mp1 has a test which uses CGI::Cookie, so you probably want to test that it
                    works. Also porting the test to mp2's test suite would be great too.

                    __________________________________________________________________
                    Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
                    http://stason.org/ mod_perl Guide ---> http://perl.apache.org
                    mailto:stas@... http://use.perl.org http://apacheweek.com
                    http://modperlbook.org http://apache.org http://ticketmaster.com
                  • Nick Tonkin
                    ... As you can see that s what I went with. - nick -- ~~~~~~~~~~~~~~~~~~~~ Nick Tonkin {|8^)
                    Message 9 of 16 , Apr 10 10:17 PM
                      On Fri, 11 Apr 2003, Stas Bekman wrote:

                      >
                      > > Hm. So $ENV{QUERY_STRING} gets set (to what?) when there is no query string?
                      >
                      > it's set to "". Notice that we check:
                      >
                      > exists $ENV{QUERY_STRING}
                      >
                      > may be using REQUEST_METHOD is better.
                      >
                      > > Whatever. I don't care either, I just think it's more intuitive to assume that
                      > > there's always going to be a PATH_INFO ...
                      >
                      > which is a wrong assumption I suppose, otherwise it's would be always set in
                      > C. You are probably confusing it with REQUEST_URI, which is always there and
                      > also an intuitive choice.
                      >

                      As you can see that's what I went with.

                      - nick

                      --

                      ~~~~~~~~~~~~~~~~~~~~
                      Nick Tonkin {|8^)>
                    • Nick Tonkin
                      ... I don t have mp1 installed here ... and I m sure done for the night. Not sure when I can get any more mod_perl time ... - nick -- ~~~~~~~~~~~~~~~~~~~~ Nick
                      Message 10 of 16 , Apr 10 10:19 PM
                        On Fri, 11 Apr 2003, Stas Bekman wrote:

                        >
                        > > Done. Patch attached.
                        >
                        > Great!
                        >
                        > mp1 has a test which uses CGI::Cookie, so you probably want to test that it
                        > works. Also porting the test to mp2's test suite would be great too.

                        I don't have mp1 installed here ... and I'm sure done for the night. Not sure
                        when I can get any more mod_perl time ...

                        - nick

                        --

                        ~~~~~~~~~~~~~~~~~~~~
                        Nick Tonkin {|8^)>
                      • Stas Bekman
                        ... I tested your patch against the mp1 test suite, so we now have Apache- request path tested.
                        Message 11 of 16 , Apr 10 10:36 PM
                          Nick Tonkin wrote:
                          > On Fri, 11 Apr 2003, Stas Bekman wrote:
                          >
                          >
                          >>>Done. Patch attached.
                          >>
                          >>Great!
                          >>
                          >>mp1 has a test which uses CGI::Cookie, so you probably want to test that it
                          >>works. Also porting the test to mp2's test suite would be great too.
                          >
                          >
                          > I don't have mp1 installed here ... and I'm sure done for the night. Not sure
                          > when I can get any more mod_perl time ...

                          I tested your patch against the mp1 test suite, so we now have Apache->request
                          path tested.

                          __________________________________________________________________
                          Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
                          http://stason.org/ mod_perl Guide ---> http://perl.apache.org
                          mailto:stas@... http://use.perl.org http://apacheweek.com
                          http://modperlbook.org http://apache.org http://ticketmaster.com
                        Your message has been successfully submitted and would be delivered to recipients shortly.