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

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

Expand Messages
  • 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 1 of 16 , Apr 10 9:58 PM
    • 0 Attachment
      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 2 of 16 , Apr 10 10:04 PM
      • 0 Attachment
        > +# 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 3 of 16 , Apr 10 10:05 PM
        • 0 Attachment
          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 4 of 16 , Apr 10 10:07 PM
          • 0 Attachment
            > 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 5 of 16 , Apr 10 10:14 PM
            • 0 Attachment
              > 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 6 of 16 , Apr 10 10:17 PM
              • 0 Attachment
                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 7 of 16 , Apr 10 10:19 PM
                • 0 Attachment
                  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 8 of 16 , Apr 10 10:36 PM
                  • 0 Attachment
                    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.