Conversation
* Add a ton of todos - Spent some time rethinking main goal for next major version release. * Upgrade requests to latest v2.32.5 (thanks Dependabot!) * Add some docs on how to run test for project.
* The single mega `forms.py` file was getting too bloated with classes. - Instead, I've now grouped classes into route, auth, or helper classes.
* Basically, current way I'm doing configs all in `utils.py` file has to go.
* I wanted an interface like the default builtin flask app.config. But my
config info kept in the main.conf is dynamic and changes when the underlying
file changes.
- So built my own custom ConfigManager class and overwrite its getters to
reload info from config file when called, and set the default values for
the app in the event some trouble loading main.conf.
* Tests are failing because I haven't finished changing over everything to use
new ConfigManager/config object yet.
* Went through and updated everything to use new ConfigManager class and
trimmed out old dead code.
* Also added batch update mechanism to ConfigManager so can use context manager
to open, change a bunch of stuff, then write all at once.
* Also added random chance that tests run in reverse.
- Not worth getting into why, but found a bug with test order and this will
help me catch and squash similar ones like that in the future.
* Just some light restructuring. - All tests passing again so making a commit.
* Put the extensions code in its own `extensions.py` file.
* As part of the continuing restructuring efforts, moving this responsibility into its own class and getting stuff out of the utils.py file and out of the main app/ dir.
* It should've always been named "control" as these are items on the controls page.
- Renamed:
- Cmd class -> Control
- CommandService -> ControlService
- get_commands -> get_controls
- commands_list -> controls_list
- etc.
* I keep typing bin so pretty good indication that should be renamed.
* The old `views.py` file was over a thousand lines. - Instead now each route gets its own file under the blueprints dir. - With same blueprint categories as before (auth, api, main). - Now structure is more scalable.
* Separate auth functions/routes into their own files for modularity and scalability.
* As part of the continued restructuring efforts, I've moved the api code into individual blueprint files, just like with views and auth code. * I've also done the same with models classes, breaking up each into its own file and making a models pkg dir.
…haring data
* Previously I was using some module level singletons to hold share list of
blocked IPs and process info objects.
- Instead, now I'm using proper class level singletons to share betweens
app's components.
- Same old code as always, but now wrapped up in service class for ease of
use.
* By doing things this way can move logic for checking if user has access to
some route out of the utils files and use the `current_user` interface that
flask provides.
* Simplified user permissions json.
- Now just list of route names at top level.
- If route name is there, user has access.
- Probably more tweaking to do here cause should just be its own "routes"
list but whatever will get there...
* Also moved monitoring service tests into their own file.
* New user permission structure:
```json
{
"routes": [
"add",
"controls",
"install",
"settings",
"edit",
"jobs",
"delete"
"update-console",
"cmd-output",
"server-statuses"
],
"controls": [
"start",
"stop",
"restart",
"monitor",
"test-alert",
"details",
"postdetails",
"update-lgsm",
"update",
"backup",
"console",
"send"
],
"server_ids": [
"7b2dddb1-58d0-4c76-b81a-c23c820264a8",
"5a0b6f33-4682-493f-bf98-8acf1c9dcd4e"
]
}
```
* When I first built the ssh to localhost feature for admining new game servers it seems like a good way to kill two birds with one stone. - And it's held up well, however its become overloaded and now a limit on growth. * Instead, I'm going to replace ssh for local (same host) servers with new plan to run scripts via alt users through sudo and pass data in and out via Unix domain sockets for ipc / io.
…ce + reorganize * We're going to use dependency inversion with our new CommandExecService class and pass it either a local executor or a remote executor (not implemented yet). * Also put service classes with mulitple files into their own app/services/ sub dirs just to keep things organized, prevent clutter.
* Now CommandExecService is smart and can handle running stuff over ssh and running stuff locally and `run_command` is not just handed the executor it needs to get the job done!
* Replaced calls to old `run_cmd_popen` and `run_cmd_ssh` functions with updated calls to new CommandExecService.
* Not everything is a service, although I'll admit I'm still figuring out what a service vs manager really is.
* I don't like that I ended up with a bunch of service classes that weren't
real service classes.
- So I renamed a bunch of stuff to help me figure out where is a better
forever home for it someday and moved some things out of the services dir
and into the managers dir.
- Will figure out where other stuff belongs and continue with re-arch efforts
in next release.
* My integration tests are passing locally but failling on github. So added some debug to info to see what that's about.
* Still debugging integration tests.
- They pass fine on my machine so something about these github runners is
different.
* Human generated code is liable to contain bugs...
* Tests are all passing now, but genbadge creation is failing. - I think\* this will fix the missing `pkg_resources`
* Still trying to get genbadge to work.
* I wonder if I just need to update the pip version of genbadge. - 🤞
* I've decided this isn't v1.8.7, this is v1.9.0.
- Why? Well there have been significant new architectural changes and some
minor breaks in backward compat (see faq 13).
- And there's only more arch shifting to come.
- So instead of doing all this new future work under the long running 1.8.x
minor, going to start fresh on v1.9.x -> v1.10.x redesigns.
* Still need one more commit after this to fetch newest genbadge.
* Noticed a few things during manual QA on debian.
- Npm install, user actually reported but I didn't fully investigate until
now. Fixed!
- Controls page was still using deprecated function. Now fixed!
* Till manually qa testing Debian atm. Hopefully this is the last thing.
* Again doing last minute QA and realized I locked out non-same user installs. - Silly me, but that's what manual QA is for.
| if not form.validate_on_submit(): | ||
| validation_errors(form) | ||
| # Redirect back to the previous page. | ||
| return redirect(request.referrer) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix untrusted URL redirection, do not redirect directly to user-supplied values (including headers like Referer). Instead, either (1) redirect to a known safe internal route, optionally preserving needed state via query parameters or server-side session, or (2) validate the user-provided URL against a strict policy (for example, allow only relative paths or a whitelist of hosts/paths).
For this specific function, the least intrusive fix while preserving existing behavior is to treat the “go back” redirect as an internal navigation: we should only redirect if the referrer points back to this same application and not to an arbitrary external site. A simple, robust approach in Flask is:
- Parse
request.referrerwithurllib.parse.urlparse. - If it is empty or parsing shows a different hostname than the current request’s host, do not redirect there.
- Instead, fall back to a safe internal route (e.g.,
url_for("main.jobs")), which keeps the user on our site. - This does not change core functionality for normal, honest traffic (where referrer is our own page) but blocks malicious external redirects.
We only need to modify the two places where request.referrer is used: line 96 and line 130 in app/blueprints/main/jobs.py. We'll add an import of urlparse from urllib.parse and replace those redirect(request.referrer) calls with a helper that validates the referrer. Since we must keep changes within the shown snippet, we can implement a small helper function directly in this file (above jobs()) that checks whether the referrer URL is same-origin (matches request.host) and only then redirects; otherwise it redirects to url_for("main.jobs").
| @@ -9,6 +9,7 @@ | ||
| redirect, | ||
| current_app, | ||
| ) | ||
| from urllib.parse import urlparse | ||
|
|
||
| from app.utils import * | ||
| from app.models import GameServer | ||
| @@ -23,6 +24,25 @@ | ||
|
|
||
| ######### Jobs Route ######### | ||
|
|
||
| def _safe_redirect_back(fallback_endpoint: str = "main.jobs"): | ||
| """ | ||
| Redirect back to the referrer only if it is a same-origin URL. | ||
| Otherwise, redirect to the given fallback endpoint. | ||
| """ | ||
| referrer = request.referrer | ||
| if not referrer: | ||
| return redirect(url_for(fallback_endpoint)) | ||
|
|
||
| parsed_ref = urlparse(referrer) | ||
| # Only allow relative URLs or same-host absolute URLs. | ||
| if not parsed_ref.netloc: | ||
| return redirect(referrer) | ||
|
|
||
| if parsed_ref.netloc == request.host: | ||
| return redirect(referrer) | ||
|
|
||
| return redirect(url_for(fallback_endpoint)) | ||
|
|
||
| @main_bp.route("/jobs", methods=["GET", "POST"]) | ||
| @login_required | ||
| def jobs(): | ||
| @@ -92,8 +112,8 @@ | ||
| if request.method == "POST": | ||
| if not form.validate_on_submit(): | ||
| validation_errors(form) | ||
| # Redirect back to the previous page. | ||
| return redirect(request.referrer) | ||
| # Redirect back to the previous page, ensuring the URL is safe. | ||
| return _safe_redirect_back() | ||
|
|
||
| # Setup custom command if send and custom. | ||
| command = form.command.data | ||
| @@ -127,5 +147,5 @@ | ||
| return redirect(url_for("main.jobs", server_id=form.server_id.data)) | ||
|
|
||
| flash("Error adding job", category="error") | ||
| return redirect(request.referrer) | ||
| return _safe_redirect_back() | ||
|
|
| return redirect(url_for("main.jobs", server_id=form.server_id.data)) | ||
|
|
||
| flash("Error adding job", category="error") | ||
| return redirect(request.referrer) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix untrusted URL redirection you must not redirect directly to user-controlled values. Instead, either (1) redirect only to fixed, server-controlled routes, or (2) validate any dynamic redirect target against a strict allowlist or ensure it is a safe relative URL (no scheme/host, no backslashes that might be interpreted as slashes).
In this function, the problematic line is:
130: return redirect(request.referrer)The simplest fix that does not change core functionality much is to fall back to a safe internal page (for example, the jobs page for that server, or the jobs index) when the referrer is unsafe or absent, and only use request.referrer when it is clearly an internal, relative URL. Since we must not rely on project-specific helpers outside the shown snippet, we can implement a small local validation using urllib.parse.urlparse from the standard library. That lets us distinguish relative paths (safe) from absolute URLs to external hosts (unsafe).
Concretely:
- Add an import for
urlparseat the top:from urllib.parse import urlparse. - Replace
return redirect(request.referrer)with logic such as:- Get
referrer = request.referrer or "". - Normalize backslashes out of the referrer string.
- Parse it; if both
netlocandschemeare empty (meaning it’s a relative URL/path), we treat it as safe and redirect to it. - Otherwise, ignore it and redirect to a safe default internal route, e.g.
url_for("main.jobs", server_id=form.server_id.data)or justurl_for("main.jobs").
- Get
Using url_for("main.jobs", server_id=form.server_id.data) as the fallback keeps behavior close to the intended “go back to the jobs view for this server” even when the referrer is missing or malicious.
All changes are within app/blueprints/main/jobs.py:
- At the imports region (around lines 3–11), add the
urllib.parseimport. - At the POST branch failure case (around lines 129–130), replace the direct redirect with the validated-URL logic and fallback redirect.
| @@ -9,6 +9,7 @@ | ||
| redirect, | ||
| current_app, | ||
| ) | ||
| from urllib.parse import urlparse | ||
|
|
||
| from app.utils import * | ||
| from app.models import GameServer | ||
| @@ -127,5 +128,13 @@ | ||
| return redirect(url_for("main.jobs", server_id=form.server_id.data)) | ||
|
|
||
| flash("Error adding job", category="error") | ||
| return redirect(request.referrer) | ||
| referrer = request.referrer or "" | ||
| # Normalize backslashes to avoid browser-specific interpretation issues | ||
| referrer = referrer.replace("\\", "") | ||
| parsed = urlparse(referrer) | ||
| if referrer and not parsed.scheme and not parsed.netloc: | ||
| # Relative URL: considered safe to redirect to | ||
| return redirect(referrer) | ||
| # Fallback to a safe internal route if referrer is missing or unsafe | ||
| return redirect(url_for("main.jobs", server_id=form.server_id.data)) | ||
|
|
No description provided.