Repository navigation
[NoQA] Chore: Add cert info to Sentry logs - #105248
Merged
mjasikowski merged 5 commits intoNov 6, 2026
Merged
mjasikowski merged 5 commits into
mjasikowski merged 5 commits into
Conversation
Contributor
|
|
staszekscp
changed the title
staszekscp
marked this pull request as ready for review
October 29, 2026 11:13
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Contributor
Author
|
@eVoloshchak @mjasikowski Hey! May I ask for review, so we can move forward? 😄 |
mjasikowski
self-requested a review
November 2, 2026 10:40
mjasikowski
reviewed
Nov 2, 2026
Comment on lines
+33
to
+35
| /// Identifies the pin list a client shipped with, independent of the app version: the `_lastGenerated` | ||
| /// value of config/certificatePinning/pins.json these hashes were copied from. Update it together with | ||
| /// the hashes. Reported as `certificate_pinning_pin_set_version`. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Identifies the pin list a client shipped with, independent of the app version: the `_lastGenerated` | |
| /// value of config/certificatePinning/pins.json these hashes were copied from. Update it together with | |
| /// the hashes. Reported as `certificate_pinning_pin_set_version`. | |
| /// Identifies the shipped pin set independently of the app version using `_lastGenerated` | |
| /// from config/certificatePinning/pins.json. Update it with the hashes. | |
| /// Reported as `certificate_pinning_pin_set_version`. |
Comment on lines
+128
to
+139
| // MARK: - Reporting | ||
| // | ||
| // TrustKit only says *that* a pin did not match; the attributes below record *what* iOS validated | ||
| // (the full chain, the trust anchor, how the device trusts that anchor, and the network context) so | ||
| // a mismatch in Sentry can be classified as a CA we do not pin versus a TLS-intercepting proxy | ||
| // without a device in hand. Keep this set in sync with Mobile-Expensify's | ||
| // iOS/Expensify/CertificatePinningReporter.m and the table in config/certificatePinning/README.md. | ||
| // | ||
| // Sentry's attribute converter matches Bool before Int, so any number holding 0 or 1 lands as a | ||
| // boolean and the column type flaps between releases. Enum values are therefore strings, counts | ||
| // that can be 0 or 1 are strings, real flags are Bool, and the validation duration is reported in | ||
| // microseconds (never 0 or 1 in practice). |
Contributor
There was a problem hiding this comment.
Suggested change
| // MARK: - Reporting | |
| // | |
| // TrustKit only says *that* a pin did not match; the attributes below record *what* iOS validated | |
| // (the full chain, the trust anchor, how the device trusts that anchor, and the network context) so | |
| // a mismatch in Sentry can be classified as a CA we do not pin versus a TLS-intercepting proxy | |
| // without a device in hand. Keep this set in sync with Mobile-Expensify's | |
| // iOS/Expensify/CertificatePinningReporter.m and the table in config/certificatePinning/README.md. | |
| // | |
| // Sentry's attribute converter matches Bool before Int, so any number holding 0 or 1 lands as a | |
| // boolean and the column type flaps between releases. Enum values are therefore strings, counts | |
| // that can be 0 or 1 are strings, real flags are Bool, and the validation duration is reported in | |
| // microseconds (never 0 or 1 in practice). | |
| // MARK: - Reporting | |
| // | |
| // Record TrustKit's certificate chain, a fresh system trust result, and network settings | |
| // to help investigate pin failures in Sentry. These fields help distinguish an unpinned | |
| // CA from a proxy, but do not establish the cause on their own. | |
| // Keep these fields in sync with Mobile-Expensify's iOS/Expensify/CertificatePinningReporter.m | |
| // and the field descriptions in config/certificatePinning/README.md. | |
| // | |
| // In attributes, enum names and counts are strings, flags are Bool, and validation | |
| // duration is an integer number of microseconds. Send fields listed in extraKeys as | |
| // extras and convert all other values to strings for tags. |
| } else if path.usesInterfaceType(.wiredEthernet) { | ||
| currentInterfaceType = "wired" | ||
| } else if path.usesInterfaceType(.other) { | ||
| currentInterfaceType = "other" // VPN or tunnel |
Contributor
There was a problem hiding this comment.
Suggested change
| currentInterfaceType = "other" // VPN or tunnel | |
| currentInterfaceType = "other" // Virtual or otherwise unknown interface |
| occurrenceCounts[dedupKey] = occurrence | ||
|
|
||
| var attributes: [String: Any] = [ | ||
| // Existing attributes, unchanged |
Contributor
There was a problem hiding this comment.
Suggested change
| // Existing attributes, unchanged |
| // Existing attributes, unchanged | ||
| certificatePinningHostTag: hostname, | ||
| certificatePinningModeTag: enforcePinning ? "enforce" : "monitor", | ||
| // Typed replacement for the old numeric `certificate_pinning_evaluation_result` |
Contributor
There was a problem hiding this comment.
Suggested change
| // Typed replacement for the old numeric `certificate_pinning_evaluation_result` | |
| // TrustKit's evaluation result and connection decision, plus the system trust result |
Comment on lines
+211
to
+212
| // One captured error per failed validation in both modes, exactly as before; the detail above | ||
| // rides along as tags and extras. |
Contributor
There was a problem hiding this comment.
Suggested change
| // One captured error per failed validation in both modes, exactly as before; the detail above | |
| // rides along as tags and extras. | |
| // Capture one error for each failed validation in either mode, with these attributes | |
| // attached as tags and extras. |
Comment on lines
+252
to
+254
| /// `proceed` means the anchor is explicitly trusted by the user or an MDM profile ("Enable Full Trust for | ||
| /// Root Certificates"); `unspecified` means a system root with no user override. This is the cheapest | ||
| /// single signal separating a TLS-intercepting proxy from a public CA we do not pin. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// `proceed` means the anchor is explicitly trusted by the user or an MDM profile ("Enable Full Trust for | |
| /// Root Certificates"); `unspecified` means a system root with no user override. This is the cheapest | |
| /// single signal separating a TLS-intercepting proxy from a public CA we do not pin. | |
| /// `proceed` indicates a successful evaluation based on a user trust decision; | |
| /// `unspecified` indicates success without an explicit user decision. | |
| /// Neither result alone identifies a proxy or proves the anchor is a public CA. |
Comment on lines
+311
to
+315
| /// Re-runs the system trust evaluation on the chain TrustKit validated. TrustKit 3.x does not expose the | ||
| /// original SecTrust, so this is the only way to learn how the device classified the anchor | ||
| /// (`SecTrustResultType`) and, for an untrusted chain, why ("expired", "not trusted", hostname mismatch...). | ||
| /// Revocation checking is best-effort and may touch the network, which is why this never runs on the | ||
| /// main queue. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Re-runs the system trust evaluation on the chain TrustKit validated. TrustKit 3.x does not expose the | |
| /// original SecTrust, so this is the only way to learn how the device classified the anchor | |
| /// (`SecTrustResultType`) and, for an untrusted chain, why ("expired", "not trusted", hostname mismatch...). | |
| /// Revocation checking is best-effort and may touch the network, which is why this never runs on the | |
| /// main queue. | |
| /// Evaluates TrustKit's certificate chain with an SSL policy for hostname. | |
| /// Returns the new evaluation's SecTrustResultType and any error messages. | |
| /// TrustKit does not expose its original SecTrust in the callback, so this evaluation | |
| /// may differ from the original. It runs on reportingQueue because trust evaluation | |
| /// can block while fetching certificates or checking revocation. |
Comment on lines
+351
to
+353
| /// Presence only: never the proxy host or PAC URL, which would identify the customer's network. Per-interface | ||
| /// settings under `__SCOPED__` reveal tunnels (utun/ipsec/ppp/tap), which is where per-app VPN and TLS | ||
| /// inspection usually live. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Presence only: never the proxy host or PAC URL, which would identify the customer's network. Per-interface | |
| /// settings under `__SCOPED__` reveal tunnels (utun/ipsec/ppp/tap), which is where per-app VPN and TLS | |
| /// inspection usually live. | |
| /// Reports whether HTTP proxy and PAC settings are enabled, and whether `__SCOPED__` | |
| /// contains an interface whose name starts with utun, tun, tap, ppp, or ipsec. | |
| /// These names suggest a tunnel but do not prove this connection uses a VPN. | |
| /// The proxy host and PAC URL are omitted to protect network details. |
Comment on lines
+29
to
+35
| /** | ||
| * The HybridApp pin lists live in the Mobile-Expensify submodule and are what actually ships to the stores. | ||
| * They drifted from pins.json once already (the Sectigo roots were missing for a month and every HybridApp | ||
| * build reported the 2026-10 Sectigo chain on staging.expensify.com as a mismatch), so they are checked | ||
| * here too whenever the submodule is checked out. CI for this suite does not fetch the submodule, so that | ||
| * check only runs locally and in HybridApp builds; a missing submodule is skipped rather than failed. | ||
| */ |
Contributor
There was a problem hiding this comment.
Suggested change
| /** | |
| * The HybridApp pin lists live in the Mobile-Expensify submodule and are what actually ships to the stores. | |
| * They drifted from pins.json once already (the Sectigo roots were missing for a month and every HybridApp | |
| * build reported the 2026-10 Sectigo chain on staging.expensify.com as a mismatch), so they are checked | |
| * here too whenever the submodule is checked out. CI for this suite does not fetch the submodule, so that | |
| * check only runs locally and in HybridApp builds; a missing submodule is skipped rather than failed. | |
| */ | |
| /** | |
| * Keep the HybridApp pin lists used in store builds in sync with pins.json. | |
| * Include each Mobile-Expensify file that exists when this suite runs. | |
| * Missing files are skipped so the suite can run without the submodule. | |
| */ |
mjasikowski
requested changes
Nov 2, 2026
Contributor
Author
|
Thanks for the review @mjasikowski, I've applied the changes! Can I also ask for review in this PR so we have the full package? 😄 |
Contributor
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Contributor
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Contributor
|
@MelvinBot review |
Contributor
ReviewThe diagnostics design looks good. Threading is safe: all shared state is touched only on the serial 1. The first report in each session always has 2. Check the Sentry tag limits —
3. CI never runs the new HybridApp sync checks —
Please confirm the test passes locally with the submodule at the commit this PR pins. |
Contributor
|
@staszekscp mind checking validity of Melvin's claims? |
mjasikowski
self-requested a review
November 6, 2026 09:56
mjasikowski
previously approved these changes
Nov 6, 2026
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
mjasikowski
approved these changes
Nov 6, 2026
Contributor
|
🚧 mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
85 tasks done
Contributor
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.5.34-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.5.34-1 🚀
Bundle Size Analysis (Sentry): |
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.
Explanation of Change
Adds information to Sentry logs to understand Certificate Pinning warnings before going live
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/633469
$ https://github.com/Expensify/Expensify/issues/682201
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari