Repository navigation
Security hardening: constant-time auth, limit clamping, handshake gate, SSRF guard, keychain token, wss - #213
Merged
Conversation
Findings from a security review of the whole codebase that can be fixed in code without touching the deployment: - Compare the bearer token in constant time instead of with `==`, which short-circuits on the first differing byte and leaks a timing signal on a publicly reachable endpoint. - Clamp every `limit` query parameter to the bounds declared in openapi.yaml; the generator parses but does not validate them, so a huge or negative limit could load whole tables into memory or produce invalid SQL. - Reject `call`/`reply` frames on the StarActorSystem link until the peer's `hello` arrived, closing the connection instead of executing them. The hello is the only protocol-version gate and documented as the first frame. - Only allow public https URLs for HealthCheck automations and stop logging the response body, so POST /config can no longer be used to reach internal services (db, cloudflared, metadata endpoints) from inside the container. - Make MySQL TLS certificate verification configurable via DATABASE_TLS_VERIFY (logged warning while off) and refuse to start release builds without DATABASE_PASSWORD instead of falling back to the well-known default. - Keep `skipValidation` on POST /config usable (also while the adapter is offline) but log every missing entity as a warning instead of accepting the config silently. - Store the adapter's server token in the keychain (AdapterCredentialStore) instead of UserDefaults, migrating existing installs and deleting the plaintext copy; KeychainHelper now reports write failures. - Let ServerAddress carry a TLS flag so the adapter can dial wss:// and the token never crosses the network in clear text. Stored raw values and the 8888 port migration keep working. Tests cover the constant-time compare, limit clamping, the handshake gate, ServerAddress parsing/encoding and the HealthCheck URL rules. Docs list the environment variables and the new adapter settings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QpC3PnuBWmyrYM3xpoSFr6
POST /config?skipValidation=true used to bypass the adapter entirely; the previous commit still called `homeManager.getAllEntitiesLive()` to log the missing entities, which force-unwraps the HomeManager storage and crashed AuthenticationIntegrationTests (no HomeManager configured there). Restore the original control flow and keep only the warning that validation was skipped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QpC3PnuBWmyrYM3xpoSFr6
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
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
Fixes for the findings of a full-codebase security review that can be addressed in code. Nothing here changes the running deployment on its own; see "Not in this PR" for the parts that need operator action.
==(timing side-channel on a publicly tunnelled endpoint)TokenAuthenticationMiddleware.constantTimeEquals– XOR-accumulate over the longer input, length folded inlimitquery params passed unclamped to Fluent (limit=2000000000loads whole tables,limit=0is invalid SQL); the OpenAPI generator does not enforceminimum/maximumOpenAPIController.clampedLimit(1…1000, default 100) used by/entities/history,/config/automations/{name}/runs,/config/automations/runs/recentcall/replyframes executed before the peer'shello, bypassing the protocol-version gateStarActorSystem.receivecloses the connection on any non-hello frame before a valid hello; the real client/server flow is unaffected because both sides only send calls after.upHealthCheckfetched any config-supplied URL from inside the container and logged the body (SSRF intodb,cloudflared, metadata endpoints)HealthCheck.validate: https only, public hosts only (rejects loopback/private/link-local/CGNAT IPv4, loopback/ULA/link-local IPv6,localhost,.local/.internal/.localhost, single-label docker names); only the status code is logged. Known limit: the check is on the host string, so a public DNS name that resolves to an internal address still passescertificateVerification = .nonehard-coded, password defaulted tovapor_passwordDATABASE_TLS_VERIFY=trueenables full verification (warning logged while off); release builds refuse to start withoutDATABASE_PASSWORDPOST /config?skipValidation=trueaccepted configs with unknown entities silentlyUserDefaults(plaintext plist, in backups)AdapterCredentialStorestores it in the keychain, migrates existing installs on first launch and deletes the plaintext copy;KeychainHelperwrite functions now return successws://, so token + HomeKit traffic cross the LAN unencryptedServerAddress.useTLS+ "Use TLS (wss://)" toggle in the adapter settings; stored raw values (host###port) and the 8888→8080 migration keep working, TLS values are stored ashost###port###tlsDocs:
docs/setup-server.mdgets an environment-variable table,docs/setup-FlowKitAdapter.mda "Server connection" section,CLAUDE.mdthe security invariants.Behaviour changes to be aware of
DATABASE_PASSWORD. Both compose files already set it, so no action is needed, but a deployment relying on the implicit default would stop at startup with a clear message.localhost:8080.HealthCheckpointing at anhttp://or internal URL now fails at execution with a descriptive error instead of running. The current config (https://hc-ping.com/…) passes.Not in this PR
.envin JulianKahnert/HomeAutomation-config#17 (merged). The values that were committed stay in git history, so rotating at leastAUTH_TOKENis still recommended./adapter/v1from the public hostname is a follow-up design change.Test plan
swift build+swift test(new tests:SecurityHardeningTests,HandshakeGateTests,ServerAddressTests,HealthCheckURLValidationTests)ServerAuthTokengone fromdefaults read de.juliankahnert.HomeKitAdapterApp) and the link comes upswift runfrom HomeAutomation-config still deploys the config; withskipEntityValidation = truethe server log shows the warning🤖 Generated with Claude Code
https://claude.ai/code/session_01QpC3PnuBWmyrYM3xpoSFr6