fix: trying to fix ssl certification issue while downloading on some macs#1
Merged
ParticularlyPythonicBS merged 1 commit intomainfrom Feb 1, 2026
Merged
Conversation
WalkthroughAdds a POST /api/download_tutorial endpoint that streams a remote tutorial SQLite into Changes
Sequence DiagramsequenceDiagram
participant Client
participant Backend
participant RemoteStorage
participant FileSystem
Client->>Backend: POST /api/download_tutorial
Backend->>Backend: create_secure_ssl_context()
Backend->>RemoteStorage: urlopen(remote_url, context)
RemoteStorage-->>Backend: stream response
Backend->>FileSystem: ensure assets/ exists
Backend->>FileSystem: write to temp file via shutil.copyfileobj(stream, temp)
Backend->>FileSystem: temp.replace(assets/tutorial_database.sqlite)
FileSystem-->>Backend: replace complete
Backend-->>Client: 200 OK {"status":"ok","path":".../assets/tutorial_database.sqlite"}
Note over Backend: On error -> log and return 500 {"detail":"Download failed: ..."}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@backend/main.py`:
- Around line 168-170: The code creates an insecure SSL context by setting
ctx.check_hostname = False and ctx.verify_mode = ssl.CERT_NONE; replace this
with a shared utility (e.g., create_secure_ssl_context) that builds an SSL
context using certifi roots (ssl.create_default_context(cafile=certifi.where())
or ctx.load_verify_locations(certifi.where())) and does not disable hostname
verification; update backend/main.py to call that new create_secure_ssl_context
function instead of creating and mutating ctx locally and reuse it from
temoa_runner.py to remove duplication.
- Around line 177-179: Replace the current except block that does "except
Exception as e:" and calls logging.error(...) then raises HTTPException with a
plain detail; instead call logging.exception(...) to include the traceback and
raise the HTTPException using "raise HTTPException(... ) from e" to preserve the
exception chain (the block that uses logging.error(f\"Failed to download
tutorial: {e}\") and raises HTTPException should be updated). Ensure the
HTTPException detail string remains informative but the underlying exception is
chained and the traceback is logged.
In `@backend/tests/test_download.py`:
- Around line 30-34: The test currently asserts an intentionally insecure SSL
configuration by checking kwargs["context"] is an ssl.SSLContext with
ctx.check_hostname False and ctx.verify_mode == ssl.CERT_NONE; update the test
to either document this expectation or make it flexible: add a concise comment
in test_download.py explaining that the test expects an insecure SSLContext (why
and whether it's intentional), or change the assertions around ctx, e.g., allow
either disabled verification or a verified context by checking for presence of a
context and conditionally asserting verification state, referencing kwargs, ctx,
ssl.SSLContext, ctx.check_hostname and ctx.verify_mode to locate the code to
modify.
- Around line 37-41: The test's mock patches the global "urllib.request.urlopen"
which doesn't intercept the call used by the /api/download_tutorial endpoint;
update test_download_tutorial_failure to patch urllib.request.urlopen on the
actual module that defines the endpoint/handler (the module that contains the
download_tutorial function used by the route) — e.g. patch
"<module_where_download_tutorial_is_defined>.urllib.request.urlopen" — so the
exception is raised where the code calls urlopen, then keep the existing
client.post("/api/download_tutorial") and assertions.
In `@temoa_runner.py`:
- Around line 126-128: The code in temoa_runner.py creates an SSLContext (ctx =
ssl.create_default_context()) and then disables verification via
ctx.check_hostname = False and ctx.verify_mode = ssl.CERT_NONE; update this to
avoid bypassing certificate validation by either: A) use certifi by creating the
context with cafile=certifi.where() (add certifi to pyproject.toml and import
certifi) and remove the check_hostname/verify_mode overrides, or B) make the
insecure path explicit and opt-in via an environment variable (e.g.,
TEMOA_SKIP_CERT_VERIFY) so that by default validation remains enabled; if you
keep the opt-out, log a clear warning when TEMOA_SKIP_CERT_VERIFY is set and
replicate the same change in backend/main.py and any tests that currently expect
disabled verification so they either use certifi or set the env var explicitly
in test setup.
ParticularlyPythonicBS
force-pushed
the
fix/ssl_cert_issues
branch
from
fd6ea5f to
b0aa40d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@backend/tests/test_download.py`:
- Around line 36-44: Replace the runtime-dependent assertions by parametrizing
the test to control TEMOA_SKIP_CERT_VERIFY explicitly: create a pytest test
(e.g., test_download_tutorial_ssl_context) parametrized over skip_verify values
("0","1"), use monkeypatch.setenv("TEMOA_SKIP_CERT_VERIFY", skip_verify) and
patch/Mock urllib.request.urlopen, shutil.copyfileobj and open so
client.post("/api/download_tutorial") runs deterministically, then capture the
urlopen call args to extract the SSL context (ctx) and assert ctx.check_hostname
and ctx.verify_mode according to skip_verify; this ensures deterministic
behavior and removes reliance on the external environment variable at runtime.
- Around line 11-13: The test test_download_tutorial_success is patching global
imports (urllib.request.urlopen, shutil.copyfileobj, builtins.open) so the mocks
may not intercept calls; update the patch targets to the module under test
(e.g., patch "backend.main.urllib.request.urlopen",
"backend.main.shutil.copyfileobj", and "backend.main.open" or use
patch("backend.main.open", new_callable=MagicMock)) so they match how functions
are imported/used in that module and mirror the correct approach used in
test_download_tutorial_failure.
- Around line 1-6: Add FastAPI to project dependencies so tests importing
TestClient and backend.main's app can import fastapi: update pyproject.toml to
include "fastapi" (and optionally "starlette"/"pydantic" versions if required)
either under [project.dependencies] if backend needs it at runtime or under
[project.optional-dependencies.dev] / [dependency-groups.dev] (depending on your
pyproject layout) so that imports like from fastapi.testclient import TestClient
and backend.main's app resolve during CI.
In `@backend/utils.py`:
- Around line 7-25: Duplicate create_secure_ssl_context() found in backend.utils
and temoa_runner.py; remove the copy in temoa_runner.py and import the single
implementation from backend.utils instead (i.e., replace the function definition
in temoa_runner.py with a from backend.utils import create_secure_ssl_context
and use that), or if temoa_runner.py must be standalone, keep the function but
add a clear comment stating it is intentionally duplicated and link to
backend.utils.create_secure_ssl_context for maintenance; update any references
to ensure they call create_secure_ssl_context() from the shared location.
- Around line 15-17: Replace the root logger call with a module-level named
logger: create a logger via logging.getLogger(__name__) in backend/utils.py
(module scope) and change the logging.warning(...) invocation that warns about
"SSL certificate verification is DISABLED via TEMOA_SKIP_CERT_VERIFY." to use
logger.warning(...) instead so filtering and configuration target this module
(ensure the logger is defined before the warning is emitted).
In `@temoa_runner.py`:
- Line 20: Replace the deprecated typing.List import and annotations with the
built-in list type: remove List from the import line ("from typing import List,
Optional") and update any annotations that use List[...] (e.g., the annotation
at/around line 98) to use list[...] instead; leave Optional as-is (or switch to
X | None if you prefer modern union syntax) so the code uses built-in generics
supported by Python 3.12+.
b0aa40d to
14a485c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/main.py`:
- Around line 159-177: The download_tutorial handler can hang (no timeout) and
may leave partial files; update download_tutorial to open the URL with a
reasonable timeout (e.g., timeout=10) and write to a temporary file (e.g.,
target_path.with_suffix(".tmp") or tempfile.NamedTemporaryFile) then atomically
replace target_path with Path.replace once write completes; ensure assets
directory exists (assets_path.mkdir(parents=True, exist_ok=True)); on any
exception or interruption remove the temp file in a finally block and
re-raise/return the HTTPException as currently done; keep
create_secure_ssl_context usage and include the temp-file cleanup logic tied to
the temp filename used.
In `@backend/utils.py`:
- Around line 1-27: The project is importing certifi in backend/utils.py (see
create_secure_ssl_context and the certifi import) but certifi is not declared as
a runtime dependency; add "certifi" to the [project] dependencies in
pyproject.toml so installations include the CA bundle at runtime, then run a
dependency install to verify import resolution and ensure
create_secure_ssl_context continues to use certifi.where() for the secure
default path; keep the TEMOA_SKIP_CERT_VERIFY logic unchanged.
In `@temoa_runner.py`:
- Around line 149-162: The download loop can leave partial files and block
indefinitely; update the logic in the for loop that uses
create_secure_ssl_context(), urllib.request.urlopen and shutil.copyfileobj so
downloads use urlopen(..., timeout=...) and write to a temporary file (e.g.,
target.with_suffix(".part") or similar) and only atomically rename the temp file
to target on successful completion; on any exception remove the temp file (or
ensure it does not mask future retries) and re-raise/log the error so future
runs will retry the download instead of skipping due to target.exists().
14a485c to
7ba3352
Compare
ParticularlyPythonicBS
force-pushed
the
fix/ssl_cert_issues
branch
from
7ba3352 to
edfc065
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/main.py`:
- Around line 179-180: The code currently calls logging.exception("Failed to
download tutorial") using the root logger; add a module-level named logger
(logger = logging.getLogger(__name__)) at the top of backend/main.py and replace
the root logging call with logger.exception(...) so the endpoint uses the named
logger consistently with backend/utils.py; update any other direct uses of
logging in this module (e.g., around the HTTPException raise) to use logger
instead.
Comment on lines
+179
to
+180
| logging.exception("Failed to download tutorial") | ||
| raise HTTPException(status_code=500, detail=f"Download failed: {str(e)}") from e |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a named logger for consistency with backend/utils.py.
The endpoint uses logging.exception() on the root logger. For consistency with backend/utils.py which uses a named logger, consider creating a module-level logger.
♻️ Proposed fix
Add at module level:
logger = logging.getLogger(__name__)Then replace:
- logging.exception("Failed to download tutorial")
+ logger.exception("Failed to download tutorial")🧰 Tools
🪛 Ruff (0.14.14)
[warning] 179-179: exception() call on root logger
(LOG015)
[warning] 180-180: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In `@backend/main.py` around lines 179 - 180, The code currently calls
logging.exception("Failed to download tutorial") using the root logger; add a
module-level named logger (logger = logging.getLogger(__name__)) at the top of
backend/main.py and replace the root logging call with logger.exception(...) so
the endpoint uses the named logger consistently with backend/utils.py; update
any other direct uses of logging in this module (e.g., around the HTTPException
raise) to use logger instead.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.