Repository navigation
Conversation
f50f4fd to
74a0896
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several native request paths omit the shared cookies, and stale-cookie cleanup remains unresolved.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Adds shared WebView cookie mirroring so native iOS requests can authenticate through cookie-based proxies.
Changes:
- Adds app-group cookie storage and mirrors WebKit cookies at launch.
- Applies shared cookies to API, authentication, and webhook sessions.
- Review identified unwired request paths, stale-cookie cleanup gaps, and missing tests.
| File | Summary |
|---|---|
Sources/Shared/Environment/Environment.swift |
Configures shared app-group cookie storage. |
Sources/Shared/API/Webhook/Networking/WebhookManager.swift |
Applies shared cookies to webhook sessions. |
Sources/Shared/API/HAAPI.swift |
Applies shared cookies to REST sessions. |
Sources/HANetworking/Sources/HANetworkingEnvironment.swift |
Exposes cookie storage; some direct sessions remain unwired. |
Sources/HANetworking/Sources/AuthenticationAPI.swift |
Configures refresh/revoke sessions; token-fetch sessions still need cookies. |
Sources/App/Frontend/WebView/WebViewCookieMirror.swift |
Mirrors WebView cookies; cleanup and test coverage are still needed. |
Sources/App/AppDelegate.swift |
Starts cookie mirroring at application launch. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
0c4ca99 to
8db3e28
Compare
14103f5 to
9565bdb
Compare
45964eb to
2f3205d
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved synchronization and cookie-delivery issues affect startup, authentication, onboarding connectivity, notification-content HLS, and test reliability.
Review effort: Lite
Findings: 2
Open (2)
Resolved since last review (2)
2f3205d to
5adc430
Compare
|
What happens if the app does not have a webview available, such as when running something on background (widget tap, shortcuts actions etc)? We should not rely on webview cookies for basic operations, if this is required by cloudflare then I would call it a bug on their side or a miscompatibility to Home Assistant remote connection. |
It's just one of authentication methods, the most convenient one for the frontend users (typical oauth). They have other methods like service tokens for headless script scenarios too. I'm actually working on a "Cloudflare Access" integration precisely to present this all in a single user-friendly UI as I've seen bunch of forum threads where people were struggling to set it up, got confused by the variety of options, or configured the bypasses to be too broad / less secure than they could be, and iOS is the only platform currently blocking me due to this issue - access via the browser, Android app and webhooks already works as expected. For disclosure, I work at Cloudflare but started working on this to secure my personal HA instance. So not doing this as part of the job, just want to make this easier for other folks too. Besides, this fix is not Cloudflare-specific, it will work for any cookie-based authentication proxies for remote access. |
|
Note: this has now grown a bit more than the initial diff due to Copilot's reviews, but they were all fair - main bulk came from new tests and making sure that media loads correctly as well. |
|
I understand what is the goal but this would mean that the companion apps would officially start supporting secondary authentication methods, which we currently don't support, this would need an architecture proposal and validation to be provider agnostic. I don't think this aligns to the current Home Assistant (as a whole) goal, we have put a lot of effort to provide mTLS support for both companions apps so access can be made fully secure (which Cloudflare also supports). |
If this was introducing something unprecedented, sure, but given that this PR simply closes the gap for something Android app already had for the last 5 years (same author not sending an IOS PR at the same time looks like a accidental oversight), and on the Web worked naturally since inception, this is more like a bugfix than an entirely new proposal.
I've seen that work, and it's certainly a pretty big undertaking, but it also is (entirely IMO) more complicated / less user-friendly than OAuth login via Google or whichever IdP users already have. Dealing with custom certificates across a variety of platforms is tricky, and not for every user. |
5adc430 to
c08094f
Compare
The fact that other platforms support such does not make it the right decision moving forward. Time goes by, and decisions are revisited. Secondary auth support is something companion apps do not and don’t aim to support in the short period of time. I'm not against the "fix“, I'm just challenging the solution. I can imagine this bringing edge cases where these cookies get erased or lost, and then background operations stop working. Users will get confused because they don’t understand what’s happening behind the scenes and get frustrated. Just for information so you have the context, the road for mTLS does not end where it is currently. The (current) goal being discussed is to simplify and automate the certificate generation and synchronization. |
Okay I'm not sure where to go from here. Should I keep working on this PR or are you saying it's a no-go? I'd still like to release my CF Access integration which I've been dogfooding and pretty happy with how it's shaping up. If it has to come with caveats that it doesn't work on iOS and that it's not blessed by Home Assistant so users know to direct issues and frustration at my repo, that's obviously fine, but would be nice to have a consistent experience across all platforms (if nothing else, because my wife is on iPhone heh).
Yeah, I was wondering about that too, certainly doable and glad to hear it's in the plans. I'm guessing it will only help mobile companions though and not web / desktop access which OAuth version can cover as there is no low-level control over TLS connection on the web from inside the apps? Probably enough for most regular users, just want to be clear about the differences. |
|
I think in the end these would be complementary not competing solutions, with mTLS being best for apps with long-term access, and cookie based flow for Web sessions as well as access from Google/Alexa/MCPs (which my integration covers too, as they also use OAuth). |
|
I have shared this PR with some Android devs and I'll share with some core developers as well, allow me some time to review this with them and I'll follow up with you |
c08094f to
032be8d
Compare
|
Thanks, appreciate it and happy to talk over more via other mediums if more convenient. |
ea19084 to
1de8a19
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5912 +/- ##
==========================================
+ Coverage 50.92% 51.22% +0.29%
==========================================
Files 1216 1217 +1
Lines 80591 80609 +18
==========================================
+ Hits 41042 41289 +247
+ Misses 39549 39320 -229
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f4636ac to
3946758
Compare
Without this fix, the iOS app fails to load anything except frontend behind authentication proxies such as Cloudflare Access. Such auth proxies gate every request on the session cookie issued at login. The frontend gets that cookie through the WebView, but WebKit keeps its cookies where native requests cannot read them. So anything besides the frontend - sensors, location, widgets, Assist voice replies, camera streams, token refresh, connection diagnostics and even onboarding's own code exchange - got the 401 unauthorised error. The Android app had the same problem (home-assistant/android#1708) and fixed it couple of years ago in home-assistant/android#1797 by sharing its WebView's cookie store with its HTTP client. This change implements the analogous fix for iOS as well, the last client where such a setup did not work. iOS offers no way to directly share the WebKit's store, so `WebViewCookieMirror` copies it into an app-group cookie storage and every native request presents it, including media that AVFoundation fetches itself. Two gaps remain: 1. The WebSocket handshake still does not receive cookies. A follow-up will fix that once home-assistant/HAKit#124 has landed. 2. The Watch app never receives cookies either, because it has an app-group container of its own. Android has the same gap, since its jar skips Wear OS.
3946758 to
b3f4112
Compare
This should be resolved now, will recheck after the next CI run. |
bgoncal
left a comment
bgoncal
left a comment
There was a problem hiding this comment.
I'm still reviewing the idea with other developers, meanwhile can you reduce the amount of comments generated by the AI model? Stick to comments that are really needed and can't be understood by reading the code
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
This PR depends on the one you opened for HAKit right? |
Heh, that's actually what I did - initially it didn't generate any, and I wanted to add explanatory comments to the tricky bits to make sure it's clear to reviewers and future readers alike, so had Claude write some and added a few myself. I guess I might have overcorrected trying to explain the cookie sync mechanism - is that the one you are referring to or are there any others you feel are excessive?
As it stands right now, not quite, it will build fine but live updates via WebSocket won't work, as I intentionally didn't want to block one PR on the other. If HAKit merges land first, I'll update this PR, otherwise I'll send a tiny follow-up. |
I guess, technically, all of these can be understood by reading the code, I just tend to leave comments where it might require gathering context across files to understand it, and faster to read an explanation in-place. I'm happy to trim down any that feel excessive though. |
|
Ok, so after some debate with other developers, I endup in a situation that I need to understand the impact of maintainability and what are we winning on adding this right now. Could you walk me through the average user who gets impacted by this? 1 - What's the user setup? |
AI Policy
Select exactly one option that describes AI usage in this contribution:
Summary
Without this fix, the iOS app fails to load anything except frontend behind authentication proxies such as Cloudflare Access.
Such auth proxies gate every request on the session cookie issued at login. The frontend gets that cookie through the WebView, but WebKit keeps its cookies where native requests cannot read them. So anything besides the frontend - sensors, location, widgets, Assist voice replies, camera streams, token refresh, connection diagnostics and even onboarding's own code exchange - got the 401 unauthorised error.
The Android app had the same problem (home-assistant/android#1708) and fixed it couple of years ago in home-assistant/android#1797 by sharing its WebView's cookie store with its HTTP client. This PR implements the analogous fix for iOS as well, the last client where such setup did not work.
iOS offers no way to directly share the WebKit's store, so
WebViewCookieMirrorcopies it into an app-group cookie storage and every native request presents it.Two gaps remain:
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes