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

Make link to issue template url safe #3508

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JCZuurmond
Copy link
Member

Changes

Make link to issue template url safe using Python's urllib

Linked issues

Introduced in #3498

@JCZuurmond JCZuurmond added the migrate/code Abstract Syntax Trees and other dark magic label Jan 10, 2025
@JCZuurmond JCZuurmond self-assigned this Jan 10, 2025
@JCZuurmond JCZuurmond requested a review from a team as a code owner January 10, 2025 08:21
Copy link

✅ 58/58 passed, 2 skipped, 29m15s total

Running from acceptance #7958

Copy link
Contributor

@asnare asnare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works as intended.

Please test the URL, and ensure that the source code being embedded in the URL includes each of the following characters at least once somewhere: +, &, %, (space) and =.

@@ -96,7 +97,7 @@ def _definitely_failure(source_code: str, e: Exception) -> MaybeTree:
end_col=(e.error.end_offset or 2) - 1,
),
)
new_issue_url = (
new_issue_url = urllib.parse.quote_plus(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the URL produced by this. :) (I think it is mangled and does not work?)

I think the problem here is:

  • The quote() and quote_plus() methods are intended only quoting/escaping individual path and query parameter components of a URL, not the whole thing. For example, the scheme ends up being encoded as https:%3A%2F%2f by this.
  • The query parameters in this URL are already quoted/escaped: that's why the various + bits are in the title parameter already, as well as all the %-encoded bits and pieces. (In the first generation of web servers this was the encoding used for form parameters, before UTF-8 and %-encoding came along).

Looking at the context, I suspect you only intended to escape the source code being inserted into the URL, not the already-escaped URL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migrate/code Abstract Syntax Trees and other dark magic
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

2 participants