Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inconsistent URL encoding/decoding with WMS sources #59143

Open
2 tasks done
cioddi opened this issue Oct 20, 2024 · 1 comment · May be fixed by #59144
Open
2 tasks done

inconsistent URL encoding/decoding with WMS sources #59143

cioddi opened this issue Oct 20, 2024 · 1 comment · May be fixed by #59144
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter!

Comments

@cioddi
Copy link

cioddi commented Oct 20, 2024

What is the bug or the crash?

QGIS improperly encodes/decodes data source URLs, including URL parameter values, causing the original values to be altered and leading to data loss. Additionally, QgsDataSourceUri does not fully URL-encode the values when assembling the data source string leading to potential data loss.

When constructing a QgsDataSourceUri, parameter values are not fully URL-encoded. In this case only the =is encoded to %26. This can result in incorrect URL parameters being sent in the requests. For example:

  • Original URL added as WMS source:
    https://127.0.0.1/%26%3F%26%3F/?non_standard_param=%26%3F%26%3F%26%3F

  • Resulting QgsDataSourceUri string:
    dpiMode=7&featureCount=10&tilePixelRatio=0&url=https://127.0.0.1/%26%3F%26%3F/?non_standard_param%3D%26%3F%26%3F%26%3F

  • Actual GetCapabilities Request Sent:
    https://127.0.0.1/&?&?/?non_standard_param=&?&?&?SERVICE=WMS&REQUEST=GetCapabilities

Expected behaviour

  • Original URL added as WMS source:
    https://127.0.0.1/%26%3F%26%3F/?non_standard_param=%26%3F%26%3F%26%3F

  • Resulting QgsDataSourceUri string (values are fully URL-encoded:
    dpiMode=7&featureCount=10&tilePixelRatio=0&url=https%3A%2F%2F127.0.0.1%2F%2526%253F%2526%253F%2F%3Fnon_standard_param%3D%2526%253F%2526%253F%2526%253F

  • Actual GetCapabilities Request Sent:
    https://127.0.0.1/%26%3F%26%3F/?non_standard_param=%26%3F%26%3F%26%3F&SERVICE=WMS&REQUEST=GetCapabilities

Steps to reproduce the issue

  1. open QGIS and the request debug console
  2. add a WMS data source with the URL https://127.0.0.1/%26%3F%26%3F/?non_standard_param=%26%3F%26%3F%26%3F
  • in qgsnewhttpconnection.cpp (363) QUrl decodes some of the URL-encoded characters resulting in https://127.0.0.1/%26?%26?/
    QUrl url( txtUrl->text().trimmed() );
  • in qgsnewhttpconnection.cpp (381) the QUrl query is set to non_standard_param=%26%3F%26%3F%26%3F
    url.setQuery( query );
  • in qgsowsconnection.cpp (87) the QgsDataSourceUri param "url" is set to the original url https://127.0.0.1/%26%3F%26%3F/?non_standard_param=%26%3F%26%3F%26%3F resulting in the param value
    mUri.setParam( QStringLiteral( "url" ), url );
  1. right click the new WMS data source and click refresh
  • in qgswmsdataitems.cpp (61) the QgsDataSourceUri is parsed. mUri has the value dpiMode=7&featureCount=10&tilePixelRatio=0&url=https://127.0.0.1/%26%3F%26%3F/?non_standard_param%3D%26%3F%26%3F%26%3F. The key-value separator "=" has been URL-encoded to %3D the other character left untouched making it impossible to retain the original URL. The "url" param of uri becomes https://127.0.0.1/&%3F&%3F/?non_standard_param=&%3F&%3F&%3F resulting in an empty value for non_standard_param.
    uri.setEncodedUri( mUri );
  • in qgswmsprovider.cpp (280) the whole thing is URL-decoded resulting in https://127.0.0.1/&?&?/?non_standard_param=&?&?&?
    // some services provide a percent/url encoded (legend) uri string, always decode here
    uri = QUrl::fromPercentEncoding( uri.toUtf8() );
    
  1. the GetCapabilities request is made to https://127.0.0.1/&?&?/?non_standard_param=&?&?&?SERVICE=WMS&REQUEST=GetCapabilities
  • qgswmscapabilities.cpp (2559) QNetworkRequest request( url );

Versions

QGIS 3.38.3 (probably all previous versions since QgsDataSourceUri was added and used for non database providers)
Master

Supported QGIS version

  • I'm running a supported QGIS version according to the roadmap.

New profile

Additional context

Related Issues:

@cioddi cioddi added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Oct 20, 2024
@cioddi cioddi linked a pull request Oct 20, 2024 that will close this issue
@AndrewAnnex
Copy link
Contributor

I would like to +1 this issue and provide some additional examples that could be incorporated into the tests, the titiler dynamic tile server accepts many optional parameters in query URLs to enable dynamic styling of COGs, for example:

https://titiler.xyz/cog/tiles/WebMercatorQuad/16/34060/23336@1x?url=https%3A%2F%2Fdata.geo.admin.ch%2Fch.swisstopo.swissalti3d%2Fswissalti3d_2019_2573-1085%2Fswissalti3d_2019_2573-1085_0.5_2056_5728.tif&bidx=1&rescale=1600%2C2100&colormap_name=gist_earth

returns the following RGB rendering of a elevation model scaled between 1600 and 2100 meters with the gist_earth colormap from matplotlib.

23336@1x

While it's trivial to do the same with QGIS's style manager, there are lots of other capabilities including custom algorithms and other parameters important for working with titiler that get striped from the tile request urls by QGIS.

The following GDAL WMS xml file can be loaded in QGIS, but the styling from the above url example get's stripped

<GDAL_WMS>
  <Service name="TMS">
    <ServerUrl>https://titiler.xyz/cog/tiles/WebMercatorQuad/${z}/${x}/${y}@1x.png?url=https://data.geo.admin.ch/ch.swisstopo.swissalti3d/swissalti3d_2019_2573-1085/swissalti3d_2019_2573-1085_0.5_2056_5728.tif&bidx=1&rescale=1600,2100&colormap_name=gist_earth</ServerUrl>
    <SRS>EPSG:3857</SRS>
    <ImageFormat>image/png</ImageFormat>
  </Service>
  <DataWindow>
    <UpperLeftX>-20037508.34</UpperLeftX>
    <UpperLeftY>20037508.34</UpperLeftY>
    <LowerRightX>20037508.34</LowerRightX>
    <LowerRightY>-20037508.34</LowerRightY>
    <TileLevel>17</TileLevel>
    <TileCountX>1</TileCountX>
    <TileCountY>1</TileCountY>
    <YOrigin>top</YOrigin>
  </DataWindow>
  <Projection>EPSG:3857</Projection>
  <BlockSizeX>256</BlockSizeX>
  <BlockSizeY>256</BlockSizeY>
  <BandsCount>3</BandsCount>
  <MaxConnections>4</MaxConnections>
  <ZeroBlockHttpCodes>204,404</ZeroBlockHttpCodes>
  <ZeroBlockOnServerException>true</ZeroBlockOnServerException>
</GDAL_WMS>
image

For those interested in testing with this file, the source geotiff is fairly small with bounds in EPSG:3857 of 789324.756450882, 5766907.715332875, 790765.7771960256, 5768353.435890727 or in EPSG:4326 of 7.090624928537461, 45.91605844102821, 7.1035698381384185, 45.92509300025415

I'd like to help get #59144 merged, but it looks ready to me at first glance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants