Skip to content

Commit 2044599

Browse files
f0sselzedkipp
f0ssel
and
zedkipp
authored
fix(cli): prevent session token exfiltration via external app URLs (#26146) (#26317)
Backport of #26146 to `release/2.29` (SEC-100 / PLAT-266). Conflict resolution: dropped a hunk touching `docs/user-guides/devcontainers/customizing-dev-containers.md`, which does not exist on this branch. > 🤖 Backport prepared by Coder Agents on behalf of @f0ssel. --------- Co-authored-by: Zach <3724288+zedkipp@users.noreply.github.com>
1 parent c05b4d9 commit 2044599

2 files changed

Lines changed: 134 additions & 16 deletions

File tree

‎cli/open.go‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,11 @@ func (r *RootCmd) open() *serpent.Command {
3939

4040
const vscodeDesktopName = "VS Code Desktop"
4141

42+
// externalSessionTokenPlaceholder is the literal substring in an external
43+
// workspace-app URL that the CLI replaces with the user's session token
44+
// when the app belongs to a trusted (top-level) agent.
45+
const externalSessionTokenPlaceholder = "$SESSION_TOKEN"
46+
4247
func (r *RootCmd) openVSCode() *serpent.Command {
4348
var (
4449
generateToken bool
@@ -381,8 +386,13 @@ func (r *RootCmd) openApp() *serpent.Command {
381386
pathAppURL := strings.TrimPrefix(region.PathAppURL, baseURL.String())
382387
appURL := buildAppLinkURL(baseURL, ws, agt, foundApp, region.WildcardHostname, pathAppURL)
383388

384-
if foundApp.External {
385-
appURL = replacePlaceholderExternalSessionTokenString(client, appURL)
389+
externalSubAgentApp := foundApp.External && agt.ParentID.Valid
390+
if foundApp.External && !agt.ParentID.Valid {
391+
// Template-defined apps run on a top-level agent and are
392+
// admin-authored, so their URLs are trusted. Substitute the
393+
// session token placeholder so the OS open handler receives
394+
// a usable URL.
395+
appURL = strings.ReplaceAll(appURL, externalSessionTokenPlaceholder, client.SessionToken())
386396
}
387397

388398
// Check if we're inside a workspace. Generally, we know
@@ -393,6 +403,18 @@ func (r *RootCmd) openApp() *serpent.Command {
393403
_, _ = fmt.Fprintf(inv.Stdout, "%s\n", appURL)
394404
return nil
395405
}
406+
407+
// Sub-agent external app URLs are set at runtime. Only open
408+
// sub-agent URLs that don't contain the placeholder to prevent
409+
// token exfiltration.
410+
if externalSubAgentApp && strings.Contains(appURL, externalSessionTokenPlaceholder) {
411+
cliui.Warnf(inv.Stderr,
412+
"This app was registered from inside the workspace rather than from the workspace template. "+
413+
"Inspect the URL below carefully and, if you trust the source, substitute the $SESSION_TOKEN placeholder "+
414+
"with your session token and manually open it:")
415+
_, _ = fmt.Fprintf(inv.Stdout, "%s\n", appURL)
416+
return nil
417+
}
396418
_, _ = fmt.Fprintf(inv.Stderr, "Opening %s\n", appURL)
397419

398420
if !testOpenError {
@@ -662,15 +684,3 @@ func buildAppLinkURL(baseURL *url.URL, workspace codersdk.Workspace, agent coder
662684
}
663685
return u.String()
664686
}
665-
666-
// replacePlaceholderExternalSessionTokenString replaces any $SESSION_TOKEN
667-
// strings in the URL with the actual session token.
668-
// This is consistent behavior with the frontend. See: site/src/modules/resources/AppLink/AppLink.tsx
669-
func replacePlaceholderExternalSessionTokenString(client *codersdk.Client, appURL string) string {
670-
if !strings.Contains(appURL, "$SESSION_TOKEN") {
671-
return appURL
672-
}
673-
674-
// We will just re-use the existing session token we're already using.
675-
return strings.ReplaceAll(appURL, "$SESSION_TOKEN", client.SessionToken())
676-
}

‎cli/open_test.go‎

Lines changed: 110 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package cli_test
22

33
import (
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) {
646652
w.RequireContains("region not found")
647653
})
648654

649-
t.Run("ExternalAppSessionToken", func(t *testing.T) {
655+
t.Run("ExternalAppOnTopLevelAgentSubstitutes", func(t *testing.T) {
650656
t.Parallel()
651657

658+
// Apps on the top-level (template-defined) agent are trusted, so the
659+
// CLI substitutes $SESSION_TOKEN regardless of scheme.
652660
client, ws, _ := setupWorkspaceForAgent(t, func(agents []*proto.Agent) []*proto.Agent {
653661
agents[0].Apps = []*proto.App{
654662
{
655663
Slug: "app1",
656-
Url: "https://example.com/app1?token=$SESSION_TOKEN",
664+
Url: "vscode://coder.coder-remote/open?token=$SESSION_TOKEN",
657665
External: true,
658666
},
659667
}
@@ -670,4 +678,104 @@ func TestOpenApp(t *testing.T) {
670678
w.RequireContains("test.open-error")
671679
w.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
}

0 commit comments

Comments
 (0)