fix(upload): detect SSO interstitials titled with the org display name - #55
Merged
Conversation
When an org's display name differs from its slug, the SAML SSO
interstitial's title shows the display name ("Sign in to Acme
Holdings, Inc" for slug acme-inc) and the page carries no
/orgs/<slug>/sso link, so neither isSignInInterstitial nor the
slug-based isSAMLProtected recognized it and users with full repo
access were told they may lack upload permission.
Add isAuthInterstitial as a fallback after both specific checks: any
"Sign in to ..." title combined with a missing "currentUser" marker
is an auth interstitial (a real repo page always embeds currentUser
and its title starts "GitHub - owner/repo"). Since a stale session
and an unauthorized SSO org are indistinguishable here, the error
names both causes with their fixes.
Fixes #52
Sign-in and SSO interstitials now surface their own error messages before the generic one, so this symptom usually indicates a genuine access problem rather than an expired session.
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
1 participant
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.
Fixes #52.
Problem
When an organization's display name differs from its slug, its SAML SSO interstitial is titled with the display name (
<title>Sign in to Acme Holdings, Inc</title>for slugacme-inc) and the page carries no/orgs/<slug>/ssolink. Neither existing detector fires:isSignInInterstitialrequires the exact titleSign in to GitHubisSAMLProtectedbuilds both of its patterns from the slug, which the display-name title never containsThe lookup falls through to the generic "you may not have upload access" error — misleading for a user who has
adminon the repo, and easy to "disprove" withgh api repos/<org>/<repo> --jq .permissions, which buries the correct SSO hint in the parentheses.Fix
Add
isAuthInterstitialas a fallback checked after the two specific detectors: a title matchingSign in to <anything>combined with a missing"currentUser"marker identifies any auth interstitial. This cannot fire on a real repo page — those always embedcurrentUserin the JS payload (asnullwhen unauthenticated) and their titles startGitHub - <owner>/<repo>.A stale session and an unauthorized SSO org are indistinguishable at this point (the caller only knows the slug, never the display name), so the new error names both causes and their fixes: re-extract the session token, or authorize at
/orgs/<owner>/sso.Branch order is unchanged for pages the specific detectors already handle, so the precise stale-session and slug-matched SSO messages still win when they apply.
Testing
TestIsAuthInterstitialcovering the display-name title from the issue, GitHub's own sign-in page, both repo-page variants, a repo whose description mentions signing in, and the degenerate bareSign in totitleTestGetUploadTokencase asserting the issue Wrong error message when an organization uses SAML SSO #52 page yields the dual-cause message and not the access errorgofmt,go vet,go test ./...all pass