fix: prevent session token exfiltration via external app URLs by zedkipp · Pull Request #26146 · coder/coder · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions cli/open.go
99 changes: 97 additions & 2 deletions cli/open_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package cli_test

import (
"bytes"
"context"
"database/sql"
"net/url"
"os"
"path"
Expand All @@ -21,6 +23,9 @@ import (
"github.com/coder/coder/v2/agent/agenttest"
"github.com/coder/coder/v2/cli/clitest"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbfake"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/provisionersdk/proto"
Expand Down Expand Up @@ -703,14 +708,16 @@ func TestOpenApp(t *testing.T) {
w.RequireContains("region not found")
})

t.Run("ExternalAppSessionToken", func(t *testing.T) {
t.Run("ExternalAppOnTopLevelAgentSubstitutes", func(t *testing.T) {
t.Parallel()

// Apps on the top-level (template-defined) agent are trusted, so the
// CLI substitutes $SESSION_TOKEN regardless of scheme.
client, ws, _ := setupWorkspaceForAgent(t, func(agents []*proto.Agent) []*proto.Agent {
agents[0].Apps = []*proto.App{
{
Slug: "app1",
Url: "https://example.com/app1?token=$SESSION_TOKEN",
Url: "vscode://coder.coder-remote/open?token=$SESSION_TOKEN",
External: true,
},
}
Expand All @@ -724,4 +731,92 @@ func TestOpenApp(t *testing.T) {
w.RequireContains("test.open-error")
w.RequireContains(client.SessionToken())
})

t.Run("ExternalAppOnSubAgentWithPlaceholderPrintsURLAndDoesNotOpen", func(t *testing.T) {
t.Parallel()

// Sub-agent app URLs are attacker-influenceable through workspace
// configuration and runtime registration. The CLI must not
// substitute the session token, and must not hand the URL to the
// OS open handler. The URL is printed to stdout so a user who
// trusts the source can substitute and open it manually.
ownerClient, store := coderdtest.NewWithDatabase(t, nil)
ownerClient.SetLogger(testutil.Logger(t).Named("client"))
first := coderdtest.CreateFirstUser(t, ownerClient)
userClient, user := coderdtest.CreateAnotherUserMutators(t, ownerClient, first.OrganizationID, nil, func(r *codersdk.CreateUserRequestWithOrgs) {
r.Username = "subagentowner"
})
r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
Name: "subagentws",
OrganizationID: first.OrganizationID,
OwnerID: user.ID,
}).WithAgent().Do()

require.NotEmpty(t, r.Agents, "expected at least one workspace agent")
mainAgent := r.Agents[0]

subAgent := dbgen.WorkspaceSubAgent(t, store, mainAgent, database.WorkspaceAgent{
Name: "devcontainer",
})
_ = dbgen.WorkspaceApp(t, store, database.WorkspaceApp{
AgentID: subAgent.ID,
Slug: "subapp",
External: true,
Url: sql.NullString{Valid: true, String: "vscode://coder.coder-remote/open?token=$SESSION_TOKEN"},
})

inv, root := clitest.New(t, "open", "app", r.Workspace.Name+".devcontainer", "subapp", "--test.open-error")
clitest.SetupConfig(t, userClient, root)
var stdout, stderr bytes.Buffer
inv.Stdout = &stdout
inv.Stderr = &stderr

w := clitest.StartWithWaiter(t, inv)
w.RequireSuccess()
require.NotContains(t, stderr.String(), "test.open-error")
require.NotContains(t, stdout.String(), "test.open-error")
require.Contains(t, stdout.String(), "vscode://coder.coder-remote/open?token=$SESSION_TOKEN")
require.NotContains(t, stdout.String(), userClient.SessionToken())
require.Contains(t, stderr.String(), "substitute")
})

t.Run("ExternalAppOnSubAgentWithoutPlaceholderOpensAsIs", func(t *testing.T) {
t.Parallel()

// Sub-agent app URLs that don't reference $SESSION_TOKEN carry no
// token to leak. The CLI auto-opens them like any other external
// app; only placeholder-bearing URLs are gated.
ownerClient, store := coderdtest.NewWithDatabase(t, nil)
ownerClient.SetLogger(testutil.Logger(t).Named("client"))
first := coderdtest.CreateFirstUser(t, ownerClient)
userClient, user := coderdtest.CreateAnotherUserMutators(t, ownerClient, first.OrganizationID, nil, func(r *codersdk.CreateUserRequestWithOrgs) {
r.Username = "subagentowner2"
})
r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
Name: "subagentws2",
OrganizationID: first.OrganizationID,
OwnerID: user.ID,
}).WithAgent().Do()

require.NotEmpty(t, r.Agents, "expected at least one workspace agent")
mainAgent := r.Agents[0]

subAgent := dbgen.WorkspaceSubAgent(t, store, mainAgent, database.WorkspaceAgent{
Name: "devcontainer",
})
_ = dbgen.WorkspaceApp(t, store, database.WorkspaceApp{
AgentID: subAgent.ID,
Slug: "subapp",
External: true,
Url: sql.NullString{Valid: true, String: "https://example.com/some/path"},
})

inv, root := clitest.New(t, "open", "app", r.Workspace.Name+".devcontainer", "subapp", "--test.open-error")
clitest.SetupConfig(t, userClient, root)

w := clitest.StartWithWaiter(t, inv)
w.RequireError()
w.RequireContains("test.open-error")
w.RequireContains("https://example.com/some/path")
})
}
21 changes: 0 additions & 21 deletions docs/user-guides/devcontainers/customizing-dev-containers.md
Loading