fix(cli): prevent session token exfiltration via external app URLs (#… · coder/coder@2044599
11package cli_test
2233import (
4+"bytes"
45"context"
6+"database/sql"
57"net/url"
68"os"
79"path"
@@ -21,6 +23,10 @@ import (
2123"github.com/coder/coder/v2/agent/agenttest"
2224"github.com/coder/coder/v2/cli/clitest"
2325"github.com/coder/coder/v2/coderd/coderdtest"
26+"github.com/coder/coder/v2/coderd/database"
27+"github.com/coder/coder/v2/coderd/database/dbauthz"
28+"github.com/coder/coder/v2/coderd/database/dbfake"
29+"github.com/coder/coder/v2/coderd/database/dbgen"
2430"github.com/coder/coder/v2/coderd/database/dbtime"
2531"github.com/coder/coder/v2/codersdk"
2632"github.com/coder/coder/v2/provisionersdk/proto"
@@ -646,14 +652,16 @@ func TestOpenApp(t *testing.T) {
646652w.RequireContains("region not found")
647653 })
648654649-t.Run("ExternalAppSessionToken", func(t *testing.T) {
655+t.Run("ExternalAppOnTopLevelAgentSubstitutes", func(t *testing.T) {
650656t.Parallel()
651657658+// Apps on the top-level (template-defined) agent are trusted, so the
659+// CLI substitutes $SESSION_TOKEN regardless of scheme.
652660client, ws, _ := setupWorkspaceForAgent(t, func(agents []*proto.Agent) []*proto.Agent {
653661agents[0].Apps = []*proto.App{
654662 {
655663Slug: "app1",
656-Url: "https://example.com/app1?token=$SESSION_TOKEN",
664+Url: "vscode://coder.coder-remote/open?token=$SESSION_TOKEN",
657665External: true,
658666 },
659667 }
@@ -670,4 +678,104 @@ func TestOpenApp(t *testing.T) {
670678w.RequireContains("test.open-error")
671679w.RequireContains(client.SessionToken())
672680 })
681+682+t.Run("ExternalAppOnSubAgentWithPlaceholderPrintsURLAndDoesNotOpen", func(t *testing.T) {
683+t.Parallel()
684+685+// Sub-agent app URLs are attacker-influenceable through workspace
686+// configuration and runtime registration. The CLI must not
687+// substitute the session token, and must not hand the URL to the
688+// OS open handler. The URL is printed to stdout so a user who
689+// trusts the source can substitute and open it manually.
690+ownerClient, store := coderdtest.NewWithDatabase(t, nil)
691+ownerClient.SetLogger(testutil.Logger(t).Named("client"))
692+first := coderdtest.CreateFirstUser(t, ownerClient)
693+userClient, user := coderdtest.CreateAnotherUserMutators(t, ownerClient, first.OrganizationID, nil, func(r *codersdk.CreateUserRequestWithOrgs) {
694+r.Username = "subagentowner"
695+ })
696+r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
697+Name: "subagentws",
698+OrganizationID: first.OrganizationID,
699+OwnerID: user.ID,
700+ }).WithAgent().Do()
701+702+ctx := testutil.Context(t, testutil.WaitShort)
703+agents, err := store.GetWorkspaceAgentsByWorkspaceAndBuildNumber(dbauthz.AsSystemRestricted(ctx), database.GetWorkspaceAgentsByWorkspaceAndBuildNumberParams{
704+WorkspaceID: r.Workspace.ID,
705+BuildNumber: r.Build.BuildNumber,
706+ })
707+require.NoError(t, err)
708+require.NotEmpty(t, agents, "expected at least one workspace agent")
709+mainAgent := agents[0]
710+711+subAgent := dbgen.WorkspaceSubAgent(t, store, mainAgent, database.WorkspaceAgent{
712+Name: "devcontainer",
713+ })
714+_ = dbgen.WorkspaceApp(t, store, database.WorkspaceApp{
715+AgentID: subAgent.ID,
716+Slug: "subapp",
717+External: true,
718+Url: sql.NullString{Valid: true, String: "vscode://coder.coder-remote/open?token=$SESSION_TOKEN"},
719+ })
720+721+inv, root := clitest.New(t, "open", "app", r.Workspace.Name+".devcontainer", "subapp", "--test.open-error")
722+clitest.SetupConfig(t, userClient, root)
723+var stdout, stderr bytes.Buffer
724+inv.Stdout = &stdout
725+inv.Stderr = &stderr
726+727+w := clitest.StartWithWaiter(t, inv)
728+w.RequireSuccess()
729+require.NotContains(t, stderr.String(), "test.open-error")
730+require.NotContains(t, stdout.String(), "test.open-error")
731+require.Contains(t, stdout.String(), "vscode://coder.coder-remote/open?token=$SESSION_TOKEN")
732+require.NotContains(t, stdout.String(), userClient.SessionToken())
733+require.Contains(t, stderr.String(), "substitute")
734+ })
735+736+t.Run("ExternalAppOnSubAgentWithoutPlaceholderOpensAsIs", func(t *testing.T) {
737+t.Parallel()
738+739+// Sub-agent app URLs that don't reference $SESSION_TOKEN carry no
740+// token to leak. The CLI auto-opens them like any other external
741+// app; only placeholder-bearing URLs are gated.
742+ownerClient, store := coderdtest.NewWithDatabase(t, nil)
743+ownerClient.SetLogger(testutil.Logger(t).Named("client"))
744+first := coderdtest.CreateFirstUser(t, ownerClient)
745+userClient, user := coderdtest.CreateAnotherUserMutators(t, ownerClient, first.OrganizationID, nil, func(r *codersdk.CreateUserRequestWithOrgs) {
746+r.Username = "subagentowner2"
747+ })
748+r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{
749+Name: "subagentws2",
750+OrganizationID: first.OrganizationID,
751+OwnerID: user.ID,
752+ }).WithAgent().Do()
753+754+ctx := testutil.Context(t, testutil.WaitShort)
755+agents, err := store.GetWorkspaceAgentsByWorkspaceAndBuildNumber(dbauthz.AsSystemRestricted(ctx), database.GetWorkspaceAgentsByWorkspaceAndBuildNumberParams{
756+WorkspaceID: r.Workspace.ID,
757+BuildNumber: r.Build.BuildNumber,
758+ })
759+require.NoError(t, err)
760+require.NotEmpty(t, agents, "expected at least one workspace agent")
761+mainAgent := agents[0]
762+763+subAgent := dbgen.WorkspaceSubAgent(t, store, mainAgent, database.WorkspaceAgent{
764+Name: "devcontainer",
765+ })
766+_ = dbgen.WorkspaceApp(t, store, database.WorkspaceApp{
767+AgentID: subAgent.ID,
768+Slug: "subapp",
769+External: true,
770+Url: sql.NullString{Valid: true, String: "https://example.com/some/path"},
771+ })
772+773+inv, root := clitest.New(t, "open", "app", r.Workspace.Name+".devcontainer", "subapp", "--test.open-error")
774+clitest.SetupConfig(t, userClient, root)
775+776+w := clitest.StartWithWaiter(t, inv)
777+w.RequireError()
778+w.RequireContains("test.open-error")
779+w.RequireContains("https://example.com/some/path")
780+ })
673781}