add twitch#channel URI #11
Conversation
| resolvers._channel(providers, mediaURL, options, underscore.extend({ | ||
| _channel: { | ||
| providerName: 'twitch', | ||
| param1: 2, |
There was a problem hiding this comment.
yes. that's the point of the PR. i refactored so the twitch and youtube code could maximally reuse code.
There was a problem hiding this comment.
where do we use it? in other library? I can't find it in this PR
There was a problem hiding this comment.
take a look at media/provider.json!
| @@ -47,6 +47,7 @@ | |||
| "level": "1.7.0", | |||
| "npm-check-updates": "^2.12.1", | |||
| "nsp": "^2.8.0", | |||
| "oembed-parser": "^1.0.8", | |||
There was a problem hiding this comment.
we need security review for this one. I see that there is no new usage, where we just missing it from the dependencies?
cc @diracdeltas
There was a problem hiding this comment.
There was a problem hiding this comment.
we should not use ombed-parser in general since it uses Node's networking stack (https://github.com/ndaidong/oembed-parser/blob/master/src/utils/fetchEmbed.js), but this usage is ok for now since it only uses the provider.json rules file
getMedia.js
Outdated
| @@ -107,12 +121,12 @@ const resolvers = { | |||
| if (err) return next(providers, mediaURL, options, firstErr || err, callback) | |||
|
|
|||
| metascraper.scrapeHtml(body).then((result) => { | |||
| console.log('result: ' + JSON.stringify(result, null, 2)) | |||
There was a problem hiding this comment.
this contains potentially sensitive information about the page and should not be logged by default
There was a problem hiding this comment.
oops! debug thing not removed. fixed.
There was a problem hiding this comment.
This does not pass security review because it adds new remote connections in the main process (to twitch.tv). Please see https://bravesoftware.slack.com/archives/C0NPFB6H5/p1517516188000009 for details.
There was a problem hiding this comment.
as discussed with @diracdeltas we will address security problem in another PR, so we can merge this one
mrose17
dismissed
diracdeltas’s stale review
as discussed with @diracdeltas we will address security problem in another PR, so we can merge this one
|
Is this all still needed for brave/browser-laptop#13142 ? If not let's revert. |
|
yes, it is essential for twitch support in the browser; accordingly, reversion would be most unfortunate. |
res ipsa loquitur