Skip to content

Use HTTPS for MusicBrainz URLs#68

Merged
phw merged 1 commit into
metabrainz:masterfrom
Rigo-V:https
Feb 3, 2026
Merged

Use HTTPS for MusicBrainz URLs#68
phw merged 1 commit into
metabrainz:masterfrom
Rigo-V:https

Conversation

@Rigo-V

@Rigo-V Rigo-V commented Jan 22, 2026

Copy link
Copy Markdown

Since these redirect to HTTPS anyway, why not do it explicitly and maybe prevent some machine in the middle attacks.

@phw

phw commented Jan 22, 2026

Copy link
Copy Markdown
Member

Thanks. I had previously refrained from this change as it is a potential API breaking change. But probably we can risk that. The only way I see this breaking someones code is if that code would specifically check or parse the generated URLs in a way that doesn't deal with the protocol change. But we can prominently point this change out in the release notes. It's not like libdiscid is constantly changing anyway :)

In any case, the tests need to be adjusted.

Since these redirect to HTTPS anyway, do it explicitly and maybe prevent
some machine in the middle attacks or at least save some redirects.

Signed-off-by: Riku Viitanen <riku.viitanen@protonmail.com>
@Rigo-V

Rigo-V commented Jan 22, 2026

Copy link
Copy Markdown
Author

I see. I only found "http://" in test_put.c, and updated it to match.

@phw phw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, LGTM

@phw phw merged commit d9e81a9 into metabrainz:master Feb 3, 2026
5 of 6 checks passed
@phw

phw commented Feb 6, 2026

Copy link
Copy Markdown
Member

@Rigo-V I'm actually considering reverting this change. I found several usages like https://github.com/njh/perl-musicbrainz-discid/blob/main/t/10discid.t#L34 . There might be more such reliance on the URL in application code.

I updated the language bindings I personally control to not care about http / https in tests. Affected were hare-discid, rust-discid and go-discid.

But given that this is technically a breaking change we might want to avoid releasing this with the next release. Maybe announce that this will change in a future release. Or we put it behind a build flag maybe?

Also thinking about it we should likely skip this change for the web service URL completely. This method was deprecated a long time ago an the URL it returns is useless. It is only kept for backward compatibility, and breaking the URL in it seems pointless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants