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

All services need a manifest.json #4

Open
micahflee opened this issue Nov 7, 2021 · 9 comments
Open

All services need a manifest.json #4

micahflee opened this issue Nov 7, 2021 · 9 comments

Comments

@micahflee
Copy link
Contributor

micahflee commented Nov 7, 2021

If there will be non-Tor Browser clients, then every service should have a machine-readable manifest at /manifest.json that the clients can read.

The manifests will describe to the client the relevant general settings, and the relevant mode settings, for each mode. This is what the mode settings object looks like: https://github.com/onionshare/onionshare/blob/develop/cli/onionshare_cli/mode_settings.py#L37-L60

Here's what they should look like for each mode:

Share mode:

{
    "version": 1,
    "mode": "share",
    "data": {
        "title": "Custom title",
        "autostop_sharing": true,
        "files": [
            { "name": "/filename1.txt", "size": 1024 },
            { "name": "/filename2.txt", "size": 42 },
            { "name": "/Documents/" },
            { "name": "/Documents/nested_file.txt", "size": 15360 },
            { "name": "/Documents/Empty Folder/" }
        ]
    }
}

The reason the autostop_sharing setting should be exposed is so the client knows if it can download individual files or not. If this is false, the client's only option is to download all files as a zip. If it's true, it can download individual files.

Filenames that end in a trailing slash are directories.

Receive mode:

{
    "version": 1,
    "mode": "receive",
    "data": {
        "title": "Custom title",
        "disable_text": false,
        "disable_files": false
    }
}

Receive mode should expose disable_text and disable_files to allow the client to know what data it can submit.

Website mode:

Website mode doesn't need a manifest.

Chat mode:

{
    "version": 1,
    "mode": "chat",
    "data": {
        "title": "Custom title"
    }
}
@tladesignz
Copy link

tladesignz commented Nov 8, 2021

I have some questions:

  1. Regarding file share mode:

This is highly OnionShare specific. Isn't there any related standard, which we could use? E.g. some manifest format, which just lists all accessible files? (Where we hide the static UI stuff.)

Or at least, remove info, which needs a lot of context to interpret?

E.g. for the "autostop_sharing": true you need to know a lot about OnionShare and the design it has. Why not just link to the zip file instead?

While thinking about that: Shouldn't we just point to the zip file, always? Or do both, like, "here's everything together", and "here's the single items".

  1. Receive mode:

Do we really need that? I mean, if the goal is, that only one app is needed, that means, both have the downloader, anyway. So for the user, this basically ends up to just being a question of who sends whom the link to operate on.

But software-wise, that means we not only need a downloader, but also an uploader. That's a lot more to implement.

Ok, maybe there's some benefit in having the users being able to decide who's the passive one, but does that justify the effort?

Besides all that: From an API design standpoint, this

"disable_text": false,
"disable_files": false

seems quite ugly. Shouldn't we rather tell the consuming client, what we support, not what we don't want? Again, right now, this info needs a lot of context, but, when we do something like this, we probably should also think about downloader clients, which aren't necessarily OnionShare.

  1. Website mode:

The "website" mode is just, well, a web server. What's the point of the manifest there at all? That thing is not meant for a non-browser to access. There's no additional information for a downloader software piece, but we do expose to an attacker, that OnionShare is used.

@grote
Copy link

grote commented Nov 8, 2021

Thanks for kicking this off Micah!

I am only looking at share mode now as this is what we'll definitely need soon on mobile.

Here are some random comments from the top of my brain:

  1. Should we include some version integer number in the root maybe, so we can make changes more easily in the future?
  2. I am a bit worried about infinite folder nesting nesting that's possible here and that could be challenging for parsers, open the door for at least memory exhaustion attacks. Do we need that nesting and if so, is there a limit?
  3. If autostop_sharing is off, how do we get the download links for nested files? Do we need to manually construct them by walking back the tree? Maybe it would be easier to clients to include absolute links instead of the name for all files?

Why not just link to the zip file instead?

While I'd also prefer the more simple approach as a developer, I understand that OnionShare desktop currently supports this feature and you probably don't want to remove the ability to download individual files from the desktop app, so the mobile download clients would need to be able to handle it as well, right?

@tladesignz
Copy link

tladesignz commented Nov 8, 2021

Why not just link to the zip file instead?

While I'd also prefer the more simple approach as a developer, I understand that OnionShare desktop currently supports this feature and you probably don't want to remove the ability to download individual files from the desktop app, so the mobile download clients would need to be able to handle it as well, right?

Sure, definitely be able to download single items, I'm not against that.

What I meant was, instead of showing highly contextual flags like autostop_sharing, why not just do, what is the effect of that flag: You cannot download single items, if this is true, but you're only able to download the ZIP file. Handing out the file listing is purely for informational purposes, in that case.

However, if we do want to keep that information, I would suggest not calling it autstop_sharing but something, which makes sense from the perspective of the consumer of the API, like single_item_download_possible=true/false or complete_archive_only=true/false.

@micahflee
Copy link
Contributor Author

I'm just now reading these replies.

@tladesignz:

  1. Regarding file share mode:

This is highly OnionShare specific. Isn't there any related standard, which we could use? E.g. some manifest format, which just lists all accessible files? (Where we hide the static UI stuff.)

Or at least, remove info, which needs a lot of context to interpret?

E.g. for the "autostop_sharing": true you need to know a lot about OnionShare and the design it has. Why not just link to the zip file instead?

While thinking about that: Shouldn't we just point to the zip file, always? Or do both, like, "here's everything together", and "here's the single items".

The information listed for each mode just maps to the settings that can be set for that mode. Since the goal of the manifest file is to make OnionShare machine-readable so we can have non-Tor Browser clients (starting with Android and iOS, for at least certain modes), I think it makes sense to include the relevant settings that I client would need to understand in order to fully implement it.

The link to download all the files is always /download. If more than one file is shared, then the downloads a zip. If only one files is shared, that downloads just one file. The HTTP response includes the header Content-Disposition: attachment; filename="filename.zip", and then serves the file in the body. So, the manifest doesn't actually have to include a download link.

The reason why it makes sense to include a full directory listing in all cases is so the client can show you what's in the the zip that you're about to download. Even if you can't download individual items, you should still be able to browse the folders to see what's there before deciding to download.

  1. Receive mode:

Do we really need that? I mean, if the goal is, that only one app is needed, that means, both have the downloader, anyway. So for the user, this basically ends up to just being a question of who sends whom the link to operate on.

That's only the goal for Android and iOS. Since receive mode isn't part of MVP the mobile apps can ignore it for now, and it probably makes sense to do some user study work to figure out if it should ever be supported in mobile.

Receive mode's primary use-case in desktop I think would be the same in mobile: offer a way for people to anonymously send you tips. You can post the onion link in your social media profiles, your website, etc., and then people can safely send you information without needing to figure out how to make an anonymous email address, without having to trust a service provider, etc. I could see it being useful for the anonymous tips to go straight to your phone.

Besides all that: From an API design standpoint, this

"disable_text": false,
"disable_files": false

seems quite ugly. Shouldn't we rather tell the consuming client, what we support, not what we don't want?

It's just because disable_text and disable_files map directly to the receive mode setting names in the desktop app.

receive

We could use friendlier names, but it would just mean we'll have different names for the same settings in the server vs in the client.

  1. Website mode:

The "website" mode is just, well, a web server. What's the point of the manifest there at all? That thing is not meant for a non-browser to access. There's no additional information for a downloader software piece, but we do expose to an attacker, that OnionShare is used.

This is a great point! I agree that website mode doesn't need a manifest.

@grote:

  1. Should we include some version integer number in the root maybe, so we can make changes more easily in the future?

Yes, good idea.

  1. I am a bit worried about infinite folder nesting nesting that's possible here and that could be challenging for parsers, open the door for at least memory exhaustion attacks. Do we need that nesting and if so, is there a limit?

What if we make it way simpler, like instead of a nested object, just a list, with filenames that have trailing slashes being directories? Like:

[
    "/filename1.txt",
    "/filename2.txt",
    "/Documents/",
    "/Documents/nested_file.txt",
    "/Documents/Empty Folder/"
]

Although, it occurs to me that the client will probably also want the file size in bytes for each file, which isn't included above... So maybe something like:

[
    { "name": "/filename1.txt", "size": 1024 },
    { "name": "/filename2.txt", "size": 42 },
    { "name": "/Documents/" },
    { "name": "/Documents/nested_file.txt", "size": 15360 },
    { "name": "/Documents/Empty Folder/" }
]
  1. If autostop_sharing is off, how do we get the download links for nested files? Do we need to manually construct them by walking back the tree? Maybe it would be easier to clients to include absolute links instead of the name for all files?

Good point. If we change the file listing structure to above, then the download link for an individual file would just be its name. However, you will need to manually parse these if you plan to build a file browser, e.g. list everything in the root, and make Documents a link that when clicked lists everything in that folder, etc.

While I'd also prefer the more simple approach as a developer, I understand that OnionShare desktop currently supports this feature and you probably don't want to remove the ability to download individual files from the desktop app, so the mobile download clients would need to be able to handle it as well, right?

Yes, I would definitely prefer it if, for share mode at least, mobile and desktop were interoperable.

@micahflee
Copy link
Contributor Author

I edited the text of the issue up at top to reflect some of the stuff you've all suggested.

@grote
Copy link

grote commented Nov 15, 2021

The HTTP response includes the header Content-Disposition: attachment; filename="filename.zip"

Where does that filename come from btw.? Is there a rule for generating it?

instead of a nested object, just a list, with filenames that have trailing slashes being directories?

Sounds good!

If we change the file listing structure to above, then the download link for an individual file would just be its name. However, you will need to manually parse these if you plan to build a file browser, e.g. list everything in the root, and make Documents a link that when clicked lists everything in that folder, etc.

Right. I guess there can be no slashes in the filenames, right? So each slash is a file path separator no matter which platform OnionShare was generating them on? In this case, I guess splitting a string by slashes is still better than supporting infinite nesting.

It would be a bit more tricky to build the folder structure, but I don't see a way around this when not using potentially infinite trees.

Btw. could there be zip bomb attacks? The zipped json that gets send over the wire is small, but explodes in the app that wants to parse it?

@micahflee
Copy link
Contributor Author

Where does that filename come from btw.? Is there a rule for generating it?

If more than one file is shared and it zips them up, the filename is generated here:

https://github.com/onionshare/onionshare/blob/develop/cli/onionshare_cli/web/share_mode.py#L546-L548

Basically, it's onionshare_XXXXXX.zip where XXXXXX is 6 random lower-case letters or numbers. (I'm open to changing this if anyone has a better suggestion.)

Right. I guess there can be no slashes in the filenames, right? So each slash is a file path separator no matter which platform OnionShare was generating them on? In this case, I guess splitting a string by slashes is still better than supporting infinite nesting.

It would be a bit more tricky to build the folder structure, but I don't see a way around this when not using potentially infinite trees.

Btw. could there be zip bomb attacks? The zipped json that gets send over the wire is small, but explodes in the app that wants to parse it?

These are great questions, and I honestly haven't given them much thought at all because OnionShare has never been a client before. We could do some validation, e.g. w can decide to limit the nesting to 100 folders deep (to pick something arbitrary) and throw errors if it's deeper than that.

@tladesignz
Copy link

The information listed for each mode just maps to the settings that can be set for that mode. Since the goal of the manifest file is to make OnionShare machine-readable so we can have non-Tor Browser clients (starting with Android and iOS, for at least certain modes), I think it makes sense to include the relevant settings that I client would need to understand in order to fully implement it.

Yes, obviously your goal was to just map out relevant settings, and of course, from the view point of the programmers of the app, it's easier to keep the naming scheme.

However, the point I want to drive home here, is, that, when you design an API, IMHO you should design it from the consumer's perspective. You should want to make it as easy as possible for somebody from the outside to understand what each knob is supposed to do, without consulting the source code or the UI first.

That means:

  • No implicit behaviour and behaviour changes.
  • Names reflect, what they mean for the response of the API, not any other abstract concept which has nothing to do with it.
  • No hidden features.

Imagine somebody, who doesn't have a clue what OnionShare is, but wants to extend their downloader browser plugin to assist their users in automatically downloading stuff from OnionShare endpoints.

The link to download all the files is always /download. If more than one file is shared, then the downloads a zip. If only one files is shared, that downloads just one file. The HTTP response includes the header Content-Disposition: attachment; filename="filename.zip", and then serves the file in the body. So, the manifest doesn't actually have to include a download link.

Yes, I already figured that out, and I suggest, you put that detailed info in the test plan.

About implicit behaviour... see my last comment.

The reason why it makes sense to include a full directory listing in all cases is so the client can show you what's in the the zip that you're about to download. Even if you can't download individual items, you should still be able to browse the folders to see what's there before deciding to download.

Agreed, that makes sense. I therefore suggest two sections:

  1. List of items you can download
  2. If (1) is one or multiple archives, listing of contents of that archive.

(Might actually better be nested instead of two sections.)

Since receive mode isn't part of MVP the mobile apps can ignore it for now, and it probably makes sense to do some user study work to figure out if it should ever be supported in mobile.
👍

We could use friendlier names, but it would just mean we'll have different names for the same settings in the server vs in the client.

Again, see my comment above...

This is a great point! I agree that website mode doesn't need a manifest.
👍

[
    { "name": "/filename1.txt", "size": 1024 },
    { "name": "/filename2.txt", "size": 42 },
    { "name": "/Documents/" },
    { "name": "/Documents/nested_file.txt", "size": 15360 },
    { "name": "/Documents/Empty Folder/" }
]

This looks great to me! I suggest to drop the leading slash, though, because that points to the root file system, typically. Paths are relative to the root of the ZIP or the OnionShare URL, so should these. (e.g. ZIP file entries are like that, too, AFAIK)

Btw. could there be zip bomb attacks? The zipped json that gets send over the wire is small, but explodes in the app that wants to parse it?

The JSON gets zipped? You mean implicitly gziped by the web server software?

So, then, that's an inherent risk for all clients of any JSON API, isn't it? And the unzipping is transparently done by the HTTP client library used, which should, hopefully, be robust enough to not eat all the devices memory. No? Seems, like we would get sidetracked a lot with that question, if it turns out, there's no good HTTP client libraries out there handling that case.

@tladesignz
Copy link

Picking this up again, because this is a needed feature.

Should we put the manifest in the .well-known folder?
https://en.wikipedia.org/wiki/Well-known_URI

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

No branches or pull requests

3 participants