Detect SAML SSO interstitial and give an actionable error (#16)

Detect the SAML SSO 'Sign in to <owner>' interstitial when the uploadToken is absent, and return an actionable error pointing at /orgs/<owner>/sso instead of a misleading write-access message.

Closes #17

Author: mxschmitt

internal/upload/token.go

@@ -11,6 +11,32 @@ import (
 
 var uploadTokenRe = regexp.MustCompile(`"uploadToken":"([^"]+)"`)
 
+// isSAMLProtected reports whether the repo page is a SAML SSO "Sign in to
+// <owner>" interstitial rather than the real repo page. When an organization
+// enforces SAML SSO and the browser session is authenticated but not
+// SSO-authorized for that org, GitHub serves this interstitial with HTTP 200 —
+// so the uploadToken is absent even though the user has write access.
+//
+// The SSO authorization is server-side state (it lasts ~24h and is granted only
+// by completing the identity-provider handshake in a browser), so it is NOT a
+// cookie that can be copied; the fix is to re-authorize at /orgs/<owner>/sso.
+//
+// We require signals SPECIFIC to the interstitial and scoped to THIS owner. We
+// deliberately do NOT match the words "SAML"/"single sign-on" anywhere on the
+// page: those appear in GitHub's site chrome/help links on virtually every page
+// (and in any repo that is simply about SAML), which would be a false positive.
+func isSAMLProtected(body []byte, owner string) bool {
+	if owner == "" {
+		return false
+	}
+	// Owners are case-insensitive on GitHub, and the page may render a different
+	// case than the user typed, so both checks are case-insensitive.
+	o := regexp.QuoteMeta(owner)
+	orgSSOLink := regexp.MustCompile(`(?i)/orgs/` + o + `/sso`).Match(body)
+	ssoTitle := regexp.MustCompile(`(?i)<title>\s*Sign in to ` + o + `\b`).Match(body)
+	return orgSSOLink || ssoTitle
+}
+
 // GetUploadToken fetches the repo page and extracts the uploadToken
 // from the JS payload. Requires authenticated cookies in the client.
 func GetUploadToken(client *http.Client, owner, repo string) (string, error) {
@@ -39,7 +65,16 @@ func GetUploadToken(client *http.Client, owner, repo string) (string, error) {
 
 	match := uploadTokenRe.FindSubmatch(body)
 	if match == nil {
-		return "", fmt.Errorf("uploadToken not found on repo page — do you have write access to %s/%s?", owner, repo)
+		// Distinguish the common SAML-SSO case from a genuine lack of access, so
+		// the user isn't wrongly told to check their permissions.
+		if isSAMLProtected(body, owner) {
+			return "", fmt.Errorf("%s enforces SAML SSO and your session is not authorized for it — "+
+				"authorize in a browser at https://github.com/orgs/%s/sso (lasts ~24h), then retry. "+
+				"Write access alone is not enough", owner, owner)
+		}
+		return "", fmt.Errorf("uploadToken not found on repo page — do you have write access to %s/%s? "+
+			"(or, if %s enforces SAML SSO, authorize at https://github.com/orgs/%s/sso)",
+			owner, repo, owner, owner)
 	}
 
 	return string(match[1]), nil

internal/upload/token_test.go

@@ -0,0 +1,164 @@
+package upload
+
+import (
+	"io"
+	"net/http"
+	"strings"
+	"testing"
+)
+
+func TestIsSAMLProtected(t *testing.T) {
+	cases := []struct {
+		name  string
+		owner string
+		body  string
+		want  bool
+	}{
+		{
+			name:  "SSO interstitial title",
+			owner: "GymPod",
+			body:  `<title>Sign in to GymPod</title>`,
+			want:  true,
+		},
+		{
+			name:  "owner-scoped /orgs/<owner>/sso link",
+			owner: "GymPod",
+			body:  `<a href="/orgs/GymPod/sso?return_to=%2FGymPod%2Frepo">Single sign-on</a>`,
+			want:  true,
+		},
+		{
+			// The key false positive a naive "contains SAML" check would hit:
+			// site chrome / help links mention SSO on essentially every page.
+			name:  "normal repo page with SSO words in chrome must NOT match",
+			owner: "GymPod",
+			body:  `<title>GymPod/realtime-core</title><footer><a href="/help/saml">single sign-on docs</a></footer>`,
+			want:  false,
+		},
+		{
+			name:  "a repo that is ABOUT saml must NOT match",
+			owner: "crewjam",
+			body:  `<title>GitHub - crewjam/saml: SAML library for go</title> ... single sign-on ...`,
+			want:  false,
+		},
+		{
+			name:  "another org's sso link must NOT match this owner",
+			owner: "GymPod",
+			body:  `<a href="/orgs/SomeOtherOrg/sso">x</a>`,
+			want:  false,
+		},
+		{
+			name:  "owner with regex metacharacters is matched literally",
+			owner: "a.b",
+			body:  `<title>Sign in to axb</title>`, // '.' must NOT act as a wildcard
+			want:  false,
+		},
+		{
+			// GitHub owners are case-insensitive and the page may render a
+			// different case than the user typed; the link must still match.
+			name:  "lowercase owner matches a canonical-case sso link",
+			owner: "gympod",
+			body:  `<a href="/orgs/GymPod/sso">Single sign-on</a>`,
+			want:  true,
+		},
+		{
+			// Substring of another org's name must not match.
+			name:  "owner that is a substring of another org must NOT match",
+			owner: "pod",
+			body:  `<a href="/orgs/GymPod/sso">x</a>`,
+			want:  false,
+		},
+		{
+			// Defensive: an empty owner must never match (and must not panic).
+			name:  "empty owner never matches",
+			owner: "",
+			body:  `<a href="/orgs/Anything/sso"><title>Sign in to </title>`,
+			want:  false,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := isSAMLProtected([]byte(tc.body), tc.owner); got != tc.want {
+				t.Errorf("isSAMLProtected(%q owner=%q) = %v, want %v", tc.body, tc.owner, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestGetUploadToken(t *testing.T) {
+	cases := []struct {
+		name        string
+		owner       string
+		body        string
+		wantToken   string   // non-empty => expect success
+		errContains []string // substrings the error must include
+		errExcludes []string // substrings the error must NOT include
+	}{
+		{
+			name:      "success extracts the token",
+			owner:     "octocat",
+			body:      `window.x={"uploadToken":"TKN123"};`,
+			wantToken: "TKN123",
+		},
+		{
+			name:        "SAML interstitial gives an actionable SSO error, not write-access",
+			owner:       "GymPod",
+			body:        `<title>Sign in to GymPod</title><a href="/orgs/GymPod/sso">Single sign-on</a>`,
+			errContains: []string{"SAML SSO", "/orgs/GymPod/sso", "Write access alone is not enough"},
+			errExcludes: []string{"do you have write access to GymPod"},
+		},
+		{
+			name:        "no token and no SSO markers gives the generic message",
+			owner:       "octocat",
+			body:        `<html>just a page, no token</html>`,
+			errContains: []string{"do you have write access to octocat/hello"},
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			client := &http.Client{Transport: stubTransport(http.StatusOK, tc.body)}
+			tok, err := GetUploadToken(client, tc.owner, "hello")
+
+			if tc.wantToken != "" {
+				if err != nil {
+					t.Fatalf("expected success, got error: %v", err)
+				}
+				if tok != tc.wantToken {
+					t.Errorf("token = %q, want %q", tok, tc.wantToken)
+				}
+				return
+			}
+
+			if err == nil {
+				t.Fatal("expected an error, got nil")
+			}
+			for _, s := range tc.errContains {
+				if !strings.Contains(err.Error(), s) {
+					t.Errorf("error missing %q; got: %s", s, err.Error())
+				}
+			}
+			for _, s := range tc.errExcludes {
+				if strings.Contains(err.Error(), s) {
+					t.Errorf("error should not contain %q; got: %s", s, err.Error())
+				}
+			}
+		})
+	}
+}
+
+// stubTransport answers every request with the given status and body, so
+// GetUploadToken's hardcoded github.com URL is served locally without a network
+// call.
+type roundTripFunc func(*http.Request) (*http.Response, error)
+
+func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
+
+func stubTransport(status int, body string) http.RoundTripper {
+	return roundTripFunc(func(r *http.Request) (*http.Response, error) {
+		return &http.Response{
+			StatusCode: status,
+			Body:       io.NopCloser(strings.NewReader(body)),
+			Header:     make(http.Header),
+			Request:    r,
+		}, nil
+	})
+}