Opened 16 years ago

Closed 16 years ago

Last modified 16 years ago

#2528 closed defect (fixed)

restore version as optional parameter for GetCapabilities

Reported by: tomkralidis Owned by: tomkralidis
Priority: normal Milestone: 5.2 release
Component: WMS Server Version: svn-trunk (development)
Severity: normal Keywords:
Cc: mapserverbugs, pal.kristensen@…


Email from Pål Kristensen:

I did a build of mapserver nightly this evening and noticed some changed WMS behavior compared to my last build (approx 2 months ago). The old build does reply to at GetCapabilities request that misses the version parameter with a version 1.1.1 capabilities xml-file. The new build fails withe an error message that says that the version parameter is missing. According to the WMS 1.1.1 standard version is a mandatory parameter so the new behavior is correct according to the standard. Unfortunately many of the clients around do not comply with the standard, and of course the server gets the blame.

Change History (14)

comment:1 by tomkralidis, 16 years ago

Cc: mapserverbugs added

The only related change I could spot is I have tested this in branch-5.0 and it does work. Will fix/revert for trunk.

comment:2 by tomkralidis, 16 years ago

Fixed in r7407. Pål: can you confirm? I will then close this ticket.

comment:3 by dmorissette, 16 years ago

Tom, in r7407 you only commented out the test. Please don't leave unused code commented out as it accumulates over time (unless there is a good reason to leave it there). I have removed it in r7410.

comment:4 by tomkralidis, 16 years ago

Forgot. Thanks for removing.

comment:5 by dmorissette, 16 years ago

BTW, do we not need to distinguish between version being not specified (optional) and version being specified in an invalid format? There is a legit case to produce an exception if the version format is invalid, but not if it's simply omitted.

I'm working on a fix to handle this.

comment:6 by tomkralidis, 16 years ago

Good point. Might want to add some tests to msautotest as well for this.

comment:7 by dmorissette, 16 years ago

Resolution: fixed
Status: newclosed

I have modified msOWSParseVersionString() to return either OWS_VERSION_NOTSET (-1) or OWS_VERSION_BADFORMAT (-2) if the version cannot be parsed, updated all calling code to use those two new #defines instead of the -1 constant, and finally updated the test that was at the root of this ticket in mapwms.c to still produce an exception if the version format is invalid, but not if it's omitted since it's optional in the GetCapabilities case.

I have not added tests to msautotests, I will leave that to you Tom.

comment:8 by tomkralidis, 16 years ago

Thanks for this. I've tested with:

$ ./mapserv QUERY_STRING="map=/tmp/"

and I do not get back an exception, but I should.

Does msOWSParseVersionString need fixing?

comment:9 by tomkralidis, 16 years ago

Dan: here's a patch that works against stuff like "1", "1.", ".1"; not sure if you're working on this already, I will commit if you are not and this change is satisfactory.

Index: mapows.c
--- mapows.c    (revision 7411)
+++ mapows.c    (working copy)
@@ -391,8 +391,19 @@
     if (pszVersion)
         int nVersion = 0;
+        int nValid = 0;
+        int i;
         digits = msStringSplit(pszVersion, '.', &numDigits);
-        if (digits == NULL || numDigits < 2 || numDigits > 3)
+        for (i=0;i<numDigits;i++) {
+          if (strcasecmp(digits[i], "\0") == 0) {
+            nValid = 1;
+            break;
+          }
+        }
+        if (digits == NULL || numDigits < 2 || numDigits > 3 || nValid == 1)
                        "Invalid version (%s). Version must be in the "

comment:10 by dmorissette, 16 years ago

Do we need to be that picky?

comment:11 by tomkralidis, 16 years ago

Well, if we are going to be checking for an invalid format. Here's what is from OWS Common 1.1, subclause 7.3.1:

" Each OWS Implementation Specification revision shall specify a version number, which enables interacting clients and servers to agree on which version of the specification they are conforming to. A version number shall contain three non-negative integers separated by decimal points, in the form "x.y.z". The integers y and z shall not exceed 99. "

Since MapServer does not support any versions of any specifications which break these conditions.

comment:12 by dmorissette, 16 years ago

WMS 1.0.0 used to be more open on the version format, that's probably why the current implementation is more relaxed:

6.1 Version Numbering and Negotiation

6.1.1 Version Number Form

The specification version number contains up to three positive integers, 
separated by decimal points. The following forms are possible: "x", "x.y", 
or "x.y.z".

The current implementation treats ".1" as 0.1, and "1." as "1.0" which is an acceptable behavior to me even if it's not not strictly *enforcing* the text of the OWS common.

My vote goes to leaving things as is unless there is a CITE test that we fail because of that.

comment:13 by tomkralidis, 16 years ago

Yet another new (OWS Common) vs old "common approaches" issue. We can leave this for now, but at least we have this on record.

Having said this, I'll probably make a tighter msOWSCommonParseVersionString in mapowscommon.c if/when required, given the SOS support and in general on how OGC services are moving w.r.t. OWS Common.

comment:14 by dmorissette, 16 years ago

Please no, we don't need a separate one.

My point is simply that being more strict than we are today in this specific case would bring no value. The current msOWSParseVersionString() makes enough checks to make sure that any reasonable input values results in a valid request response, and there is no known possibility that feeding junk through the VERSION param can crash MapServer or result in unpredictable behavior. Adding more validation would be purely useless overhead as fas as MapServer is concerned.

Let's leave it up to CITE to make tests to sTrIcTlY eNfOrCe the text of the specs. ;)

Note: See TracTickets for help on using tickets.