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(extension): #107: allow http for localhost #153

Merged
merged 5 commits into from
Aug 13, 2024

Conversation

VanishMax
Copy link
Contributor

Closes #107

Relates to #129 and the reverting PR #145.

Allows http protocol for localhost

@VanishMax VanishMax requested a review from a team August 12, 2024 15:52
@VanishMax VanishMax self-assigned this Aug 12, 2024
Copy link

changeset-bot bot commented Aug 12, 2024

⚠️ No Changeset found

Latest commit: e0840f5

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

@@ -16,12 +16,12 @@
},
"content_scripts": [
{
"matches": ["https://*/*"],
"matches": ["https://*/*", "http://localhost/*"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line, instead of the previously used one, must work correctly according to the docs: https://developer.chrome.com/docs/extensions/develop/concepts/match-patterns#special

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked a test upload, it works 👍

@@ -16,12 +16,12 @@
},
"content_scripts": [
{
"matches": ["https://*/*"],
"matches": ["https://*/*", "http://localhost/*"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked a test upload, it works 👍

Comment on lines +38 to +43
if (
!(
parsedOrigin.protocol in ValidProtocol ||
(globalThis.__DEV__ && isHttpLocalhost(parsedOrigin))
)
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: we should have a test for this in validate.test.ts. Also, could you check that test suite to ensure we've covered all cases for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grod220 thanks for pointing this out. I should pay more attention to the tests

@VanishMax VanishMax requested a review from grod220 August 13, 2024 13:20
@VanishMax VanishMax merged commit 74258a8 into main Aug 13, 2024
3 checks passed
@VanishMax VanishMax deleted the fix/#107-allow-http-for-localhost branch August 13, 2024 17:04
if (
!(
parsedOrigin.protocol in ValidProtocol ||
(globalThis.__DEV__ && isHttpLocalhost(parsedOrigin))
Copy link
Contributor

Choose a reason for hiding this comment

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

Stumbled upon this today. I was trying to connect a production Prax to the dev Dex explorer and it would silently fail. Think we should drop the requirement that the dev version of Prax is needed to connect to localhost sites:

parsedOrigin.protocol in ValidProtocol ||
- (globalThis.__DEV__ && isHttpLocalhost(parsedOrigin))
+ isHttpLocalhost(parsedOrigin)

@VanishMax can you open a follow PR on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grod220 no problem! #208

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept localhost connections without https
2 participants