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: It seemd more logical to test for $ENV{PATH_INFO} than $ENV{QUERY_STRING}, dunno if you agree.
    Message 1 of 16 , Apr 10, 2003
      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 2 of 16 , Apr 10, 2003
        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 3 of 16 , Apr 10, 2003
          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 4 of 16 , Apr 10, 2003
            > +# 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 5 of 16 , Apr 10, 2003
              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 6 of 16 , Apr 10, 2003
                > 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 7 of 16 , Apr 10, 2003
                  > 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 8 of 16 , Apr 10, 2003
                    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 9 of 16 , Apr 10, 2003
                      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 10 of 16 , Apr 10, 2003
                        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.