Skip to content

Serve local file assets through loopback proxy - #39

Open
ArtBreguez wants to merge 2 commits into
OpenFusionProject:mainfrom
ArtBreguez:main
Open

ArtBreguez wants to merge 2 commits into
OpenFusionProject:mainfrom
ArtBreguez:main

Conversation

@ArtBreguez

Copy link
Copy Markdown

Summary

  • Serve custom local file:/// asset directories through a loopback HTTP server when proxied asset downloads are enabled.
  • Keep the local directory as the server root so main.unity3d, .resourceFiles, and additional .unity3d assets resolve through the same --asseturl path used for remote assets.
  • Decode file URIs and URL-encoded request paths while rejecting traversal attempts.

Validation

  • npm ci completed successfully.
  • npm run build passed compilation, lint, type checking, page generation, and optimization.
  • cargo fmt --check passed.
  • Added Rust unit coverage for file URI parsing, URL decoding, path joining, and parent-directory rejection.

@datah4zard

Copy link
Copy Markdown
Member

Thanks for the PR. Unfortunately, I don't think this is the right approach:

  • This is a lot of code for an overcomplicated solution. The point of the proxy is to work around the client's lack of TLS support. The client already has native support for the file:// protocol, so all of this machinery can be avoided by just not spinning up the proxy server when the asset URL is using file scheming.
  • The additions don't really follow the conventions of the codebase. Unit tests are good in practice, but you added a bunch to exercise code that is unlikely to break. On top of that, the guards for traversal attacks aren't needed; there is no writing, only reading, and that goes straight to the game client, so it's an empty threat.
  • In the future, can you please update the PR description with why this fixes the issue you're tackling? You documented the change but not how you came up with the fix. This would have probably helped guide you in the right direction early on.

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 this pull request may close these issues.

2 participants