-
Notifications
You must be signed in to change notification settings - Fork 369
Extend unmanaged OAuth flow to drive code exchange in-process #2896
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
c97124d
Extend unmanaged OAuth flow to drive code exchange in-process
trungutt a93ae4d
Add out-of-band callback route for unmanaged OAuth drive-flow
trungutt 6dd7608
Rename OAuth elicitation meta keys from cagent/ to docker-agent/
trungutt 3e8c310
Propagate user-initiated cancellation across the WithoutCancel boundary
trungutt 111c56f
Fix lint issues in OAuth tests and helpers
trungutt 10c7a3c
Merge remote-tracking branch 'upstream/main' into extend-unmanaged-oa…
trungutt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| package server | ||
|
|
||
| import ( | ||
| "context" | ||
| "io" | ||
| "net" | ||
| "net/http" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/docker/docker-agent/pkg/config" | ||
| ) | ||
|
|
||
| // httpDoStatus is a slim variant of httpDo that exposes the response | ||
| // status code. The standard helper assumes 2xx and only returns the body; | ||
| // these tests assert on 4xx, so they need direct access to the status. | ||
| func httpDoStatus(t *testing.T, ctx context.Context, method, socketPath, path string) int { | ||
| t.Helper() | ||
| req, err := http.NewRequestWithContext(ctx, method, "http://_"+path, http.NoBody) | ||
| require.NoError(t, err) | ||
| client := &http.Client{ | ||
| Transport: &http.Transport{ | ||
| DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) { | ||
| var d net.Dialer | ||
| return d.DialContext(ctx, "unix", strings.TrimPrefix(socketPath, "unix://")) | ||
| }, | ||
| }, | ||
| } | ||
| resp, err := client.Do(req) | ||
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
| _, err = io.Copy(io.Discard, resp.Body) | ||
| require.NoError(t, err) | ||
| return resp.StatusCode | ||
| } | ||
|
|
||
| func startServerBare(t *testing.T, ctx context.Context) string { | ||
| t.Helper() | ||
| var store mockStore | ||
| runConfig := config.RuntimeConfig{} | ||
| sources, err := config.ResolveSources(t.TempDir(), nil) | ||
| require.NoError(t, err) | ||
| srv, err := New(ctx, store, &runConfig, 0, sources, "") | ||
| require.NoError(t, err) | ||
|
|
||
| socketPath := "unix://" + filepath.Join(t.TempDir(), "sock") | ||
| ln, err := Listen(ctx, socketPath) | ||
| require.NoError(t, err) | ||
| go func() { <-ctx.Done(); _ = ln.Close() }() | ||
| go func() { _ = srv.Serve(ctx, ln) }() | ||
| return socketPath | ||
| } | ||
|
|
||
| // The happy path (waiter registered, callback delivered) is covered end | ||
| // to end in TestUnmanagedOAuthFlow_DriveFlow_AcceptsDirectCallback in | ||
| // pkg/tools/mcp. The server-side tests here focus on the input | ||
| // validation and the 404 response shape so the embedder's HTTP client | ||
| // can rely on it. | ||
|
|
||
| // Short test names because the macOS unix-socket path limit (104 bytes) | ||
| // includes t.TempDir() which embeds the test name. | ||
|
|
||
| func TestMcpOAuthCb_Unknown(t *testing.T) { | ||
| ctx := t.Context() | ||
| lnPath := startServerBare(t, ctx) | ||
|
|
||
| status := httpDoStatus(t, ctx, http.MethodPost, lnPath, | ||
| "/api/mcp-oauth/callback?state=unknown-state&code=abc") | ||
| assert.Equal(t, http.StatusNotFound, status) | ||
| } | ||
|
|
||
| func TestMcpOAuthCb_NoState(t *testing.T) { | ||
| ctx := t.Context() | ||
| lnPath := startServerBare(t, ctx) | ||
|
|
||
| status := httpDoStatus(t, ctx, http.MethodPost, lnPath, | ||
| "/api/mcp-oauth/callback?code=abc") | ||
| assert.Equal(t, http.StatusBadRequest, status) | ||
| } | ||
|
|
||
| func TestMcpOAuthCb_NoCode(t *testing.T) { | ||
| ctx := t.Context() | ||
| lnPath := startServerBare(t, ctx) | ||
|
|
||
| status := httpDoStatus(t, ctx, http.MethodPost, lnPath, | ||
| "/api/mcp-oauth/callback?state=some-state") | ||
| assert.Equal(t, http.StatusBadRequest, status) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[MEDIUM]
/api/mcp-oauth/callbackis unauthenticated when no bearer token is configuredThe endpoint is registered under the
groupthat only hasBearerTokenMiddlewarewhenauthToken != "":When the server runs without
--auth-token(a documented/valid configuration), any network-reachable client can POST to/api/mcp-oauth/callback?state=<s>&code=<c>with no credentials.The pending-OAuth registry provides meaningful defence: only states the runtime has generated and is currently awaiting are accepted (unknown states → 404). However, if an attacker can observe a live state value (it appears in the elicitation
metasent to the client over the session stream, at debug-level logs, and in the authorize URL the client is asked to open), they can inject a malicious authorizationcode. The token exchange would then proceed with an attacker-controlled code, potentially yielding the attacker's own tokens bound to the victim's session.Suggestions:
--auth-tokenweakens the callback endpoint's security posture.