Fix URL encoding issue in HttpDownloader to handle special characters

[hstct]

closes #5686

[lubosmj]

Please, write a changelog message named after the issue number, like so: CHANGES/5686.bugfix.

https://pulpproject.org/pulpcore/docs/dev/tutorials/quickstart/?h=changelog#3-update-the-changelog-entry

[hstct]

Please, write a changelog message named after the issue number, like so: CHANGES/5686.bugfix.

https://pulpproject.org/pulpcore/docs/dev/tutorials/quickstart/?h=changelog#3-update-the-changelog-entry

Ah yea, sorry it was late yesterday xD

[hstct]

For additional context:

The issue we faced wasn't simply that the filename contained %3a, which should be decoded to :, but rather that the decoding process shouldn't have occurred in the first place. In reality, %3A is the encoded version of :, but only the % symbol was being encoded by the web server, resulting in the string becoming %253a. This allowed the file to be found despite the unexpected encoding.

[hstct]

Added some tests for this as well.

[hstct]

idk why the test is failing on github 😢

[git-hyagi]

hi @hstct

I was checking this error, and I think it might be related to the way aiohttp server is parsing the request (but it is just a guess, I could not make it work yet):

pulpcore/pulpcore/pytest_plugin.py

Lines 322 to 328 in e360a13

	def gen_fixture_server(gen_threaded_aiohttp_server):
	def _gen_fixture_server(fixtures_root, ssl_ctx):
	app = web.Application()
	call_record = add_recording_route(app, fixtures_root)
	return gen_threaded_aiohttp_server(app, ssl_ctx, call_record)
	yield _gen_fixture_server

I will do more tests and let you know if I find something.

[git-hyagi]

I managed to make the test work by modifying the pulpcore/pytest_plugin.py file with:

@pytest.fixture
def gen_fixture_server(gen_threaded_aiohttp_server):
    def _gen_fixture_server(fixtures_root, ssl_ctx):
        app = web.Application(middlewares=[_aiohttp_request_middleware])
        call_record = add_recording_route(app, fixtures_root)
        return gen_threaded_aiohttp_server(app, ssl_ctx, call_record)

    yield _gen_fixture_server


@web.middleware
async def _aiohttp_request_middleware(request,handler):
    unquoted_url = urllib.parse.unquote(request.url.human_repr())
    url = urllib.parse.urlparse(unquoted_url)
    response = await handler(request.clone(rel_url=url.path))
    return response

but I don't know the impact of this change in the other tests, or if this could be even considered a viable "solution".

[hstct]

hi @git-hyagi thanks for the help!

As far as I can tell and this has been the whole problem with this issue in general is that it always is dependent on how the web server is serving the files, whether or not it will encode %3a to %253a or read it as an encoded :. So your approach really did help out here. I've been running tests for pulp_rpm, pulpcore, and pulp_deb locally and they seem to work fine. However I doubt that there are many tests that purposefully use problematic filenames. And that is what it is in the end, files having problematic names with special characters that get encoded when served online. So this is a workaround to make it more robust but I also can't tell if it covers all edge cases.

[git-hyagi]

Thank you for the contribution!
LGTM

[gerrod3]

Nice testing!

[quba42]

I am afraid we may have shot ourselves in the foot here. Our internal integration tests revealed that while this fixes what we set out to fix, it broke the far more important use case of "download URL's with special characters in them".

I have just verified this using latest pulpcore main branch in oci-env. I tried syncing a fairly standard APT repo, which failed because Pulp tried to download adduser_3.118%252Bdeb11u1_all.deb instead of adduser_3.118+deb11u1_all.deb.

I suggest we revert this PR for now, until we can figure out a better approach. Unfortunately I fear there may be no good way to make the down-loader robust for both use cases.