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.

Sign up for GitHub

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

Jump to bottom

Validate Alternate Repository Location URLS #16817

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

Validate Alternate Repository Location URLS #16817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions warehouse/api/simple.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@

from warehouse.cache.http import add_vary, cache_control
from warehouse.cache.origin import origin_cache
from warehouse.constants import (
MIME_PYPI_SIMPLE_V1_HTML,
MIME_PYPI_SIMPLE_V1_JSON,
MIME_TEXT_HTML,
)
from warehouse.packaging.models import JournalEntry, Project
from warehouse.packaging.utils import (
_simple_detail,
Expand All @@ -26,10 +31,6 @@
)
from warehouse.utils.cors import _CORS_HEADERS

MIME_TEXT_HTML = "text/html"
MIME_PYPI_SIMPLE_V1_HTML = "application/vnd.pypi.simple.v1+html"
MIME_PYPI_SIMPLE_V1_JSON = "application/vnd.pypi.simple.v1+json"


def _select_content_type(request: Request) -> str:
# The way this works, is this will return a list of
Expand Down
9 changes: 9 additions & 0 deletions warehouse/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,12 @@
ONE_GIB = 1 * 1024 * 1024 * 1024
MAX_FILESIZE = 100 * ONE_MIB
MAX_PROJECT_SIZE = 10 * ONE_GIB

MIME_TEXT_HTML = "text/html"
MIME_PYPI_SIMPLE_V1_HTML = "application/vnd.pypi.simple.v1+html"
MIME_PYPI_SIMPLE_V1_JSON = "application/vnd.pypi.simple.v1+json"

MIME_PYPI_SIMPLE_V1_ALL = [
MIME_PYPI_SIMPLE_V1_JSON,
MIME_PYPI_SIMPLE_V1_HTML,
]
21 changes: 21 additions & 0 deletions warehouse/manage/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

import wtforms

from urllib3 import PoolManager, Timeout
Copy link
Member

Choose a reason for hiding this comment

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

question: ‏we use requests in other places in the code (example), and this appears to be the first time we're using urllib3 directly. Is that to get both connect and read timeouts? If so, then we can pass those as a tuple to requests like so:

response = requests.head(field.data, timeout=(1.0, 1.0), headers=...)

But that apparently has the same affect as only specifying it once, since th value will be applied to both, so we could satisfy with

response = requests.head(field.data, timeout=1.0, headers=...)

https://requests.readthedocs.io/en/latest/user/advanced/#timeouts

I'm also wondering why we aren't using request.http.head(...) from

warehouse/warehouse/http.py

Lines 48 to 53 in 1fbb4ac

def includeme(config):
config.add_request_method(
ThreadLocalSessionFactory(config.registry.settings.get("http")),
name="http",
reify=True,
)
more - is it that the forms don't have the request object available to them?


import warehouse.utils.otp as otp
import warehouse.utils.webauthn as webauthn

Expand All @@ -25,6 +27,7 @@
TOTPValueMixin,
WebAuthnCredentialMixin,
)
from warehouse.constants import MIME_PYPI_SIMPLE_V1_ALL
from warehouse.i18n import localize as _
from warehouse.organizations.models import (
OrganizationRoleType,
Expand Down Expand Up @@ -713,6 +716,23 @@ class CreateTeamForm(SaveTeamForm):
__params__ = SaveTeamForm.__params__


def validate_is_simple(form, field):
try:
timeout = Timeout(connect=1.0, read=1.0)
http = PoolManager(timeout=timeout)
response = http.request(
"HEAD", field.data, headers={"Accept": ", ".join(MIME_PYPI_SIMPLE_V1_ALL)}
)
if response.headers.get("Content-Type") not in MIME_PYPI_SIMPLE_V1_ALL:
raise wtforms.validators.ValidationError(
_("Unable to parse simple index at given url")
)
except Exception as exc:
raise wtforms.validators.ValidationError(
_(f"Unable to parse simple index at given url: {exc}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_(f"Unable to parse simple index at given url: {exc}")
_(f"The URL provided does not respond as a Simple repository: {exc}")

And maybe even tack on a URL? https://packaging.python.org/specifications/simple-repository-api/

)


class AddAlternateRepositoryForm(forms.Form):
"""Form to add an Alternate Repository Location for a Project."""

Expand Down Expand Up @@ -744,6 +764,7 @@ class AddAlternateRepositoryForm(forms.Form):
),
),
forms.URIValidator(),
validate_is_simple,
]
)
description = wtforms.TextAreaField(
Expand Down
15 changes: 7 additions & 8 deletions warehouse/manage/views/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,7 +1129,7 @@
self.add_alternate_repository_form_class = AddAlternateRepositoryForm

@view_config(request_method="GET")
def manage_project_settings(self):
def manage_project_settings(self, add_alternate_repository_form=None):
Copy link
Member

Choose a reason for hiding this comment

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

question: is this because we're using two methods for GET vs POST, and how to make sure that the form values aren't reset on validation failure? I don't know if I've seen this pattern before, and wanted to understand it more.‏

if not self.request.organization_access:
# Disable transfer of project to any organization.
organization_choices = set()
Expand All @@ -1153,19 +1153,23 @@
active_organizations_owned | active_organizations_managed
) - current_organization

add_alt_repo_form = self.add_alternate_repository_form_class()
add_alt_repo_form = (
self.add_alternate_repository_form_class()
if add_alternate_repository_form is None
else add_alternate_repository_form
)

return {
"project": self.project,
"MAX_FILESIZE": MAX_FILESIZE,
"MAX_PROJECT_SIZE": MAX_PROJECT_SIZE,
"transfer_organization_project_form": (
self.transfer_organization_project_form_class(
organization_choices=organization_choices,
)
),
"add_alternate_repository_form_class": add_alt_repo_form,
}

Check warning

Code scanning / CodeQL

Reflected server-side cross-site scripting Medium

Cross-site scripting vulnerability due to a
user-provided value
Loading
.

@view_config(
request_method="POST",
Expand All @@ -1182,12 +1186,7 @@
self.request._("Invalid alternate repository location details"),
queue="error",
)
return HTTPSeeOther(
self.request.route_path(
"manage.project.settings",
project_name=self.project.name,
)
)
return self.manage_project_settings(add_alternate_repository_form=form)

Check warning

Code scanning / CodeQL

Reflected server-side cross-site scripting Medium

Cross-site scripting vulnerability due to a
user-provided value
Loading
.

Copilot Autofix AI about 2 months ago

To fix the reflected server-side cross-site scripting vulnerability, we need to ensure that any user-provided data is properly escaped before being rendered in the template. In this case, we can use the html.escape() function from Python's standard library to escape the form data before it is included in the dictionary returned by the manage_project_settings method.

Suggested changeset 1
warehouse/manage/views/__init__.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/warehouse/manage/views/__init__.py b/warehouse/manage/views/__init__.py
--- a/warehouse/manage/views/__init__.py
+++ b/warehouse/manage/views/__init__.py
@@ -1161,2 +1161,3 @@
 
+        import html
         return {
@@ -1170,3 +1171,3 @@
             ),
-            "add_alternate_repository_form_class": add_alt_repo_form,
+            "add_alternate_repository_form_class": html.escape(str(add_alt_repo_form)),
         }
EOF
@@ -1161,2 +1161,3 @@

import html
return {
@@ -1170,3 +1171,3 @@
),
"add_alternate_repository_form_class": add_alt_repo_form,
"add_alternate_repository_form_class": html.escape(str(add_alt_repo_form)),
}
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options

# add the alternate repository location entry
alt_repo = AlternateRepository(
Expand Down
Loading