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
    ... it s checked by defined $mod_perl::VERSION ... that s correct. But since you do: $r || Apache- request the second option is irrelevant. Apache- request
    Message 1 of 16 , Apr 10, 2003
      Nick Tonkin wrote:

      >>>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'
      >
      >
      > what about
      >
      > eval "require mod_perl";
      >
      > which never gets checked?

      it's checked by 'defined $mod_perl::VERSION'

      >>well, that's a tiny bit code dup. and also if you use Apache->request, you
      >>don't need to use %ENV at all.
      >
      >
      > Eh? You said:
      >
      > if $r is not available, there are a few things to choose from:
      > 1) can use Apache->request
      > - Apache->request will die if not available
      > 2) can use %ENV
      > - but SetupEnv might be off, so we can do:
      > die "Turn SetupEnv on or run $r->subprocess_env"
      > unless exists $ENV{QUERY_STRING};

      that's correct. But since you do: $r || Apache->request the second option is
      irrelevant. Apache->request already throws an exception if it's not available.

      __________________________________________________________________
      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
      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 2 of 16 , Apr 10, 2003
        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 3 of 16 , Apr 10, 2003
          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 4 of 16 , Apr 10, 2003
            > > 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 5 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 6 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 7 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 8 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 9 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 10 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 11 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 12 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 13 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 14 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.