flask to fastapi conversion#187
Closed
ryansurf wants to merge 1 commit into
Closed
Conversation
Contributor
Reviewer's GuideMigrates the HTTP server from Flask to FastAPI/uvicorn, adjusts endpoints and tests accordingly, and updates dependencies and template usage to match the new stack. Sequence diagram for FastAPI root endpoint invoking CLI subprocesssequenceDiagram
actor Browser
participant Uvicorn
participant FastAPIApp
participant CLIProcess
Browser->>Uvicorn: HTTP GET /
Uvicorn->>FastAPIApp: dispatch request
FastAPIApp->>FastAPIApp: parse query_params
FastAPIApp->>FastAPIApp: build args string
FastAPIApp->>CLIProcess: subprocess.run([python, cli.py, args])
CLIProcess-->>FastAPIApp: stdout (surf report)
FastAPIApp-->>Browser: 200 text/plain (surf report)
note over FastAPIApp,CLIProcess: On subprocess error, FastAPIApp raises HTTPException(500)
Sequence diagram for FastAPI /help endpoint serving help.txtsequenceDiagram
actor Browser
participant Uvicorn
participant FastAPIApp
participant FileSystem
Browser->>Uvicorn: HTTP GET /help
Uvicorn->>FastAPIApp: dispatch request
FastAPIApp->>FileSystem: read BASE_DIR/help.txt
FileSystem-->>FastAPIApp: file handle
FastAPIApp-->>Browser: 200 FileResponse text/plain
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 5 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The
index.htmltemplate still assumes Flask’surl_for('serve_script')and aserve_scriptroute, but those no longer exist; consider adding a FastAPI static file route (e.g., viaStaticFiles) and updating the template to userequest.url_foraccordingly. - With the removal of the
/homeendpoint, there is no FastAPI route that rendersindex.htmlor passesenv_varsinto the template; if the frontend still relies on this, add a corresponding@app.get("/home")usingtemplates.TemplateResponseand include bothrequestandenv_varsin the context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `index.html` template still assumes Flask’s `url_for('serve_script')` and a `serve_script` route, but those no longer exist; consider adding a FastAPI static file route (e.g., via `StaticFiles`) and updating the template to use `request.url_for` accordingly.
- With the removal of the `/home` endpoint, there is no FastAPI route that renders `index.html` or passes `env_vars` into the template; if the frontend still relies on this, add a corresponding `@app.get("/home")` using `templates.TemplateResponse` and include both `request` and `env_vars` in the context.
## Individual Comments
### Comment 1
<location path="src/server.py" line_range="47-51" />
<code_context>
- @app.route("/help")
- def serve_help():
+ @app.get("/help")
+ async def serve_help():
"""Serves the help.txt file."""
- return send_from_directory(
- Path(__file__).resolve().parents[1], "help.txt"
- )
-
- @app.route("/home")
- def serve_index():
- """Serves index.html."""
- return render_template("index.html", env_vars=env.model_dump())
-
- @app.route("/script.js")
- def serve_script():
- """Serves the frontend JavaScript."""
- return send_file("static/script.js")
-
- @app.route("/")
- def default_route():
+ HELP_FILE_PATH = BASE_DIR / "help.txt"
+ return FileResponse(path=HELP_FILE_PATH, media_type="text/plain")
+
+
</code_context>
<issue_to_address>
**suggestion:** Consider handling the case where `help.txt` is missing to avoid an uninformative 500 error.
If `help.txt` is missing or unreadable, this will likely return a generic 500. Consider checking `HELP_FILE_PATH.exists()` (and possibly readability) and raising a `HTTPException(status_code=404, detail="help.txt not found")` or similar to return a clearer response and avoid unnecessary tracebacks in the logs.
Suggested implementation:
```python
from pathlib import Path
from fastapi import HTTPException
```
```python
@app.get("/help")
async def serve_help():
"""Serves the help.txt file."""
HELP_FILE_PATH = BASE_DIR / "help.txt"
if not HELP_FILE_PATH.exists() or not HELP_FILE_PATH.is_file():
# Avoid an uninformative 500 if the help file is missing
raise HTTPException(status_code=404, detail="help.txt not found")
return FileResponse(path=HELP_FILE_PATH, media_type="text/plain")
```
</issue_to_address>
### Comment 2
<location path="src/server.py" line_range="44-47" />
<code_context>
- """Serves index.html."""
- return render_template("index.html", env_vars=env.model_dump())
-
- @app.route("/script.js")
- def serve_script():
- """Serves the frontend JavaScript."""
- return send_file("static/script.js")
-
- @app.route("/")
</code_context>
<issue_to_address>
**issue (bug_risk):** The template still references `url_for('serve_script')`, but the corresponding endpoint has been removed.
In `index.html` the script tag still uses `{{ url_for('serve_script') }}`, but the `serve_script` route was removed during the FastAPI migration. This will result in a broken script URL unless `serve_script` is defined elsewhere. Please either add an equivalent FastAPI endpoint (e.g., via `StaticFiles`) or update the template to reference the new script path.
</issue_to_address>
### Comment 3
<location path="src/templates/index.html" line_range="90" />
<code_context>
- // load .env variables from Flask server
- const env = {{ env_vars|tojson }};
+ // load .env variables from FastAPI server
+ const env = {{ env_vars | safe }};
</script>
<script src="{{ url_for('serve_script') }}"></script>
</code_context>
<issue_to_address>
**🚨 issue (security):** Using `| safe` for `env_vars` may introduce XSS risk and assumes it is already valid JS/JSON.
Previously `env_vars` was passed through `tojson`, which both produced valid JSON and escaped it for safe embedding. With `| safe`, the value is injected into the script as-is. If `env_vars` includes any user-controlled data, this is an XSS vector, and if it’s a Python dict rather than a JSON string, the JS will be invalid. Prefer a JSON-encoding approach compatible with your template engine instead of marking this value safe.
</issue_to_address>
### Comment 4
<location path="src/server.py" line_range="87" />
<code_context>
- app = create_app(env)
- app.run(host="0.0.0.0", port=env.PORT, debug=env.DEBUG)
+
+ uvicorn.run(app, host="0.0.0.0", port=env.PORT)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `DEBUG` setting from `ServerSettings` is no longer used to control development features like reload.
With the switch to `uvicorn.run`, `env.DEBUG` no longer affects runtime behavior. If it’s still meant to control development mode, consider passing it through (e.g. `reload=env.DEBUG` and/or `log_level="debug" if env.DEBUG else "info"`) so behavior stays consistent.
</issue_to_address>
### Comment 5
<location path="tests/test_help_endpoint.py" line_range="14-18" />
<code_context>
app = create_app(env)
- client = app.test_client()
+ client = TestClient(app)
resp = client.get("/help")
assert resp.status_code == HTTPStatus.OK
- assert len(resp.data) > 0
+ assert len(resp.text) > 0
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the `/help` test by asserting the actual file contents and content type.
`len(resp.text) > 0` only checks that something is returned. With the new `FileResponse`, you can strengthen this by reading `help.txt` from disk and asserting equality with `resp.text`, and optionally asserting the content type header starts with `text/plain` to better guard against regressions in how `/help` is served.
Suggested implementation:
```python
from fastapi import FastAPI
from fastapi.testclient import TestClient
from http import HTTPStatus
from pathlib import Path
```
```python
client = TestClient(app)
resp = client.get("/help")
assert resp.status_code == HTTPStatus.OK
help_contents = Path("help.txt").read_text(encoding="utf-8")
assert resp.text == help_contents
assert resp.headers["content-type"].startswith("text/plain")
```
</issue_to_address>
### Comment 6
<location path="src/server.py" line_range="64-68" />
<code_context>
result = subprocess.run(
[sys.executable, str(CLI_PATH), args],
capture_output=True,
text=True,
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+47
to
+51
| @app.get("/help") | ||
| async def serve_help(): | ||
| """Serves the help.txt file.""" | ||
| return send_from_directory( | ||
| Path(__file__).resolve().parents[1], "help.txt" | ||
| ) | ||
|
|
||
| @app.route("/home") | ||
| def serve_index(): | ||
| """Serves index.html.""" | ||
| return render_template("index.html", env_vars=env.model_dump()) | ||
|
|
||
| @app.route("/script.js") | ||
| def serve_script(): | ||
| """Serves the frontend JavaScript.""" | ||
| return send_file("static/script.js") | ||
|
|
||
| @app.route("/") | ||
| def default_route(): | ||
| HELP_FILE_PATH = BASE_DIR / "help.txt" | ||
| return FileResponse(path=HELP_FILE_PATH, media_type="text/plain") |
Contributor
There was a problem hiding this comment.
suggestion: Consider handling the case where help.txt is missing to avoid an uninformative 500 error.
If help.txt is missing or unreadable, this will likely return a generic 500. Consider checking HELP_FILE_PATH.exists() (and possibly readability) and raising a HTTPException(status_code=404, detail="help.txt not found") or similar to return a clearer response and avoid unnecessary tracebacks in the logs.
Suggested implementation:
from pathlib import Path
from fastapi import HTTPException@app.get("/help")
async def serve_help():
"""Serves the help.txt file."""
HELP_FILE_PATH = BASE_DIR / "help.txt"
if not HELP_FILE_PATH.exists() or not HELP_FILE_PATH.is_file():
# Avoid an uninformative 500 if the help file is missing
raise HTTPException(status_code=404, detail="help.txt not found")
return FileResponse(path=HELP_FILE_PATH, media_type="text/plain")
Comment on lines
-44
to
-47
| @app.route("/script.js") | ||
| def serve_script(): | ||
| """Serves the frontend JavaScript.""" | ||
| return send_file("static/script.js") |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The template still references url_for('serve_script'), but the corresponding endpoint has been removed.
In index.html the script tag still uses {{ url_for('serve_script') }}, but the serve_script route was removed during the FastAPI migration. This will result in a broken script URL unless serve_script is defined elsewhere. Please either add an equivalent FastAPI endpoint (e.g., via StaticFiles) or update the template to reference the new script path.
| // load .env variables from Flask server | ||
| const env = {{ env_vars|tojson }}; | ||
| // load .env variables from FastAPI server | ||
| const env = {{ env_vars | safe }}; |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Using | safe for env_vars may introduce XSS risk and assumes it is already valid JS/JSON.
Previously env_vars was passed through tojson, which both produced valid JSON and escaped it for safe embedding. With | safe, the value is injected into the script as-is. If env_vars includes any user-controlled data, this is an XSS vector, and if it’s a Python dict rather than a JSON string, the JS will be invalid. Prefer a JSON-encoding approach compatible with your template engine instead of marking this value safe.
| app = create_app(env) | ||
| app.run(host="0.0.0.0", port=env.PORT, debug=env.DEBUG) | ||
|
|
||
| uvicorn.run(app, host="0.0.0.0", port=env.PORT) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The DEBUG setting from ServerSettings is no longer used to control development features like reload.
With the switch to uvicorn.run, env.DEBUG no longer affects runtime behavior. If it’s still meant to control development mode, consider passing it through (e.g. reload=env.DEBUG and/or log_level="debug" if env.DEBUG else "info") so behavior stays consistent.
Comment on lines
+14
to
+18
| client = TestClient(app) | ||
| resp = client.get("/help") | ||
|
|
||
| assert resp.status_code == HTTPStatus.OK | ||
| assert len(resp.data) > 0 | ||
| assert len(resp.text) > 0 |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen the /help test by asserting the actual file contents and content type.
len(resp.text) > 0 only checks that something is returned. With the new FileResponse, you can strengthen this by reading help.txt from disk and asserting equality with resp.text, and optionally asserting the content type header starts with text/plain to better guard against regressions in how /help is served.
Suggested implementation:
from fastapi import FastAPI
from fastapi.testclient import TestClient
from http import HTTPStatus
from pathlib import Path client = TestClient(app)
resp = client.get("/help")
assert resp.status_code == HTTPStatus.OK
help_contents = Path("help.txt").read_text(encoding="utf-8")
assert resp.text == help_contents
assert resp.headers["content-type"].startswith("text/plain")
Comment on lines
64
to
68
| result = subprocess.run( | ||
| [sys.executable, Path("src") / "cli.py", args], | ||
| [sys.executable, str(CLI_PATH), args], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
General:
Code:
Summary by Sourcery
Migrate the HTTP server from Flask to FastAPI, updating dependencies, runtime wiring, and tests accordingly.
New Features:
Bug Fixes:
Enhancements:
Build: