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.

Sign up for GitHub

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

Jump to bottom

fix(extension): #107: allow http for localhost #129

Merged
merged 2 commits into from
Aug 5, 2024
Merged

fix(extension): #107: allow http for localhost #129

merged 2 commits into from
Aug 5, 2024

Conversation

VanishMax
Copy link
Contributor

Closes #107

  1. Changes to the manifest to inject content scripts into http://localhost
  2. Changes to the assertValidSender function to allow http://localhost

@VanishMax VanishMax requested a review from a team July 26, 2024 12:49
@VanishMax VanishMax self-assigned this Jul 26, 2024
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: e9019d1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

turbocrime
turbocrime approved these changes Jul 26, 2024
View reviewed changes
apps/extension/src/senders/validate.ts Outdated Show resolved Hide resolved
apps/extension/src/senders/validate.ts Outdated Show resolved Hide resolved
turbocrime
turbocrime reviewed Jul 26, 2024
View reviewed changes
apps/extension/public/manifest.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd like to condition the manifest inclusion of localhost but that would require some additional build config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by this?

turbocrime
turbocrime suggested changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@turbocrime turbocrime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ValidSender type needs correction but other than that, this would work

apps/extension/src/senders/validate.ts Outdated
@@ -13,6 +13,10 @@ type ValidSender = chrome.runtime.MessageSender & {
url: `${ValidProtocol}//${string}/${string}`;
Copy link
Contributor

@turbocrime turbocrime Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the origin/url type are incorrect now that validprotocol may conditionally be http at any moment. i don't think it matters anywhere so probably just set these to 'string'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to string

@turbocrime
Copy link
Contributor

deps also need to support this https://github.com/penumbra-zone/web/blob/9da630e851cd7303372b3e1aac63d398305bba62/packages/transport-chrome/src/session-manager.ts#L99-L105

@VanishMax VanishMax mentioned this pull request Jul 29, 2024
Merged
@VanishMax
Copy link
Contributor Author

@turbocrime pinging for your re-review

@turbocrime turbocrime force-pushed the fix/#107-allow-http-for-localhost branch from f0198bb to e9019d1 Compare August 5, 2024 18:34
@turbocrime turbocrime merged commit 8141aa4 into main Aug 5, 2024
3 checks passed
@turbocrime turbocrime deleted the fix/#107-allow-http-for-localhost branch August 5, 2024 18:36
grod220 added a commit that referenced this pull request Aug 8, 2024
@grod220
Revert "fix(extension): #107: allow http for localhost (#129)"
7455586 
This reverts commit 8141aa4.
@grod220 grod220 mentioned this pull request Aug 8, 2024
Merged
grod220 added a commit that referenced this pull request Aug 8, 2024
@grod220
Revert "fix(extension): #107: allow http for localhost" (#145)
75acabe 
* Revert "fix(extension): #107: allow http for localhost (#129)"

This reverts commit 8141aa4.

* Update version
@VanishMax VanishMax mentioned this pull request Aug 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbocrime turbocrime turbocrime approved these changes

Assignees

@VanishMax VanishMax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accept localhost connections without https
2 participants
@VanishMax @turbocrime