search

TeamNewPipe / NewPipeExtractor

NewPipe's core library for extracting data from streaming sites
GNU General Public License v3.0
1.4k stars 421 forks source link

[Bandcamp] Upgrade incoming links to HTTPS #1177

Closed fynngodau closed 4 months ago

fynngodau commented 6 months ago

Fixes https://github.com/TeamNewPipe/NewPipe/issues/11074 by upgrading incoming links to HTTPS before making any queries towards them.

cw2k commented 6 months ago

Okay I was curious and did a code review on this.

Oh dear that fix feels kinda half-baked. Question #1 So did you track down why NewPipe doesn't came along with http?

I just was keen to google on that and found:

To resolve this problem add android:usesCleartextTraffic="true" to application tag in AndroidManifest.xml

https://stackoverflow.com/questions/60175852/cleartext-communication-not-permitted-by-network-security-policy-working-on-my-m

About implementation: Okay let's say turning all http to https is really necessary.

So it's about to add 6 times Utils.replaceHttpWithHttps( url ) in 5 different files.

Question #2 Wouldn't it be good to find one spot to do this transformation? I saw at least two time NewPipe.getDownloader().get(url) so wouldn't it be good to patch that ::get( ) method instead of all locations that are calling it? (no number here - is a rhetorical question.)

Okay let's have a second look back into the stack trace:

Caused by: java.net.UnknownServiceException: CLEARTEXT communication to projectconvolution.bandcamp.com not permitted by network security policy
    at okhttp3.internal.connection.RealConnection.connect(RealConnection.kt:188)
...
    at okhttp3.internal.connection.RealCall.execute(RealCall.kt:154)
    at org.schabi.newpipe.DownloaderImpl.execute(DownloaderImpl.java:163)
    at org.schabi.newpipe.extractor.downloader.Downloader.get(Downloader.java:77)

So I suggest to handle any java.net.UnknownServiceException in org.schabi.newpipe.extractor.downloader.Downloader.get(Downloader.java:77)

put a try { ... } catch (Exception e) { there and just repeat that f...ing request in the try-block with the modification of .get( Utils.replaceHttpWithHttps(url ) ).

On debugging and testing you may refine that errorhandler to be more specific with catch (UnknownServiceException e)

About documentation: Hmm I don't see any comments about that fix. Question #3 Why is that? No time, I / we know it all, forgot about, too much tying /I don't like typing.

If some workaround like this is needed, it is important to drop on or more comment in the code why the heck that Http -> Https is needed here. Or at least some comment like // HACK: or // TODO:

fynngodau commented 6 months ago

So did you track down why NewPipe doesn't came along with http?

It's because Android forbids cleartext communication by default since a few versions.

To resolve this problem add android:usesCleartextTraffic="true" to application tag in AndroidManifest.xml

There's no good reason to allow plaintext communication in the context of Bandcamp. I treat this as an extractor issue and not an app issue because other downstreams may also benefit.

Wouldn't it be good to find one spot to do this transformation?

As far as I'm aware some of the behaviour (in specific, that ID sometimes equals URL) is rather specific to the Bandcamp extractor.

Generally I am in favour of changing the Downloader such that it throws when a plaintext query is made; this way we can simulate the behaviour of a client that is not allowed to communicate over plaintext in our tests and can make that guarantee to downstreams. I don't know whether this would be considered good behaviour for the other extractors though.

If some workaround like this is needed, it is important to drop on or more comment in the code why the heck that Http -> Https is needed here.

I think the benfit of not using plaintext communication is obvious. It is furthermore obvious that there are no guarantees about the URL that the link handler is called with. I also don't see a reason for anyone to remove the call by accident in the future.

put a try { ... } catch (Exception e) { there and just repeat that f...ing request in the try-block with the modification of .get( Utils.replaceHttpWithHttps(url ) ).

That's not such a good solution because the Extractor won't necessarily run on Android and we can't know the network policy of the app that runs it. We also shouldn't generally auto-retry failed requests on the Extractor side; if a client desires this behaviour, they can always implement it themselves exactly as they need it.

I'd rather have the Downloader throw on all http queries to force the caller to guarantee https urls, like I mentioned above.

Lastly please don't use #<number> syntax on GitHub in comments unless you want to refer to issues.

cw2k commented 6 months ago

Generally I am in favour of changing the Downloader such that it throws when a plaintext query is made; this way we can simulate the behaviour of a client that is not allowed to communicate over plaintext in our tests and can make that guarantee to downstreams.

Despite not being really happy with that 'secure' crap that just creates problems. Imagine what else can go wrong with that certificate shit. _Start of poor-man-spoiler use Quote reply to watch it_

Okay I definitely agree on that. So enforcing https makes sence. So what would be the best way to 'promote' that into the code? I would propose to enforce it by type. Like change all `get( final String url,` to `get( final MyUrl url,`. This MyUrl type class encapsulates String. ... and just disallows any "http:". It'll Throws an error in the constructor it it sees some "http:". You will probably add some more handy methods there for url string handling ( or inherit from where it is already there.) It may need a little work to implement, but on the other side it'll be worth the effort since it may "clears up" the code. Like removing string handling or "Filter functions" like that replaceHttpWithHttps() to unnecessary clutter up the code. All the code is 'hidden' in the MyUrl (type)class.
fynngodau commented 6 months ago

I've opened https://github.com/TeamNewPipe/NewPipeExtractor/pull/1181 to force all extractor queries to be HTTPS.

Adding something like an opaque type would be cool, but those are supported in neither Java nor Kotlin. Other than that, I think a new class would rather increase code complexity. Considering the Java codebase, throwing in Downloader really seems like the best option to me (if not transparently upgrading).