From 2d9a3543d44fa42cf76332e01e2e95e6c649d5bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Dvo=C5=99=C3=A1k?= <150935816+Dvorinka@users.noreply.github.com> Date: Sun, 20 Sep 2026 16:05:01 +0200 Subject: [PATCH] feat(oauth): built-in CLI client, loopback redirect, consent denial, PKCE downgrade fix (#231) Port of upstream cloudreve/cloudreve#3588 adapted to fork conventions: the CLI client is a public client (empty secret, mandatory PKCE) like the desktop/iOS clients rather than carrying a hardcoded secret. Generated with [Devin](https://devin.ai) Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- ROADMAP.md | 1 + inventory/migration.go | 30 +++++++ inventory/migration_oauth_test.go | 42 ++++++++++ routers/controllers/oauth.go | 6 ++ routers/router.go | 5 ++ service/oauth/oauth.go | 11 ++- service/oauth/oauth_test.go | 132 ++++++++++++++++++++++++++++++ service/oauth/redirect.go | 47 +++++++++++ service/oauth/redirect_test.go | 44 ++++++++++ service/oauth/response.go | 3 +- 10 files changed, 317 insertions(+), 4 deletions(-) create mode 100644 inventory/migration_oauth_test.go create mode 100644 service/oauth/redirect.go create mode 100644 service/oauth/redirect_test.go diff --git a/ROADMAP.md b/ROADMAP.md index 5f230fac..dde13695 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -229,6 +229,7 @@ Order = user-visible value first; each ships with backend + UI + tests. - [x] WeChat scan login (upstream #2729 item 2) — `GET /session/wechat/login` redirects to `open.weixin.qq.com/connect/qrconnect` (scope `snsapi_login`, `#wechat_redirect` fragment); callback exchanges the code at `sns/oauth2/access_token` and binds by unionid (openid fallback); shares the single-use SSO state/ticket machinery and `sso_binding` table; provisioned accounts use synthetic `@connect.wechat.local` addresses with nickname from `/sns/userinfo`; account linking via `?link=1` + unbind via the shared provider route; admin UserSession section gains a WeChat accordion (enabled/AppID/AppSecret/register-enabled, callback URL shown); login page + security settings gain WeChat buttons; en+zh locales - [x] SMS verification-code sign-in + phone binding — generic HTTP SMS gateway (`sms_*` settings: endpoint/method/headers/body template with `{phone}`/`{code}` placeholders, SSRF-guarded outbound call); `users.phone` unique optional column; KV-stored 6-digit codes (5-min TTL, single-use, 60s resend throttle) across `login`/`bind`/`reset` scenes; `POST /session/sms/send` (IP rate-limit + login-CAPTCHA gate) / `POST /session/sms/login` (auto-provisions synthetic `sms_*@sms.local` accounts when enabled, 2FA continuation preserved) / `POST /user/reset_sms` / `PUT|DELETE /user/setting/phone`; masked phone in user settings response; login page gains an SMS phase + reset-via-SMS mode in forgot password, security settings gain a phone-binding section, admin UserSession gains an SMS gateway accordion; en+zh locales - [x] Direct-link traffic packs (upstream #2178 item 11) — `users.dl_traffic` (bytes, `-1` = unlimited default preserving legacy behavior); `RedirectDirectLink` atomically charges the owner's balance by file size before issuing the signed entity URL (`CodeInsufficientTraffic` = 40094 on exhaustion, balance never goes negative); new `traffic` SKU + gift-code type top up the balance permanently (unlimited users stay unlimited); admin VAS gains a Traffic product section + traffic gift-code type, Shop gains a Traffic packs tab, Finance shows the remaining allowance; en+zh locales +- [x] CLI OAuth + consent denial (ported from upstream PR #3588) — built-in `Cloudreve CLI` public client (`http://127.0.0.1/callback`, desktop scope set, empty secret + mandatory PKCE per our public-client convention); `redirectURIMatches` implements RFC 8252 loopback matching (any port on `127.0.0.1`/`[::1]` literal only — no `localhost`, no userinfo/fragments/encoded paths, strict query equality); `POST /session/oauth/consent/deny` returns `access_denied` after full client+redirect validation (`Deny` is internal, `json:"-"`); token exchange rejects PKCE downgrade (verifier without a registered challenge); migration preserves admin edits; tests cover redirect matrix, denial, downgrade, and migration idempotence ## 6. Phase D — desktop, all platforms diff --git a/inventory/migration.go b/inventory/migration.go index a03c254c..df2a4bb6 100644 --- a/inventory/migration.go +++ b/inventory/migration.go @@ -440,6 +440,9 @@ const ( OAuthClientDesktopGUID = "393a1839-f52e-498e-9972-e77cc2241eee" OAuthClientDesktopName = "application:oauth.desktop" OAuthClientDesktopRedirectURI = "/callback/desktop" + OAuthClientCLIGUID = "6326d2af-2fef-4a99-94da-1ee8ef0ca53f" + OAuthClientCLIName = "Cloudreve CLI" + OAuthClientCLIRedirectURI = "http://127.0.0.1/callback" OAuthClientiOSGUID = "220db97a-44a3-44f7-99b6-d767262b4daa" OAuthClientiOSName = "application:setting.iOSApp" OAuthClientiOSRedirectURI = "/callback/ios" @@ -454,6 +457,33 @@ func migrateOAuthClient(l logging.Logger, client *ent.Client, ctx context.Contex return err } + if err := migrateOAuthClientCLI(l, client, ctx); err != nil { + return err + } + + return nil +} + +// migrateOAuthClientCLI preserves administrator changes to an existing built-in client. +func migrateOAuthClientCLI(l logging.Logger, client *ent.Client, ctx context.Context) error { + if _, err := client.OAuthClient.Query().Where(oauthclient.GUID(OAuthClientCLIGUID)).First(ctx); err == nil { + l.Info("Default OAuth client (GUID=%s) already exists, skip migrating.", OAuthClientCLIGUID) + return nil + } else if !ent.IsNotFound(err) { + return fmt.Errorf("failed to query default CLI OAuth client: %w", err) + } + + if _, err := client.OAuthClient.Create(). + SetGUID(OAuthClientCLIGUID). + SetSecret(""). + SetName(OAuthClientCLIName). + SetRedirectUris([]string{OAuthClientCLIRedirectURI}). + SetScopes([]string{"profile", "email", "openid", "offline_access", "UserInfo.Write", "Workflow.Write", "Files.Write", "Shares.Write"}). + SetProps(&types.OAuthClientProps{Icon: "/static/img/cloudreve.svg", RefreshTokenTTL: 7776000}). + SetIsEnabled(true). + Save(ctx); err != nil { + return fmt.Errorf("failed to create default CLI OAuth client: %w", err) + } return nil } diff --git a/inventory/migration_oauth_test.go b/inventory/migration_oauth_test.go new file mode 100644 index 00000000..20cd0ab4 --- /dev/null +++ b/inventory/migration_oauth_test.go @@ -0,0 +1,42 @@ +package inventory + +import ( + "context" + "path/filepath" + "testing" + + "github.com/cloudreve/Cloudreve/v4/ent/enttest" + "github.com/cloudreve/Cloudreve/v4/ent/oauthclient" + "github.com/cloudreve/Cloudreve/v4/pkg/logging" + "github.com/stretchr/testify/require" +) + +func TestBuiltinCLIOAuthMigration(t *testing.T) { + ctx := context.Background() + client := enttest.Open(t, "sqlite3", filepath.Join(t.TempDir(), "migration.db")) + t.Cleanup(func() { require.NoError(t, client.Close()) }) + logger := logging.NewConsoleLogger(logging.LevelError) + + require.NoError(t, migrateOAuthClient(logger, client, ctx)) + desktop, err := client.OAuthClient.Query().Where(oauthclient.GUID(OAuthClientDesktopGUID)).Only(ctx) + require.NoError(t, err) + cli, err := client.OAuthClient.Query().Where(oauthclient.GUID(OAuthClientCLIGUID)).Only(ctx) + require.NoError(t, err) + require.Equal(t, "Cloudreve CLI", cli.Name) + require.Empty(t, cli.Secret) + require.Equal(t, []string{"http://127.0.0.1/callback"}, cli.RedirectUris) + require.Equal(t, desktop.Scopes, cli.Scopes) + require.Equal(t, int64(7776000), cli.Props.RefreshTokenTTL) + require.True(t, cli.IsEnabled) + + _, err = client.OAuthClient.UpdateOne(cli).SetIsEnabled(false).SetName("Disabled by administrator").Save(ctx) + require.NoError(t, err) + require.NoError(t, migrateOAuthClient(logger, client, ctx)) + preserved, err := client.OAuthClient.Get(ctx, cli.ID) + require.NoError(t, err) + require.False(t, preserved.IsEnabled) + require.Equal(t, "Disabled by administrator", preserved.Name) + count, err := client.OAuthClient.Query().Where(oauthclient.GUID(OAuthClientCLIGUID)).Count(ctx) + require.NoError(t, err) + require.Equal(t, 1, count) +} diff --git a/routers/controllers/oauth.go b/routers/controllers/oauth.go index 621d90f3..e188863a 100644 --- a/routers/controllers/oauth.go +++ b/routers/controllers/oauth.go @@ -43,6 +43,12 @@ func GrantAppConsent(c *gin.Context) { c.JSON(200, serializer.Response{Data: res}) } +// DenyAppConsent validates the client redirect without granting access. +func DenyAppConsent(c *gin.Context) { + ParametersFromContext[*oauth.GrantService](c, oauth.GrantParamCtx{}).Deny = true + GrantAppConsent(c) +} + type ExchangeErrorResponse struct { Error string `json:"error"` ErrorDescription string `json:"error_description"` diff --git a/routers/router.go b/routers/router.go index 60032166..f0bef26b 100644 --- a/routers/router.go +++ b/routers/router.go @@ -400,6 +400,11 @@ func initMasterRouter(dep dependency.Dep) *gin.Engine { controllers.FromJSON[oauth.GrantService](oauth.GrantParamCtx{}), controllers.GrantAppConsent, ) + oauthRouter.POST("consent/deny", + middleware.LoginRequired(), + controllers.FromJSON[oauth.GrantService](oauth.GrantParamCtx{}), + controllers.DenyAppConsent, + ) oauthRouter.POST("token", middleware.RateLimitByIP("oauth_token", 20, time.Minute), controllers.FromForm[oauth.ExchangeTokenService](oauth.ExchangeTokenParamCtx{}), diff --git a/service/oauth/oauth.go b/service/oauth/oauth.go index e99a7d9a..a97372ba 100644 --- a/service/oauth/oauth.go +++ b/service/oauth/oauth.go @@ -49,6 +49,7 @@ type ( GrantParamCtx struct{} GrantService struct { ClientID string `json:"client_id" binding:"required"` + Deny bool `json:"-"` ResponseType string `json:"response_type" binding:"required,eq=code"` RedirectURI string `json:"redirect_uri" binding:"required"` State string `json:"state" binding:"max=4096"` @@ -77,7 +78,7 @@ func (s *GrantService) Issue(c *gin.Context) (*GrantResponse, error) { // 2. Validate redirect URL: must match one of the registered redirect URIs redirectValid := false for _, uri := range app.RedirectUris { - if uri == s.RedirectURI { + if redirectURIMatches(uri, s.RedirectURI) { redirectValid = true break } @@ -86,6 +87,10 @@ func (s *GrantService) Issue(c *gin.Context) (*GrantResponse, error) { return nil, serializer.NewError(serializer.CodeParamErr, "Invalid redirect URI", nil) } + if s.Deny { + return &GrantResponse{Error: "access_denied", State: s.State}, nil + } + // Parse requested scopes (space-separated per OAuth 2.0 spec) requestedScopes := strings.Fields(s.Scope) @@ -172,8 +177,8 @@ func (s *ExchangeTokenService) Exchange(c *gin.Context) (*TokenResponse, error) return nil, serializer.NewError(serializer.CodeCredentialInvalid, "Redirect URI mismatch", nil) } - // 3. Verify PKCE: SHA256(code_verifier) should match code_challenge - if authCode.CodeChallenge != "" { + // 3. Verify PKCE, rejecting a verifier when the grant omitted its challenge. + if authCode.CodeChallenge != "" || s.CodeVerifier != "" { verifierHash := sha256.Sum256([]byte(s.CodeVerifier)) expectedChallenge := base64.RawURLEncoding.EncodeToString(verifierHash[:]) if expectedChallenge != authCode.CodeChallenge { diff --git a/service/oauth/oauth_test.go b/service/oauth/oauth_test.go index 803fe772..72d7f079 100644 --- a/service/oauth/oauth_test.go +++ b/service/oauth/oauth_test.go @@ -1,9 +1,21 @@ package oauth import ( + "context" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "errors" + "net/http/httptest" "testing" + "github.com/cloudreve/Cloudreve/v4/application/dependency" "github.com/cloudreve/Cloudreve/v4/ent" + "github.com/cloudreve/Cloudreve/v4/inventory" + "github.com/cloudreve/Cloudreve/v4/pkg/auth" + "github.com/cloudreve/Cloudreve/v4/pkg/cache" + "github.com/cloudreve/Cloudreve/v4/pkg/logging" + "github.com/gin-gonic/gin" "github.com/stretchr/testify/require" ) @@ -22,3 +34,123 @@ func TestValidateClientAuth(t *testing.T) { require.NoError(t, validateClientAuth(public, "", "challenge")) require.NoError(t, validateClientAuth(public, "stale-known-secret", "challenge")) } + +type oauthTestClient struct { + inventory.OAuthClientClient + app *ent.OAuthClient + grants, reads int +} + +func (c *oauthTestClient) GetByGUIDWithGrants(context.Context, string, int) (*ent.OAuthClient, error) { + if c.app == nil { + return nil, errors.New("unknown client") + } + return c.app, nil +} +func (c *oauthTestClient) UpsertGrant(context.Context, int, int, []string) error { + c.grants++ + return nil +} +func (c *oauthTestClient) GetByGUID(context.Context, string) (*ent.OAuthClient, error) { + c.reads++ + return nil, errors.New("stop after grant verification") +} + +type oauthTestCache struct { + cache.Driver + code *AuthorizationCode + writes int +} + +func (c *oauthTestCache) Get(string) (any, bool) { return c.code, c.code != nil } +func (c *oauthTestCache) Set(string, any, int) error { c.writes++; return nil } +func (c *oauthTestCache) Delete(string, ...string) error { return nil } + +type oauthTestDep struct { + dependency.Dep + client *oauthTestClient + kv *oauthTestCache +} + +func (d oauthTestDep) OAuthClientClient() inventory.OAuthClientClient { return d.client } +func (d oauthTestDep) KV() cache.Driver { return d.kv } +func (d oauthTestDep) UserClient() inventory.UserClient { return nil } +func (d oauthTestDep) TokenAuth() auth.TokenAuth { return nil } +func (d oauthTestDep) Logger() logging.Logger { + return logging.NewConsoleLogger(logging.LevelError) +} + +func oauthTestContext(client *oauthTestClient, kv *oauthTestCache) *gin.Context { + gin.SetMode(gin.TestMode) + c, engine := gin.CreateTestContext(httptest.NewRecorder()) + engine.ContextWithFallback = true + ctx := context.WithValue(context.Background(), dependency.DepCtx{}, oauthTestDep{client: client, kv: kv}) + ctx = context.WithValue(ctx, inventory.UserCtx{}, &ent.User{ID: 1}) + c.Request = httptest.NewRequest("POST", "/", nil).WithContext(ctx) + return c +} + +func TestConsentDenialValidatesRedirectWithoutIssuingGrant(t *testing.T) { + client := &oauthTestClient{app: &ent.OAuthClient{RedirectUris: []string{"http://127.0.0.1/callback"}}} + kv := &oauthTestCache{} + c := oauthTestContext(client, kv) + s := GrantService{ClientID: "client", RedirectURI: "http://127.0.0.1:49152/callback", State: "state", Deny: true} + response, err := s.Issue(c) + require.NoError(t, err) + require.Equal(t, "access_denied", response.Error) + require.Equal(t, "state", response.State) + require.Empty(t, response.Code) + require.Zero(t, client.grants) + require.Zero(t, kv.writes) + + s.RedirectURI = "http://evil.example/callback" + response, err = s.Issue(c) + require.ErrorContains(t, err, "Invalid redirect URI") + require.Nil(t, response) + client.app = nil + s.RedirectURI = "http://127.0.0.1:49152/callback" + response, err = s.Issue(c) + require.ErrorContains(t, err, "App not found") + require.Nil(t, response) + require.Zero(t, client.grants) + require.Zero(t, kv.writes) +} + +func TestTokenExchangeBindsProvidedRedirectAndRejectsPKCEDowngrade(t *testing.T) { + uri := "http://127.0.0.1:49152/callback" + other := "http://127.0.0.1:49153/callback" + verifier := "verifier" + hash := sha256.Sum256([]byte(verifier)) + challenge := base64.RawURLEncoding.EncodeToString(hash[:]) + for _, test := range []struct { + name string + redirect string + challenge, verifier string + valid bool + }{ + {"legacy omission", "", "", "", true}, + {"exact redirect", uri, challenge, verifier, true}, + {"other port", other, challenge, verifier, false}, + {"PKCE downgrade", uri, "", verifier, false}, + {"missing verifier", uri, challenge, "", false}, + } { + t.Run(test.name, func(t *testing.T) { + client := &oauthTestClient{} + kv := &oauthTestCache{code: &AuthorizationCode{ClientID: "client", RedirectURI: uri, CodeChallenge: test.challenge}} + s := ExchangeTokenService{ClientID: "client", RedirectURI: test.redirect, CodeVerifier: test.verifier} + _, err := s.Exchange(oauthTestContext(client, kv)) + require.Error(t, err) + if test.valid { + require.Equal(t, 1, client.reads, "valid grant reaches client credential validation") + } else { + require.Zero(t, client.reads, "invalid grant must fail before client credential validation") + } + }) + } +} + +func TestConsentRequestCannotSetInternalDenial(t *testing.T) { + var service GrantService + require.NoError(t, json.Unmarshal([]byte(`{"deny":true}`), &service)) + require.False(t, service.Deny) +} diff --git a/service/oauth/redirect.go b/service/oauth/redirect.go new file mode 100644 index 00000000..4e27eb7d --- /dev/null +++ b/service/oauth/redirect.go @@ -0,0 +1,47 @@ +package oauth + +import ( + "net/netip" + "net/url" + "strconv" + "strings" +) + +// redirectURIMatches permits only the port variation required for native loopback clients by RFC 8252. +func redirectURIMatches(registered, requested string) bool { + registration, err := url.Parse(registered) + if err != nil { + return false + } + if registration.Scheme != "http" { + return registered == requested + } + address, err := netip.ParseAddr(registration.Hostname()) + if err != nil || !address.IsLoopback() { + return registered == requested + } + + redirect, err := url.Parse(requested) + if err != nil { + return false + } + for _, uri := range []*url.URL{registration, redirect} { + if uri.User != nil || uri.Fragment != "" || uri.RawFragment != "" || uri.Opaque != "" || strings.HasSuffix(uri.Host, ":") { + return false + } + if port := uri.Port(); port != "" { + number, err := strconv.Atoi(port) + if err != nil || number < 1 || number > 65535 { + return false + } + } + } + if strings.Contains(registered, "#") || strings.Contains(requested, "#") { + return false + } + return registration.Scheme == redirect.Scheme && + registration.Hostname() == redirect.Hostname() && + registration.EscapedPath() == redirect.EscapedPath() && + registration.RawQuery == redirect.RawQuery && + registration.ForceQuery == redirect.ForceQuery +} diff --git a/service/oauth/redirect_test.go b/service/oauth/redirect_test.go new file mode 100644 index 00000000..557f407d --- /dev/null +++ b/service/oauth/redirect_test.go @@ -0,0 +1,44 @@ +package oauth + +import "testing" + +func TestRedirectURIMatches(t *testing.T) { + for _, test := range []struct { + registered string + requested string + matches bool + }{ + {"http://127.0.0.1/callback", "http://127.0.0.1:49152/callback", true}, + {"http://[::1]/callback", "http://[::1]:49152/callback", true}, + {"http://127.0.0.1:8000/callback?a=1", "http://127.0.0.1:65535/callback?a=1", true}, + {"/callback/desktop", "/callback/desktop", true}, + {"cloudreve://mount", "cloudreve://mount", true}, + {"https://app.example/callback", "https://app.example/callback", true}, + {"http://localhost/callback", "http://localhost:49152/callback", false}, + {"https://127.0.0.1/callback", "https://127.0.0.1:49152/callback", false}, + {"http://192.168.0.1/callback", "http://192.168.0.1:49152/callback", false}, + {"http://127.0.0.1/callback", "http://127.0.0.2:49152/callback", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1.evil.example:49152/callback", false}, + {"http://127.0.0.1/callback", "http://user@127.0.0.1:49152/callback", false}, + {"http://user@127.0.0.1/callback", "http://user@127.0.0.1/callback", false}, + {"http://127.0.0.1/callback", "https://127.0.0.1:49152/callback", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:0/callback", false}, + {"http://127.0.0.1:0/callback", "http://127.0.0.1:0/callback", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:65536/callback", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:abc/callback", false}, + {"http://127.0.0.1:abc/callback", "http://127.0.0.1:abc/callback", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:/callback", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:49152/callback#", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:49152/callback#fragment", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:49152/other", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:49152/%63allback", false}, + {"http://127.0.0.1/callback?a=1", "http://127.0.0.1:49152/callback?a=2", false}, + {"http://127.0.0.1/callback", "http://127.0.0.1:49152/callback?", false}, + } { + t.Run(test.registered+" -> "+test.requested, func(t *testing.T) { + if got := redirectURIMatches(test.registered, test.requested); got != test.matches { + t.Fatalf("matches = %v, want %v", got, test.matches) + } + }) + } +} diff --git a/service/oauth/response.go b/service/oauth/response.go index 88d591d1..b8299dac 100644 --- a/service/oauth/response.go +++ b/service/oauth/response.go @@ -55,7 +55,8 @@ func BuildAppRegistration(app *ent.OAuthClient, grant *ent.OAuthGrant) *AppRegis } type GrantResponse struct { - Code string `json:"code"` + Code string `json:"code,omitempty"` + Error string `json:"error,omitempty"` State string `json:"state"` }