Harden the web server: bind to --hostname, add XSRF protection, validate /config paths - #116
Merged
Conversation
…paths The web app has three unauthenticated state-changing endpoints: /upload writes and extracts a ZIP into --data-root, /task_cfg rewrites a task's config, and /config persists a credential path. Three issues let those be reached from outside the intended trust boundary. Bind to the requested address - app.listen() was called without `address`, so Tornado bound 0.0.0.0 and :: regardless of --hostname; the flag only affected the URL printed at startup. A server started with the localhost default was reachable from the whole network. Pass address=hostname, and document that 0.0.0.0 is the explicit opt-in to network exposure. XSRF protection - No xsrf_cookies, so any page the user visited could POST to the server. Enable it, with samesite='Strict' on the cookie so a cross-site request fails both on the missing cookie and the unknown token. - The pages are rendered by Jinja2, not Tornado templates, so xsrf_form_html() is unavailable. Add VizSeqBaseRequestHandler. render_template(), which injects xsrf_token into every render (reading it is also what sets the cookie), and route the five GET handlers through it. - base.html carries the token in a meta tag plus a $.ajaxSetup that sends X-XSRFToken on non-GET same-origin requests, covering both AJAX callers without touching either. upload.html gets a hidden _xsrf field for its multipart form. Validate credential paths instead of probing them - ConfigHandler.post reported op.exists() for any caller-supplied path, answering "does this file exist" for arbitrary locations. Use the existing _data.set_g_cred_path(), which requires an existing, readable, regular .json file, so the response no longer distinguishes present from absent except for .json files. Add the readability check to that helper; it validated exists/isfile/extension only. - This also applies the credentials to the running process. The old code persisted the path but never called set_g_cred_path(), so GOOGLE_APPLICATION_CREDENTIALS was never set and /g_translate could not pick up a new credential without a restart. Verified: the server accepts on loopback and is refused on this host's routable address; /upload, /config and /task_cfg return 403 without a token, and both the header and form-field token paths reach the real handler logic with ZIP traversal rejection and cleanup unchanged; /etc/passwd and /etc both report valid=false while a real .json file is accepted, applied and persisted. Known follow-up: tests/test_web.py's two upload tests predate XSRF and POST without a token, so they now get 403 where they expect 400. They need to GET /upload first and forward the hidden-field token and the _xsrf cookie. Left untouched here.
The previous commit enabled xsrf_cookies but left tests/test_web.py alone, noting it as a follow-up. Tornado runs check_xsrf_cookie() in _execute, before the handler, so the two upload tests POSTed without a token, got 403 and never reached the validation they were written to exercise. Both asserted 400. Forward the token - Add _get_xsrf_token(), which GETs /upload and parses _xsrf out of Set-Cookie. AsyncHTTPTestCase has no cookie jar, so the cookie has to be echoed back by hand; _xsrf_headers() builds that Cookie header. - _multipart_zip() takes an optional token and emits it as an _xsrf form part, matching how upload.html actually submits, rather than the X-XSRFToken header path that base.html's $.ajaxSetup uses. Both reach the same check_xsrf_cookie(); the form field is the one this endpoint's own template relies on. - Route the two upload tests through _post_zip(). Their assertions are unchanged: ZIP traversal and corrupt-archive handling are what they test, and they test it again now. Cover the new behavior - test_upload_accepts_a_valid_archive_with_an_xsrf_token: a tokened upload reaches the handler, redirects 303, unpacks, and removes the archive. Without this the suite would still pass if XSRF rejected everything. - test_state_changing_posts_without_an_xsrf_token_are_rejected: /upload, /task_cfg and /config all return 403 untokened, and no data is written. Verified: 11 passed. The pre-change file fails exactly two tests against the current server, and flipping xsrf_cookies to False fails the new rejection test, so it is not vacuously green.
meta-cla
Bot
added
the
CLA Signed
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The comment claimed set_g_cred_path()'s validation kept the endpoint from doubling as a probe for arbitrary filesystem paths. That is backwards: the validation constrains the file's kind (a readable .json) but not its location, so the valid/invalid response is an existence oracle for readable JSON files anywhere on disk. Record what is actually true, why the path is left unconstrained (service account credentials normally live outside --data-root), and why the oracle is accepted rather than fixed (localhost-default bind plus XSRF). CodeQL alert #15 (py/path-injection) is dismissed on the same rationale.
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
2 participants
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.
Summary
Closes three ways the local web server was more exposed than it needed to be.
Bind to
--hostname.app.listen()was called without an address, so the server listened on every interface regardless of what--hostnamesaid — the flag only affected the URL printed in the log. It now binds to the given host, which defaults to localhost. Pass--hostname 0.0.0.0to get the old behavior deliberately.XSRF protection.
/upload,/configand/task_cfgmutate state with no authentication, so any page in the user's browser could POST to them. The app now enables Tornado'sxsrf_cookieswithsamesite=Strict. Arender_template()helper on the base handler embeds the token in every rendered page (readingself.xsrf_tokenis what sets the cookie); the upload form carries it in a hidden field, and an$.ajaxSetuphook inbase.htmlattachesX-XSRFTokento same-origin, state-changing AJAX requests.Validate
/configpaths. The POST handler accepted any path that passedos.path.exists, which made it a probe for arbitrary filesystem paths. It now routes throughset_g_cred_path(), which requires a readable.jsonfile, and applies the credentials to the running process so/g_translatepicks them up without a restart.set_g_cred_path()also gained an explicit readability check.Test plan
pytest tests/test_web.py— 11 passed, 6 subtests passed./upload,/configand/task_cfgare rejected with 403.🤖 Generated with Claude Code