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

Bug: MusicBrainz lookup URL is hardcoded to always use https #437

Closed
unkissedfrog opened this issue Dec 25, 2019 · 5 comments · Fixed by #450
Closed

Bug: MusicBrainz lookup URL is hardcoded to always use https #437

unkissedfrog opened this issue Dec 25, 2019 · 5 comments · Fixed by #450
Labels
Accepted Accepted issue on our roadmap Bug Generic bug: can be used together with more specific labels Priority: low Low priority
Milestone

Comments

@unkissedfrog
Copy link

Hi,
when trying to copy a CD, whipper seems not to be able to establish a network connection (perhups because of trying to resolv the musicbrainz address via https on port 80?):

INFO:whipper.command.cd:using configured read offset 99
INFO:whipper.command.cd:checking device /dev/sr3
CDDB disc id: 7407300a
MusicBrainz disc id EXBOGqIv3kyBD7MFtN2djbtQteA-
MusicBrainz lookup URL https://musicbrainz.org:80 ; use MusicBrainz server at host[:port]/cdtoc/attach?toc=1+10+138172+150+15907+31804+45701+57296+72532+83266+95846+105313+126155&tracks=10&id=EXBOGqIv3kyBD7MFtN2djbtQteA-
Disc duration: 00:30:40.293, 10 audio tracks
WARNING:whipper.common.program:network error: (NetworkError(),)
WARNING:whipper.common.program:continuing without metadata
Submit this disc to MusicBrainz at the above URL.

INFO:whipper.command.cd:FreeDB identifies disc as ['Airbourne / Boneshaker']
CRITICAL:whipper.command.cd:unable to retrieve disc metadata, --unknown argument not passed

Passing the --unknown argument works fine.

I used version 0.9

whipper -v
whipper 0.9.0

@github-actions
Copy link

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

To help make it easier for us to investigate your issue, please follow the contributing instructions.

@JoeLametta JoeLametta changed the title WARNING:whipper.common.program:network error: (NetworkError(),) Bug: MusicBrainz lookup URL is hardcoded to always use https Dec 28, 2019
@JoeLametta
Copy link
Collaborator

JoeLametta commented Dec 28, 2019

Hi, I think I've identified the cause of this issue: title updated.

Method getMusicBrainzSubmitURL, lines 400 & 401:

def getMusicBrainzSubmitURL(self):
host = config.Config().get_musicbrainz_server()
discid = self.getMusicBrainzDiscId()
values = self._getMusicBrainzValues()
query = urlencode([
('toc', ' '.join([str(v) for v in values])),
('tracks', self.getAudioTracks()),
('id', discid),
])
return urlunparse((
'https', host, '/cdtoc/attach', '', query, ''))

The example configuration included in the README uses musicbrainz.org:80 as server. In that case whipper tries to connect to a URL with the https scheme but 80 (http) as port number.
80, 443 are standard ports but not the only ones allowed...

@JoeLametta JoeLametta added Accepted Accepted issue on our roadmap Bug Generic bug: can be used together with more specific labels Priority: low Low priority labels Dec 28, 2019
@JoeLametta JoeLametta added this to the 1.0 milestone Dec 28, 2019
@Freso
Copy link
Member

Freso commented Dec 29, 2019

This would be a good GCI task, esp. since we have a student churning through GCI whipper tasks. :)

@ABCbum
Copy link
Contributor

ABCbum commented Dec 29, 2019

This would be a good GCI task, esp. since we have a student churning through GCI whipper tasks. :)
@Freso 😆😆, thanksss

@JoeLametta
Copy link
Collaborator

JoeLametta commented Jan 10, 2020

Crosspost (from IRC) with slightly more details:

TL;DR: I think the issue the user experienced was an unrelated network error. Still there are bugs:

  1. MusicBrainz submit URL always has https as protocol: hardcoded, even when inappropriate. It's just a graphical issue.
  2. Whipper appears to always communicate with MusicBrainz using musicbrainzngs over http. The musicbrainzngs.set_hostname(server) method is called every time (even when no custom server has been defined). Related to bug number 3.
  3. musicbrainzngs.set_hostname(server)
    always defaults to http. With musicbrainzngs version 0.7 (tagged yesterday), the method set_hostname takes an optional argument named use_https (it defaults to False) which whipper never passes. Relevant upstream issue: Use HTTPS for web service queries alastair/python-musicbrainzngs#197

Bug number 1 is actually the one described in this issue.

JoeLametta added a commit to ABCbum/whipper that referenced this issue Jan 16, 2020
The tests need to be updated.

Fixes:
- MusicBrainz submit URL always has https as protocol: hardcoded, even when
inappropriate. It's just a graphical issue.
- Whipper appears to always communicate with MusicBrainz using musicbrainzngs
over http. The musicbrainzngs.set_hostname(server).
- musicbrainzngs.set_hostname(server) always defaults to http. With musicbrainzngs
version 0.7 the method `set_hostname` takes an optional argument named `use_https`
(defaults to `False`) which whipper never passes.

Fixes whipper-team#437.

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
JoeLametta added a commit to ABCbum/whipper that referenced this issue Jan 16, 2020
The tests need to be updated.

Fixes:
- MusicBrainz submit URL always has https as protocol: hardcoded, even when
inappropriate. It's just a graphical issue.
- Whipper appears to always communicate with MusicBrainz using musicbrainzngs
over http. The musicbrainzngs.set_hostname(server).
- `musicbrainzngs.set_hostname(server)` always defaults to http. Since musicbrainzngs
version 0.7 the method `set_hostname` takes an optional argument named `use_https`
(defaults to False) which whipper never passes.

Fixes whipper-team#437.

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
JoeLametta added a commit to ABCbum/whipper that referenced this issue Jan 17, 2020
Fixed some bugs:
- MusicBrainz submit URL always has https as protocol: hardcoded, even when
inappropriate. It's just a graphical issue.
- Whipper appears to always communicate with MusicBrainz using musicbrainzngs
over http. The musicbrainzngs.set_hostname(server).
- `musicbrainzngs.set_hostname(server)` always defaults to http. Since musicbrainzngs
version 0.7 the method `set_hostname` takes an optional argument named `use_https`
(defaults to False) which whipper never passes.

Changed behaviour of `server` option (`musicbrainz` section of whipper's configuration file).
Now it expects an URL with a valid scheme (scheme must be `http` or `http`, empty scheme isn't allowed anymore).
Only the scheme and netloc parts of the URL are taken into account.

Fixes whipper-team#437.

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Bug Generic bug: can be used together with more specific labels Priority: low Low priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants