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
    Nick Tonkin wrote: The patch looks good (though I haven t tested it). One thing: [...] ... the only situation when CGI env vars might be unavailable is under
    Message 1 of 16 , Apr 10, 2003
    • 0 Attachment
      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.

      > 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.

      __________________________________________________________________
      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:Nick Tonkin wrote: The patch looks good (though I haven t tested it). One thing: [...] + my $r; +
      Message 2 of 16 , Apr 10, 2003
      • 0 Attachment
        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^)>
      • Stas Bekman
        ... 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:
        Message 3 of 16 , Apr 10, 2003
        • 0 Attachment
          > > 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
        • 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 4 of 16 , Apr 10, 2003
          • 0 Attachment
            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 5 of 16 , Apr 10, 2003
            • 0 Attachment
              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 6 of 16 , Apr 10, 2003
              • 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 7 of 16 , Apr 10, 2003
                • 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 8 of 16 , Apr 10, 2003
                  • 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 9 of 16 , Apr 10, 2003
                    • 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 10 of 16 , Apr 10, 2003
                      • 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 11 of 16 , Apr 10, 2003
                        • 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 12 of 16 , Apr 10, 2003
                          • 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 13 of 16 , Apr 10, 2003
                            • 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.