-
Notifications
You must be signed in to change notification settings - Fork 417
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
Conversation
Okay I was curious and did a code review on this. Oh dear that fix feels kinda half-baked. I just was keen to google on that and found:
About implementation: 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? Okay let's have a second look back into the stack trace:
So I suggest to handle any java.net.UnknownServiceException in put a On debugging and testing you may refine that errorhandler to be more specific with About documentation: 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: |
It's because Android forbids cleartext communication by default since a few versions.
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.
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.
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.
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 |
Despite not being really happy with that 'secure' crap that just creates problems. Imagine what else can go wrong with that certificate shit. Okay I definitely agree on that. So enforcing https makes sence. You will probably add some more handy methods there for url string handling ( or inherit from where it is already there.) All the code is 'hidden' in the MyUrl (type)class. |
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 |
There was a problem hiding this 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
7d0d217
to
f441036
Compare
Fixes TeamNewPipe/NewPipe#11074 by upgrading incoming links to HTTPS before making any queries towards them.