-
Notifications
You must be signed in to change notification settings - Fork 289
Allow self.alt host for self-requests
#3003
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
Allow self.alt host for self-requests
#3003
Conversation
|
@itowlson this overall looks good, an alternate suggestion that came up during the original PR adding this feature also seems to indicate that this will benefit go. |
karthik2804
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.
I am okay with adding this to just the wasi-http.
radu-matei
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.
Self-request permission is still given via allowed_outbound_hosts = ["http://self"], not http://self.alt/. Should we allow self.alt as another way of expressing the same permission?
Not a blocker; but allowing self.alt since it's what you use here makes sense to me.
I implemented this only in wasi-http because that's what JS goes through. Should I implement it in spin-http too? It looked faffier there because of the way the types line up, but certainly do-able.
This only being in WASI HTTP makes perfect sense to me.
LGTM-ing since this is a pretty big blocker for the JS SDK.
Thanks!
|
nit: Commit message says |
I would say yes, to reduce confusion. |
12871e9 to
f6a3786
Compare
|
Added bonus host. Note that both Fixed commit message - thanks for the spot @lann! |
lann
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.
If we want to get this through quickly that's fine with me, but we really should have test coverage for this.
Yep, agree. Note we can't run tests at the moment because the runner got lost in the repo move, but worth having for when the lights come back on... |
Signed-off-by: itowlson <[email protected]>
f6a3786 to
aa47c0d
Compare
|
Added a test which Works On My Machine even if CI is packing a sad right now. |
spin.alt host for self-requestsself.alt host for self-requests
This is a proposal to address https://github.com/fermyon/spin-js-sdk/issues/298.
The issue there is that JS guests use the inbuilt
fetchfunction to make HTTP requests. Iffetchsees a URL without a host, it helpfully prepends the host it thinks it's running on before letting Spin see it. So for a self-request such asfetch("/back"), Spin seeshttp://localhost:3000/backor whatever. Which is not on the allow list so gets the banhammer.This PR reluctantly compromises with
fetchby allowing self-requests via the pseudo-hostself.alt. So if you dofetch("http://self.alt/back"), Spin will treat it as a self-request: it will validate permission using the self-request permission and route it as a self request.The developer experience is, admittedly, not very lovely. But it hopefully gives JS folks an escape hatch.
Notes for reviewers:
allowed_outbound_hosts = ["http://self"], nothttp://self.alt. Should we allowself.altas another way of expressing the same permission?