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

Please add a way to request URL whitelisting by addon code. #34

Open
Grocel opened this issue Aug 25, 2023 · 6 comments
Open

Please add a way to request URL whitelisting by addon code. #34

Grocel opened this issue Aug 25, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@Grocel
Copy link

Grocel commented Aug 25, 2023

The Problem

Some addons might rely on external HTTP request to work properly. So does my addon 3D Stream Radio: https://github.com/Grocel/3D-Stream-Radio
In the case of the radio addon, it ships with default playlist files (lists of URLs), that can be (and are) curated by server admins. These are basically trusted by server URLs. These files are stored on the server side, but are used/seen by clients whenever it has been viewed/picked on a radio entity.
At the moment the radio can not play server curated URLs, as these are not whitelisted by default. It is useful to not allow arbitrarily random URLs, especially those from other clients. But it causes bad UX on addons such as the radio with no way to account for this properly yet. The user just would see the default playlist as "being broken" for no apparent reason.

My request

I would like to request a callable function to request a particular URL to be whitelisted on the client (the server too?) via addon code unless it has been specifically blacklisted.

The addon, which is already being trusted by being installed, would be able add their necessary HTTP calls to the whitelist.
In case of 3D Stream Radio, the addon would whitelist all the URLs in the playlist items the server has under its belt.

One way would be an addon-shipped config file, but I also would need a dynamic way for whitelisting on the fly, as the admins could change playlist contents, etc. I would like the whitelisting request to not issue any user facing prompts unless specifically asked for by addon code, e.g. by a parameter being true etc. (A better suggestion on this was posted below)

Bonus request

A server side function for whitelist requesting, so addons wouldn't need to network them to all clients by them-selves. There is also joined-after management to account for etc.

Solution

I am not sure what solution would be the best to implement, so I would like to leave it up to your creativity. I am also open for alternative solutions you might find.

@plally
Copy link
Member

plally commented Aug 25, 2023

I'm not sure about other addons dynamically adding urls to the default whitelist. I would like the default url whitelist to stay controlled by the server owner/admins as much as possible. It would be better if it required approval by someone with a specific permission.
For example an admin types !whitelistconfig and they get a popup with urls from addons to approve.
Could maybe do a chat print or some sort of notification for them if there are requests that need approval or denying.

This is a different issue but I noticed alot of your pls files have urls for streams in them, currently streams do not work with this addon installed. This is due to the content of the requests being checked by making a GET request to the url and if the HTTP request never finishes the content cant be checked with the gmod HTTP library as far as I know.

@Grocel
Copy link
Author

Grocel commented Aug 25, 2023

Yes, that popup for admin approval thing sounds fine to me. The addon requests the approval. Requested URLs are put into a pool reserved for said addon. Admin decides when ever to accept/decline/block changes in the pool. Maybe add a parameter for a description text too, so the addon could tell the admin what the approval request is about.

Something like this:

local pool = CFCHTTP.StartWhitelistRequest("addonname", "Some optional description")
pool:AddUrl("...", "Some optional description for this url")
pool:RemoveUrl("...", "not needed anymore") -- maybe, I am not sure

pool:SetCallback(function(allowed) print(allowed) end) -- gets called when the admin approves or not

pool:RequestChanges()

-- Pool is queued
-- Admin gets !whitelistconfig notification
-- !whitelistconfig shows all requested changes in a fancy UI, along with the given descriptions.

-- During checks the all addon pools are checked until one results true or all have resulted false (cached)

Rules would be:

  • Addons can only request changes for their own pool.
  • Each addon can only have one pool.
  • Using other addon's pool would be considered as abuse as like as bypassing the detoured functions by calling their originals (e.g. _G._http_Post) would be.
  • Admins must be careful what they whitelist and what addon they install.

The issue with the get request never finishing is awkward. So streams played with sound.PlayURL() will never work. Is it possible to give it a timeout or a size limit using Range? Maybe you could use HEAD method to check if it was a stream or a read able file type? These options should work with HTTP() according to the wiki.

@plally plally added the enhancement New feature or request label Aug 26, 2023
@plally plally self-assigned this Aug 26, 2023
@plally
Copy link
Member

plally commented Aug 27, 2023

I did try the range header with a stream and it did not work, I think that depends on the server actually implementing that functionality. A HEAD request wouldnt get the body of a response so I don't know if it would be a reliable way to determine if a file is a playlist file.

@Grocel
Copy link
Author

Grocel commented Aug 27, 2023

The HEAD request could give you the content type and the content length. (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers#message_body_information) For streams the type would be something like audio/mpeg or any other type that would represent audio or other non-text media. The length would be missing, zero or -1 for a stream (technically a download of unknown/infinite file size).

You would use it as a pre-flight thing, that would be started before the actual GET request. Check Content-Type and Content-Length from the HEAD request. If HEAD request fails, make it fail the check like it would do if the GET fails.

This is what I would do:

  • If it looks like a stream: Give it a pass if the GET request times out or never returns.
  • If it doesn't look like a stream: Reject the check if the GET request times out or never returns.
  • Always (in both cases) do the content check if the GET request returns.
  • If HEAD fails, make it fail (or whatever you do on failed requests) the check before the GET request becomes a thing.

This way it should be safe from any relevant HTTP manipulation, but make it usable for streaming and non-streaming content.

@Grocel
Copy link
Author

Grocel commented Sep 2, 2023

Apparently custom HTTP methods such as HEAD do not work in GMod yet. It it is being worked on:
Rubat: Fix custom HTTP methods not working

@Grocel
Copy link
Author

Grocel commented Sep 13, 2023

With the latest GMod update, the supposed issues with HTTP methods were resolved.

@plally plally mentioned this issue Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants