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.

Sign up for GitHub

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

Jump to bottom

allow for empty callback URL path #1049

Closed
wants to merge 1 commit into from
Closed

allow for empty callback URL path #1049

wants to merge 1 commit into from

Conversation

mandelsoft
Copy link

Summary

When configuring an empty path for the OIDC callback URL the used http MUX panics with "http: invalid pattern".

This fix uses the MUX only for non-empty callback paths. If the path is empty the handler is directly used
for the local http server without any MUX.

Release Note

Support for OIDC callback URLs without path for usage of interactive mode

Documentation

Fixes: #1048

@mandelsoft
allow for empty callback URL path
785fcbc 
When configuring an empty path for the OIDC callback URL
the used http MUX panics with "http: invalid pattern".

This fix uses the MUX only for non-empty callback paths.
If the path is empty the handler is directly used
for the local http server without any MUX.

Signed-off-by: Uwe Krueger <uwe.krueger@sap.com>
@haydentherapper
Copy link
Contributor

In what case is the callback URL empty?

@mandelsoft
Copy link
Author

mandelsoft commented Mar 29, 2023 •
edited

I wanted to use it with a local OIDC provider which was already configured with fixed callback URL for online scenario,
And this one was configured without path.

We have changed it later, but basically it should work with an empty path, also.

@haydentherapper
Copy link
Contributor

@bobcallaway What do you think about this? I’m not sure the use case, but it does clean up an error condition

@bobcallaway
Copy link
Member

@bobcallaway What do you think about this? I’m not sure the use case, but it does clean up an error condition

I'd prefer something like this (mostly b/c it has a test, and it will only listen on / instead of any URL endpoint like the code in this PR):

diff --git a/pkg/oauthflow/interactive.go b/pkg/oauthflow/interactive.go
index dfc1f0c..eee7bad 100644
--- a/pkg/oauthflow/interactive.go
+++ b/pkg/oauthflow/interactive.go
@@ -184,6 +184,9 @@ func startRedirectListener(state, htmlPage, redirectURL string, doneCh chan stri
                if err != nil {
                        return nil, nil, err
                }
+               if urlListener.Path == "" {
+                       urlListener.Path = "/"
+               }
 
                listener, err = net.Listen("tcp", urlListener.Host)
                if err != nil {
diff --git a/pkg/oauthflow/interactive_test.go b/pkg/oauthflow/interactive_test.go
index 78591bc..3c55811 100644
--- a/pkg/oauthflow/interactive_test.go
+++ b/pkg/oauthflow/interactive_test.go
@@ -16,10 +16,27 @@ package oauthflow
 
 import (
        "bytes"
+       "net/http"
+       "net/http/httptest"
        "os"
        "testing"
 )
 
+// TestIssue1048 ensures that startRedirectListener does not panic due to the lack of a specified path in the redirectURL
+func TestIssue1048(t *testing.T) {
+       doneCh := make(chan string)
+       doneErr := make(chan error)
+       // this tests calling startRedirectListener with a redirectURL without a trailing slash
+       s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }))
+       t.Log(s.URL)
+       defer func() {
+               close(doneCh)
+               close(doneErr)
+               s.Close()
+       }()
+       startRedirectListener("something", "<html><body>something</body></html>", s.URL, doneCh, doneErr)
+}
+
 func TestInteractiveFlow_IO(t *testing.T) {
        t.Run("default", func(t *testing.T) {
                f := &InteractiveIDTokenGetter{}

@haydentherapper
Copy link
Contributor

@mandelsoft Would you like to update this PR to what Bob posted?

@haydentherapper
Copy link
Contributor

Closing due to no activity, feel free to reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Panic http: invalid pattern when configuring a callback URL without path
3 participants
@mandelsoft @haydentherapper @bobcallaway