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

WebTorrent extension progress #7

Closed
wants to merge 1 commit into from
Closed

WebTorrent extension progress #7

wants to merge 1 commit into from

Conversation

feross
Copy link
Collaborator

@feross feross commented Nov 11, 2016

  • set 'key' field in manifest, for deterministic extension ID
  • Use require('webtorrent') and require('create-torrent')
    • Add webpack loader for json and 'fs' empty shim
  • Replace require('magnet-uri') with require('parse-torrent') which is
    super-set
  • Fix TODOs
  • Move state mutation out of render()
  • Implement "Save .torrent File" button
  • Remove "Copy Magnet Link" button (already possible via location bar)
  • var -> let, const
  • Make UI look like part of Brave

- set 'key' field in manifest, for deterministic extension ID
- Use require('webtorrent') and require('create-torrent')
  - Add webpack loader for json and 'fs' empty shim
- Replace require('magnet-uri') with require('parse-torrent') which is
super-set
- Fix TODOs
- Move state mutation out of render()
- Implement "Save .torrent File" button
- Remove "Copy Magnet Link" button (already possible via location bar)
- var -> let, const
- Make UI look like part of Brave
@Flet
Copy link

Flet commented Nov 11, 2016

Ohh, this looks neat :)

}
window.state = state /* for easier debugging */

// TODO: REMOVE THIS HACK ONCE THIS IS ADDRESSED:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer need this now that WebTorrent it in a node process, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you're right.

function handleError (error) {
state.errorMessage = error.message
}
start () {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean!

}

// TODO(feross): Test how this behaves with different file types
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was experimenting with this last night, and ended up removing resizeToContent entirely.

Sizing an <iframe> to its content is pretty janky in the best situations, and doesn't work at all in cases like this where the outer URL and inner URL have different schemes. (chrome-extension vs http)

Copy link
Owner

@dcposch dcposch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dcposch
Copy link
Owner

dcposch commented Nov 11, 2016

After experimenting with it, I think that <iframe> resizing won't work.

Here's an alternative UX

  • A whole torrent (eg, a magnet link with no ix property) looks the same as it does now. Start button, download status, file list if available with hyperlinks to each file.
  • A single file (a magnet link with ix=<file index>) does one of two things.
    • If the torrent is not yet running, it redirects to the whole-torrent page, where you get a Start button.
    • If the torrent is running, the URL used under the hood points directly to the webtorrent-to-http server. The URL bar still says magnet:?..., but under the hood, rather than loading chrome-extension://.../webtorrent.html?..., we load http://localhost:<high port>/<file index>. This way, there's no iframe, and the browser handles the file in the same way as if it were loaded over HTTP.

Security

  • Say someone puts an HTML file containing scripts into a torrent, and we load it as described above. The browser sandbox should prevent it from doing harm, but we would have to review the webtorrent HTTP server carefully to ensure it can't be exploited, because user scripts would be able to send it requests. The easiest solution would be to disable scripts completely in content loaded from torrents.
  • Related: what if someone makes a torrent where the first file is HTML and contains <img src="/1">? The second file (index 1) is a JPG. This would work--which is actually pretty cool, IMO!--but maybe we should make the webtorrent-to-HTTP server use filenames instead of file indices to let people do this in a way that's more readable.

Thoughts? @feross @yan

@dcposch
Copy link
Owner

dcposch commented Nov 12, 2016

cherry-picked!

@dcposch dcposch closed this Nov 12, 2016
@feross
Copy link
Collaborator Author

feross commented Nov 12, 2016

I like the alternate UX. I don't think we want to actually redirect ix=__ magnet links to an http://localhost url since then we need more code to map that back to magnet: in the location bar.

Proposal: we keep the chrome-extension:// URL under-the-hood for ease of implementation (since this is how it's already done), but when we see ix=__ and the torrent is already active, then we render the content into an iframe and make it `position: absolute; top: 0; left: 0; right: 0; bottom: 0; so it takes up the full viewport and has it's own scrollbar (when necessary).

@feross
Copy link
Collaborator Author

feross commented Nov 12, 2016

maybe we should make the webtorrent-to-HTTP server use filenames instead of file indices to let people do this in a way that's more readable.

That should be a pretty easy PR to webtorrent. Sounds good to me. But I thought we're ignoring the webpage in a torrent use-case for now?

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.

3 participants