Allowlist the Galaxy host in the bash sandbox when GALAXY_URL is scheme-less#365
Draft
dannon wants to merge 2 commits into
Draft
Allowlist the Galaxy host in the bash sandbox when GALAXY_URL is scheme-less#365dannon wants to merge 2 commits into
dannon wants to merge 2 commits into
Conversation
…me-less hostFromUrl fed the raw GALAXY_URL straight into new URL(), which throws on a bare host like "usegalaxy.org", so the Galaxy host never made it into the sandbox's network allowlist. Run it through normalizeGalaxyUrl first -- the same helper /connect and the profile system use -- so a scheme-less host resolves the same way the connection does, and the allowlisted host is exactly the one Galaxy talks to. While here, only keep the host for http(s) so an ftp:// or file://host/ URL can't seed the allowlist; Galaxy is http(s) only anyway.
Fold the whitespace-only case into the existing junk test -- the empty-string assert just re-covered the !url guard that undefined already exercises -- and trim the protocol-gate comment down to the why.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #297.
buildSandboxConfigderives the bash sandbox's network allowlist from the Galaxy host, buthostFromUrlfedprocess.env.GALAXY_URLstraight intonew URL(). A scheme-less configured URL likeusegalaxy.orgthrows there, the error is swallowed, and the Galaxy host never lands in the allowlist -- the same scheme-less-URL class that bit the REST path in #264/#290.The fix runs the URL through
normalizeGalaxyUrlfirst -- the existing helper/connectand the profile system already use -- so a bare host resolves tohttps://usegalaxy.orgexactly the way the connection does. The allowlisted host is therefore the one Galaxy actually talks to, never broader.While in here, an adversarial review of the diff pointed out that a non-http(s)
GALAXY_URL(ftp://x.org,file://host/...) would still hand a host to the allowlist. Galaxy speaks http(s) only (validateGalaxyUrlenforces this on connect), so the host is now kept only forhttp:/https:. That tightens the allowlist relative to the old behavior rather than broadening it.Low impact in practice -- Galaxy work flows over MCP, not bash, so the tight default was mostly fine -- but it closes the gap consistently.
Test-first: extended
tests/sandbox-config.test.tswith the scheme-less case (the host gets allowlisted) alongside the existing scheme-ful case, plus loopback-http, non-http(s)-scheme, and empty-input coverage. Rootnpm test(1255 pass) and the app typecheck are green.