-
Notifications
You must be signed in to change notification settings - Fork 5
Release 0.7.0 #264
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
Release 0.7.0 #264
Conversation
fix!: changed wording of the configuration env vars for hostname and port
Release 0.5.0-a2
…vice into release-0.5.0
This reverts commit 86ab0f3.
… recusrive flag for glob.glob to search in sub directories" This reverts commit 5926837.
This reverts commit 9c3cd75.
…ead of regex pattern
…vice into release-0.5.0
grader_service/auth/login.py
Outdated
# set new login cookie | ||
# because single-user cookie may have been cleared or incorrect | ||
self.set_login_cookie(user) | ||
self.redirect(self.get_next_url(user), permanent=False) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
user-provided value
Untrusted URL redirection depends on a
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to ensure that the next
URL parameter is properly validated to prevent open redirects. We can achieve this by maintaining a list of allowed redirect URLs and checking the user-provided URL against this list. If the URL is not in the list, we should redirect to a default safe URL.
- Define a list of allowed redirect URLs.
- Modify the
_validate_next_url
method to check if the user-provided URL is in the list of allowed URLs. - If the URL is not in the list, return a default safe URL.
-
Copy modified lines R671-R677 -
Copy modified lines R716-R720
@@ -670,2 +670,9 @@ | ||
""" | ||
# List of allowed redirect URLs | ||
allowed_urls = [ | ||
self.application.base_url, | ||
self.settings['login_url'], | ||
self.settings.get('home_url', '/') | ||
] | ||
|
||
# protect against some browsers' buggy handling of backslash as slash | ||
@@ -708,2 +715,7 @@ | ||
next_url = '' | ||
|
||
# Check if the next_url is in the list of allowed URLs | ||
if next_url not in allowed_urls: | ||
self.log.warning("Disallowing redirect to untrusted URL: %r", next_url) | ||
next_url = '' | ||
|
grader_service/auth/login.py
Outdated
# auto_login failed, just 403 | ||
raise web.HTTPError(403) | ||
else: | ||
self.redirect(self.get_next_url(user)) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
user-provided value
Untrusted URL redirection depends on a
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to ensure that the _validate_next_url
method in BaseHandler
is robust enough to prevent any untrusted URL redirection. We will enhance the validation logic to ensure that only relative URLs are allowed and that any potentially dangerous URLs are rejected.
- Modify the
_validate_next_url
method inBaseHandler
to include additional checks for URL safety. - Ensure that the
get_next_url
method inBaseHandler
uses the enhanced_validate_next_url
method to validate the URL before redirecting. - Update the
LoginHandler
class ingrader_service/auth/login.py
to use the validated URL for redirection.
-
Copy modified lines R117-R120 -
Copy modified lines R147-R150
@@ -116,3 +116,6 @@ | ||
else: | ||
self.redirect(self.get_next_url(user)) | ||
next_url = self.get_next_url(user) | ||
if not next_url: | ||
next_url = '/' | ||
self.redirect(next_url) | ||
else: | ||
@@ -143,3 +146,6 @@ | ||
self._grader_user = user | ||
self.redirect(self.get_next_url(user)) | ||
next_url = self.get_next_url(user) | ||
if not next_url: | ||
next_url = '/' | ||
self.redirect(next_url) | ||
else: |
-
Copy modified lines R710-R714
@@ -709,2 +709,7 @@ | ||
|
||
# Ensure the URL is relative | ||
if not next_url.startswith('/'): | ||
self.log.warning("Disallowing non-relative redirect: %r", next_url) | ||
next_url = '' | ||
|
||
return next_url |
grader_service/auth/login.py
Outdated
if user: | ||
# register current user for subsequent requests to user (e.g. logging the request) | ||
self._grader_user = user | ||
self.redirect(self.get_next_url(user)) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
user-provided value
Untrusted URL redirection depends on a
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to ensure that the next_url
is properly validated before being used in a redirect. We can enhance the _validate_next_url
method to be more robust and ensure that only safe URLs are allowed. Additionally, we should update the get_next_url
method to use this enhanced validation.
- Enhance the
_validate_next_url
method to include stricter checks for allowed hosts and schemes. - Update the
get_next_url
method to use the enhanced_validate_next_url
method. - Ensure that the
redirect
method inlogin.py
uses the validatednext_url
.
-
Copy modified lines R144-R147
@@ -143,3 +143,6 @@ | ||
self._grader_user = user | ||
self.redirect(self.get_next_url(user)) | ||
next_url = self.get_next_url(user) | ||
if not next_url: | ||
next_url = '/' | ||
self.redirect(next_url) | ||
else: |
-
Copy modified lines R710-R714
@@ -709,2 +709,7 @@ | ||
|
||
# Ensure the URL is relative and does not contain an explicit host | ||
if parsed_next_url.netloc or parsed_next_url.scheme: | ||
self.log.warning("Disallowing redirect with explicit host or scheme: %r", next_url) | ||
next_url = '' | ||
|
||
return next_url |
async def redirect_to_next_url(self, user): | ||
"""Redirect user agent to next url that has been received in the login initiation request.""" | ||
next_url = self.get_next_url(user) | ||
self.redirect(next_url) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
user-provided value
Untrusted URL redirection depends on a
user-provided value
Untrusted URL redirection depends on a
user-provided value
Untrusted URL redirection depends on a
user-provided value
Untrusted URL redirection depends on a
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to ensure that the next_url
is validated before it is used in the redirect
function. We can achieve this by using the _validate_next_url
method from the BaseHandler
class to validate the next_url
before redirecting.
- Modify the
redirect_to_next_url
method ingrader_service/auth/lti13/handlers.py
to validate thenext_url
using the_validate_next_url
method. - Ensure that the validated
next_url
is used in theredirect
function.
-
Copy modified line R400
@@ -399,2 +399,3 @@ | ||
next_url = self.get_next_url(user) | ||
next_url = self._validate_next_url(next_url) | ||
self.redirect(next_url) |
grader_service/auth/oauth2.py
Outdated
if user is None: | ||
raise web.HTTPError(403, reason=self.authenticator.custom_403_message) | ||
|
||
self.redirect(self.get_next_url(user)) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
user-provided value
Untrusted URL redirection depends on a
user-provided value
Untrusted URL redirection depends on a
user-provided value
Untrusted URL redirection depends on a
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to enhance the validation of the next_url
to ensure it is safe for redirection. We can achieve this by using the _validate_next_url
method from BaseHandler
in the OAuthCallbackHandler
class to validate the next_url
before using it for redirection. This will ensure that the URL is within the same host and protocol, mitigating the risk of URL redirection attacks.
- Modify the
get_next_url
method inOAuthCallbackHandler
to use_validate_next_url
for validating thenext_url
. - Ensure that the
next_url
is validated before it is used in theredirect
method.
-
Copy modified lines R195-R197
@@ -194,3 +194,5 @@ | ||
if next_url: | ||
return next_url | ||
next_url = self._validate_next_url(next_url) | ||
if next_url: | ||
return next_url | ||
# JupyterHub 0.8 adds default .get_next_url for a fallback |
self.gitbase, | ||
lecture.code, | ||
str(assignment.id), | ||
assignment.settings.assignment_type, |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to ensure that the constructed file path is contained within a safe root folder. We can achieve this by normalizing the path using os.path.normpath
and then checking that the normalized path starts with the expected base directory. This will prevent directory traversal attacks.
- Normalize the
git_repo_path
andsubmission_repo_path
usingos.path.normpath
. - Verify that the normalized paths start with the base directory (
self.gitbase
).
-
Copy modified line R644 -
Copy modified line R650 -
Copy modified line R653 -
Copy modified lines R659-R662
@@ -643,3 +643,3 @@ | ||
# Path to repository which will store edited submission files | ||
git_repo_path = os.path.join( | ||
git_repo_path = os.path.normpath(os.path.join( | ||
self.gitbase, | ||
@@ -649,6 +649,6 @@ | ||
str(submission_id), | ||
) | ||
)) | ||
|
||
# Path to repository of student which contains the submitted files | ||
submission_repo_path = os.path.join( | ||
submission_repo_path = os.path.normpath(os.path.join( | ||
self.gitbase, | ||
@@ -658,3 +658,6 @@ | ||
submission.username | ||
) | ||
)) | ||
|
||
if not git_repo_path.startswith(self.gitbase) or not submission_repo_path.startswith(self.gitbase): | ||
raise Exception("Invalid path") | ||
|
lecture.code, | ||
str(assignment.id), | ||
assignment.settings.assignment_type, | ||
submission.username |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to ensure that the constructed file paths are safe and do not allow path traversal attacks. We can achieve this by normalizing the paths and ensuring they are contained within a safe root directory. Specifically, we will:
- Normalize the
git_repo_path
andsubmission_repo_path
usingos.path.normpath
. - Ensure that the normalized paths start with the expected base directory (
self.gitbase
).
-
Copy modified line R644 -
Copy modified lines R650-R652 -
Copy modified line R655 -
Copy modified lines R661-R663
@@ -643,3 +643,3 @@ | ||
# Path to repository which will store edited submission files | ||
git_repo_path = os.path.join( | ||
git_repo_path = os.path.normpath(os.path.join( | ||
self.gitbase, | ||
@@ -649,6 +649,8 @@ | ||
str(submission_id), | ||
) | ||
)) | ||
if not git_repo_path.startswith(self.gitbase): | ||
raise HTTPError(HTTPStatus.BAD_REQUEST, reason="Invalid path") | ||
|
||
# Path to repository of student which contains the submitted files | ||
submission_repo_path = os.path.join( | ||
submission_repo_path = os.path.normpath(os.path.join( | ||
self.gitbase, | ||
@@ -658,3 +660,5 @@ | ||
submission.username | ||
) | ||
)) | ||
if not submission_repo_path.startswith(self.gitbase): | ||
raise HTTPError(HTTPStatus.BAD_REQUEST, reason="Invalid path") | ||
|
submission.username | ||
) | ||
|
||
if os.path.exists(git_repo_path): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to ensure that the constructed file paths are safe and do not allow directory traversal attacks. We can achieve this by normalizing the paths and ensuring they are contained within a designated root directory. Specifically, we will:
- Normalize the constructed paths using
os.path.normpath
. - Verify that the normalized paths start with the expected base directory.
-
Copy modified line R644 -
Copy modified lines R650-R652 -
Copy modified line R655 -
Copy modified lines R661-R663
@@ -643,3 +643,3 @@ | ||
# Path to repository which will store edited submission files | ||
git_repo_path = os.path.join( | ||
git_repo_path = os.path.normpath(os.path.join( | ||
self.gitbase, | ||
@@ -649,6 +649,8 @@ | ||
str(submission_id), | ||
) | ||
)) | ||
if not git_repo_path.startswith(self.gitbase): | ||
raise HTTPError(HTTPStatus.BAD_REQUEST, reason="Invalid submission path") | ||
|
||
# Path to repository of student which contains the submitted files | ||
submission_repo_path = os.path.join( | ||
submission_repo_path = os.path.normpath(os.path.join( | ||
self.gitbase, | ||
@@ -658,3 +660,5 @@ | ||
submission.username | ||
) | ||
)) | ||
if not submission_repo_path.startswith(self.gitbase): | ||
raise HTTPError(HTTPStatus.BAD_REQUEST, reason="Invalid submission path") | ||
|
) | ||
|
||
if os.path.exists(git_repo_path): | ||
shutil.rmtree(git_repo_path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we need to ensure that the constructed file paths are safe and do not allow directory traversal attacks. This can be achieved by normalizing the paths and ensuring they are within a designated safe directory.
- Normalize the constructed paths using
os.path.normpath
. - Verify that the normalized paths start with the expected base directory.
We will apply these changes to the construction of git_repo_path
, submission_repo_path
, and tmp_path
.
-
Copy modified line R644 -
Copy modified lines R650-R652 -
Copy modified line R655 -
Copy modified lines R661-R663 -
Copy modified line R676 -
Copy modified lines R683-R685
@@ -643,3 +643,3 @@ | ||
# Path to repository which will store edited submission files | ||
git_repo_path = os.path.join( | ||
git_repo_path = os.path.normpath(os.path.join( | ||
self.gitbase, | ||
@@ -649,6 +649,8 @@ | ||
str(submission_id), | ||
) | ||
)) | ||
if not git_repo_path.startswith(self.gitbase): | ||
raise HTTPError(HTTPStatus.BAD_REQUEST, "Invalid submission ID") | ||
|
||
# Path to repository of student which contains the submitted files | ||
submission_repo_path = os.path.join( | ||
submission_repo_path = os.path.normpath(os.path.join( | ||
self.gitbase, | ||
@@ -658,3 +660,5 @@ | ||
submission.username | ||
) | ||
)) | ||
if not submission_repo_path.startswith(self.gitbase): | ||
raise HTTPError(HTTPStatus.BAD_REQUEST, "Invalid submission path") | ||
|
||
@@ -671,3 +675,3 @@ | ||
# files in the edit repository | ||
tmp_path = os.path.join( | ||
tmp_path = os.path.normpath(os.path.join( | ||
self.application.grader_service_dir, | ||
@@ -678,3 +682,5 @@ | ||
str(submission.id), | ||
) | ||
)) | ||
if not tmp_path.startswith(self.application.grader_service_dir): | ||
raise HTTPError(HTTPStatus.BAD_REQUEST, "Invalid temporary path") | ||
|
grader_service/utils.py
Outdated
def random_port(): | ||
"""Get a single random port.""" | ||
sock = socket.socket() | ||
sock.bind(('', 0)) |
Check warning
Code scanning / CodeQL
Binding a socket to all network interfaces Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we should bind the socket to the loopback interface (127.0.0.1
) instead of all interfaces. This change will limit the socket's exposure to the local machine only, reducing the security risks associated with binding to all interfaces.
- Update the
random_port
function in thegrader_service/utils.py
file. - Change the binding address from
('', 0)
to('127.0.0.1', 0)
.
-
Copy modified line R56
@@ -55,3 +55,3 @@ | ||
sock = socket.socket() | ||
sock.bind(('', 0)) | ||
sock.bind(('127.0.0.1', 0)) | ||
port = sock.getsockname()[1] |
…ars in filenames (#266)
No description provided.