Repository navigation
Prevent caller-controlled h5pyd endpoint overrides - #156
Conversation
|
fwiw @vedina |
|
we should not reject segments like # , because it will break all h5viewers - we frequently want to focus on part of the h5 file, not entire one. So stripping # is no go, it will be functionality breaking. If paths are validated (non ascii, long, etc), there should be offline validator implemented, so that nexus file writers know what to comply with. Also check how applying validator on each call affect performance - e.g. h5pyd API requires 10-20 calls for properly showing domain (see e.g. latest in-host spectrasearch nexus overview router) Besides, when we use HSDS via h5view or even nexusformat library, we are not calling h5pyd.File directly . There is also React code using HSDS API directly. Therefore validation code should be in a shared library (pyambit) , not here in rcapi, otherwise our own writer may produce invalid domains. And the writer code should offer transformation given a local file path (i.e. original input file) to valid domain. This will be used in import / indexing pipeline. [Opus review pending] |
|
Reviewed at The core of this is right. Rejecting My comments are all about the rules layered on top of that core, which reject a good deal more than the URL-shaped inputs they were aimed at. On severity: I checked the rules in #1-#3 against real NeXus files and against the NXpaths the current writer produces, and each of them already has matches. There are files that store and index cleanly today which would return 400 once this lands, and study links that would stop resolving. Nothing about those inputs is hostile — they are ordinary names carrying a space, a percent sign or a non-ASCII character. The failure is also silent from the caller's side: a bare 1. The fragment is validated and then thrown away
This is not hypothetical. The fragment is an NXpath, and NXpath segments are group names generated from endpoint and instrument metadata. Names with a leading or trailing space, or containing Suggestion: split on 2.
|
|
Haha, this time I omitted my own internal Opus review, cause it was getting late night -- and here we go. :D |
I pointed Opus at the bigger picture, several related repos that are producer / consumers ; so it is not necessary the same :) |
|
Thanks for the thorough review. I have addressed the compatibility blockers in The validator now separates the file domain at the first The revision also:
I kept the request-side validator in ramanchada-api because arbitrary HTTP input must be checked at this trust boundary. A shared producer-side serializer or sanitizer in pyambit would still be useful, but it should complement rather than replace this check and is better handled as a separate cross-repository change. I have likewise left the proposed h5pyd-opening wrapper for follow-up so this PR remains focused. The validator runs once per |
|
it would be useful to run the validator them against all domains in hsds so that we know if there is anything to reindex .. |
|
Claude Opus 5 here, reviewing on behalf of Luchesar. Follow-up review of the aggregate diff Verdict: no blocking findingsThe The short version of why: in h5pyd at the pinned rev All three direct
There is no fourth. I also checked the indirect route: Worth stating the impact plainly, because the PR description undersells it: I ran ~30 adversarial variants against the head validator. All rejected, no bypass found: every URL scheme including
One deliberate divergence from the previous review, which I think is correct. @vedina wrote that the Non-blocking findingsN1 — the Also worth writing down what the rule can and cannot buy, since it is easy to over-credit: Starlette decodes the query string before the validator runs, so a literal N2 — a N3 — N4 — the error still doesn't say which rule fired. Point #2 asked for this and it didn't land; all eight raise sites use the identical N5 — the single-wrapper refactor (#4) is still open. All three sinks are guarded today and I confirmed there is no fourth, so the invariant holds — but it lives in three separate assignments, and N6 — N7 — cosmetic. The Status of the previous review's findings
Verification evidenceThe tests do hit the real FastAPI decoding boundary. This was worth checking rather than assuming — if httpx had stripped the So the Additional evidence I hadn't expected to get: CI runs the full suite for non-dependabot actors, which includes @vedina's statements about producer and consumer behaviour check out against source. Taking them as evidence and verifying rather than assuming:
Questions and assumptionsQ1. I assumed no stored Q2. I assumed the HSDS endpoint always comes from trusted runtime config. Confirmed for h5pyd's own resolution ( Residual risks, intentionally outside this PRListing these so the boundary is explicit, not as asks:
Bottom lineNo blockers. The blocking findings from the previous review (#1, #2, #3) were addressed correctly and, as far as I can tell, without introducing a regression or a bypass: I looked specifically for one in the review-driven deltas, including the -- |
|
Let me use simple words, these reports are too long and awkwardly difficult to read by a dumb human. what my concern is
Partly correct. Domain is not hsds_investigation, domain is the entire file path. Which is currently generated based on local paths , usually derived from user files. And they do have all varieties of nonascii symbols. |
|
@vedina, this is now narrowed to the endpoint override only:
Because existing file names are no longer validated, this change should not require reindexing them. New head: Could you please re-review this smaller aggregate diff? |
|
Small follow-up at Production code is unchanged. The focused suite now covers only the three endpoint-replacing prefixes, the two explicitly non-overriding h5pyd forms, one ordinary pass-through case, and rejection before each direct h5pyd sink. |
Summary
Prevent request data from replacing h5pyd's configured HSDS network endpoint.
Motivation
The
domainparameter should identify a resource on the configured HSDS service, not select another server. The pinned h5pyd version treatshttp://,https://, andhttp+unix://values as endpoint overrides and may forward caller or ambient credentials there.Solution
h5pyd.Filecall.400 Bad Requestfrom the HDF5 download route before h5pyd is invoked.domainvalue unchanged.Deliberate Scope
This PR only prevents caller-selected h5pyd network endpoints. It does not add a general HSDS path policy or change fragments, suffixes, Unicode, percent escapes, traversal-like strings, controls, relative paths, length limits, copying, errors, authorization, or producer behavior.
hdf5://and//host/pathremain unchanged because neither form selects another network endpoint in the pinned h5pyd implementation. The recognized endpoint forms must be re-audited before upgrading h5pyd.Testing
poetry run pytest tests/test_hsds_endpoint_override.py: 9 passedpoetry run pytest: 54 passed, with the existing unrelatedtests/test_api.py::test_infofailure because the image-generated build version is unavailable locallypoetry check: completed with existing deprecation warnings onlygit diff --check: passed