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

Incorrect port when BASE_URL contains protocol's default port #155

Closed
pedorich-n opened this issue Jun 4, 2024 · 6 comments
Closed

Incorrect port when BASE_URL contains protocol's default port #155

pedorich-n opened this issue Jun 4, 2024 · 6 comments

Comments

@pedorich-n
Copy link

pedorich-n commented Jun 4, 2024

Describe the bug
I am running the multi-scrobbler behind the reverse proxy on the address http://multi-scrobbler.server.lan and specify the same address in BASE_URL (also tried http://multi-scrobbler.server.lan:80).

Two issues arise:

  1. Because URL trims the default protocol's port (80 for HTTP and 443 for HTTPS), MS assumes I didn't specify the port and tries to use its default port for the base url instead. Which is not correct in my case:
    const base = normalizeUrl(baseUrl ?? 'http://localhost', {removeSingleSlash: true});
    const u = new URL(base);
    let localUrl = u.toString();
    if(u.port === '' && u.pathname === '/') {
    localUrl = `${u.origin}:${items.mainPort}`;
    }
  2. The items.mainPort in the code mentioned above doesn't exist, so I ended up with http://multi-scrobbler.server.lan:undefined as a base url for redirects. Looks like this commit caused this issue.

Expected behavior
Use the default port when specified.

Logs

[App] [API] User-defined base URL for UI and redirect URLs (spotify, deezer, lastfm): http://multi-scrobbler.server.lan:undefined

Versions (please complete the following information):

  • multi-scrobbler: 0.7.1, docker

Additional context
The port gets lost after normalization because the normalize-url also uses URL.

I think the code should either assume the port is 80 when not specified, as it seems to be the behavior of URL, or at least respect the explicitly specified port even when it's the default.

The current workaround for me is to explicitly specify the redirect URLs for sources.

@FoxxMD
Copy link
Owner

FoxxMD commented Jun 4, 2024

Thanks for catching this and the detailed write up!

FoxxMD added a commit that referenced this issue Jun 4, 2024
* Move code into testable function
* Fix port erasure when url uses standard port but it is explicitly specified
* Simplify building default url and fix using undefined mainPort variable
* Fix port erasure when https protocol is explicitly specified
* Refactor localUrl usage as URL instead of string
  * Implement pathname joining on URL to make handling trailing/leading slashes take place in nodejs lib
* Implements tests for URL building/manipulation
@FoxxMD
Copy link
Owner

FoxxMD commented Jun 4, 2024

@pedorich-n please try foxxmd/multi-scrobbler:develop docker image and let me know if that fixes URL issues for you

@pedorich-n
Copy link
Author

pedorich-n commented Jun 5, 2024

Thanks for such a quick fix!
The URL does appear to be correct now, with the BASE_URL=http://multi-scrobbler.server.lan:80:

[App] [API] User-defined base URL for UI and redirect URLs (spotify, deezer, lastfm): http://multi-scrobbler.server.lan/
...
[App] [Sources] [Spotify - spotify] Redirect URL that will be used on auth callback: 'http://multi-scrobbler.server.lan/callback'

But I am getting a 400 HTTP code from MS when trying to authorize Spotify now. I don't know if that's something on MS's side or my reverse proxy or what... The URL I see is http://multi-scrobbler.server.lan/api/callback?code=<code>&state=spotify. Where did the /api/ come from? 🤔

In the logs, I can see

[2024-06-05 20:07:04.265 +0900] INFO   : [App] [API] Received auth code callback from Spotify
[2024-06-05 20:07:02.364 +0900] INFO   : [App] [API] Redirecting to spotify authorization URL

But the page still shows "Auth Interaction Required" and no currentCreds-spotify.json file is created.


Edit:
Upon rolling back to 0.7.1 and using the explicit redirectUri everything works again. So I assume something changed in the dev branch. 🤔 Also I can see /api/ in URL too, so I guess that's not the problem.

@FoxxMD
Copy link
Owner

FoxxMD commented Jun 5, 2024

Oops! Missed a string conversion of the base url when passing to the spotify api. Try the newest develop tag, it should be fixed now.

@pedorich-n
Copy link
Author

Yes, it works now!
Thank you for the fix!

@FoxxMD
Copy link
Owner

FoxxMD commented Jul 9, 2024

Released in v0.8.0

@FoxxMD FoxxMD closed this as completed Jul 9, 2024
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

No branches or pull requests

2 participants