flaws in encoding logic

[bartvde@…]

e-mails from Paul Ramsey:

As a followup to the discussion of encoding WMS URL parameters, the
logic in the mapwmslayer.c file seems to be backwards (and partially
missing).
 
The msSetWMSParamString function includes a boolean to optionally
encode the values passed (which is nice planning):
 
static int msSetWMSParamString(wmsParamsObj *psWMSParams,
                               const char *name, const char * value,
                               int urlencode)
 
But the calls to the function seem to use it backwards, encoding things
that shouldn't be encoded (LAYERS, BBOX, STYLES) and not encoding
others:
 
    msSetWMSParamString(psWMSParams, "LAYERS",  pszName, MS_TRUE);
    msSetWMSParamString(psWMSParams, "TRANSPARENT", "TRUE", MS_FALSE);
 
        snprintf(szBuf, 100, "%.15g,%.15g,%.15g,%.15g",
                 bbox.minx, bbox.miny, bbox.maxx, bbox.maxy);
        msSetWMSParamString(psWMSParams, "BBOX",    szBuf, MS_TRUE);
 
It doesn't look like the individual layers get encoded either. So the
upshot is that the Mapserver WMS client is probably generating
out-of-spec requests.

I think the specification is clear enough: there is a difference
between a "," and %2C.  This is particularly important for LAYERS, for
example:
 
LAYERS=Roads%2C%20Rough,Roads%2C%20Paved
 
If you decoded the LAYERS value *before* splitting on the "," you would
end up with four layers. If you did it after, you would end up with
two, as is intended.
 
The same twisted logic I assume applies to the BBOX parameter because
of the European use of "," as the decimal place in floating point
numbers.
 
So, basically, the behavior described in the specification is not
optional on either the client *or* the server side.  If you treat it is
if it were, you will occasionally (but not very often) get degenerate
results in your WMS client/server interactions.
 
BTW, Ed, you have read the paragraph you quoted exactly backwards: the
section requires that the special characters *not* be encoded when used
within the roles given in the table following.  The roles for the ?,+,&
and = are standard CGI roles, so are sort of (really) redundant to be
in the WMS spec. The role for "," though, is quite particular to the
WMS spec (From the Table 1): Separator between individual values in
list-oriented parameters (such as BBOX, LAYERS and STYLES in the GetMap
request).
 
So, to reiterate, you should never see a parameter that looks like this:
 
  BBOX=-180%2C-90%2C180%2C90%2C
 
Because the specification says that in the role of separator for BBOX
(and LAYERS and STYLES) parameters the "," is to be left unencoded (and
damn the RFC!)
 
  BBOX=-180,-90,180,90
 
On the server side, this little quirk of the spec requires some extra
steps in processing the magic parameters.  For LAYER, BBOX and STYLES,
first split on comma, then decode the individual elements, then
continue processing.  For any parameter *except* LAYER, BBOX and
STYLES, ordinary CGI handling is sufficient.  This of course will break
any client that currently uses naive URL encoding on *all* value
parameters.  Support for broken (out of spec) clients requires an extra
two-step to first check if there are any literal commas in the value
parameter.

[bartvde@…]

*** Bug 1303 has been marked as a duplicate of this bug. ***

[dmorissette]

This ticket is about WMS Client. See also ticket #1297 for related issues in the WMS server.

Both should probably be addressed at the same time (in 5.2?).

[pete]

Note that the FORMAT parameter is also incorrectly encoded. The slash between the type and the subtype should be literal.

[pramsey]

Patch for this issue, as of r7494

[pramsey]

The attached patch (against r7494) creates in-spec wms client requests, allowing Mapserver to pull data from the ultra-strict ERMapper image web server.

[pramsey]

Patch for this issue, as of r7498

[tomkralidis]

Paul: how will this affect (if at all) functionality with connecting to existing WMS servers. As well, how does this relate to #1296, where you suggest a wontfix.

Just want to coordinate #1296 and #1297 so as not to break anything both client and server.

[pramsey]

Replying to tomkralidis:

Paul: how will this affect (if at all) functionality with connecting to existing WMS servers. As well, how does this relate to #1297, where you suggest a wontfix.

This bug is about Mapserver-as-WMS-client, and the patch should allow Mapserver to connect to servers that are fairly strict about their spec compliance (ER Mapping Image server is one I know of). Because the server kicks out badly encoded requests as errors, you *must* encode properly in order to work with these servers.

Bug #1297 is about Mapserver-as-WMS-server, and I think we should leave Mapserver's currently lax behavior (decode everything uniformly) in place. There are only two use cases where Mapserver would benefit from a more compliant behavior as a server:

• if Mapserver is configured with LAYERs in which the NAME has an embedded comma: NAME "foo,bar"
• if somehow we want Mapserver to correctly decode BBOXes in which the ordinates are European-style floating points, with a "," used to denote the decimal.

Neither of these cases has been complained of so far, and the workaround for the first is completely trivial ("commas don't work? stop using commas!").

[tomkralidis]

Paul: thanks for the clarification. I agree with wontfix'ing #1297. Can you post a patch again against latest svn (r7519) and I will apply.

[pramsey]

Patch for this issue, as of r7519

[tomkralidis]

Paul: Thanks for the updated patch. Applied in trunk in r7520. No applicable changes in msautotest.

Can you check for sanity and then close this ticket? Thanks

[pramsey]

Looks sane, works against ER Mapper and Mapserver.