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

Double slashes when generating the inventory file URL #12782

Closed
replaceafill opened this issue Aug 14, 2024 · 7 comments · Fixed by #12807
Closed

Double slashes when generating the inventory file URL #12782

replaceafill opened this issue Aug 14, 2024 · 7 comments · Fixed by #12807
Labels
easy issue that can be solved by beginners good first issue help wanted type:bug
Milestone

Comments

@replaceafill
Copy link

Describe the bug

This might be considered an edge case but I've noticed that if the value tuple of a intersphinx_mapping contains a base URI with a trailing slash and None as the inventory file, Sphinx will try to load a URL with double slashes.

How to Reproduce

Using Sphinx 8.0.2 set the following in the conf.py file of a sphinx-quickstart project:

extensions = ["sphinx.ext.intersphinx"]
intersphinx_mapping = {'python': ('https://docs.python.org/3/', None)}

Run make html and notice the double slashes in the output message:

...
loading intersphinx inventory 'python' from https://docs.python.org/3//objects.inv ...
...

The double slashes URL could be problematic depending on the web server setup.

Environment Information

Platform:              linux; (Linux-6.5.0-45-generic-x86_64-with-glibc2.35)
Python version:        3.10.12 (main, Jul 29 2024, 16:56:48) [GCC 11.4.0])
Python implementation: CPython
Sphinx version:        8.0.2
Docutils version:      0.21.2
Jinja2 version:        3.1.4
Pygments version:      2.18.0

Sphinx extensions

["sphinx.ext.intersphinx"]

Additional context

I think the problem could have been introduced in #12087 where:

if not inv:
inv = posixpath.join(uri, INVENTORY_FILENAME)

was changed into:

inv = f'{project.target_uri}/{INVENTORY_FILENAME}' if location is None else location

posixpath.join removes the trailing slash in the target URI, but the f-string doesn't.

@AA-Turner
Copy link
Member

PR welcome!

A

@picnixz
Copy link
Member

picnixz commented Aug 15, 2024

Ah yes, this one is on me. I don't remember why I did this. Maybe I incorrectly thought that the target_uri was stripped of its slash and wanted to reduce the imports. If this is the case, then I'm sorry. I'll do something for that this w-e or someone can fix it (it shouldn't be that hard).

@picnixz picnixz added the easy issue that can be solved by beginners label Aug 15, 2024
@jayaddison
Copy link
Contributor

Would removesuffix('/') be sufficient to fix this, or should we reinstate posixpath joining?

@picnixz
Copy link
Member

picnixz commented Aug 20, 2024

Technically, since we are dealing with URLs, we should have used rstrip('/') because if you put 3 forward slashes, then

>>> posixpath.join('https://a.com//', 'b')
'https://a.com//b'

I'll prepare a PR.

@jayaddison
Copy link
Contributor

Websites probably shouldn't accept significant-multiple-separators in their paths, but I think that technically it's valid for them to do that. (serving different content at foo.invalid/file.txt from foo.invalid/////file.txt, for example).

But foo.invalidobjects.env is never what we want, so I wonder whether the logic here should in fact be base + ('/' if not base.endswith('/') else '') + inv_filename.

@jayaddison
Copy link
Contributor

(and then it's a question of whether we do that ourselves or via posixpath -- as presented -- or some other mechanism. it's not a file path, so posixpath seems a bit odd to me, but in this use case, the relevant logic does probably overlap fairly closely..)

@picnixz
Copy link
Member

picnixz commented Aug 21, 2024

I revert it to the old behaviour in #12807. I don't want to mess with URLs and get complaints later :'). If we want to move from our current logic, more places need to be changed (there are some ifs where we test whether the URI is in some set of URIs where we explicitly add a forward slash, I don't remember where but I know it exists).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2024
@AA-Turner AA-Turner added this to the 8.1.0 milestone Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
easy issue that can be solved by beginners good first issue help wanted type:bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants