Detect SAML SSO interstitial and give an actionable error#16
Merged
Conversation
When an org enforces SAML SSO and the browser session is authenticated but not SSO-authorized for that org, GitHub serves a "Sign in to <owner>" interstitial (HTTP 200) without the uploadToken. Previously this surfaced as "do you have write access to <owner>/<repo>?", which is misleading — the user can have full admin/write and still need an active SSO session. GetUploadToken now detects the interstitial (via the owner-scoped /orgs/<owner>/sso link or the "Sign in to <owner>" title — NOT a broad "contains SAML" match, which false-positives on site chrome and on repos that are merely about SAML) and points the user at /orgs/<owner>/sso, noting the SSO grant is server-side (~24h) so copying cookies can't fix it. The generic no-token message also gains an SSO hint. Adds internal/upload/token_test.go (first tests for the package): table tests for isSAMLProtected incl. false-positive guards, and error-wording tests for GetUploadToken via a stubbed transport. go test ./... and go vet pass.
The previous TestGetUploadToken_Success couldn't actually exercise GetUploadToken (its URL is hardcoded to github.com), so it silently only re-tested the uploadToken regex while spinning up an unused httptest server + client — dead code with a misleading name. Replace the three GetUploadToken_* functions with one table-driven TestGetUploadToken that drives the real code path via the stubTransport helper, now including a genuine happy-path (token extracted) case the old test lacked. Remove the unused serve/httptest scaffolding. TestIsSAMLProtected (the false-positive guards that are the point of the patch) is unchanged. Net: 2 focused test funcs, no dead code, broader real coverage. go test ./... + go vet pass.
Drop the "copying additional cookies cannot fix it (the SSO grant is
server-side)" tangent — it's debugging-context noise a normal user doesn't
need. Keep the load-bearing correction ("Write access alone is not enough"),
which is the whole reason the message exists. Trim the generic branch's SSO
hint to a one-line pointer. Test assertion updated to match.
- Make the /orgs/<owner>/sso link check case-insensitive to match the title check and GitHub's case-insensitive owners (a user typing a different case than the page renders previously relied on the title fallback alone). - Return early for an empty owner so detection can never match on it. - Add regression tests: lowercase-owner vs canonical-case link, substring owner non-match, and empty-owner safety.
Owner
|
Thanks @mxschmitt, this is great. Pulled it down, build/vet/test all green. Threw a few extra cases at it too — hyphenated org names, orgs where the display name isn't the slug, a non-English title — and the Opened #17 to track the underlying issue. Tagging this as v1.0.1 shortly — thanks again. |
drogers0
approved these changes
Jun 3, 2026
drogers0
left a comment
drogers0
left a comment
Owner
There was a problem hiding this comment.
Reviewed locally — go vet/test all green and the edge cases hold up (details in the comment above). LGTM.
mxschmitt
added a commit
to mxschmitt/gh-image
that referenced
this pull request
Jun 4, 2026
…shooting - README: add a skills.sh badge and a one-line "also available as an agent skill (npx skills add drogers0/gh-image)" pointer under Installation. - SKILL.md: update the SSO troubleshooting rows to match v1.0.1 (drogers0#16) — the tool now emits a dedicated "enforces SAML SSO …" message and the generic no-token message carries its own /orgs/<org>/sso hint. Rebased onto main (post-drogers0#16).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a repo belongs to an org that enforces SAML SSO and your browser session is authenticated but not SSO-authorized for that org, GitHub serves a "Sign in to org" interstitial page with HTTP 200 — so
uploadTokenis absent even though you have full write access.Today that surfaces as:
…which sends the user down the wrong path: they do have write access (often admin), and the real fix is a 10-second SSO re-authorization. An SSO session is server-side state (~24h, granted only by completing the IdP handshake in a browser), so it is not something more cookies can supply.
Change
GetUploadTokennow detects the SSO interstitial and points the user at/orgs/<owner>/sso:Before
After
(The non-SSO "no token" message is unchanged apart from a short SSO hint, as a fallback if detection ever misses due to markup drift.)
Detection is deliberately conservative
It matches signals specific to the interstitial and scoped to the repo's owner:
/orgs/<owner>/ssolink, or<title>Sign in to <owner></title>.It intentionally does not match the words "SAML"/"single sign-on" appearing anywhere on the page — those live in GitHub's site chrome/help links on virtually every page (and in any repo that's simply about SAML), which would be a ~100% false positive. Verified against real captured HTML: a normal repo page and
crewjam/samlboth correctly do not match.Safety
match == nil(no-token) branch. The happy path and all other behavior are unchanged — the only difference is which error you get when the upload was already going to fail.Tests
Adds
internal/upload/token_test.go(first tests for the package):TestIsSAMLProtected— true cases + false-positive guards (site chrome, a SAML-about repo, another org's link, substring owner, regex-metachar owner, case-insensitive link, empty owner).TestGetUploadToken— table-driven via a stubbedhttp.RoundTripper: happy path (token extracted), the SSO error wording, and the generic message.go test ./...andgo vet ./...pass.