Conversation
…i v5
- uv-managed project; ruff lint/format; pytest suite with library fakes
- Sessions: opaque composite bearer token (kasa/tapo tokens + regional
hosts + terminal id) minted at login; still stateless pass-through
- Typed error mapping: 401 auth/expired, 404 unknown device, 409 offline,
502 cloud errors, 504 timeouts; MFA supported via mfa_code form field
- New endpoints: GET /devices (strip children flattened, live is_on),
GET /devices/{id}, POST /devices/{id}/power (on/off/toggle)
- Per-token TTL device cache; blocking cloud calls moved off event loop
- Removed: broken retry decorator, dead configuration pkg, Heroku
workflows, GET /user/token, debug prints
Some devices (e.g. the DL110 doorbell) report their alias base64-encoded through getDeviceList; the official app decodes them. Strictly decodes only clean base64 that yields printable text.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: ecf3e6882a10
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| security:code-auditor | 1 |
| architecture:solid | 8 |
| automation:ci-deploy | 0 |
| documentation:docs | 3 |
security:code-auditor (1 finding)
Blocking - app/services/tplink_service.py:140
Server-Side Request Forgery via client-controlled, unsigned session token. The bearer token is opaque base64url JSON with no signature (app/token_blob.py), and the client fully controls its contents since it is never validated against anything the server minted itself beyond requiring a
kasa_tokenkey to be present (decode_session_token only checks base64/JSON decoding and dict-with-key shape). In_create_session(app/services/tplink_service.py:139-147),blob["kasa_host"]andblob["tapo_host"]are written straight intomanager._kasa_api.host/manager._tapo_api.host, which the tplinkcloud library then uses as the destination host for outbound HTTP calls (login-restore, get_devices, sys_info, power control) on every request that uses that token. An attacker can forge a token such asbase64url({"kasa_token":"x","kasa_host":"http://169.254.169.254"})(or any internal/attacker-controlled host) and call GET /devices with it as the bearer token; the service will then issue outbound requests to that attacker-chosen host, carrying the token'skasa_token/tapo_tokenvalues as credentials. This is a concrete SSRF into internal infrastructure (cloud metadata endpoints, internal admin services, etc.), and doubles as a credential-exfiltration primitive since an attacker who has stolen or is replaying a real victimkasa_tokencan redirect it to an attacker-controlled collector by only changingkasa_host(the token has no signature binding the host to the token issuer). It is also not fully blind:load_deviceswraps any transport exception intoTPLinkCloudError(f"...: {exc}"), which errors.py returns verbatim to the client as the 502 body, giving an attacker connectivity/error-message oracle for the SSRF target (e.g. distinguishing open vs. closed internal ports, DNS failures, TLS errors). Fix: sign/HMAC the session token (or bind kasa_host/tapo_host to a server-side allowlist of TP-Link's known regional API domains) so a client cannot supply an arbitrary destination host that the server will make credentialed outbound requests to, and avoid echoing raw upstream exception text in the 502 response body.
architecture:solid (8 findings)
Major - app/services/tplink_service.py:200
U-S3 — the effect-offloading rule is applied inconsistently, and the inconsistency is invisible at the call sites.
load_devicesstates the operative fact about this dependency: "get_devices()has an async signature but fetches the cloud device list with blockingrequestsinternally" (lines 164-166), and offloads it withasyncio.to_thread(asyncio.run, ...). Every other cloud call in this class is then awaited directly on the event loop: the sys_info fan-out here (200-203),get_sys_info/get_net_infoindevice_detail(269-271),is_on/power_on/power_offinset_power(317-322), and the three_power_toolscalls (335, 340, 345).If those share the library's blocking transport, the
asyncio.gatherat line 200 buys no concurrency at all — the N calls run serially and stall the whole process, so oneGET /deviceson a 30-device account freezes every other request for the duration; the 30scloud_timeout_secondsceiling becomes a whole-server stall budget. This is a close call and I am not blocking on it: the falsifier is simple — ifTPLinkDeviceDataProvider/the device-level calls use an async HTTP client while only the manager's device-list path is blocking, the split is correct and the only thing missing is a comment saying so at line 199.Whichever way it resolves, make the rule explicit and uniform: one documented seam (a small
_offload()helper or a note naming which library calls are genuinely async) so the next maintainer does not have to guess per call site. Related:asyncio.to_threadis not cancellable, so when theasyncio.timeoutat line 169 fires, the worker thread and the event loopasyncio.runcreated inside it keep running to completion — repeated timeouts occupy the default thread pool rather than freeing it.
Major - app/cache.py:36
U-D1 —
_locksis unbounded mutable state fed by unauthenticated input, and it leaks whenever the factory raises. The session map is deliberately bounded (TTLCache(maxsize=...)), but the parallel_locksdict has no bound and no TTL, and the only cleanup (self._locks.pop(key, None), line 36) sits inside theasync with lockbody afterawait factory(). When the factory raises, the exception propagates and that line never runs.That path is reachable pre-auth:
get_session→gateway.session(token)→_create_session→decode_session_tokenraisesInvalidServiceTokenErrorfor any malformed bearer token (app/token_blob.py:32). Every distinct garbage token therefore adds a permanent entry keyed by its digest — unbounded growth in a long-lived process, driven by anonymous callers, with no eviction path since the lock is only ever popped on success.Fix: move the cleanup into a
finally:so the lock is released on both paths, e.g.lock = self._locks.setdefault(key, asyncio.Lock()) try: async with lock: session = self._cache.get(key) if session is None: session = await factory() self._cache[key] = session finally: self._locks.pop(key, None) return sessionand cover it with a test that fires two concurrent requests carrying the same invalid token and asserts
len(cache._locks) == 0afterwards.
Major - app/services/tplink_service.py:142
U-D1 / U-G1 — the service reaches into the third-party library's private state.
_create_sessionandlogindepend onmanager._kasa_api.host(lines 121, 142),manager._tapo_api(113, 143, 147) andmanager._tapo_token(145). These are collaborators obtained by reaching through the dependency rather than being handed over through its contract, so the session's wiring is invisible to the library's own compatibility guarantees: a patch release that renames_kasa_apior stops setting_tapo_tokenbreaks token restoration with no signal from this repo's tests. tests/fakes.pyFakeManagermirrors those same private names, so the fakes will keep passing after the real library drifts — the test double encodes the private shape instead of a contract.The comments ("no public getter", "no public setter") name the right fix: the library is owned by the same author (the PR already references piekstra/tplink-cloud-api#117), so add
get_api_host()/set_api_host()and aset_tapo_auth(token, host)upstream and depend on those. If that must wait, at minimum (a) pin a compatible-release upper bound (see the pyproject finding) and (b) add one test that constructs a realTPLinkDeviceManagerwithout credentials and asserts these attributes exist, so a library bump fails CI rather than production.
Major - pyproject.toml:15
U-G1 — the declared dependency floor contradicts the API the code requires.
tplink-cloud-api>=4.2.0, but this diff importsTPLinkAuthError,TPLinkMFARequiredError,TPLinkTokenExpiredError(app/errors.py:3-8) and uses the v5-only dual-cloud surfaceinclude_tapo=/get_tapo_token()/_tapo_api(app/services/tplink_service.py:113, 123, 137). uv.lock resolves 5.2.0, so lockfile-based installs (Docker, CI) work; anything that installs from the manifest instead —pip install ., a downstream re-resolve, or a resolver pinned lower for another constraint — can legally pick 4.x and fail at import time. The manifest is the contract, and it currently understates it.Raise the floor to the release that actually carries this API (
>=5.2.0), and given the reliance on library private attributes noted separately, add an upper bound (>=5.2.0,<6) so a major bump is a deliberate change rather than a silent one.
Minor - app/main.py:23
U-D2 — the composition root has no seam for its main collaborator, so consumers wire around it.
create_appis otherwise a clean single composition root, but it constructsTPLinkGateway(settings)itself with no way to pass one in. The consequence is visible in tests/conftest.py:60-61, which callscreate_app(settings)and then reassignsapplication.state.gatewayafterwards — the graph is built twice and the real gateway (with itsSessionCache) is created and discarded on every test app. That also means the test path exercises a differently-wired app than production.Fix is one parameter:
def create_app(settings: Settings | None = None, gateway: TPLinkGateway | None = None)withapp.state.gateway = gateway or TPLinkGateway(settings). Tests then passTPLinkGateway(settings, manager_factory=fleet.factory)in, and the root stays the only place the graph is assembled. (app.dependency_overrides[get_gateway]is the equally idiomatic FastAPI alternative.)
Minor - app/services/tplink_service.py:173
U-L2 — the catch-all relabels this service's own bugs as an upstream outage, silently.
except Exception as exc: raise TPLinkCloudError(f"TP-Link cloud is unreachable: {exc}")converts any non-TPLinkCloudErrorfailure into a 502 via the handler in app/errors.py:64. ATypeError/AttributeError/KeyError— exactly the failure mode the private-attribute coupling above makes likely on a library bump — is reported to the client as "the TP-Link cloud is unreachable" and never logged, so the operator sees a vendor outage where the real cause is this repo. tests/test_errors.py:32 pins the intended behavior (ConnectionError→ 502), which is right; the problem is the width of the net.Narrow it to the transport-level exceptions actually being mapped (
ConnectionError,OSError, and whatever the library'srequestslayer surfaces), andlogger.exception(...)before wrapping so the original traceback survives regardless.
Minor - app/services/tplink_service.py:260
U-L1 —
DeviceDetailis declared a subtype ofDeviceSummary, but the two are produced by independent code paths that can disagree.device_summariesreturns typedDeviceSummaryobjects built by_summarize(226-258);device_detailreturns a hand-builtdictthat re-derives all ten of the same fields (287-300). Theis_onsemantics are already computed from different sources:_summarizereads a child outlet's state out of the parent'schildrenlist via_info_value(237-238), whiledevice_detailreadsstatefrom the child's own jsonified sys_info (282). Since app/models/devices.py:26 declaresDeviceDetail(DeviceSummary), consumers may reasonably assumeGET /devicesandGET /devices/{id}report the same device identically — today that holds by coincidence, not construction, and the response_model validation will not catch a semantic divergence, only a missing field.Fix: have
device_detailcallself._summarize(device, parents, {device_id: sys_info})and extend the result (DeviceDetail(**summary.model_dump(), sys_info=..., net_info=...)), so the shared fields have exactly one producer, and return the model rather than a dict for the same reason the list endpoint does.
Nits - app/services/__init__.py:1
U-G1 — new public surface with no consumer. This barrel re-exports
TPLinkGateway,TPLinkSessionandjsonify, but nothing imports fromapp.services: app/dependencies.py:6, app/main.py:10, tests/conftest.py:5 and tests/test_devices.py:143 all import fromapp.services.tplink_servicedirectly.jsonifyin particular is a module-internal serialization helper being promoted to package API. Either drop the re-exports and let the module be the import path, or switch the existing importers to the barrel so the declared surface has a real consumer.
documentation:docs (3 findings)
Major - README.md:43
AUTH_ERROR_CODES(default[-20651]) is not a real setting:app/settings.pyhas no such field, andapp/errors.pymaps auth failures via typed exceptions (TPLinkAuthError,TPLinkTokenExpiredError,TPLinkMFARequiredError) from tplink-cloud-api v5, not error-code matching. This is leftover from the old (2021) service and will send a deployer looking for an env var that does nothing. Remove the row, or replace it with the actual override that exists:TPLINK_CLOUD_API_HOST(tplink_cloud_api_hostin Settings, defaultNone, used only for testing against a non-default cloud host).
Major - README.md:44
TPLINK_API_URLwith defaulthttps://wap.tplinkcloud.comdoes not exist inapp/settings.py; the only related field istplink_cloud_api_host, which defaults toNone(uses the tplink-cloud-api library's own defaults, not a hardcoded URL). SettingTPLINK_API_URLas documented would silently have no effect. Fix the row to match the real field name/default, or drop it if it's not meant to be a user-facing override.
Minor - README.md:40
Settingsalso definesdevice_cache_max_sessions(default 32, caps the in-process per-token session/device-list cache), which isn't listed in the Configuration table even thoughDEVICE_CACHE_TTLfrom the same struct is. Worth adding a row so operators know the cache has a bounded session count, not just a TTL.
Reviewer Coverage
security:code-auditor— complete (broad); inspected 27 assigned files (45 inspected across reviewers):app/__init__.py,app/cache.py,app/configuration/__init__.py,app/configuration/configuration.py,app/dependencies.py,app/errors.py,app/models/device_response.py,app/models/device_sys_info.py,app/models/devices_power_current_response.py,app/models/devices_power_day_response.py,app/models/devices_power_month_response.py,app/models/user.py,app/routers/device.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/tplink_service.py,app/settings.py,app/token_blob.py,tests/conftest.py,tests/fakes.py,tests/local_env_vars.py,tests/test_api.py,tests/test_auth.py,tests/test_devices.py,tests/test_errors.py,tests/test_power.py; skipped: none; constraints: nonearchitecture:solid—⚠️ incomplete (skipped files); inspected 13 assigned files (45 inspected across reviewers):app/cache.py,app/dependencies.py,app/errors.py,app/main.py,app/models/__init__.py,app/models/devices.py,app/models/power.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/__init__.py,app/services/tplink_service.py,pyproject.toml; skipped:app/routers/device.py; constraints: Could not run the project's own verification (uv sync / pytest / ruff): dependency installation needs network access that this sandbox blocks. All findings are from static reading of the head checkout. The tplinkcloud v5 package is not installed and not inspectable here, so statements about which library calls block are reasoned from the diff's own comments plus the offloading it applies, not from library source. app/settings.py and app/token_blob.py are new files referenced by these findings but are not in changed_files, so no finding is anchored there. git commands were blocked in this sandbox, so base-branch file contents were unavailable. New-vs-pre-existing lines were judged from the change map (all cited files are added or largely rewritten).automation:ci-deploy— complete (broad); inspected 12 assigned files (45 inspected across reviewers):.dockerignore,.github/workflows/ci.yml,.github/workflows/heroku-app-logs.yml,.github/workflows/package-publish-heroku.yml,.github/workflows/package-publish.yml,.github/workflows/package-python.yml,.github/workflows/release.yml,Dockerfile,app/requirements.txt,pyproject.toml,requirements.txt,uv.lock; skipped: none; constraints: nonedocumentation:docs— complete (broad); inspected 1 assigned file (45 inspected across reviewers):README.md; skipped: none; constraints: none
Inspected files (45)
.dockerignore.github/workflows/ci.yml.github/workflows/heroku-app-logs.yml.github/workflows/package-publish-heroku.yml.github/workflows/package-publish.yml.github/workflows/package-python.yml.github/workflows/release.ymlDockerfileREADME.mdapp/__init__.pyapp/cache.pyapp/configuration/__init__.pyapp/configuration/configuration.pyapp/dependencies.pyapp/errors.pyapp/main.pyapp/models/__init__.pyapp/models/device_response.pyapp/models/device_sys_info.pyapp/models/devices.pyapp/models/devices_power_current_response.pyapp/models/devices_power_day_response.pyapp/models/devices_power_month_response.pyapp/models/power.pyapp/models/user.pyapp/requirements.txtapp/routers/device.pyapp/routers/device_power.pyapp/routers/devices.pyapp/routers/user.pyapp/services/__init__.pyapp/services/tplink_service.pyapp/settings.pyapp/token_blob.pypyproject.tomlrequirements.txttests/conftest.pytests/fakes.pytests/local_env_vars.pytests/test_api.pytests/test_auth.pytests/test_devices.pytests/test_errors.pytests/test_power.pyuv.lock
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 6m 43s | $3.86 | claude-sonnet-5, claude-opus-5 | cr 0.10.286
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | security:code-auditor, architecture:solid, automation:ci-deploy, documentation:docs |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 6m 43s wall · 10m 11s compute |
| Cost | $3.86 |
| Tokens | 170 in / 45.3k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 6
- Out: 3.4k
- Cache read: 68.5k
- Cache create: 20.3k
- Cost: $0.19
- Duration: 36s
security:code-auditor— claude-sonnet-5- In: 28
- Out: 6.1k
- Cache read: 522.9k
- Cache create: 50.9k
- Cost: $0.56
- Duration: 1m 19s
architecture:solid— claude-opus-5- In: 76
- Out: 23.5k
- Cache read: 1.7M
- Cache create: 61.3k
- Cost: $2.07
- Duration: 5m 38s
automation:ci-deploy— claude-sonnet-5- In: 34
- Out: 6.5k
- Cache read: 607.6k
- Cache create: 24.0k
- Cost: $0.42
- Duration: 1m 13s
documentation:docs— claude-sonnet-5- In: 20
- Out: 3.9k
- Cache read: 381.0k
- Cache create: 31.5k
- Cost: $0.36
- Duration: 56s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 1.9k
- Cache read: 97.4k
- Cache create: 32.9k
- Cost: $0.26
- Duration: 26s
| self._devices = await asyncio.to_thread(asyncio.run, self._manager.get_devices()) | ||
| except (TPLinkCloudError, TimeoutError): | ||
| raise | ||
| except Exception as exc: |
There was a problem hiding this comment.
U-L2 — the catch-all relabels this service's own bugs as an upstream outage, silently. except Exception as exc: raise TPLinkCloudError(f"TP-Link cloud is unreachable: {exc}") converts any non-TPLinkCloudError failure into a 502 via the handler in app/errors.py:64. A TypeError/AttributeError/KeyError — exactly the failure mode the private-attribute coupling above makes likely on a library bump — is reported to the client as "the TP-Link cloud is unreachable" and never logged, so the operator sees a vendor outage where the real cause is this repo. tests/test_errors.py:32 pins the intended behavior (ConnectionError → 502), which is right; the problem is the width of the net.
Narrow it to the transport-level exceptions actually being mapped (ConnectionError, OSError, and whatever the library's requests layer surfaces), and logger.exception(...) before wrapping so the original traceback survives regardless.
Reply inline to this comment.
| | `DEVICE_CACHE_TTL` | `60` | Seconds a token's device list is cached before refetching | | ||
| | `CLOUD_TIMEOUT_SECONDS` | `30` | Ceiling for any fan-out to the TP-Link cloud | | ||
| | `CORS_ORIGINS` | `[]` | JSON list of allowed origins; empty disables CORS (same-origin deployments behind a reverse proxy don't need it) | | ||
| | `AUTH_ERROR_CODES` | `[-20651]` | TP-Link error codes treated as auth failures (401) | |
There was a problem hiding this comment.
AUTH_ERROR_CODES (default [-20651]) is not a real setting: app/settings.py has no such field, and app/errors.py maps auth failures via typed exceptions (TPLinkAuthError, TPLinkTokenExpiredError, TPLinkMFARequiredError) from tplink-cloud-api v5, not error-code matching. This is leftover from the old (2021) service and will send a deployer looking for an env var that does nothing. Remove the row, or replace it with the actual override that exists: TPLINK_CLOUD_API_HOST (tplink_cloud_api_host in Settings, default None, used only for testing against a non-default cloud host).
Reply inline to this comment.
| manager.set_auth_token(blob["kasa_token"]) | ||
| if blob.get("kasa_host"): | ||
| # Restore the regional host discovered at login (no public setter) | ||
| manager._kasa_api.host = blob["kasa_host"] |
There was a problem hiding this comment.
U-D1 / U-G1 — the service reaches into the third-party library's private state. _create_session and login depend on manager._kasa_api.host (lines 121, 142), manager._tapo_api (113, 143, 147) and manager._tapo_token (145). These are collaborators obtained by reaching through the dependency rather than being handed over through its contract, so the session's wiring is invisible to the library's own compatibility guarantees: a patch release that renames _kasa_api or stops setting _tapo_token breaks token restoration with no signal from this repo's tests. tests/fakes.py FakeManager mirrors those same private names, so the fakes will keep passing after the real library drifts — the test double encodes the private shape instead of a contract.
The comments ("no public getter", "no public setter") name the right fix: the library is owned by the same author (the PR already references piekstra/tplink-cloud-api#117), so add get_api_host()/set_api_host() and a set_tapo_auth(token, host) upstream and depend on those. If that must wait, at minimum (a) pin a compatible-release upper bound (see the pyproject finding) and (b) add one test that constructs a real TPLinkDeviceManager without credentials and asserts these attributes exist, so a library bump fails CI rather than production.
Reply inline to this comment.
| | `CLOUD_TIMEOUT_SECONDS` | `30` | Ceiling for any fan-out to the TP-Link cloud | | ||
| | `CORS_ORIGINS` | `[]` | JSON list of allowed origins; empty disables CORS (same-origin deployments behind a reverse proxy don't need it) | | ||
| | `AUTH_ERROR_CODES` | `[-20651]` | TP-Link error codes treated as auth failures (401) | | ||
| | `TPLINK_API_URL` | `https://wap.tplinkcloud.com` | TP-Link cloud endpoint | |
There was a problem hiding this comment.
TPLINK_API_URL with default https://wap.tplinkcloud.com does not exist in app/settings.py; the only related field is tplink_cloud_api_host, which defaults to None (uses the tplink-cloud-api library's own defaults, not a hardcoded URL). Setting TPLINK_API_URL as documented would silently have no effect. Fix the row to match the real field name/default, or drop it if it's not meant to be a user-facing override.
Reply inline to this comment.
| "pydantic-settings>=2.7", | ||
| "python-multipart>=0.0.20", | ||
| "cachetools>=5.5", | ||
| "tplink-cloud-api>=4.2.0", |
There was a problem hiding this comment.
U-G1 — the declared dependency floor contradicts the API the code requires. tplink-cloud-api>=4.2.0, but this diff imports TPLinkAuthError, TPLinkMFARequiredError, TPLinkTokenExpiredError (app/errors.py:3-8) and uses the v5-only dual-cloud surface include_tapo= / get_tapo_token() / _tapo_api (app/services/tplink_service.py:113, 123, 137). uv.lock resolves 5.2.0, so lockfile-based installs (Docker, CI) work; anything that installs from the manifest instead — pip install ., a downstream re-resolve, or a resolver pinned lower for another constraint — can legally pick 4.x and fail at import time. The manifest is the contract, and it currently understates it.
Raise the floor to the release that actually carries this API (>=5.2.0), and given the reliance on library private attributes noted separately, add an upper bound (>=5.2.0,<6) so a major bump is a deliberate change rather than a silent one.
Reply inline to this comment.
| @@ -1,5 +1,3 @@ | |||
| from .tplink_service import TPLinkService | |||
| from .tplink_service import TPLinkGateway, TPLinkSession, jsonify | |||
There was a problem hiding this comment.
U-G1 — new public surface with no consumer. This barrel re-exports TPLinkGateway, TPLinkSession and jsonify, but nothing imports from app.services: app/dependencies.py:6, app/main.py:10, tests/conftest.py:5 and tests/test_devices.py:143 all import from app.services.tplink_service directly. jsonify in particular is a module-internal serialization helper being promoted to package API. Either drop the re-exports and let the module be the import path, or switch the existing importers to the barrel so the declared surface has a real consumer.
Reply inline to this comment.
| # One sys_info call per online physical device; children derive their | ||
| # state from the parent's response. | ||
| async with asyncio.timeout(self._settings.cloud_timeout_seconds): | ||
| results = await asyncio.gather( |
There was a problem hiding this comment.
U-S3 — the effect-offloading rule is applied inconsistently, and the inconsistency is invisible at the call sites. load_devices states the operative fact about this dependency: "get_devices() has an async signature but fetches the cloud device list with blocking requests internally" (lines 164-166), and offloads it with asyncio.to_thread(asyncio.run, ...). Every other cloud call in this class is then awaited directly on the event loop: the sys_info fan-out here (200-203), get_sys_info/get_net_info in device_detail (269-271), is_on/power_on/power_off in set_power (317-322), and the three _power_tools calls (335, 340, 345).
If those share the library's blocking transport, the asyncio.gather at line 200 buys no concurrency at all — the N calls run serially and stall the whole process, so one GET /devices on a 30-device account freezes every other request for the duration; the 30s cloud_timeout_seconds ceiling becomes a whole-server stall budget. This is a close call and I am not blocking on it: the falsifier is simple — if TPLinkDeviceDataProvider/the device-level calls use an async HTTP client while only the manager's device-list path is blocking, the split is correct and the only thing missing is a comment saying so at line 199.
Whichever way it resolves, make the rule explicit and uniform: one documented seam (a small _offload() helper or a note naming which library calls are genuinely async) so the next maintainer does not have to guess per call site. Related: asyncio.to_thread is not cancellable, so when the asyncio.timeout at line 169 fires, the worker thread and the event loop asyncio.run created inside it keep running to completion — repeated timeouts occupy the default thread pool rather than freeing it.
Reply inline to this comment.
| include_tapo=bool(blob.get("tapo_token")), | ||
| ) | ||
| manager.set_auth_token(blob["kasa_token"]) | ||
| if blob.get("kasa_host"): |
There was a problem hiding this comment.
Server-Side Request Forgery via client-controlled, unsigned session token. The bearer token is opaque base64url JSON with no signature (app/token_blob.py), and the client fully controls its contents since it is never validated against anything the server minted itself beyond requiring a kasa_token key to be present (decode_session_token only checks base64/JSON decoding and dict-with-key shape). In _create_session (app/services/tplink_service.py:139-147), blob["kasa_host"] and blob["tapo_host"] are written straight into manager._kasa_api.host / manager._tapo_api.host, which the tplinkcloud library then uses as the destination host for outbound HTTP calls (login-restore, get_devices, sys_info, power control) on every request that uses that token. An attacker can forge a token such as base64url({"kasa_token":"x","kasa_host":"http://169.254.169.254"}) (or any internal/attacker-controlled host) and call GET /devices with it as the bearer token; the service will then issue outbound requests to that attacker-chosen host, carrying the token's kasa_token/tapo_token values as credentials. This is a concrete SSRF into internal infrastructure (cloud metadata endpoints, internal admin services, etc.), and doubles as a credential-exfiltration primitive since an attacker who has stolen or is replaying a real victim kasa_token can redirect it to an attacker-controlled collector by only changing kasa_host (the token has no signature binding the host to the token issuer). It is also not fully blind: load_devices wraps any transport exception into TPLinkCloudError(f"...: {exc}"), which errors.py returns verbatim to the client as the 502 body, giving an attacker connectivity/error-message oracle for the SSRF target (e.g. distinguishing open vs. closed internal ports, DNS failures, TLS errors). Fix: sign/HMAC the session token (or bind kasa_host/tapo_host to a server-side allowlist of TP-Link's known regional API domains) so a client cannot supply an arbitrary destination host that the server will make credentialed outbound requests to, and avoid echoing raw upstream exception text in the 502 response body.
Reply inline to this comment.
| if session is None: | ||
| session = await factory() | ||
| self._cache[key] = session | ||
| self._locks.pop(key, None) |
There was a problem hiding this comment.
U-D1 — _locks is unbounded mutable state fed by unauthenticated input, and it leaks whenever the factory raises. The session map is deliberately bounded (TTLCache(maxsize=...)), but the parallel _locks dict has no bound and no TTL, and the only cleanup (self._locks.pop(key, None), line 36) sits inside the async with lock body after await factory(). When the factory raises, the exception propagates and that line never runs.
That path is reachable pre-auth: get_session → gateway.session(token) → _create_session → decode_session_token raises InvalidServiceTokenError for any malformed bearer token (app/token_blob.py:32). Every distinct garbage token therefore adds a permanent entry keyed by its digest — unbounded growth in a long-lived process, driven by anonymous callers, with no eviction path since the lock is only ever popped on success.
Fix: move the cleanup into a finally: so the lock is released on both paths, e.g.
lock = self._locks.setdefault(key, asyncio.Lock())
try:
async with lock:
session = self._cache.get(key)
if session is None:
session = await factory()
self._cache[key] = session
finally:
self._locks.pop(key, None)
return sessionand cover it with a test that fires two concurrent requests carrying the same invalid token and asserts len(cache._locks) == 0 afterwards.
Reply inline to this comment.
|
|
||
| app = FastAPI(title="TP-Link Kasa API Service", version="2.0.0") | ||
| app.state.settings = settings | ||
| app.state.gateway = TPLinkGateway(settings) |
There was a problem hiding this comment.
U-D2 — the composition root has no seam for its main collaborator, so consumers wire around it. create_app is otherwise a clean single composition root, but it constructs TPLinkGateway(settings) itself with no way to pass one in. The consequence is visible in tests/conftest.py:60-61, which calls create_app(settings) and then reassigns application.state.gateway afterwards — the graph is built twice and the real gateway (with its SessionCache) is created and discarded on every test app. That also means the test path exercises a differently-wired app than production.
Fix is one parameter: def create_app(settings: Settings | None = None, gateway: TPLinkGateway | None = None) with app.state.gateway = gateway or TPLinkGateway(settings). Tests then pass TPLinkGateway(settings, manager_factory=fleet.factory) in, and the root stays the only place the graph is assembled. (app.dependency_overrides[get_gateway] is the equally idiomatic FastAPI alternative.)
Reply inline to this comment.
…pling Blocking security fix: - The session token's regional API host became the outbound request destination; an unsigned, client-supplied token could point it at an internal/attacker host (SSRF + credential exfil). Validate the host against an allowlist of TP-Link domains (https only) at both mint and restore; reject otherwise with 401. Added SSRF tests incl. the metadata endpoint. Also from review: - cache: release the per-key lock in finally so a raising factory (e.g. an invalid token from an anonymous caller) can't grow _locks without bound - narrow load_devices' catch-all to transport errors; log the traceback and stop echoing raw upstream text into the 502 body (removes an SSRF oracle and stops relabeling our own bugs as a vendor outage) - device_detail now builds its shared fields through _summarize, so the list and detail endpoints can't disagree about is_on/rssi - pin tplink-cloud-api>=5.2.0,<6 (code needs the v5 API; bound the internals coupling) - create_app takes an injectable gateway; tests use it instead of reassigning - drop the unused app.services barrel - guard test asserts the library internals we depend on still exist - README: fix stale AUTH_ERROR_CODES/TPLINK_API_URL rows, add cache-size + host-allowlist document the token as unsigned + host-allowlisted, not merely 'opaque'
|
Thanks — thorough pass, and the SSRF finding was a real one. All findings addressed in b7f8c7d (38 tests, +9). Blocking — SSRF via session-token host ( Major — Major — private-attribute coupling ( Major — dependency floor ( Major — offloading consistency ( Minor — all applied: Docs — all applied: removed the stale |
|
⏱️ Automated review timed out after 30 minutes, so no review was posted. Ways to get this PR reviewed:
|
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: b7f8c7d6bce5
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| security:code-auditor | 0 |
| architecture:solid | 3 |
| automation:ci-deploy | 0 |
| documentation:docs | 0 |
architecture:solid (3 findings)
Minor - app/services/tplink_service.py:63
U-L1 — the allowlist's matching rule is weaker than its documented contract, and only the default value hides it. The docstring promises "allow only https TP-Link domains", but the match is a raw
hostname.endswith(suffix)with no label boundary; the boundary comes solely from the caller remembering to write a leading dot. The shipped default[".tplinkcloud.com"](app/settings.py:11) is safe, and the suffix-spoof case is tested (tests/test_security.py:25). Butallowed_cloud_host_suffixesis operator-configurable and its comment invites widening ("Widen only to add a test host"), so an operator who adds"wap.tplinkcloud.com"or"test.local"— the natural way to name a host, without a leading dot — silently also allowsevilwap.tplinkcloud.com/eviltest.local. A setting whose safety depends on undocumented punctuation is a contract the next maintainer will break.Fix inside
_host_allowed: normalize each entry rather than trusting its shape, e.g.entry = suffix.lower().lstrip('.')thenhostname == entry or hostname.endswith('.' + entry). That makes the two forms equivalent and the boundary structural. Worth a parametrized case with a dot-less allowlist entry to pin it.
Minor - app/services/tplink_service.py:149
U-L2 — one error type is now carrying two unrelated meanings, and on the mint path it tells the client a lie.
_validated_hostraisesInvalidServiceTokenError, whose documented meaning is "The bearer token is not one this service minted" (app/errors.py:11-15) and whose handler answers 401 with"Unrecognized bearer token; log in again". On the restore path (line 170, 175) that is exactly right. On the mint path (lines 131, 133) it is not: there is no bearer token in play — the caller just POSTed credentials, TP-Link accepted them (possibly consuming an MFA code), and the anomaly is that the vendor returned a regional host outside the allowlist. The client is handed a 401 telling it to log in again, which will fail identically every time, and nothing is logged, so the operator sees an auth failure rather than "TP-Link handed us an unexpected host" — the one signal that would explain it.This is a fresh consequence of the SSRF fix, not a pre-existing issue. Concrete fix: give the mint path its own error (e.g.
UntrustedCloudHostError, mapped to 502) plus alogger.error("TP-Link returned a non-allowlisted API host: %s", host), and keepInvalidServiceTokenErrorfor the restore path where the name is accurate. If you prefer not to add a type, raisingTPLinkCloudErrorat mint already maps to 502 through the existing handler.Related U-T1: tests/test_security.py:19-38 parametrizes only the restore direction. The mint-time branch at lines 131/133 has no test, so whichever status it ends up returning is currently unpinned — a fleet fixture whose
FakeApi.hostis off-allowlist would cover it in three lines.
Minor - app/services/tplink_service.py:201
U-S3 — the offloading seam is now documented (good), but the timeout around it still cannot do what it looks like it does. The new comment at lines 192-198 resolves the earlier ambiguity about which library calls block and why only this one is pushed to a thread — that part is settled. What remains is that
asyncio.to_threadis not cancellable: when theasyncio.timeoutat line 200 fires, only the awaiting task unwinds. The worker thread keeps runningasyncio.run(get_devices())— its own nested event loop and blockingrequestscall included — until the cloud answers or the socket gives up, which with no explicit socket timeout in the library can be far longer thancloud_timeout_seconds. Under a slow-cloud episode each timed-out request permanently occupies a slot in the default executor (min(32, cpu+4)threads) for the remainder of its natural life, so the 504s can cascade into a starved thread pool rather than a bounded per-request failure.The honest fix is a transport-level deadline the thread itself observes — pass a request timeout to the library if v5 accepts one, or run the fetch on a dedicated bounded
ThreadPoolExecutorvialoop.run_in_executorso exhaustion is explicit and observable. At minimum, note in the comment that the timeout bounds the response, not the thread, so nobody later reads it as a resource guarantee.
Reviewer Coverage
security:code-auditor— complete (constrained); inspected 29 assigned files (47 inspected across reviewers):app/__init__.py,app/cache.py,app/configuration/__init__.py,app/configuration/configuration.py,app/dependencies.py,app/errors.py,app/models/device_response.py,app/models/device_sys_info.py,app/models/devices_power_current_response.py,app/models/devices_power_day_response.py,app/models/devices_power_month_response.py,app/models/user.py,app/routers/device.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/tplink_service.py,app/settings.py,app/token_blob.py,tests/conftest.py,tests/fakes.py,tests/local_env_vars.py,tests/test_api.py,tests/test_auth.py,tests/test_cache.py,tests/test_devices.py,tests/test_errors.py,tests/test_power.py,tests/test_security.py; skipped: none; constraints: nonearchitecture:solid—⚠️ incomplete (skipped files); inspected 13 assigned files (47 inspected across reviewers):app/cache.py,app/dependencies.py,app/errors.py,app/main.py,app/models/__init__.py,app/models/devices.py,app/models/power.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/__init__.py,app/services/tplink_service.py,pyproject.toml; skipped:app/routers/device.py; constraints: Could not run the project's verification (uv sync / pytest / ruff): dependency installation needs network access this sandbox blocks. Findings are from static reading of the head checkout. Prior-round findings that this head fixes (pyproject floor, lock leak, gateway injection, catch-all wrapping, DeviceDetail duplication, unused services barrel, private-attr contract test) were re-verified and are not re-filed. The tplinkcloud v5 package is not installed here, so the comment at tplink_service.py:192-198 claiming device commands use aiohttp (and only login/device-list block) could not be verified against library source; I accepted the author's live verification. app/settings.py, app/token_blob.py and the tests are not in changed_files, so findings that reference them are anchored in app/services/tplink_service.py instead. git commands were blocked, so base-branch contents and a real diff were unavailable. New-vs-pre-existing lines were judged from the change map and from the previous review round's threads on the earlier head.automation:ci-deploy— complete (constrained); inspected 12 assigned files (47 inspected across reviewers):.dockerignore,.github/workflows/ci.yml,.github/workflows/heroku-app-logs.yml,.github/workflows/package-publish-heroku.yml,.github/workflows/package-publish.yml,.github/workflows/package-python.yml,.github/workflows/release.yml,Dockerfile,app/requirements.txt,pyproject.toml,requirements.txt,uv.lock; skipped: none; constraints: nonedocumentation:docs— complete (constrained); inspected 1 assigned file (47 inspected across reviewers):README.md; skipped: none; constraints: none
Inspected files (47)
.dockerignore.github/workflows/ci.yml.github/workflows/heroku-app-logs.yml.github/workflows/package-publish-heroku.yml.github/workflows/package-publish.yml.github/workflows/package-python.yml.github/workflows/release.ymlDockerfileREADME.mdapp/__init__.pyapp/cache.pyapp/configuration/__init__.pyapp/configuration/configuration.pyapp/dependencies.pyapp/errors.pyapp/main.pyapp/models/__init__.pyapp/models/device_response.pyapp/models/device_sys_info.pyapp/models/devices.pyapp/models/devices_power_current_response.pyapp/models/devices_power_day_response.pyapp/models/devices_power_month_response.pyapp/models/power.pyapp/models/user.pyapp/requirements.txtapp/routers/device.pyapp/routers/device_power.pyapp/routers/devices.pyapp/routers/user.pyapp/services/__init__.pyapp/services/tplink_service.pyapp/settings.pyapp/token_blob.pypyproject.tomlrequirements.txttests/conftest.pytests/fakes.pytests/local_env_vars.pytests/test_api.pytests/test_auth.pytests/test_cache.pytests/test_devices.pytests/test_errors.pytests/test_power.pytests/test_security.pyuv.lock
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 07s | $4.35 | claude-sonnet-5, claude-opus-5 | cr 0.10.286
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | security:code-auditor, architecture:solid, automation:ci-deploy, documentation:docs |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 5m 07s wall · 5m 37s compute |
| Cost | $4.35 |
| Tokens | 106 in / 22.7k out |
Per-workstream usage
security:code-auditor— claude-sonnet-5- In: 18
- Out: 3.5k
- Cache read: 536.2k
- Cache create: 54.5k
- Cost: $0.54
- Duration: 47s
architecture:solid— claude-opus-5- In: 52
- Out: 14.1k
- Cache read: 2.6M
- Cache create: 107.9k
- Cost: $2.71
- Duration: 3m 34s
automation:ci-deploy— claude-sonnet-5- In: 20
- Out: 3.2k
- Cache read: 504.2k
- Cache create: 39.2k
- Cost: $0.43
- Duration: 41s
documentation:docs— claude-sonnet-5- In: 10
- Out: 1.4k
- Cache read: 258.8k
- Cache create: 47.1k
- Cost: $0.38
- Duration: 22s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 529
- Cache read: 118.0k
- Cache create: 39.6k
- Cost: $0.28
- Duration: 12s
| # typed errors (e.g. TPLinkTokenExpiredError -> 401). | ||
| try: | ||
| async with asyncio.timeout(self._settings.cloud_timeout_seconds): | ||
| self._devices = await asyncio.to_thread(asyncio.run, self._manager.get_devices()) |
There was a problem hiding this comment.
U-S3 — the offloading seam is now documented (good), but the timeout around it still cannot do what it looks like it does. The new comment at lines 192-198 resolves the earlier ambiguity about which library calls block and why only this one is pushed to a thread — that part is settled. What remains is that asyncio.to_thread is not cancellable: when the asyncio.timeout at line 200 fires, only the awaiting task unwinds. The worker thread keeps running asyncio.run(get_devices()) — its own nested event loop and blocking requests call included — until the cloud answers or the socket gives up, which with no explicit socket timeout in the library can be far longer than cloud_timeout_seconds. Under a slow-cloud episode each timed-out request permanently occupies a slot in the default executor (min(32, cpu+4) threads) for the remainder of its natural life, so the 504s can cascade into a starved thread pool rather than a bounded per-request failure.
The honest fix is a transport-level deadline the thread itself observes — pass a request timeout to the library if v5 accepts one, or run the fetch on a dedicated bounded ThreadPoolExecutor via loop.run_in_executor so exhaustion is explicit and observable. At minimum, note in the comment that the timeout bounds the response, not the thread, so nobody later reads it as a resource guarantee.
Reply inline to this comment.
| return False | ||
| hostname = parsed.hostname.lower() | ||
| return any( | ||
| hostname == suffix.lstrip(".") or hostname.endswith(suffix) for suffix in allowed_suffixes |
There was a problem hiding this comment.
U-L1 — the allowlist's matching rule is weaker than its documented contract, and only the default value hides it. The docstring promises "allow only https TP-Link domains", but the match is a raw hostname.endswith(suffix) with no label boundary; the boundary comes solely from the caller remembering to write a leading dot. The shipped default [".tplinkcloud.com"] (app/settings.py:11) is safe, and the suffix-spoof case is tested (tests/test_security.py:25). But allowed_cloud_host_suffixes is operator-configurable and its comment invites widening ("Widen only to add a test host"), so an operator who adds "wap.tplinkcloud.com" or "test.local" — the natural way to name a host, without a leading dot — silently also allows evilwap.tplinkcloud.com / eviltest.local. A setting whose safety depends on undocumented punctuation is a contract the next maintainer will break.
Fix inside _host_allowed: normalize each entry rather than trusting its shape, e.g. entry = suffix.lower().lstrip('.') then hostname == entry or hostname.endswith('.' + entry). That makes the two forms equivalent and the boundary structural. Worth a parametrized case with a dot-less allowlist entry to pin it.
Reply inline to this comment.
| if host is None: | ||
| return None | ||
| if not _host_allowed(host, self._settings.allowed_cloud_host_suffixes): | ||
| raise InvalidServiceTokenError() |
There was a problem hiding this comment.
U-L2 — one error type is now carrying two unrelated meanings, and on the mint path it tells the client a lie. _validated_host raises InvalidServiceTokenError, whose documented meaning is "The bearer token is not one this service minted" (app/errors.py:11-15) and whose handler answers 401 with "Unrecognized bearer token; log in again". On the restore path (line 170, 175) that is exactly right. On the mint path (lines 131, 133) it is not: there is no bearer token in play — the caller just POSTed credentials, TP-Link accepted them (possibly consuming an MFA code), and the anomaly is that the vendor returned a regional host outside the allowlist. The client is handed a 401 telling it to log in again, which will fail identically every time, and nothing is logged, so the operator sees an auth failure rather than "TP-Link handed us an unexpected host" — the one signal that would explain it.
This is a fresh consequence of the SSRF fix, not a pre-existing issue. Concrete fix: give the mint path its own error (e.g. UntrustedCloudHostError, mapped to 502) plus a logger.error("TP-Link returned a non-allowlisted API host: %s", host), and keep InvalidServiceTokenError for the restore path where the name is accurate. If you prefer not to add a type, raising TPLinkCloudError at mint already maps to 502 through the existing handler.
Related U-T1: tests/test_security.py:19-38 parametrizes only the restore direction. The mint-time branch at lines 131/133 has no test, so whichever status it ends up returning is currently unpinned — a fleet fixture whose FakeApi.host is off-allowlist would cover it in three lines.
Reply inline to this comment.
- _host_allowed normalizes each allowlist entry (strip leading dot, match on the label boundary) so a bare-domain entry can't allow a lookalike host; added a boundary test with a dot-less entry - a bad regional host on the mint path now raises TPLinkCloudError (502) rather than InvalidServiceTokenError (401): the user just authenticated, so an unexpected host is a cloud anomaly, not a bad bearer token. Added a test. - document that the asyncio.timeout bounds the response, not the worker thread (to_thread isn't cancellable); the thread is bounded instead by the library's own ~15s per-request socket timeout, so a slow cloud frees the pool slot on that timescale rather than hanging
|
Thanks — SSRF confirmed closed, and these three are all fair. Addressed in the latest push (tests 38 → 40):
|
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 7ff8972bda1b
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| security:code-auditor | 0 |
| architecture:solid | 0 |
| automation:ci-deploy | 0 |
| documentation:docs | 0 |
Reviewer Coverage
security:code-auditor— complete (constrained); inspected 29 assigned files (47 inspected across reviewers):app/__init__.py,app/cache.py,app/configuration/__init__.py,app/configuration/configuration.py,app/dependencies.py,app/errors.py,app/models/device_response.py,app/models/device_sys_info.py,app/models/devices_power_current_response.py,app/models/devices_power_day_response.py,app/models/devices_power_month_response.py,app/models/user.py,app/routers/device.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/tplink_service.py,app/settings.py,app/token_blob.py,tests/conftest.py,tests/fakes.py,tests/local_env_vars.py,tests/test_api.py,tests/test_auth.py,tests/test_cache.py,tests/test_devices.py,tests/test_errors.py,tests/test_power.py,tests/test_security.py; skipped: none; constraints: nonearchitecture:solid—⚠️ incomplete (skipped files); inspected 13 assigned files (47 inspected across reviewers):app/cache.py,app/dependencies.py,app/errors.py,app/main.py,app/models/__init__.py,app/models/devices.py,app/models/power.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/__init__.py,app/services/tplink_service.py,pyproject.toml; skipped:app/routers/device.py; constraints: Also re-verified fixed: host-allowlist label boundary (tplink_service.py:62-69 + boundary test) and the InvalidServiceTokenError overload at mint, now TPLinkCloudError -> 502 with a test (tests/test_auth.py:35). Could not run the project's verification (uv sync / pytest / ruff): dependency installation needs network access this sandbox blocks. Findings are from static reading of the head checkout at 7ff8972. Re-verified as fixed at this head and not re-filed: pyproject floor+upper bound, cache lock leak (try/finally + tests/test_cache.py), create_app gateway injection, narrowed and logged error wrapping, DeviceDetail single producer, emptied services barrel. The to_thread cancellation gap I raised last round was closed the documentation way (lines 209-213) rather than with a dedicated executor. That was an available option I offered; I am not re-litigating the author's choice. git commands were blocked, so no real diff was available. New-vs-pre-existing lines were judged from the change map and from this reviewer's threads on the two earlier heads. tplinkcloud v5 is not installed here, so two comments asserting library behavior could not be verified: device commands using aiohttp (tplink_service.py:201-207) and the ~15s socket timeout that now bounds the worker thread (lines 209-213).automation:ci-deploy— complete (constrained); inspected 12 assigned files (47 inspected across reviewers):.dockerignore,.github/workflows/ci.yml,.github/workflows/heroku-app-logs.yml,.github/workflows/package-publish-heroku.yml,.github/workflows/package-publish.yml,.github/workflows/package-python.yml,.github/workflows/release.yml,Dockerfile,app/requirements.txt,pyproject.toml,requirements.txt,uv.lock; skipped: none; constraints: nonedocumentation:docs— complete (constrained); inspected 1 assigned file (47 inspected across reviewers):README.md; skipped: none; constraints: none
Inspected files (47)
.dockerignore.github/workflows/ci.yml.github/workflows/heroku-app-logs.yml.github/workflows/package-publish-heroku.yml.github/workflows/package-publish.yml.github/workflows/package-python.yml.github/workflows/release.ymlDockerfileREADME.mdapp/__init__.pyapp/cache.pyapp/configuration/__init__.pyapp/configuration/configuration.pyapp/dependencies.pyapp/errors.pyapp/main.pyapp/models/__init__.pyapp/models/device_response.pyapp/models/device_sys_info.pyapp/models/devices.pyapp/models/devices_power_current_response.pyapp/models/devices_power_day_response.pyapp/models/devices_power_month_response.pyapp/models/power.pyapp/models/user.pyapp/requirements.txtapp/routers/device.pyapp/routers/device_power.pyapp/routers/devices.pyapp/routers/user.pyapp/services/__init__.pyapp/services/tplink_service.pyapp/settings.pyapp/token_blob.pypyproject.tomlrequirements.txttests/conftest.pytests/fakes.pytests/local_env_vars.pytests/test_api.pytests/test_auth.pytests/test_cache.pytests/test_devices.pytests/test_errors.pytests/test_power.pytests/test_security.pyuv.lock
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 41s | $4.33 | claude-sonnet-5, claude-opus-5 | cr 0.10.286
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | security:code-auditor, architecture:solid, automation:ci-deploy, documentation:docs |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 41s wall · 3m 15s compute |
| Cost | $4.33 |
| Tokens | 70 in / 12.7k out |
Per-workstream usage
security:code-auditor— claude-sonnet-5- In: 12
- Out: 1.9k
- Cache read: 443.0k
- Cache create: 73.5k
- Cost: $0.60
- Duration: 25s
architecture:solid— claude-opus-5- In: 26
- Out: 7.0k
- Cache read: 1.7M
- Cache create: 140.8k
- Cost: $2.44
- Duration: 1m 48s
automation:ci-deploy— claude-sonnet-5- In: 16
- Out: 2.3k
- Cache read: 503.1k
- Cache create: 53.5k
- Cost: $0.51
- Duration: 31s
documentation:docs— claude-sonnet-5- In: 10
- Out: 1.2k
- Cache read: 319.6k
- Cache create: 59.6k
- Cost: $0.47
- Duration: 19s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 436
- Cache read: 129.3k
- Cache create: 44.1k
- Cost: $0.31
- Duration: 9s
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 7ff8972bda1b
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| security:code-auditor | 0 |
| architecture:solid | 0 |
| automation:ci-deploy | 0 |
| documentation:docs | 0 |
Reviewer Coverage
security:code-auditor— complete (constrained); inspected 29 assigned files (47 inspected across reviewers):app/__init__.py,app/cache.py,app/configuration/__init__.py,app/configuration/configuration.py,app/dependencies.py,app/errors.py,app/models/device_response.py,app/models/device_sys_info.py,app/models/devices_power_current_response.py,app/models/devices_power_day_response.py,app/models/devices_power_month_response.py,app/models/user.py,app/routers/device.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/tplink_service.py,app/settings.py,app/token_blob.py,tests/conftest.py,tests/fakes.py,tests/local_env_vars.py,tests/test_api.py,tests/test_auth.py,tests/test_cache.py,tests/test_devices.py,tests/test_errors.py,tests/test_power.py,tests/test_security.py; skipped: none; constraints: nonearchitecture:solid—⚠️ incomplete (skipped files); inspected 13 assigned files (47 inspected across reviewers):app/cache.py,app/dependencies.py,app/errors.py,app/main.py,app/models/__init__.py,app/models/devices.py,app/models/power.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/__init__.py,app/services/tplink_service.py,pyproject.toml; skipped:app/routers/device.py; constraints: Also verified fixed: DeviceDetail now built through the single _summarize producer (tplink_service.py:341-346), emptied app/services/init.py, allowlist label boundary (tplink_service.py:62-69), mint-path error split to TPLinkCloudError/502 (line 150-158). Could not run the project's verification (uv sync / pytest / ruff): dependency installation needs network access this sandbox blocks. Findings are from static reading of the head checkout at 7ff8972. The dossier lists most architecture threads as Unresolved, but those threads were opened against ecf3e68/b7f8c7d. Re-read at this head, the code they describe has changed; verified fixes are listed below rather than re-filed. The to_thread cancellation gap was closed by documenting the library's socket timeout as the real bound (lines 209-213) instead of a dedicated executor. That was an option I offered last round; I am not re-litigating the author's choice. Verified fixed here: pyproject floor+upper bound (line 19), cache lock try/finally (cache.py:31-41) with tests/test_cache.py, create_app gateway injection (main.py:17,25), narrowed+logged error wrapping (tplink_service.py:217-228). git commands were blocked, so no real diff was available. New-vs-pre-existing lines were judged from the change map and from this reviewer's own threads on the two earlier heads of this PR. tplinkcloud v5 is not installed here, so two comments asserting library behavior could not be verified: device commands using aiohttp (tplink_service.py:201-207) and the ~15s socket timeout said to bound the worker thread (lines 209-213).automation:ci-deploy— complete (constrained); inspected 12 assigned files (47 inspected across reviewers):.dockerignore,.github/workflows/ci.yml,.github/workflows/heroku-app-logs.yml,.github/workflows/package-publish-heroku.yml,.github/workflows/package-publish.yml,.github/workflows/package-python.yml,.github/workflows/release.yml,Dockerfile,app/requirements.txt,pyproject.toml,requirements.txt,uv.lock; skipped: none; constraints: nonedocumentation:docs— complete (constrained); inspected 1 assigned file (47 inspected across reviewers):README.md; skipped: none; constraints: none
Inspected files (47)
.dockerignore.github/workflows/ci.yml.github/workflows/heroku-app-logs.yml.github/workflows/package-publish-heroku.yml.github/workflows/package-publish.yml.github/workflows/package-python.yml.github/workflows/release.ymlDockerfileREADME.mdapp/__init__.pyapp/cache.pyapp/configuration/__init__.pyapp/configuration/configuration.pyapp/dependencies.pyapp/errors.pyapp/main.pyapp/models/__init__.pyapp/models/device_response.pyapp/models/device_sys_info.pyapp/models/devices.pyapp/models/devices_power_current_response.pyapp/models/devices_power_day_response.pyapp/models/devices_power_month_response.pyapp/models/power.pyapp/models/user.pyapp/requirements.txtapp/routers/device.pyapp/routers/device_power.pyapp/routers/devices.pyapp/routers/user.pyapp/services/__init__.pyapp/services/tplink_service.pyapp/settings.pyapp/token_blob.pypyproject.tomlrequirements.txttests/conftest.pytests/fakes.pytests/local_env_vars.pytests/test_api.pytests/test_auth.pytests/test_cache.pytests/test_devices.pytests/test_errors.pytests/test_power.pytests/test_security.pyuv.lock
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 07s | $5.37 | claude-sonnet-5, claude-opus-5 | cr 0.10.286
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | security:code-auditor, architecture:solid, automation:ci-deploy, documentation:docs |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 07s wall · 2m 54s compute |
| Cost | $5.37 |
| Tokens | 70 in / 12.0k out |
Per-workstream usage
security:code-auditor— claude-sonnet-5- In: 6
- Out: 1.1k
- Cache read: 218.6k
- Cache create: 85.7k
- Cost: $0.60
- Duration: 14s
architecture:solid— claude-opus-5- In: 40
- Out: 8.3k
- Cache read: 3.4M
- Cache create: 174.5k
- Cost: $3.64
- Duration: 1m 54s
automation:ci-deploy— claude-sonnet-5- In: 8
- Out: 1.1k
- Cache read: 259.4k
- Cache create: 65.3k
- Cost: $0.49
- Duration: 16s
documentation:docs— claude-sonnet-5- In: 10
- Out: 1.1k
- Cache read: 366.9k
- Cache create: 72.1k
- Cost: $0.56
- Duration: 19s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 400
- Cache read: 61.3k
- Cache create: 10.1k
- Cost: $0.09
- Duration: 8s
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 7ff8972bda1b
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| security:code-auditor | 0 |
| architecture:solid | 0 |
| automation:ci-deploy | 0 |
| documentation:docs | 0 |
Reviewer Coverage
security:code-auditor— complete (constrained); inspected 29 assigned files (47 inspected across reviewers):app/__init__.py,app/cache.py,app/configuration/__init__.py,app/configuration/configuration.py,app/dependencies.py,app/errors.py,app/models/device_response.py,app/models/device_sys_info.py,app/models/devices_power_current_response.py,app/models/devices_power_day_response.py,app/models/devices_power_month_response.py,app/models/user.py,app/routers/device.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/tplink_service.py,app/settings.py,app/token_blob.py,tests/conftest.py,tests/fakes.py,tests/local_env_vars.py,tests/test_api.py,tests/test_auth.py,tests/test_cache.py,tests/test_devices.py,tests/test_errors.py,tests/test_power.py,tests/test_security.py; skipped: none; constraints: nonearchitecture:solid— complete (constrained); inspected 13 assigned files (47 inspected across reviewers):app/cache.py,app/dependencies.py,app/errors.py,app/main.py,app/models/__init__.py,app/models/devices.py,app/models/power.py,app/routers/device_power.py,app/routers/devices.py,app/routers/user.py,app/services/__init__.py,app/services/tplink_service.py,pyproject.toml; skipped:app/routers/device.py; constraints: Also verified fixed: DeviceDetail built through the single _summarize producer (tplink_service.py:341-346); app/services/init.py is now empty; allowlist label boundary (tplink_service.py:62-69); mint-path error split to TPLinkCloudError/502 (lines 150-158). Could not run the project's verification (uv sync / pytest / ruff): dependency installation needs network access this sandbox blocks. Findings are from static reading of the head checkout at 7ff8972. The dossier still lists several architecture threads as Unresolved, but they were opened against ecf3e68/b7f8c7d. Re-read in this checkout, the code each describes has changed; verified fixes are listed below rather than re-filed as findings. The to_thread cancellation gap was closed by documenting the library's socket timeout as the real bound (lines 209-213) rather than a dedicated executor — an option offered in an earlier round. Not re-litigating the author's choice. Verified fixed here: pyproject floor+upper bound (line 19); cache lock try/finally (cache.py:31-41); create_app gateway injection (main.py:17,25); error wrapping narrowed to ConnectionError/OSError with logger.exception (tplink_service.py:217-228). git commands were blocked, so no real diff was available. New-vs-pre-existing lines were judged from the change map and from this reviewer's own threads on the earlier heads of this PR. tplinkcloud v5 is not installed here, so two comments asserting library behavior could not be verified: device commands using aiohttp (tplink_service.py:201-207) and the ~15s socket timeout said to bound the worker thread (lines 209-213).automation:ci-deploy— complete (constrained); inspected 12 assigned files (47 inspected across reviewers):.dockerignore,.github/workflows/ci.yml,.github/workflows/heroku-app-logs.yml,.github/workflows/package-publish-heroku.yml,.github/workflows/package-publish.yml,.github/workflows/package-python.yml,.github/workflows/release.yml,Dockerfile,app/requirements.txt,pyproject.toml,requirements.txt,uv.lock; skipped: none; constraints: nonedocumentation:docs— complete (constrained); inspected 1 assigned file (47 inspected across reviewers):README.md; skipped: none; constraints: none
Inspected files (47)
.dockerignore.github/workflows/ci.yml.github/workflows/heroku-app-logs.yml.github/workflows/package-publish-heroku.yml.github/workflows/package-publish.yml.github/workflows/package-python.yml.github/workflows/release.ymlDockerfileREADME.mdapp/__init__.pyapp/cache.pyapp/configuration/__init__.pyapp/configuration/configuration.pyapp/dependencies.pyapp/errors.pyapp/main.pyapp/models/__init__.pyapp/models/device_response.pyapp/models/device_sys_info.pyapp/models/devices.pyapp/models/devices_power_current_response.pyapp/models/devices_power_day_response.pyapp/models/devices_power_month_response.pyapp/models/power.pyapp/models/user.pyapp/requirements.txtapp/routers/device.pyapp/routers/device_power.pyapp/routers/devices.pyapp/routers/user.pyapp/services/__init__.pyapp/services/tplink_service.pyapp/settings.pyapp/token_blob.pypyproject.tomlrequirements.txttests/conftest.pytests/fakes.pytests/local_env_vars.pytests/test_api.pytests/test_auth.pytests/test_cache.pytests/test_devices.pytests/test_errors.pytests/test_power.pytests/test_security.pyuv.lock
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 48s | $6.75 | claude-sonnet-5, claude-opus-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | security:code-auditor, architecture:solid, automation:ci-deploy, documentation:docs |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 2m 48s wall · 2m 14s compute |
| Cost | $6.75 |
| Tokens | 60 in / 8.1k out |
Per-workstream usage
security:code-auditor— claude-sonnet-5- In: 6
- Out: 1.0k
- Cache read: 221.2k
- Cache create: 116.7k
- Cost: $0.78
- Duration: 14s
architecture:solid— claude-opus-5- In: 36
- Out: 5.3k
- Cache read: 3.5M
- Cache create: 216.1k
- Cost: $4.05
- Duration: 1m 23s
automation:ci-deploy— claude-sonnet-5- In: 6
- Out: 811
- Cache read: 95.0k
- Cache create: 180.8k
- Cost: $1.13
- Duration: 14s
documentation:docs— claude-sonnet-5- In: 6
- Out: 633
- Cache read: 192.6k
- Cache create: 101.3k
- Cost: $0.67
- Duration: 12s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 328
- Cache read: 70.4k
- Cache create: 14.6k
- Cost: $0.11
- Duration: 8s
2 participants
Summary
Full modernization of the 2021 service:
mfa_codeform fieldGET /devices(power-strip outlets flattened alongside parents, live is_on from one sys_info call per physical device, name/model/state filters),GET /devices/{id},POST /devices/{id}/power(on/off/toggle incl. strip children)GET /user/token, Heroku workflows, debug prints that logged device MACsVerification
Notes