Skip to content
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 query strings for precomputed chunks URLs #316

Open
Tomaz-Vieira opened this issue Jun 9, 2021 · 3 comments · May be fixed by #322
Open

Allow query strings for precomputed chunks URLs #316

Tomaz-Vieira opened this issue Jun 9, 2021 · 3 comments · May be fixed by #322

Comments

@Tomaz-Vieira
Copy link
Contributor

Currently, neuroglancer will error out on datasets of the kind precomputed://http:// if they contains query parameters. This is particularly annoying when serving data from Opentsack Swift's temporary URLs, since those come with signatures in the query parameters.

It seems natural to allow URLs to be derived from the base URL as usual, but also appending the query params to the string, but I haven't given too much thought on what consequences that could have.

@jbms
Copy link
Collaborator

jbms commented Jun 9, 2021

I think this would be reasonable to support. The only downside is that it prevents using the query string for any neuroglancer-specific parameters. But instead precomputed://...#parameter=value can be used for that purpose.

@Tomaz-Vieira
Copy link
Contributor Author

As I was messing around with this, I also discovered a bug (or maybe a feature, depending on how you look at it), so I figured I'd ask before I submit a PR.

the resolvePath function in src/neuroglancer/datasource/precomputed/frontend.ts can be exploited by a carefully generated info. I discovered this when I was trying to get my server to generate a "patched" version of a precomputed chunks info that resided on a third party server, and that still directed Neuroglancer to fetch the tiles from that third party server (instead of fetching them from my own server). I eventually found that setting the scale.key to something like ../../../../../http://third-party-server/path/to/scale would achieve just that. Following is a working example:

resolvePath("http://example.com/some/path", "../../../../../http://somewhere-else/some/other/path")
// resolves to "http://somewhere-else/some/other/path"

Now, this works, but doesn't look at all like something I should be doing. It is useful in some cases, though. I haven't given too much thought to the security implications, but in principle it seems that the scale keys could work just like any other URLs on HTML elements: relative paths would be relative to the path of the info file, absolute paths would start from the info file domain, and paths with a preceding protocol would be able to point anywhere on the web, including other domains. And then resolvePath would handle all of those cases gracefully. I understand, though, that this would change the precomputed chunks spec in a way what might be backwards-incompatible.

I still managed to achieve what I was trying to do by awkwardly redirecting every request for a tile to the third-party URL where that tile resides, but that introduces one unnecessary round trip and is some more pointless load on my server.

Any thoughts on that?

@jbms
Copy link
Collaborator

jbms commented Jun 21, 2021

This was indeed not intended functionality. I don't think there is any security risk in Neuroglancer, since neuroglancer supports only read access, but I think it would be better not to allow it in order to avoid incompatibility with other implementations.

If this were supported by implementations like tensorstore that support writing, it could be used similarly to a symlink attack to overwrite unintended data, though I suppose there could be an option to limit the allowed redirect destinations.

The other issue is that it would force other implementations to support the same protocols as neuroglancer. Neuroglancer currently supports some custom protocols like gs:// and gs+ngauth://, and it would be somewhat unfortunate to have to support these in other implementations. In tensorstore there is also an explicit key-value store abstraction, that the precomputed format is built on top of, and this redirect functionality doesn't really fit with that.

On the other hand potentially this redirect functionality could be documented as being supported exclusively by neuroglancer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants