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

url.join is a footgun and needs to be deprecated #1024

Closed
djugei opened this issue Feb 7, 2025 · 2 comments
Closed

url.join is a footgun and needs to be deprecated #1024

djugei opened this issue Feb 7, 2025 · 2 comments

Comments

@djugei
Copy link

djugei commented Feb 7, 2025

problem description

The semantics of what url.join does depends on the value of the input parameter. this is bad.
for example you may want to join in a path after a base url. If that path contains a ":" at the wrong spot this overwrites the entire url because it gets interpreted as a protocol specifier.
Very stringly typed, kinda reminds me of building sql queries from concatenating strings, and very much does not lead the developer down a "pit of success".

proposed solution

deprecate the join api.
introduce .replace_x and, where applicable, .extend_x for x in [scheme, host, port, path, "file" (last part of the path), query, fragment, username, password]

with clear documentation, especially for the path part, on what extending does (straight string append, dependency on leading slashes, interaction with the "file" part, etc.), or splitting the extend function into multiple clearly defined ones.

reasoning

path handling is hard and a big source of security issues. it should be solved in the common dependency instead of by each individual aplication.

i am willing to contribute the required code.

@valenting
Copy link
Collaborator

See #333 and #934

@djugei
Copy link
Author

djugei commented Feb 11, 2025

#333 describes join as a surprising/undescriptive name for what it does. it was closed with "no better name found".
that reasoning does not apply to this issue. This issue is about join being a bad api, no matter the name,
Therefore proposing deprecation and replacement.

additionally i would like to know if a pr which does what is described in the issue would be accepted.
#934 is a partial implementation of what i described here.

until then i would ask for a reopen, or alternatively for a close as won'tfix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants