Skip to content

fix: parse drive url tokens from path only - #1756

Closed
hiSandog wants to merge 1 commit into
larksuite:mainfrom
hiSandog:fix/cli-cleanup-20260706
Closed

hiSandog wants to merge 1 commit into
larksuite:mainfrom
hiSandog:fix/cli-cleanup-20260706

Conversation

@hiSandog

@hiSandog hiSandog commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Drive URL token inference now parses the URL path rather than scanning raw input, so query strings and non-URL paths cannot spoof supported Drive resource markers.

Changes

  • Parse Drive shortcut URL tokens from parsed URL paths with an explicit scheme/host check.
  • Update the apply-permission marker comment to match the path-based behavior.
  • Add regression tests for add-comment and apply-permission query, mid-path, and non-URL marker cases.

Test Plan

  • Unit tests pass: env GOCACHE=/private/tmp/cli-gocache go test ./shortcuts/drive
  • Manual local verification confirms the lark-cli flow works as expected

Related Issues

  • None

Summary by CodeRabbit

  • Bug Fixes
    • Improved link handling for drive comments and permission actions so document types and tokens are only detected from the actual URL path.
    • Prevents query strings, fragments, or unrelated path segments from being mistaken for valid document markers.
    • Added coverage to ensure invalid or spoofed inputs continue to return clear “unsupported input” or “could not infer token” errors.

@github-actions github-actions Bot added domain/ccm PR touches the ccm domain size/M Single-domain feat or fix with limited business impact labels Jul 6, 2026
@coderabbitai

coderabbitai Bot commented Jul 6, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change reworks extractURLToken in drive_add_comment.go to parse input as a URL via net/url and match markers only against the parsed URL path, preventing query strings or fragments from spoofing token type. Documentation and tests for drive_add_comment and drive_apply_permission were updated accordingly.

Changes

URL Marker Matching Hardening

Layer / File(s) Summary
extractURLToken URL-path parsing
shortcuts/drive/drive_add_comment.go
Adds net/url import and reworks extractURLToken to parse input with url.Parse, validate scheme/host, and extract tokens only when the URL path matches the expected marker prefix, replacing raw substring search.
Doc-ref and token extraction tests
shortcuts/drive/drive_add_comment_test.go
Extends TestParseCommentDocRef with marker-like substrings in queries/mid-paths that should still error, and adds TestExtractURLTokenIgnoresMarkersOutsideURLPathPrefix to confirm markers outside the path prefix are ignored.
apply-permission docs and test
shortcuts/drive/drive_apply_permission.go, shortcuts/drive/drive_apply_permission_test.go
Updates inline comment to describe path-based marker matching and adds a test verifying resolvePermApplyTarget fails to infer a token when markers appear outside the expected path prefix.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

  • larksuite/cli#518: Both PRs update drive_add_comment's URL/token type inference to prevent marker-like substrings elsewhere in the URL from spoofing document type/token, with matching new tests.
  • larksuite/cli#588: Both PRs constrain resolvePermApplyTarget marker-based token/type inference to only consider the parsed URL path prefix.
  • larksuite/cli#674: Both PRs modify drive_add_comment.go doc-ref parsing logic and related tests around --doc URL handling.

Suggested reviewers: fangshuyu-768

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: parsing Drive URL tokens from the path only.
Description check ✅ Passed The description follows the template with Summary, Changes, Test Plan, and Related Issues, and is sufficiently complete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
shortcuts/drive/drive_add_comment.go (1)

1262-1280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Solid hardening — path-anchored matching with scheme/host validation.

The rewrite correctly rejects non-URL inputs (no scheme/host), and since u.Path excludes query/fragment, markers can no longer be spoofed via ?next=/wiki/... style strings. Requiring HasPrefix(path, marker) also prevents markers from matching mid-path (e.g. /space/wiki/...), which is confirmed intentional by the new test cases in drive_add_comment_test.go.

One optional nit: consider adding a short doc comment above extractURLToken documenting the new invariants (absolute URL required; marker must anchor at path start) — mirrors the comment update already made for permApplyURLMarkers in drive_apply_permission.go.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shortcuts/drive/drive_add_comment.go` around lines 1262 - 1280, Add a short
doc comment for extractURLToken describing its invariants: it only accepts
absolute URLs with scheme and host, and the marker must match at the start of
u.Path. Keep the comment aligned with the updated behavior in extractURLToken
and the related documentation style used for permApplyURLMarkers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@shortcuts/drive/drive_add_comment.go`:
- Around line 1262-1280: Add a short doc comment for extractURLToken describing
its invariants: it only accepts absolute URLs with scheme and host, and the
marker must match at the start of u.Path. Keep the comment aligned with the
updated behavior in extractURLToken and the related documentation style used for
permApplyURLMarkers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 577994f2-65e5-419e-b41e-8487d766217c

📥 Commits

Reviewing files that changed from the base of the PR and between c45ff56 and e5e8e03.

📒 Files selected for processing (4)
  • shortcuts/drive/drive_add_comment.go
  • shortcuts/drive/drive_add_comment_test.go
  • shortcuts/drive/drive_apply_permission.go
  • shortcuts/drive/drive_apply_permission_test.go

@hiSandog hiSandog closed this Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant