firefox-ios icon indicating copy to clipboard operation
firefox-ios copied to clipboard

Published 20 hours ago • verified publisher icon mozilla-mobile

Bugfix FXIOS-7328 Sec Bugzilla 1843467 SSL Lock Spoof

Open razvanlitianu opened this issue 1 year ago • 1 comments

:scroll: Tickets

Jira ticket

:bulb: Description

We are to eager in setting the security lock icon on url bar. That information is available later after the webview in fully loaded. This PR changes when the security status is updated and when the lock is shown and fixed a security spoofing issue. The video below shows multiple cases where the lock icon is set. First opening a website from an existing tab, switching tabs with different security status and loading a website in a new tab. https://github.com/user-attachments/assets/57be4b10-35b2-4a30-9b6d-40303c05e617

:pencil: Checklist

You have to check all boxes before merging

  • [x] Filled in the above information (tickets numbers and description of your work)
  • [x] Updated the PR name to follow our PR naming guidelines
  • [ ] Wrote unit tests and/or ensured the tests suite is passing
  • [ ] When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • [ ] If needed, I updated documentation / comments for complex code and public methods
  • [ ] If needed, added a backport comment (example @Mergifyio backport release/v120)

razvanlitianu avatar Aug 21 '24 13:08 razvanlitianu

Messages
:book: Project coverage: 31.53%
:book: Edited 3 files
:book: Created 0 files

Client.app: Coverage: 30.23

File Coverage
BrowserViewController.swift 4.96% ⚠️
BrowserViewController+WebViewDelegates.swift 4.72% ⚠️
TabLocationView.swift 42.34% ⚠️

Generated by :no_entry_sign: Danger Swift against 4f78dd6ea25658ccb0105a9f263ab0f56c64c2f2

mobiletest-ci-bot avatar Aug 22 '24 13:08 mobiletest-ci-bot

My only concern here is that it feels like there may be potential side effects, edge cases, or other implications from how we've changed the code to update the lock icon. But overall it seems Ok to me; I think we should ask QA for specific regression testing if possible in this area after the fix is merged.

We have had issues with this in the past, and it often felt unreliable. One problem is the lack of documentation on how it should work. Android and desktop using GeckoView might obtain this information differently. We should consult with both the web security and UX teams to clarify this, but I don't know who to contact. I will keep the PR open a bit longer to review. Additionally, the tab bar refactor presents an opportunity to address this issue since its appearance is changing.

razvanlitianu avatar Aug 30 '24 10:08 razvanlitianu

This PR has been automatically marked as stale. Please leave any comment to keep this PR opened. It will be closed automatically if no further update occurs in the next 7 days. Thank you for your contributions!

github-actions[bot] avatar Sep 20 '24 00:09 github-actions[bot]

@razvanlitianu Do you know what the status is for this? Is it still a WIP?

mattreaganmozilla avatar Sep 21 '24 00:09 mattreaganmozilla

FYI: going to close this PR for now, I'm planning to re-test the related ticket(s) for the SSL/lock icon bugs. If needed I will open up a new PR with these changes and cross-link it to this one. Thanks @razvanlitianu for digging into this. 🙏

mattreaganmozilla avatar Oct 08 '24 21:10 mattreaganmozilla