-
Notifications
You must be signed in to change notification settings - Fork 441
fix(gnoweb): update ts URL construction in _fetchQEval to handle existing http protocols #4964
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
base: master
Are you sure you want to change the base?
fix(gnoweb): update ts URL construction in _fetchQEval to handle existing http protocols #4964
Conversation
🛠 PR Checks Summary🔴 Changes related to gnoweb must be reviewed by its codeowners Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| // - tcp:// is converted to http:// (RPC over HTTP) | ||
| // - No protocol defaults to http:// | ||
| private _normalizeRemoteUrl(url: string): string { | ||
| if (url.startsWith("tcp://")) return url.replace("tcp://", "http://"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change tcp://127.0.0.1:26657 to http://127.0.0.1:26657, then we also need to change it here when we set the CSP for connect-src:
gno/gno.land/cmd/gnoweb/main.go
Line 336 in de7d586
| remote, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... probably for other changes to the remote string, but I can't test them locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... In fact, maybe it's better to correct the cfg.remote URL much earlier before it is used, like here:
gno/gno.land/cmd/gnoweb/main.go
Line 228 in de7d586
| appcfg.NodeRemote = cfg.remote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jefft0! You are right, this will keep the JS fetch clean too. I Updated the PR to normalize the URL at the Go level instead
jefft0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally with
gnoweb
gnoweb -remote 127.0.0.1:26657
gnoweb -remote http://127.0.0.1:26657
gnoweb -remote tcp://127.0.0.1:26657
Added
normalizeRemoteURL()function in main.go to ensure the remote URL has a valid HTTP(S) protocol before being used by the frontend and CSP.tcp://is converted tohttp://(RPC uses HTTP over TCP)http://(local)http://andhttps://are kept as-isSecureHeadersMiddlewareto use the normalized URL for CSPconnect-src-remote tcp://...seamlessly.Note: The root cause may be a misconfigured remote value in production (e.g., missing the
:inhttps://). This fix handles the symptom, but the remote URL configuration might also be verified just in case.