Skip to content

fix: relax csrf to exclude path based apps #11430

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

Merged
merged 12 commits into from
Jan 8, 2024
Merged

fix: relax csrf to exclude path based apps #11430

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
658d7df
fix: relax csrf to exclude path based apps
Emyrk Jan 4, 2024
fe4e2ba
add unit test to verify path based apps are not CSRF blocked
Emyrk Jan 5, 2024
f2e615d
testing needs to be parallel
Emyrk Jan 5, 2024
a4bbc80
Linting
Emyrk Jan 5, 2024
63133cf
Merge remote-tracking branch 'origin/main' into stevenmasley/relax_csrf
Emyrk Jan 5, 2024
3828354
Merge remote-tracking branch 'origin/main' into stevenmasley/relax_csrf
Emyrk Jan 5, 2024
c6c4141
Try exempting first user
Emyrk Jan 5, 2024
42a3021
Merge remote-tracking branch 'origin/main' into stevenmasley/relax_csrf
Emyrk Jan 8, 2024
043c79d
we need to add csrf mw to set cookie
Emyrk Jan 8, 2024
388e56e
add comment
Emyrk Jan 8, 2024
fee870b
add unit tests
Emyrk Jan 8, 2024
6966f87
Linting
Emyrk Jan 8, 2024
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
Prev Previous commit
Next Next commit
add unit test to verify path based apps are not CSRF blocked
  • Loading branch information
@Emyrk
Emyrk committed Jan 5, 2024
commit fe4e2bafd9468c7ebabaeb8ebaa3dffb0f86eda6
57 changes: 57 additions & 0 deletions coderd/coderd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package coderd_test
import (
"context"
"flag"
"fmt"
"io"
"net/http"
"net/netip"
Expand All @@ -21,6 +22,9 @@ import (

"cdr.dev/slog"
"cdr.dev/slog/sloggers/slogtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbfake"
"github.com/coder/coder/v2/provisionersdk/proto"

"github.com/coder/coder/v2/agent/agenttest"
"github.com/coder/coder/v2/buildinfo"
Expand Down Expand Up @@ -315,3 +319,56 @@ func TestSwagger(t *testing.T) {
require.Equal(t, "<pre>\n</pre>\n", string(body))
})
}

func TestCSRFExempt(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am working on a unit test to verify

Is it this one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. All of our existing unit tests bypass CSRF since the codersdk uses the HEADER method to auth, not the cookie.

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

client, _, api := coderdtest.NewWithAPI(t, nil)
first := coderdtest.CreateFirstUser(t, client)
owner, err := client.User(context.Background(), "me")
require.NoError(t, err)

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium)
defer cancel()

// Create a workspace.
const agentSlug = "james"
const appSlug = "web"
wrk := dbfake.WorkspaceBuild(t, api.Database, database.Workspace{
OwnerID: owner.ID,
OrganizationID: first.OrganizationID,
}).
WithAgent(func(agents []*proto.Agent) []*proto.Agent {
agents[0].Name = agentSlug
agents[0].Apps = []*proto.App{{
Slug: appSlug,
DisplayName: appSlug,
Subdomain: false,
Url: "/",
}}

return agents
}).
Do()

u := fmt.Sprintf(client.URL.JoinPath(fmt.Sprintf("/@%s/%s.%s/apps/%s", owner.Username, wrk.Workspace.Name, agentSlug, appSlug)).String())
req, err := http.NewRequestWithContext(ctx, http.MethodPost, u, nil)
req.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenCookie,
Value: client.SessionToken(),
Path: "/",
Domain: client.URL.String(),
})
require.NoError(t, err)

resp, err := client.HTTPClient.Do(req)
require.NoError(t, err)
data, _ := io.ReadAll(resp.Body)

// A StatusBadGateway means Coderd tried to proxy to the agent and failed because the agent
// was not there. This means CSRF did not block the app request, which is what we want.
require.Equal(t, http.StatusBadGateway, resp.StatusCode, "status code 500 is CSRF failure")
require.NotContains(t, string(data), "CSRF")
})
}