-
Notifications
You must be signed in to change notification settings - Fork 25
Registration #487
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
base: master
Are you sure you want to change the base?
Registration #487
Conversation
| return JsonResponse(response.json(), safe=False) | ||
| return JsonResponse({"error": "Failed to fetch schools"}, status=response.status_code) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
In general, to fix this kind of problem you should avoid sending raw exception messages or stack traces to clients. Instead, log detailed error information on the server (to Django’s logger or another logging system) and return a generic, user‑safe message, along with an appropriate HTTP status code.
For this specific file, the best fix with minimal functional change is:
- In each proxy view (
proxy_schools_active,proxy_schools_all,proxy_debaters), change theexcept RequestException as e:handler to:- Optionally log the exception using Python’s
loggingmodule (which we can safely import at the top of the file). - Return a generic JSON error message, such as
{"error": "Failed to fetch schools"}/{"error": "Failed to fetch debaters"}with status 500, instead ofstr(e).
- Optionally log the exception using Python’s
Concretely:
- Add
import loggingnear the existing imports (inside this file). - Define a module-level logger, e.g.,
logger = logging.getLogger(__name__). - In each
except RequestException as e:block, replaceJsonResponse({"error": str(e)}, status=500)with something like:- For
proxy_schools_active: log the exception (logger.exception("Error fetching active schools from %s", SCHOOL_ACTIVE_URL)) and returnJsonResponse({"error": "Failed to fetch schools"}, status=500). - For
proxy_schools_all: similarly log and return the same generic message. - For
proxy_debaters: log withschool_idcontext and returnJsonResponse({"error": "Failed to fetch debaters"}, status=500).
- For
This preserves the external behavior (clients still get an error JSON and 500) while no longer exposing the raw exception details.
-
Copy modified line R15 -
Copy modified lines R32-R33 -
Copy modified lines R689-R692 -
Copy modified lines R706-R709 -
Copy modified lines R724-R729
| @@ -12,6 +12,7 @@ | ||
| from django.utils.functional import cached_property | ||
| import requests | ||
| from requests.exceptions import RequestException | ||
| import logging | ||
|
|
||
| from mittab.apps.registration.forms import ( | ||
| JudgeForm, | ||
| @@ -28,6 +29,8 @@ | ||
| from mittab.libs.cacheing.public_cache import invalidate_all_public_caches | ||
| from .models import Registration, RegistrationConfig | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| SCHOOL_ACTIVE_URL = "https://results.apda.online/api/schools/all/" | ||
| SCHOOL_ALL_URL = SCHOOL_ACTIVE_URL | ||
| MAX_TEAMS = 200 | ||
| @@ -683,7 +686,10 @@ | ||
| {"error": "Failed to fetch schools"}, status=response.status_code | ||
| ) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) | ||
| logger.exception("Error fetching active schools from %s", SCHOOL_ACTIVE_URL) | ||
| return JsonResponse( | ||
| {"error": "Failed to fetch schools"}, status=500 | ||
| ) | ||
|
|
||
|
|
||
| @require_http_methods(["GET"]) | ||
| @@ -697,7 +703,10 @@ | ||
| {"error": "Failed to fetch schools"}, status=response.status_code | ||
| ) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) | ||
| logger.exception("Error fetching all schools from %s", SCHOOL_ALL_URL) | ||
| return JsonResponse( | ||
| {"error": "Failed to fetch schools"}, status=500 | ||
| ) | ||
|
|
||
|
|
||
| @require_http_methods(["GET"]) | ||
| @@ -712,4 +721,9 @@ | ||
| {"error": "Failed to fetch debaters"}, status=response.status_code | ||
| ) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) | ||
| logger.exception( | ||
| "Error fetching debaters for school %s from %s", school_id, url | ||
| ) | ||
| return JsonResponse( | ||
| {"error": "Failed to fetch debaters"}, status=500 | ||
| ) |
| return JsonResponse(response.json(), safe=False) | ||
| return JsonResponse({"error": "Failed to fetch schools"}, status=response.status_code) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
In general, the fix is to avoid sending raw exception messages or stack traces back to the client. Instead, log the full exception details on the server (so developers can debug) and return a generic, non-sensitive error message to the user. For these Django views, that means replacing JsonResponse({"error": str(e)}, status=500) with a response such as JsonResponse({"error": "Failed to fetch schools"}, status=500) (or a similarly generic message) and optionally adding server-side logging of the exception.
Concretely, in mittab/apps/registration/views.py, within the proxy_schools_all view at lines 689–700, we should change the except RequestException as e: block to stop using str(e) in the JSON body. To preserve debuggability without changing existing behavior otherwise, we can import Python’s standard logging module at the top of the file and log the exception in the except block, then return a generic message. We will mirror the already-existing generic message "Failed to fetch schools" used for non-OK HTTP responses for consistency. No other functionality or external interfaces need to change.
-
Copy modified line R13 -
Copy modified lines R17-R18 -
Copy modified lines R699-R701
| @@ -10,9 +10,12 @@ | ||
| from django.urls import reverse | ||
| from django.views.decorators.http import require_http_methods | ||
| from django.utils.functional import cached_property | ||
| import logging | ||
| import requests | ||
| from requests.exceptions import RequestException | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| from mittab.apps.registration.forms import ( | ||
| JudgeForm, | ||
| NEW_CHOICE_VALUE, | ||
| @@ -696,8 +696,9 @@ | ||
| return JsonResponse( | ||
| {"error": "Failed to fetch schools"}, status=response.status_code | ||
| ) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) | ||
| except RequestException: | ||
| logger.exception("Error while fetching all schools from external API") | ||
| return JsonResponse({"error": "Failed to fetch schools"}, status=500) | ||
|
|
||
|
|
||
| @require_http_methods(["GET"]) |
| return JsonResponse(response.json(), safe=False) | ||
| return JsonResponse({"error": "Failed to fetch debaters"}, status=response.status_code) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
In general, to fix information exposure through exceptions you should avoid returning raw exception objects or their messages to the client. Instead, log the detailed error on the server (for debugging and auditing) and return a generic, non-sensitive error message to the user.
For this specific case, we should modify the except RequestException as e: blocks in the proxy endpoints (proxy_schools_active, proxy_schools_all, and proxy_debaters) so they no longer return str(e) to the client. Instead, they should return a generic message like "Failed to fetch schools" or "Failed to fetch debaters". To preserve debuggability, we can log the exception using Python’s standard logging module.
Concretely:
- Add
import loggingnear the top ofmittab/apps/registration/views.py(without altering existing imports). - In each of the three proxy view functions, update the
except RequestException as e:block:- Call
logging.exception("...")with a message tailored to the endpoint, which will log the full stack trace and exception details on the server. - Replace
JsonResponse({"error": str(e)}, status=500)with aJsonResponsecontaining a generic, static error message and HTTP 500 status.
This preserves the existing functionality (still returns an error with appropriate HTTP status) while removing sensitive information from the user-facing response.
- Call
-
Copy modified line R15 -
Copy modified lines R687-R690 -
Copy modified lines R704-R707 -
Copy modified lines R722-R725
| @@ -12,6 +12,7 @@ | ||
| from django.utils.functional import cached_property | ||
| import requests | ||
| from requests.exceptions import RequestException | ||
| import logging | ||
|
|
||
| from mittab.apps.registration.forms import ( | ||
| JudgeForm, | ||
| @@ -683,7 +684,10 @@ | ||
| {"error": "Failed to fetch schools"}, status=response.status_code | ||
| ) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) | ||
| logging.exception("Error fetching active schools from %s", SCHOOL_ACTIVE_URL) | ||
| return JsonResponse( | ||
| {"error": "Failed to fetch schools"}, status=500 | ||
| ) | ||
|
|
||
|
|
||
| @require_http_methods(["GET"]) | ||
| @@ -697,7 +701,10 @@ | ||
| {"error": "Failed to fetch schools"}, status=response.status_code | ||
| ) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) | ||
| logging.exception("Error fetching all schools from %s", SCHOOL_ALL_URL) | ||
| return JsonResponse( | ||
| {"error": "Failed to fetch schools"}, status=500 | ||
| ) | ||
|
|
||
|
|
||
| @require_http_methods(["GET"]) | ||
| @@ -712,4 +719,7 @@ | ||
| {"error": "Failed to fetch debaters"}, status=response.status_code | ||
| ) | ||
| except RequestException as e: | ||
| return JsonResponse({"error": str(e)}, status=500) | ||
| logging.exception("Error fetching debaters for school %s", school_id) | ||
| return JsonResponse( | ||
| {"error": "Failed to fetch debaters"}, status=500 | ||
| ) |
We want to move registration for tournaments onto mittab. This will solve a litany of the biggest issues tournament directors face, including:
It will also clear up major issues for people registering for tournaments
This will be a much larger and more complex feature push than we've done recently. It will need to involve changes in the deployer because tournaments will have to be deployed for longer, meaning more concurrent deployments running at once. We will also eventually need SMTP to serve the full scope of desired features. The code in this PR is a rough draft, we want this done by january so there will be a lot of iteration done before then, and I expect to be able to shorten the PR significantly