fix: validate auth login scopes locally#454
Conversation
📝 WalkthroughWalkthroughThe PR adds explicit scope validation and normalization to the auth login command. When the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
|
voita seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/auth/login.go (1)
189-202:⚠️ Potential issue | 🟡 MinorRun the
--scopevs--domain/--recommendmutex check before validating scopes.Today, when a user passes both
--scope "bogus_scope"and--domain calendar, they receive aninvalid scope(s): bogus_scopeerror instead of the clearercannot use --scope together with --domain/--recommendmessage at line 201. The mutex check should run first so users get the more actionable diagnostic (and we avoid unnecessary scope-set construction on a request that will be rejected anyway).♻️ Proposed reordering
finalScope := opts.Scope - if finalScope != "" { - normalizedScope, err := validateExplicitScopes(finalScope, "user") - if err != nil { - return err - } - finalScope = normalizedScope - } // Resolve scopes from domain/permission filters if len(selectedDomains) > 0 || opts.Recommend { if opts.Scope != "" { return output.ErrValidation("cannot use --scope together with --domain/--recommend") } var candidateScopes []string if len(selectedDomains) > 0 { candidateScopes = collectScopesForDomains(selectedDomains, "user") } else { // --recommend without --domain: all domains candidateScopes = collectScopesForDomains(sortedKnownDomains(), "user") } // Filter to auto-approve scopes if --recommend or interactive "common" if opts.Recommend || scopeLevel == "common" { candidateScopes = registry.FilterAutoApproveScopes(candidateScopes) } if len(candidateScopes) == 0 { return output.ErrValidation("no matching scopes found, check domain/scope options") } finalScope = strings.Join(candidateScopes, " ") + } else if finalScope != "" { + normalizedScope, err := validateExplicitScopes(finalScope, "user") + if err != nil { + return err + } + finalScope = normalizedScope }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 189 - 202, The mutex check between --scope and --domain/--recommend should run before validating the explicit scope: move the block that checks if len(selectedDomains) > 0 || opts.Recommend and returns output.ErrValidation("cannot use --scope together with --domain/--recommend") (and its inner opts.Scope check) to occur before calling validateExplicitScopes on finalScope; keep the validateExplicitScopes call (and assignment to finalScope) only after the mutex check passes so validateExplicitScopes(finalScope, "user") is not invoked when the request will be rejected. Ensure you reference finalScope, opts.Scope, selectedDomains, opts.Recommend, validateExplicitScopes, and output.ErrValidation in the change.
🧹 Nitpick comments (1)
cmd/auth/login.go (1)
541-558: Deduplicate invalid scopes before rendering the error message.The
seenmap only filters duplicates of valid scopes; invalid tokens get appended unconditionally. Input like"foo foo"producesinvalid scope(s): foo, foo, which is awkward for both humans and the AI agents parsing these errors (per coding guidelines, error messages should be structured and specific for AI agent parsing).♻️ Proposed fix
knownScopes := knownScopesForIdentity(identity) invalid := make([]string, 0) result := make([]string, 0, len(normalized)) seen := make(map[string]bool, len(normalized)) + invalidSeen := make(map[string]bool) for _, s := range normalized { if !knownScopes[s] { - invalid = append(invalid, s) + if !invalidSeen[s] { + invalidSeen[s] = true + invalid = append(invalid, s) + } continue } if seen[s] { continue } seen[s] = true result = append(result, s) }As per coding guidelines: "Make error messages structured, actionable, and specific for AI agent parsing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 541 - 558, The code appends invalid scope tokens to invalid without deduplication, causing repeated entries; update the loop that iterates over normalized (using normalized, knownScopes, invalid, seen, result) to skip adding an invalid token if it's already recorded (either reuse the existing seen map for invalids or add a separate seenInvalid map) so invalid contains only unique scope names before the ErrValidation call; keep the rest of the error formatting the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 189-202: The mutex check between --scope and --domain/--recommend
should run before validating the explicit scope: move the block that checks if
len(selectedDomains) > 0 || opts.Recommend and returns
output.ErrValidation("cannot use --scope together with --domain/--recommend")
(and its inner opts.Scope check) to occur before calling validateExplicitScopes
on finalScope; keep the validateExplicitScopes call (and assignment to
finalScope) only after the mutex check passes so
validateExplicitScopes(finalScope, "user") is not invoked when the request will
be rejected. Ensure you reference finalScope, opts.Scope, selectedDomains,
opts.Recommend, validateExplicitScopes, and output.ErrValidation in the change.
---
Nitpick comments:
In `@cmd/auth/login.go`:
- Around line 541-558: The code appends invalid scope tokens to invalid without
deduplication, causing repeated entries; update the loop that iterates over
normalized (using normalized, knownScopes, invalid, seen, result) to skip adding
an invalid token if it's already recorded (either reuse the existing seen map
for invalids or add a separate seenInvalid map) so invalid contains only unique
scope names before the ErrValidation call; keep the rest of the error formatting
the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0f19f65-c8bb-4a3d-bc39-0de774a6e2be
📒 Files selected for processing (2)
cmd/auth/login.gocmd/auth/login_test.go
|
please check UT |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a74b59f55a9c0882351f36ef5df4b182b792ce2c🧩 Skill updatenpx skills add lttlin/cli#fix/416-auth-scope-validation -y -g |
3 participants
Problem
lark-cli auth login --scopecurrently forwards the raw scope string to the device authorization endpoint. If the input contains malformed spacing, line breaks, or a typo in one scope name, the user only sees the remote OAuth error saying the scope list is invalid, without any indication of which scope caused it.Root Cause
The explicit
--scopepath does not normalize user input or validate the requested scope names against the CLI's known scope registry before making the device authorization request.Fix
Normalize explicit scope input with
strings.Fields, deduplicate repeated entries, and validate each requested scope against the union of known registry scopes and shortcut scopes before calling the OAuth device authorization endpoint. Invalid scope names now fail fast with a local validation error that points users toauth scopesor the domain-based recommend flow.Validation
Closes #416
Summary by CodeRabbit
New Features
auth logincommand now validates scopes to ensure only recognized authentication scopes are accepted.Tests