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

[Bandcamp] Upgrade incoming links to HTTPS #1177

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Conversation

fynngodau
Copy link
Member

Fixes TeamNewPipe/NewPipe#11074 by upgrading incoming links to HTTPS before making any queries towards them.

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

@fynngodau fynngodau added bug Issue is related to a bug bandcamp service, https://bandcamp.com labels May 18, 2024
@fynngodau fynngodau marked this pull request as draft May 18, 2024 05:46
@cw2k
Copy link

cw2k commented May 18, 2024

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
Copy link
Member Author

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
Copy link

cw2k commented May 19, 2024

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 fynngodau marked this pull request as ready for review May 24, 2024 17:38
@fynngodau
Copy link
Member Author

I've opened #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).

Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Needs a rebase and can be merged afterwards

@TobiGr TobiGr merged commit 312e910 into dev Jul 22, 2024
4 checks passed
@TobiGr TobiGr deleted the bandcamp-upgrade-to-https branch July 22, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bandcamp service, https://bandcamp.com bug Issue is related to a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashes when open Bandcamp
3 participants