server: changes from external security review #89

mostlygeek · 2025-10-21T23:50:55Z

This PR incorporates the changes and recommendations from the external security review. Where applicable commits will reference the ID in the security report.

Signed-off-by: Benson Wong <benson@tailscale.com>

In serveAuthorize there was a chance an authorization code is saved but never used if a url.Parse() fails. This commit moves the parsing logic above the saving logic. Signed-off-by: Benson Wong <benson@tailscale.com>

Signed-off-by: Benson Wong <benson@tailscale.com>

The UI form endpoints and the /client/ endpoints accept POST requests for modifying OAuth client data. This commit adds checking of the Sec-Fetch-Site and Origin request headers. Signed-off-by: Benson Wong <benson@tailscale.com>

add isDangerousScheme() to disallow various schemes that may be dangerous in an OAuth redirect_uri Signed-off-by: Benson Wong <benson@tailscale.com>

Signed-off-by: Benson Wong <benson@tailscale.com>

Eliminiate a low risk scenario where a refresh token could be used to issue multiple access tokens in a race condition. The trade-off is that a users would need to re-authenticate when a token refresh fails. Signed-off-by: Benson Wong <benson@tailscale.com>

Signed-off-by: Benson Wong <benson@tailscale.com>

server/server.go

mostlygeek · 2025-10-22T03:24:08Z

server/client_test.go

 	}
 }
+
+func TestDynamicClientRegistrationCORSHeaders(t *testing.T) {


Add CORS pre-flight tests to enforce the contract we have for now. We can tighten them up in the future. DCR doesn't really work without more permissive settings, particularly with the mcp-inspector.

mostlygeek · 2025-10-22T03:24:59Z

server/client_test.go

+	}
+}
+
+func TestClientApiCSRF(t *testing.T) {


Add tests to ensure the /clients/ api handles CSRF mitigation as expected.

mostlygeek · 2025-10-22T03:25:38Z

server/oauth-metadata.go

-
-// isFunnelRequest checks if the request is coming through Tailscale Funnel
-// Migrated from legacy/tsidp.go:2392-2410
-func isFunnelRequest(r *http.Request) bool {


function moved to server.go

mostlygeek · 2025-10-22T03:26:10Z

server/oauth-metadata_test.go

 		}
 	}
 }
+func TestMetadataCORSHeaders(t *testing.T) {


Add test to ensure metadata endpoints have the appropriate CORS preflight behaviour.

mostlygeek · 2025-10-22T03:28:18Z

server/server.go

 }
+
+// isFunnelRequest checks if the request is coming through Tailscale Funnel
+func isFunnelRequest(r *http.Request) bool {


moved here from oauth-metadata.go. No changes to the code.

mostlygeek · 2025-10-22T03:38:11Z

Dockerfile


+RUN addgroup -g 1001 -S app && \
+    adduser -u 1001 -S app
+


run binary as a less privileged user than root

Signed-off-by: Benson Wong <benson@tailscale.com>

awly

Some other stuff that would be good to patch, here or in a separate PR:

LT-APP25-11: I don't think all endpoints do request method filtering (like handleUI); perhaps using http.ServeMux capabilities is the simplest thing here? https://pkg.go.dev/net/http#hdr-Patterns-ServeMux
LT-APP25-16: other misc security headers like HSTS and CSP

server/authorize.go

server/token_test.go

server/ui.go

server/server.go

server/client_test.go

server/token_test.go

Co-authored-by: Andrew Lytvynov <awly@tailscale.com> Signed-off-by: Benson Wong <benson@tailscale.com>

- add "s" to misspelled CORS header Access-Control-Allow-Method - use filippo.io/csrf for CSRF protection Signed-off-by: Benson Wong <benson@tailscale.com>

Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek · 2025-10-25T05:06:08Z

@awly thanks for the suggestions. Ready for another review.

awly · 2025-10-28T21:29:41Z

server/server.go

+	protect := csrf.New()
+	protect.AddTrustedOrigin(s.serverURL)


nit: move this to the bottom, right before the return where it's actually needed

Signed-off-by: Benson Wong <benson@tailscale.com>

tsidp completed an external security review in #89. It is safe to remove the WIP check. Fixes: #99 Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek added 9 commits October 21, 2025 16:48

server: move isFunnelRequest() into server.go

5818724

Signed-off-by: Benson Wong <benson@tailscale.com>

server: fix case where auth code is set but never used

1a4af13

In serveAuthorize there was a chance an authorization code is saved but never used if a url.Parse() fails. This commit moves the parsing logic above the saving logic. Signed-off-by: Benson Wong <benson@tailscale.com>

Dockerfile: switch to lower privilege user (LT-APP25-14)

bda3df6

Signed-off-by: Benson Wong <benson@tailscale.com>

server: add CORS preflight tests

530181d

Signed-off-by: Benson Wong <benson@tailscale.com>

server: add block list for redirect_uri schemes (LT-APP25-10)

13eeab1

add isDangerousScheme() to disallow various schemes that may be dangerous in an OAuth redirect_uri Signed-off-by: Benson Wong <benson@tailscale.com>

server: limit /authorize to GET and POST methods (LT-APP25-11)

5d4d1bc

Signed-off-by: Benson Wong <benson@tailscale.com>

.github/workflows: update CI go-version to 1.24

8a8ea35

Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek force-pushed the mostlygeek/sec-review branch from 8a8ea35 to 28c2f76 Compare October 21, 2025 23:56

server: implement csrf check compatible with golang 1.24 (LT-APP25-09)

7c49993

Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek force-pushed the mostlygeek/sec-review branch from 28c2f76 to 7c49993 Compare October 22, 2025 00:25

mostlygeek requested review from awly and remyguercio October 22, 2025 00:26

mostlygeek commented Oct 22, 2025

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

mostlygeek commented Oct 22, 2025

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

mostlygeek commented Oct 22, 2025

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

mostlygeek commented Oct 22, 2025

View reviewed changes

Dockerfile

RUN addgroup -g 1001 -S app && \

adduser -u 1001 -S app

Copy link

Contributor Author

mostlygeek Oct 22, 2025

Choose a reason for hiding this comment

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

run binary as a less privileged user than root

mostlygeek added 2 commits October 21, 2025 20:40

server: improve mux usage consistency

daccf07

Signed-off-by: Benson Wong <benson@tailscale.com>

server: make csrfCheck more specific

f2206b2

Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek force-pushed the mostlygeek/sec-review branch from 9382158 to f2206b2 Compare October 22, 2025 03:40

awly requested a review from patrickod October 23, 2025 19:56

awly reviewed Oct 24, 2025

View reviewed changes

server/token_test.go Outdated Show resolved Hide resolved

mostlygeek force-pushed the mostlygeek/sec-review branch from cab732d to a92591c Compare October 25, 2025 04:26

mostlygeek and others added 3 commits October 24, 2025 21:46

server: add missing return

4391074

Co-authored-by: Andrew Lytvynov <awly@tailscale.com> Signed-off-by: Benson Wong <benson@tailscale.com>

server: improve comment for isDangerousScheme

21d6481

Co-authored-by: Andrew Lytvynov <awly@tailscale.com> Signed-off-by: Benson Wong <benson@tailscale.com>

server: switch to filippo.io/csrf and fix CORS headers

986bd68

- add "s" to misspelled CORS header Access-Control-Allow-Method - use filippo.io/csrf for CSRF protection Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek force-pushed the mostlygeek/sec-review branch from a92591c to 986bd68 Compare October 25, 2025 04:51

server: wrap CSRF checks around all of mux

a6db914

Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek requested a review from awly October 28, 2025 21:23

awly approved these changes Oct 28, 2025

View reviewed changes

server: tidy up csrf

b14634c

Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek merged commit 08d296d into main Oct 28, 2025
2 checks passed

mostlygeek deleted the mostlygeek/sec-review branch October 28, 2025 21:40

mostlygeek mentioned this pull request Oct 30, 2025

Remove TAILSCALE_USE_WIP_CODE requirement #99

Closed

mostlygeek added a commit that referenced this pull request Oct 30, 2025

tsidp-server.go: remove TAILSCALE_USE_WIP_CODE env var check

f3f2c8f

tsidp completed an external security review in #89. It is safe to remove the WIP check. Fixes: #99 Signed-off-by: Benson Wong <benson@tailscale.com>

mostlygeek mentioned this pull request Oct 30, 2025

tsidp-server.go: remove TAILSCALE_USE_WIP_CODE env var check #100

Merged

server: changes from external security review #89

server: changes from external security review #89

Uh oh!

Conversation

mostlygeek commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mostlygeek Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

mostlygeek Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mostlygeek Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mostlygeek Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mostlygeek Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

mostlygeek Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

awly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mostlygeek commented Oct 25, 2025

Uh oh!

awly Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mostlygeek commented Oct 21, 2025 •

edited

Loading

mostlygeek Oct 22, 2025 •

edited

Loading

mostlygeek Oct 22, 2025 •

edited

Loading

mostlygeek Oct 22, 2025 •

edited

Loading