Rename QueryParams::Parse to ParseFromUrl. Add a wrapper that takes an untrusted string. #688 has missing wrapper

[peterbowey]

@matterbury @jeffkaufman This patch #688 (added to trunk-tracking) is incomplete and creates a compile error.

error log (with #688):

/root/rpmbuild/BUILD/nginx-1.7.1/ngx_pagespeed/src/ngx_pagespeed.cc: In function 'ngx_int_t net_instaweb::{anonymous}::ps_resource_handler(ngx_http_request_t*, bool, net_instaweb::{anonymous}::RequestRouting::Response)':
/root/rpmbuild/BUILD/nginx-1.7.1/ngx_pagespeed/src/ngx_pagespeed.cc:1800:18: error: 'class net_instaweb::QueryParams' has no member named 'ParseFromUrl'
     query_params.ParseFromUrl(url);
                  ^
/root/rpmbuild/BUILD/nginx-1.7.1/ngx_pagespeed/src/ngx_pagespeed.cc: In function 'ngx_int_t net_instaweb::{anonymous}::ps_simple_handler(ngx_http_request_t*, net_instaweb::NgxServerContext*, net_instaweb::{anonymous}::RequestRouting::Response)':
/root/rpmbuild/BUILD/nginx-1.7.1/ngx_pagespeed/src/ngx_pagespeed.cc:2397:18: error: 'class net_instaweb::QueryParams' has no member named 'ParseFromUrl'
     query_params.ParseFromUrl(url);
                  ^
/root/rpmbuild/BUILD/nginx-1.7.1/ngx_pagespeed/src/ngx_pagespeed.cc: At global scope:
/root/rpmbuild/BUILD/nginx-1.7.1/ngx_pagespeed/src/ngx_pagespeed.cc:1264:17: warning: 'net_instaweb::{anonymous}::ps_main_conf_t* net_instaweb::{anonymous}::ps_get_main_config(ngx_http_request_t*)' defined but not used [-Wunused-function]
 ps_main_conf_t* ps_get_main_config(ngx_http_request_t* r) {
                 ^
make[1]: *** [objs/addon/src/ngx_pagespeed.o] Error 1

If I do a reverse patch via https://github.com/pagespeed/ngx_pagespeed/commit/117994993a2872b717a3475e6ecfe6cc5d48bf7c.patch [#688], then the compile is successful.

Where is the 'wrapper'?

Notes: r3968 of modpagespeed was used on above compile.

[matterbury]

It looks like you don't have the latest PSOL code. The command on the website says wget https://dl.google.com/dl/page-speed/psol/1.7.30.4.tar.gz but you will need the latest trunk version.

I don't know the name for that but I'll ask around and post back if I find out.

[peterbowey]

@matterbury I do have the latest ngx_pagespeed trunk-tracking.

Per this: 1) git clone https://github.com/pagespeed/ngx_pagespeed.git 2) git checkout trunk-tracking

Via modpagespeed, per this: 1) gclient config http://modpagespeed.googlecode.com/svn/trunk/src 2) gclient sync --revision 3983 --force --jobs=1 3) etc, etc, etc (I assisted to debug this modpagespeed side)

I am entirely familiar with the 'entire' process - over 12 months of use... coding

[peterbowey]

According to https://code.google.com/p/modpagespeed/source/list, the "pbsol" I am using is at revision 1.8.31.2. This tracks with ngx_pagespeed trunk-tracking @ https://github.com/pagespeed/ngx_pagespeed/pull/678

[matterbury]

This file has the method that the error message says is missing: http://modpagespeed.googlecode.com/svn/trunk/src/pagespeed/kernel/http/query_params.h so I do not understand why you are getting this error.

Can you confirm that your copy of the file has the method?

On Fri, May 2, 2014 at 8:58 AM, Peter Bowey notifications@github.comwrote:

@matterbury https://github.com/matterbury I do have the latest ngx_pagespeed trunk-tracking.

Per this: 1) git clone https://github.com/pagespeed/ngx_pagespeed.git 2) git checkout trunk-tracking

Via modpagespeed, per this: 1) gclient config http://modpagespeed.googlecode.com/svn/trunk/src 2) gclient sync --revision 3983 --force --jobs=1 3) etc, etc, etc (I assisted to debug this modpagespeed side)

I am entirely familiar with the 'entire' process - over 12 months of use... coding

— Reply to this email directly or view it on GitHubhttps://github.com/pagespeed/ngx_pagespeed/issues/690#issuecomment-42028465 .

"Klaatu barada nikto" (754) 444-6288

[peterbowey]

My recently 'pulled' (today) modpagespeed r3968 has this 'method' at mod_pagespeed/src/pagespeed/kernel/http/query_params.h

// Copyright 2010-2011 Google Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
// Author: jmarantz@google.com (Joshua Marantz)

#ifndef PAGESPEED_KERNEL_HTTP_QUERY_PARAMS_H_
#define PAGESPEED_KERNEL_HTTP_QUERY_PARAMS_H_

#include "pagespeed/kernel/base/string_multi_map.h"

#include "pagespeed/kernel/base/basictypes.h"
#include "pagespeed/kernel/base/string.h"
#include "pagespeed/kernel/base/string_util.h"

namespace net_instaweb {

// Parses and rewrites URL query parameters.
class QueryParams {
 public:
  QueryParams() { }

Thus it does not match the url at http://modpagespeed.googlecode.com/svn/trunk/src/pagespeed/kernel/http/query_params.h

[peterbowey]

What is clearly 'missing' in my modpagespeed pull, is this:

class GoogleUrl;

Yet I did this only 3 hours ago - at r3983 ??

[matterbury]

On Fri, May 2, 2014 at 9:14 AM, Peter Bowey notifications@github.comwrote:

Thus it does not match the url at http://modpagespeed.googlecode.com/svn/trunk/src/pagespeed/kernel/http/query_params.h

I'm sorry, but I don't understand what you're trying to say. The file at that link has the two new methods ParseFromUrl and ParseFromUntrustedString. The error message says 'class net_instaweb::QueryParams' has no member named 'ParseFromUrl' So I don't understand what is going wrong ... do you have any ideas?

[matterbury]

Ah, thanks. So, your pull isn't the latest version.

Navigating to http://modpagespeed.googlecode.com/svn/ shows that the latest version is 3984.

Perhaps try syncing again? gclient sync --revision 3984 --force --jobs=1

On Fri, May 2, 2014 at 9:20 AM, Peter Bowey notifications@github.comwrote:

What is clearly 'missing' in my modpagespeed pull, is this:

class GoogleUrl;

Yet I did this only 3 hours ago - at r3983 ??

— Reply to this email directly or view it on GitHubhttps://github.com/pagespeed/ngx_pagespeed/issues/690#issuecomment-42031064 .

"Klaatu barada nikto" (754) 444-6288

[peterbowey]

@matterbury Thanks!

Correct, I also realized that - at the same time!

Per the 'trunk' modpagespeed notes here:

Change log
r3984 by matterb...@google.com on Apr 30 (47 hours ago)   Diff

Rename QueryParams::Parse to ParseFromUrl.
Add a wrapper around it that takes an
untrusted string: ParseFromUntrustedString

Some notes could be 'appended' about requiring modpagespeed @ pull r3984+

@jeffkaufman notes on the latest ngx_pagespeed trunk-tracking being @ r3968 threw my 'logic' here. For at that time, I could not see that ngx_pagespeed 'trunk-tracking' would work at modpagespeed 'above r3983.

I now realize the error is mine. [duh].

Close this issue and kindly update the ngx_modepagespeed notes - for trunk-tracking.

[matterbury]

No problem, glad we could sort it out!

FYI, we've recently changed the build process here so that we update ngx_pagespeed concurrently with psol and mod_pagespeed. Previously we'd periodically resync ngx to psol and fix the resulting breakages, but that was a pain for the unlucky bunny tasked with it. So, now we all update ngx as we go, which means noobs like me have to learn the new process, and there's an increased risk of getting it wrong.

Anyway, my point is, please don't hesitate to shout if something doesn't work - we want it to! :-)

[peterbowey]

@matterbury Many thanks, I realize we all can benefit here. I am pleased to hear of the close tracking with mod_pagespeed + ngx_pagespeed.

Previously, I often had to wait weeks for some possible synchronicity.. via the two releases.

Great team work!