fix: Fix Trezor signing and connecting #26882
Merged
Conversation
This file contains 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
Gudahtt
force-pushed
the
fix-trezor-connections
branch
from
1daedb7 to
eb0766e
Compare
Gudahtt
commented
Sep 4, 2024
| --- a/lib/impl/core-in-iframe.js | ||
| +++ b/lib/impl/core-in-iframe.js | ||
| @@ -116,7 +116,9 @@ class CoreInIframe { | ||
| this._log.enabled = !!this._settings.debug; | ||
| window.addEventListener('message', this.boundHandleMessage); | ||
| window.addEventListener('unload', this.boundDispose); | ||
| - await iframe.init(this._settings); | ||
| + const modifiedSettings = Object.assign({}, this.settings); |
There was a problem hiding this comment.
This is where the underscore was missing
Gudahtt
commented
Sep 4, 2024
| - await this.init(this._settings); | ||
| + var modifiedSettings = Object.assign({}, this._settings); | ||
| + modifiedSettings.env = 'webextension'; | ||
| + await this.init(modifiedSettings); |
There was a problem hiding this comment.
The same settings object is referenced here, so we'd want to use the same modifications. This was present in the original patch
Gudahtt
commented
Sep 4, 2024
| + var modifiedSettings = Object.assign({}, this.settings); | ||
| + modifiedSettings.env = 'webextension'; | ||
| this.channel.postMessage({ | ||
| type: events_2.POPUP.INIT, |
There was a problem hiding this comment.
This second call to events_2.POPUP.INIT was not present in v9.2.2, but it is present here in v9.3.0
See:
9.2.2: https://npmfs.com/package/@trezor/connect-web/9.2.2/lib/popup/index.js
9.3.0: https://npmfs.com/package/@trezor/connect-web/9.3.0/lib/popup/index.js
Trezor signing and connecting was broken in a recent change that included an update to the `@trezor/connect-web` library (in #26143). The patch had to be rewritten as part of that update, but in this rewrite an underscore was missed in referencing a variable (the patch used `this.settings` rather than `this._settings`). Additionally, two more changes have been applied to ensure the modified settings are used everywhere. One was present in the original patch but missing from the patch in #26143. The other was not present in the original patch (#23763) because the affected line was added in the library update, but it seems equally necessary. Fixes #26875
Gudahtt
force-pushed
the
fix-trezor-connections
branch
from
eb0766e to
11d653e
Compare
|
7 tasks
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26882 +/- ##
========================================
Coverage 70.15% 70.15%
========================================
Files 1425 1425
Lines 49600 49600
Branches 13877 13877
========================================
Hits 34795 34795
Misses 14805 14805 ☔ View full report in Codecov by Sentry. |
Builds ready [11d653e]
Page Load Metrics (1634 ± 84 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
danjm
approved these changes
Sep 4, 2024
mikesposito
approved these changes
Sep 4, 2024
MajorLift
approved these changes
Sep 4, 2024
There was a problem hiding this comment.
Are we not using modifiedSettings in this branch both here and in the original patch because we want a longer request timeout for handshake events?
@@ -240,7 +241,7 @@ class PopupManager extends events_1.default {
else if (message.type === events_2.POPUP.CORE_LOADED) {
+ var modifiedSettings = Object.assign({}, this.settings, { env: 'webextension' });
this.channel.postMessage({
type: events_2.POPUP.HANDSHAKE,
- payload: { settings: this.settings },
+ payload: { settings: modifiedSettings },
});
(_a = this.handshakePromise) === null || _a === void 0 ? void 0 : _a.resolve();
}There was a problem hiding this comment.
Great question. Maybe we should be using it here as well
There was a problem hiding this comment.
I'll merge this for now, but this is worth following up on
metamaskbot
added
the
release-12.6.0
gauthierpetetin
added
release-12.5.0
Gudahtt
added
release-12.1.2
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
Trezor signing and connecting was broken in a recent change that included an update to the
@trezor/connect-weblibrary (in #26143). The patch had to be rewritten as part of that update, but in this rewrite an underscore was missed in referencing a variable (the patch usedthis.settingsrather thanthis._settings).Additionally, two more changes have been applied to ensure the modified settings are used everywhere. One was present in the original patch but missing from the patch in #26143. The other was not present in the original patch (#23763) because the affected line was added in the library update, but it seems equally necessary.
Related issues
Fixes #26875
Manual testing steps
Follow reproduction steps in #26875
Screenshots/Recordings
Before
See https://www.loom.com/share/3e1f3d2cc57247a5b02e0ff8d61babb8?sid=1b17e423-16cf-4260-a1ed-674441e25b3a
After:
Screen.Recording.2024-09-03.at.21.55.10.mov
(I stopped partway through connecting, I was already past the point of it crashing by then).
Pre-merge author checklist
Pre-merge reviewer checklist