From 93c4cda5b24a15b2d7eb13a856f2707713bb2271 Mon Sep 17 00:00:00 2001 From: Carlos Date: Sun, 28 Dec 2025 22:39:02 +0100 Subject: [PATCH 1/4] fix: Add null check in queue_test to prevent crash when test record missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When queue_test() is called but no matching Test record exists in the database, platform_test.id would throw AttributeError: 'NoneType' object has no attribute 'id'. This can happen when: - The pull_request webhook was not received or failed - There's a mismatch in query parameters (commit, platform, test_type, pr_nr) - A race condition between webhook processing The fix adds a null check with a descriptive error log before attempting to access platform_test.id. This prevents the 500 error and provides debugging information. Root cause identified from webhook delivery showing 500 error at 18:26:14 on 2025-12-28 when Windows build completed for PR #1920. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- mod_ci/controllers.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/mod_ci/controllers.py b/mod_ci/controllers.py index 8b5fe5e4..a6c0e009 100755 --- a/mod_ci/controllers.py +++ b/mod_ci/controllers.py @@ -1302,6 +1302,13 @@ def queue_test(gh_commit: Commit.Commit, commit, test_type, platform, branch="ma Test.branch == branch, Test.pr_nr == pr_nr )).first() + + if platform_test is None: + log.error(f"No test record found for commit {commit[:8]}, platform {platform.value}, " + f"test_type {test_type}, pr_nr {pr_nr}. " + "This may indicate the pull_request webhook was not processed.") + return + add_customized_regression_tests(platform_test.id) if gh_commit is not None: From ea24b89b445d6cd11890a1afff140c40d528d7dd Mon Sep 17 00:00:00 2001 From: Carlos Date: Sun, 28 Dec 2025 23:33:34 +0100 Subject: [PATCH 2/4] fix: Address multiple server errors found in production logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix 'CommitStatus' object not subscriptable error in start_ci webhook handler by using attribute access (status.context) instead of dict syntax (status["context"]) for PyGithub CommitStatus objects - Fix FileNotFoundError in progress_reporter by ensuring TempFiles and TestResults directories exist before saving/moving uploaded files - Fix BuildError for login redirects by storing full URL path instead of just endpoint name, preventing crashes when endpoints require URL parameters (e.g., sample_id, test_id) - Add open redirect protection to login handler - Update tests to match new login redirect behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- mod_auth/controllers.py | 14 +++++++++++--- mod_ci/controllers.py | 14 ++++++++------ tests/test_regression/test_controllers.py | 8 ++++---- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/mod_auth/controllers.py b/mod_auth/controllers.py index 40fd0310..0824acb0 100755 --- a/mod_auth/controllers.py +++ b/mod_auth/controllers.py @@ -59,7 +59,8 @@ def login_required(f: Callable) -> Callable: def decorated_function(*args, **kwargs): if g.user is None: g.log.warning(f'login protected endpoint {request.endpoint} accessed before logging in') - return redirect(url_for('auth.login', next=request.endpoint)) + # Store the full URL path instead of just the endpoint name to preserve URL parameters + return redirect(url_for('auth.login', next=request.full_path.rstrip('?'))) return f(*args, **kwargs) @@ -225,11 +226,17 @@ def login() -> Union[Response, Dict[str, Union[str, LoginForm]]]: """Route for handling the login page.""" redirect_location = request.args.get('next', '') + # Validate redirect location to prevent open redirects + # Only allow paths starting with / (relative to this site) + if redirect_location and (not redirect_location.startswith('/') or redirect_location.startswith('//')): + redirect_location = '' + if session.get('user_id', None) is not None: flash('You are already logged in!', 'alert') if len(redirect_location) == 0: return redirect("/") - return redirect(url_for(redirect_location)) + # Redirect directly to the stored path (now stores full URL path, not endpoint name) + return redirect(redirect_location) form = LoginForm(request.form) if form.validate_on_submit(): @@ -238,7 +245,8 @@ def login() -> Union[Response, Dict[str, Union[str, LoginForm]]]: session['user_id'] = user_to_login.id if len(redirect_location) == 0: return redirect("/") - return redirect(url_for(redirect_location)) + # Redirect directly to the stored path (now stores full URL path, not endpoint name) + return redirect(redirect_location) flash('Wrong username or password', 'error-message') diff --git a/mod_ci/controllers.py b/mod_ci/controllers.py index a6c0e009..051c68ae 100755 --- a/mod_ci/controllers.py +++ b/mod_ci/controllers.py @@ -1495,10 +1495,10 @@ def start_ci(): gh_commit = repository.get_commit(test.commit) # If test run status exists, mark them as cancelled for status in gh_commit.get_statuses(): - if status["context"] == f"CI - {test.platform.value}": + if status.context == f"CI - {test.platform.value}": target_url = url_for('test.by_id', test_id=test.id, _external=True) update_status_on_github(gh_commit, Status.FAILURE, "Tests canceled", - status["context"], target_url=target_url) + status.context, target_url=target_url) elif event == "issues": g.log.debug('issues event detected') @@ -2132,7 +2132,9 @@ def upload_type_request(log, test_id, repo_folder, test, request) -> bool: if filename == '': log.warning('empty filename provided for uploading') return False - temp_path = os.path.join(repo_folder, 'TempFiles', filename) + temp_dir = os.path.join(repo_folder, 'TempFiles') + os.makedirs(temp_dir, exist_ok=True) + temp_path = os.path.join(temp_dir, filename) # Save to temporary location uploaded_file.save(temp_path) # Get hash and check if it's already been submitted @@ -2142,9 +2144,9 @@ def upload_type_request(log, test_id, repo_folder, test, request) -> bool: hash_sha256.update(chunk) file_hash = hash_sha256.hexdigest() filename, file_extension = os.path.splitext(filename) - final_path = os.path.join( - repo_folder, 'TestResults', f'{file_hash}{file_extension}' - ) + results_dir = os.path.join(repo_folder, 'TestResults') + os.makedirs(results_dir, exist_ok=True) + final_path = os.path.join(results_dir, f'{file_hash}{file_extension}') os.rename(temp_path, final_path) rto = RegressionTestOutput.query.filter( RegressionTestOutput.id == request.form['test_file_id']).first() diff --git a/tests/test_regression/test_controllers.py b/tests/test_regression/test_controllers.py index 83eb4039..0e27bed9 100644 --- a/tests/test_regression/test_controllers.py +++ b/tests/test_regression/test_controllers.py @@ -104,7 +104,7 @@ def test_regression_test_deletion_Without_login(self): """Check that it will move to the login page.""" response = self.app.test_client().get('/regression/test/9432/delete') self.assertEqual(response.status_code, 302) - self.assertIn(b'/account/login?next=regression.test_delete', response.data) + self.assertIn(b'/account/login?next=/regression/test/9432/delete', response.data) def test_delete_if_will_throw_404(self): """Check if it will throw an error 404.""" @@ -249,7 +249,7 @@ def test_category_deletion_without_login(self): """Check if it will move to the login page.""" response = self.app.test_client().get('/regression/category/9432/delete') self.assertEqual(response.status_code, 302) - self.assertIn(b'/account/login?next=regression.category_delete', response.data) + self.assertIn(b'/account/login?next=/regression/category/9432/delete', response.data) def test_category_delete_if_will_throw_404(self): """Check if it will throw an error 404.""" @@ -441,7 +441,7 @@ def test_add_output_without_login(self): """Check if during output addition without login it will move to the login page.""" response = self.app.test_client().get('/regression/test/69420/output/new') self.assertEqual(response.status_code, 302) - self.assertIn(b'/account/login?next=regression.output_add', response.data) + self.assertIn(b'/account/login?next=/regression/test/69420/output/new', response.data) def test_remove_output(self): """Check if, it will remove an output.""" @@ -484,7 +484,7 @@ def test_remove_output_without_login(self): """Check it removes output without login.""" response = self.app.test_client().get('/regression/test/69420/output/remove') self.assertEqual(response.status_code, 302) - self.assertIn(b'/account/login?next=regression.output_remove', response.data) + self.assertIn(b'/account/login?next=/regression/test/69420/output/remove', response.data) def test_add_output_empty_got(self): """Check if, it will add an output with empty got.""" From 1487c00d062be7a10601f648ee47369f35631c62 Mon Sep 17 00:00:00 2001 From: Carlos Date: Sun, 28 Dec 2025 23:45:46 +0100 Subject: [PATCH 3/4] fix: Handle login redirect errors gracefully instead of storing full path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address maintainer feedback: - Revert to using endpoint name in login_required decorator - Add try/except around url_for() to catch BuildError - Redirect to home page when endpoint requires parameters we don't have - Revert tests to original assertions This is a safer approach than storing the full URL path, which could introduce security risks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- mod_auth/controllers.py | 23 ++++++++++++----------- tests/test_regression/test_controllers.py | 8 ++++---- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/mod_auth/controllers.py b/mod_auth/controllers.py index 0824acb0..a476b9af 100755 --- a/mod_auth/controllers.py +++ b/mod_auth/controllers.py @@ -11,6 +11,7 @@ from flask import (Blueprint, abort, flash, g, redirect, request, session, url_for) from pyisemail import is_email +from werkzeug.routing import BuildError from werkzeug.wrappers.response import Response from database import EnumSymbol @@ -59,8 +60,7 @@ def login_required(f: Callable) -> Callable: def decorated_function(*args, **kwargs): if g.user is None: g.log.warning(f'login protected endpoint {request.endpoint} accessed before logging in') - # Store the full URL path instead of just the endpoint name to preserve URL parameters - return redirect(url_for('auth.login', next=request.full_path.rstrip('?'))) + return redirect(url_for('auth.login', next=request.endpoint)) return f(*args, **kwargs) @@ -226,17 +226,15 @@ def login() -> Union[Response, Dict[str, Union[str, LoginForm]]]: """Route for handling the login page.""" redirect_location = request.args.get('next', '') - # Validate redirect location to prevent open redirects - # Only allow paths starting with / (relative to this site) - if redirect_location and (not redirect_location.startswith('/') or redirect_location.startswith('//')): - redirect_location = '' - if session.get('user_id', None) is not None: flash('You are already logged in!', 'alert') if len(redirect_location) == 0: return redirect("/") - # Redirect directly to the stored path (now stores full URL path, not endpoint name) - return redirect(redirect_location) + try: + return redirect(url_for(redirect_location)) + except BuildError: + # Endpoint requires parameters we don't have, redirect to home + return redirect("/") form = LoginForm(request.form) if form.validate_on_submit(): @@ -245,8 +243,11 @@ def login() -> Union[Response, Dict[str, Union[str, LoginForm]]]: session['user_id'] = user_to_login.id if len(redirect_location) == 0: return redirect("/") - # Redirect directly to the stored path (now stores full URL path, not endpoint name) - return redirect(redirect_location) + try: + return redirect(url_for(redirect_location)) + except BuildError: + # Endpoint requires parameters we don't have, redirect to home + return redirect("/") flash('Wrong username or password', 'error-message') diff --git a/tests/test_regression/test_controllers.py b/tests/test_regression/test_controllers.py index 0e27bed9..83eb4039 100644 --- a/tests/test_regression/test_controllers.py +++ b/tests/test_regression/test_controllers.py @@ -104,7 +104,7 @@ def test_regression_test_deletion_Without_login(self): """Check that it will move to the login page.""" response = self.app.test_client().get('/regression/test/9432/delete') self.assertEqual(response.status_code, 302) - self.assertIn(b'/account/login?next=/regression/test/9432/delete', response.data) + self.assertIn(b'/account/login?next=regression.test_delete', response.data) def test_delete_if_will_throw_404(self): """Check if it will throw an error 404.""" @@ -249,7 +249,7 @@ def test_category_deletion_without_login(self): """Check if it will move to the login page.""" response = self.app.test_client().get('/regression/category/9432/delete') self.assertEqual(response.status_code, 302) - self.assertIn(b'/account/login?next=/regression/category/9432/delete', response.data) + self.assertIn(b'/account/login?next=regression.category_delete', response.data) def test_category_delete_if_will_throw_404(self): """Check if it will throw an error 404.""" @@ -441,7 +441,7 @@ def test_add_output_without_login(self): """Check if during output addition without login it will move to the login page.""" response = self.app.test_client().get('/regression/test/69420/output/new') self.assertEqual(response.status_code, 302) - self.assertIn(b'/account/login?next=/regression/test/69420/output/new', response.data) + self.assertIn(b'/account/login?next=regression.output_add', response.data) def test_remove_output(self): """Check if, it will remove an output.""" @@ -484,7 +484,7 @@ def test_remove_output_without_login(self): """Check it removes output without login.""" response = self.app.test_client().get('/regression/test/69420/output/remove') self.assertEqual(response.status_code, 302) - self.assertIn(b'/account/login?next=/regression/test/69420/output/remove', response.data) + self.assertIn(b'/account/login?next=regression.output_remove', response.data) def test_add_output_empty_got(self): """Check if, it will add an output with empty got.""" From 7072db5f079d1150a4de24ab31e32fafb3a742be Mon Sep 17 00:00:00 2001 From: Carlos Date: Sun, 28 Dec 2025 23:58:28 +0100 Subject: [PATCH 4/4] test: Update test_upload_type_request for directory creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update test assertions to account for additional os.path.join and os.makedirs calls added to ensure TempFiles and TestResults directories exist before file operations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- tests/test_ci/test_controllers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_ci/test_controllers.py b/tests/test_ci/test_controllers.py index f5cdafe1..63a38332 100644 --- a/tests/test_ci/test_controllers.py +++ b/tests/test_ci/test_controllers.py @@ -1901,7 +1901,10 @@ def test_upload_type_request(self, mock_filename, mock_os, mock_open, mock_iter, mock_log.debug.assert_called_once() mock_filename.assert_called_once() - self.assertEqual(2, mock_os.path.join.call_count) + # 4 calls: temp_dir, temp_path, results_dir, final_path + self.assertEqual(4, mock_os.path.join.call_count) + # 2 calls: makedirs for TempFiles and TestResults directories + self.assertEqual(2, mock_os.makedirs.call_count) mock_upload_file.save.assert_called_once() mock_open.assert_called_once_with(mock.ANY, "rb") mock_os.path.splitext.assert_called_once_with(mock.ANY)