Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(client-utils): only call fetch as a function, not a method #10671

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

erights
Copy link
Member

@erights erights commented Dec 11, 2024

closes: #XXXX
refs: endojs/endo#31 (comment)

Description

Testing on Brave, the browser's global fetch function works when called with its this bound to either undefined or the browser global object, but does not work if its this is bound to something else. The code this PR fixes was misbehaving because it was calling powers.fetch(...), i.e., happening to call it as a method with its this bound to the irrelevant powers object.

This refactor just expresses the intention of this code, which is just to call the fetch function without passing it some object as its this binding. This seems to fix the problem.

Not at all addressed by this PR: The fact that this problem happened from a programming patterns that seems correct and would have passed review indicates a deeper hazard. This PR does nothing to mitigate this deeper hazard. It only fixes this case where we fell into the hazard.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none, except as needed to address the more general hazard.

Testing Considerations

Only tested fetch behavior on Brave. Based on that, proceeding under untested assumption that this refactor will work on all supported browsers.

Upgrade Considerations

none

@erights erights requested review from turadg and mhofman December 11, 2024 02:11
@erights erights self-assigned this Dec 11, 2024
Copy link

cloudflare-workers-and-pages bot commented Dec 11, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6e9dbc1
Status: ✅  Deploy successful!
Preview URL: https://1ddbff40.agoric-sdk.pages.dev
Branch Preview URL: https://markm-fix-fetch-this.agoric-sdk.pages.dev

View logs

@erights erights marked this pull request as ready for review December 11, 2024 05:10
@erights erights requested a review from a team as a code owner December 11, 2024 05:10
@erights erights force-pushed the markm-fix-fetch-this branch from ff8138a to 73ddc12 Compare December 11, 2024 05:15
@erights erights added the automerge:squash Automatically squash merge label Dec 11, 2024
@mergify mergify bot merged commit fbae24c into master Dec 11, 2024
81 checks passed
@mergify mergify bot deleted the markm-fix-fetch-this branch December 11, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants