feat(slides): add image upload via +media-upload and @path placeholders in +create#450
Open
feat(slides): add image upload via +media-upload and @path placeholders in +create#450
Conversation
github-actions
bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces image media upload functionality for Feishu slides. It adds helper functions for parsing and resolving presentation references (supporting raw tokens, slides URLs, and wiki URLs), image placeholder detection and replacement in XML, a new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as lark-cli<br/>(SlidesCreate)
participant SlidesAPI as Slides API
participant WikiAPI as Wiki API
participant DriveAPI as Drive API
User->>CLI: +create --slides '[...]' with @./img.png
CLI->>CLI: Parse presentation ref & extract placeholders
alt Wiki reference
CLI->>WikiAPI: GET /wiki/v2/spaces/get_node
WikiAPI-->>CLI: obj_token (resolve to presentation ID)
end
CLI->>CLI: Validate local file paths exist
CLI->>DriveAPI: POST /drive/v1/medias/upload_all<br/>(for each unique placeholder)
DriveAPI-->>CLI: file_token
CLI->>CLI: Replace @./img.png with file_token in XML
CLI->>SlidesAPI: POST .../slides (create presentation)
SlidesAPI-->>CLI: presentation_id
CLI->>SlidesAPI: POST .../slides/.../slide (add slides with tokens)
SlidesAPI-->>CLI: slide_id
CLI-->>User: JSON output with images_uploaded, file_tokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
ViperCai
force-pushed
the
feat/slides-image-upload
branch
from
7aaaf28 to
0b8e956
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@001d597adc153b33e9ecbafc105dfe9aaccc919d🧩 Skill updatenpx skills add larksuite/cli#feat/slides-image-upload -y -g |
ViperCai
force-pushed
the
feat/slides-image-upload
branch
from
0b8e956 to
aa3bbcb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
shortcuts/slides/slides_create_test.go (1)
737-750: 补一条tok_b断言，避免替换映射漏检当前只强校验了
tok_a，建议再校验至少一页包含tok_b，防止@b.png被误替换成tok_a仍然过测。✅ 建议补充断言
// Assert each slide.create body uses tokens (not `@path` placeholders). + hasTokB := false for _, stub := range []*httpmock.Stub{slideStub1, slideStub2} { var body map[string]interface{} if err := json.Unmarshal(stub.CapturedBody, &body); err != nil { t.Fatalf("decode slide body: %v", err) } slide, _ := body["slide"].(map[string]interface{}) content, _ := slide["content"].(string) if strings.Contains(content, "@a.png") || strings.Contains(content, "@b.png") { t.Fatalf("slide content still contains placeholder: %s", content) } if !strings.Contains(content, "tok_a") { t.Fatalf("slide content missing tok_a: %s", content) } + if strings.Contains(content, "tok_b") { + hasTokB = true + } } + if !hasTokB { + t.Fatal("expected at least one slide body to contain tok_b") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_create_test.go` around lines 737 - 750, The test currently only asserts presence of "tok_a" in each slide body, which can miss cases where both placeholders were replaced with the same token; update the test around the loop that iterates over slideStub1 and slideStub2 (variables slideStub1, slideStub2, body, slide, content) to also verify that at least one slide contains "tok_b" (e.g., set a foundTokB flag when strings.Contains(content, "tok_b") is true) and fail the test if foundTokB is false after the loop, while keeping the existing checks that no "@a.png"/"@b.png" remain and that "tok_a" is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/slides/helpers.go`:
- Around line 34-45: The code currently uses extractRefToken(raw, "/slides/")
and extractRefToken(raw, "/wiki/") which match those substrings anywhere; change
the logic to first try parsing raw as a URL (using net/url.Parse) and only
accept the ref if the parsed URL's Path has the prefix "/slides/" or "/wiki/"
respectively, extracting the token from u.Path; for non-URL inputs, only accept
raw forms that start with "/slides/" or "/wiki/" (use strings.HasPrefix) and
reject cases where those markers appear in queries/fragments or mid-path; update
the same pattern in the later block (lines around 49-63) and keep returning
presentationRef{Kind: "...", Token: token} or the same validation error via
output.ErrValidation when not matched.
In `@shortcuts/slides/slides_create.go`:
- Around line 45-55: The loop that validates placeholder paths (using
extractImagePlaceholderPaths and runtime.FileIO().Stat) performs filesystem Stat
on user-supplied paths without sanitizing them; update the loop to call
validate.SafeInputPath(path) first and handle its error (returning a FlagErrorf
or wrapped input error consistent with existing patterns) before calling
runtime.FileIO().Stat, so only safe, CWD-relative paths are allowed; ensure you
reference the same variables and preserve the existing error-return behavior in
slides_create.go.
In `@shortcuts/slides/slides_media_upload.go`:
- Around line 41-46: The Validate function currently only checks
parsePresentationRef(runtime.Str("presentation")) but does not validate the
user-controlled --file input; update the Validate block to call
validate.SafeInputPath(ctx, runtime.Str("file")) and return any error so
absolute paths or ".." are rejected before Stat/upload planning. Keep the
existing presentation check (parsePresentationRef) and ensure the error from
validate.SafeInputPath is propagated from Validate.
In `@skills/lark-slides/references/xml-schema-quick-ref.md`:
- Line 135: 将句子中的笔误“本地图详见”替换为更通顺明确的表述，例如“本地路径详见”或“本地图片详见”；在该文件中查找包含文本片段“本地图详见
[lark-slides-create.md] / [lark-slides-media-upload.md]”的段落并用“本地路径详见
[lark-slides-create.md](lark-slides-create.md#本地图片path-占位符) / 本地图片详见
[lark-slides-media-upload.md](lark-slides-media-upload.md)”之类的明确表述替换原文以消除歧义。
---
Nitpick comments:
In `@shortcuts/slides/slides_create_test.go`:
- Around line 737-750: The test currently only asserts presence of "tok_a" in
each slide body, which can miss cases where both placeholders were replaced with
the same token; update the test around the loop that iterates over slideStub1
and slideStub2 (variables slideStub1, slideStub2, body, slide, content) to also
verify that at least one slide contains "tok_b" (e.g., set a foundTokB flag when
strings.Contains(content, "tok_b") is true) and fail the test if foundTokB is
false after the loop, while keeping the existing checks that no
"@a.png"/"@b.png" remain and that "tok_a" is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c18fb66d-bf05-443c-b3e5-1e414277e66e
📒 Files selected for processing (14)
shortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/slide-templates.mdskills/lark-slides/references/xml-format-guide.mdskills/lark-slides/references/xml-schema-quick-ref.md
Greptile SummaryThis PR adds first-class image support to the slides skill: a new
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant WikiAPI as wiki get_node
participant DriveAPI as drive upload_all
participant SlidesAPI as slides_ai xml_presentations
Note over CLI: +create --slides with @path placeholders
CLI->>SlidesAPI: POST create blank presentation
SlidesAPI-->>CLI: presentation id
loop Each unique placeholder path
CLI->>DriveAPI: "POST upload_all (parent_type=slide_file)"
DriveAPI-->>CLI: file token
end
Note over CLI: replaceImagePlaceholders
loop Each slide
CLI->>SlidesAPI: POST add slide (rewritten XML)
SlidesAPI-->>CLI: slide id
end
CLI-->>User: result with images_uploaded count
Note over CLI: +media-upload with wiki URL
CLI->>WikiAPI: GET get_node with wiki token
WikiAPI-->>CLI: "obj_type=slides, obj_token"
CLI->>DriveAPI: "POST upload_all (parent_type=slide_file)"
DriveAPI-->>CLI: file token
CLI-->>User: file_token and presentation_id
Reviews (5): Last reviewed commit: "feat(slides): add image upload via +medi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
shortcuts/slides/helpers.go (1)
34-38:⚠️ Potential issue | 🟠 MajorAnchor presentation token parsing to URL/raw path prefixes, not substring matches.
Current parsing accepts
/slides/or/wiki/anywhere in the input, so query/fragment/mid-string matches can be treated as valid references and target the wrong token.Suggested fix
import ( "fmt" + "net/url" "regexp" "strings" @@ func parsePresentationRef(input string) (presentationRef, error) { raw := strings.TrimSpace(input) if raw == "" { return presentationRef{}, output.ErrValidation("--presentation cannot be empty") } - if token, ok := extractRefToken(raw, "/slides/"); ok { - return presentationRef{Kind: "slides", Token: token}, nil - } - if token, ok := extractRefToken(raw, "/wiki/"); ok { - return presentationRef{Kind: "wiki", Token: token}, nil + + if u, err := url.Parse(raw); err == nil && u.Scheme != "" && u.Host != "" { + if token, ok := extractRefTokenFromPath(u.Path, "/slides/"); ok { + return presentationRef{Kind: "slides", Token: token}, nil + } + if token, ok := extractRefTokenFromPath(u.Path, "/wiki/"); ok { + return presentationRef{Kind: "wiki", Token: token}, nil + } + return presentationRef{}, output.ErrValidation("unsupported --presentation input %q: use an xml_presentation_id, a /slides/ URL, or a /wiki/ URL", raw) + } + + if token, ok := extractRefTokenFromPath(raw, "/slides/"); ok { + return presentationRef{Kind: "slides", Token: token}, nil + } + if token, ok := extractRefTokenFromPath(raw, "/wiki/"); ok { + return presentationRef{Kind: "wiki", Token: token}, nil } @@ -func extractRefToken(raw, marker string) (string, bool) { - idx := strings.Index(raw, marker) - if idx < 0 { +func extractRefTokenFromPath(path, marker string) (string, bool) { + if !strings.HasPrefix(path, marker) { return "", false } - token := raw[idx+len(marker):] + token := path[len(marker):] if end := strings.IndexAny(token, "/?#"); end >= 0 { token = token[:end] }Also applies to: 49-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/helpers.go` around lines 34 - 38, The code currently treats any substring match of "/slides/" or "/wiki/" in raw as a valid token (via extractRefToken), which allows matches in queries/fragments; change the logic to anchor to the URL/raw path prefix: parse raw with net/url.Parse (or otherwise extract the path component) and then use a prefix check (e.g., strings.HasPrefix on u.Path) when calling extractRefToken or replace extractRefToken to accept a path and only return a token when the path begins with "/slides/" or "/wiki/". Update both the block creating presentationRef for slides/wiki (the existing ifs using extractRefToken) and the similar logic around lines 49-56 so they operate on the URL path, not a substring of the whole raw input.shortcuts/slides/slides_media_upload.go (1)
41-46:⚠️ Potential issue | 🔴 CriticalValidate
--filewithvalidate.SafeInputPathbefore any file I/O.
--fileis user-controlled and reachesStatwithout safety validation. Add the guard inValidateso absolute/traversal paths are rejected early.Suggested fix
import ( "context" "fmt" "path/filepath" "github.com/larksuite/cli/extension/fileio" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/validate" "github.com/larksuite/cli/shortcuts/common" ) @@ Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { if _, err := parsePresentationRef(runtime.Str("presentation")); err != nil { return err } + if err := validate.SafeInputPath(runtime.Str("file")); err != nil { + return err + } return nil },As per coding guidelines,
**/*.go: “Validate paths usingvalidate.SafeInputPathbefore any file I/O operations”.Also applies to: 81-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_media_upload.go` around lines 41 - 46, The Validate function currently parses the presentation ref but does not validate the user-supplied file path; call validate.SafeInputPath on runtime.Str("file") inside the same Validate (and the other Validate block around lines 81-87) before any file I/O and return its error if non-nil so absolute/traversal paths are rejected early; keep the parsePresentationRef check as-is and perform the SafeInputPath check first (or alongside) using the same runtime.Str("file") input.
🧹 Nitpick comments (1)
shortcuts/slides/slides_media_upload_test.go (1)
329-332: Handle non-EOF read errors in multipart test helpers.Current helpers break on any error, which can hide malformed multipart bodies and produce partial parses.
Suggested fix
import ( "bytes" "encoding/json" + "io" "mime" "mime/multipart" @@ for { part, err := reader.NextPart() if err != nil { - break + if err == io.EOF { + break + } + t.Fatalf("read multipart part: %v", err) } data := readAll(t, part) @@ func readAll(t *testing.T, r interface { Read(p []byte) (n int, err error) }) []byte { t.Helper() - var buf bytes.Buffer - tmp := make([]byte, 4096) - for { - n, err := r.Read(tmp) - if n > 0 { - buf.Write(tmp[:n]) - } - if err != nil { - break - } - } - return buf.Bytes() + b, err := io.ReadAll(r) + if err != nil { + t.Fatalf("read body: %v", err) + } + return b }Also applies to: 350-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_media_upload_test.go` around lines 329 - 332, The helper currently treats any error from reader.NextPart() as end-of-input and simply breaks, which hides malformed multipart bodies; change the NextPart() error handling in the helper used around reader.NextPart() (and the similar block at the other occurrence) to distinguish io.EOF from other errors: if err == io.EOF then break, otherwise fail the test (e.g., t.Fatalf or return the error) and include the error message so tests surface malformed multipart bodies instead of silently truncating parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/slides/helpers.go`:
- Around line 34-38: The code currently treats any substring match of "/slides/"
or "/wiki/" in raw as a valid token (via extractRefToken), which allows matches
in queries/fragments; change the logic to anchor to the URL/raw path prefix:
parse raw with net/url.Parse (or otherwise extract the path component) and then
use a prefix check (e.g., strings.HasPrefix on u.Path) when calling
extractRefToken or replace extractRefToken to accept a path and only return a
token when the path begins with "/slides/" or "/wiki/". Update both the block
creating presentationRef for slides/wiki (the existing ifs using
extractRefToken) and the similar logic around lines 49-56 so they operate on the
URL path, not a substring of the whole raw input.
In `@shortcuts/slides/slides_media_upload.go`:
- Around line 41-46: The Validate function currently parses the presentation ref
but does not validate the user-supplied file path; call validate.SafeInputPath
on runtime.Str("file") inside the same Validate (and the other Validate block
around lines 81-87) before any file I/O and return its error if non-nil so
absolute/traversal paths are rejected early; keep the parsePresentationRef check
as-is and perform the SafeInputPath check first (or alongside) using the same
runtime.Str("file") input.
---
Nitpick comments:
In `@shortcuts/slides/slides_media_upload_test.go`:
- Around line 329-332: The helper currently treats any error from
reader.NextPart() as end-of-input and simply breaks, which hides malformed
multipart bodies; change the NextPart() error handling in the helper used around
reader.NextPart() (and the similar block at the other occurrence) to distinguish
io.EOF from other errors: if err == io.EOF then break, otherwise fail the test
(e.g., t.Fatalf or return the error) and include the error message so tests
surface malformed multipart bodies instead of silently truncating parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06cf0bcd-6d9e-4740-b846-179eaf9f6e0f
📒 Files selected for processing (14)
shortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/slide-templates.mdskills/lark-slides/references/xml-format-guide.mdskills/lark-slides/references/xml-schema-quick-ref.md
✅ Files skipped from review due to trivial changes (4)
- skills/lark-slides/references/lark-slides-xml-presentation-slide-create.md
- skills/lark-slides/references/xml-schema-quick-ref.md
- skills/lark-slides/references/xml-format-guide.md
- skills/lark-slides/references/lark-slides-media-upload.md
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/slides/helpers_test.go
- shortcuts/slides/shortcuts.go
- shortcuts/slides/slides_create.go
- shortcuts/slides/slides_create_test.go
ViperCai
force-pushed
the
feat/slides-image-upload
branch
2 times, most recently
from
5c95ce8 to
4c118ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/slides/slides_create.go (1)
48-58:⚠️ Potential issue | 🟠 MajorValidate
@pathplaceholders before touching the filesystem.Both code paths still
StatXML-supplied placeholder paths before callingvalidate.SafeInputPath(path). That keeps the new feature out of compliance with the repo’s safe-path rule and duplicates the same gap in validation and execution.Suggested fix
for _, path := range extractImagePlaceholderPaths(slides) { + if err := validate.SafeInputPath(path); err != nil { + return output.ErrValidation("unsafe file path: %s", err) + } stat, err := runtime.FileIO().Stat(path) if err != nil { return common.WrapInputStatError(err, fmt.Sprintf("--slides @%s: file not found", path)) }for i, path := range paths { + if err := validate.SafeInputPath(path); err != nil { + return tokens, i, output.ErrValidation("unsafe file path: %s", err) + } stat, err := runtime.FileIO().Stat(path) if err != nil { return tokens, i, common.WrapInputStatError(err, fmt.Sprintf("@%s: file not found", path)) }As per coding guidelines,
**/*.go: Validate paths usingvalidate.SafeInputPathbefore any file I/O operations.Also applies to: 260-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_create.go` around lines 48 - 58, The loop that validates image placeholder paths currently calls runtime.FileIO().Stat(path) before ensuring the path is allowed; change the order to call validate.SafeInputPath(path) first (using the same path from extractImagePlaceholderPaths(slides)) and return an appropriate wrapped error if SafeInputPath fails, then proceed to runtime.FileIO().Stat(path) and the existing checks; update the error returns (used in this block with common.WrapInputStatError and common.FlagErrorf) to reflect failures from validate.SafeInputPath where applicable so no filesystem access occurs before SafeInputPath approval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/slides/slides_create.go`:
- Around line 31-34: SlidesCreate currently unconditionally includes
"docs:document.media:upload" in its Scopes which breaks text-only flows; modify
the constructor or setup where SlidesCreate.Scopes is assigned so you first call
extractImagePlaceholderPaths(slides) (or equivalent) and only append
"docs:document.media:upload" to the Scopes slice when that function returns at
least one placeholder; leave "slides:presentation:create" and
"slides:presentation:write_only" unchanged so preflight still catches other
permission issues.
---
Duplicate comments:
In `@shortcuts/slides/slides_create.go`:
- Around line 48-58: The loop that validates image placeholder paths currently
calls runtime.FileIO().Stat(path) before ensuring the path is allowed; change
the order to call validate.SafeInputPath(path) first (using the same path from
extractImagePlaceholderPaths(slides)) and return an appropriate wrapped error if
SafeInputPath fails, then proceed to runtime.FileIO().Stat(path) and the
existing checks; update the error returns (used in this block with
common.WrapInputStatError and common.FlagErrorf) to reflect failures from
validate.SafeInputPath where applicable so no filesystem access occurs before
SafeInputPath approval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7001e2c6-fa18-43cd-8744-1929f5ba0e1b
📒 Files selected for processing (14)
shortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/slide-templates.mdskills/lark-slides/references/xml-format-guide.mdskills/lark-slides/references/xml-schema-quick-ref.md
✅ Files skipped from review due to trivial changes (5)
- skills/lark-slides/references/lark-slides-xml-presentation-slide-create.md
- shortcuts/slides/helpers_test.go
- skills/lark-slides/references/lark-slides-create.md
- shortcuts/slides/helpers.go
- skills/lark-slides/references/lark-slides-media-upload.md
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/slides/shortcuts.go
- skills/lark-slides/references/xml-schema-quick-ref.md
- skills/lark-slides/references/xml-format-guide.md
- shortcuts/slides/slides_create_test.go
- shortcuts/slides/slides_media_upload.go
- shortcuts/slides/slides_media_upload_test.go
ViperCai
force-pushed
the
feat/slides-image-upload
branch
from
4c118ac to
3198b6c
Compare
fangshuyu-768
previously requested changes
Apr 13, 2026
Collaborator
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
A few inline follow-ups on scopes and placeholder matching.
fangshuyu-768
dismissed
their stale review
Superseded by the newer inline review.
ViperCai
force-pushed
the
feat/slides-image-upload
branch
from
3198b6c to
ee3a336
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
shortcuts/slides/slides_create.go (1)
53-67:⚠️ Potential issue | 🟠 MajorValidate
@pathplaceholders withSafeInputPathbefore filesystem access.The paths extracted from
<img src="@...">placeholders in user-supplied XML are validated withFileIO().Statbut not withvalidate.SafeInputPathfirst. This allows<img src="@../secret.png">or absolute paths to bypass the CWD-relative restriction documented for this feature.Suggested fix
+import "github.com/larksuite/cli/internal/validate" + for _, path := range placeholders { + if err := validate.SafeInputPath(path); err != nil { + return output.ErrValidation("--slides @%s: unsafe path: %s", path, err) + } stat, err := runtime.FileIO().Stat(path) if err != nil { return common.WrapInputStatError(err, fmt.Sprintf("--slides @%s: file not found", path))As per coding guidelines,
**/*.go: Validate paths usingvalidate.SafeInputPathbefore any file I/O operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_create.go` around lines 53 - 67, The extracted placeholder paths from extractImagePlaceholderPaths must be validated with validate.SafeInputPath before any filesystem access; update the loop over placeholders to call validate.SafeInputPath(path) for each path and return a wrapped error if it fails, then proceed to runtime.FileIO().Stat(path) and the existing regular-file check; ensure you reference the same variables and functions (placeholders, path, validate.SafeInputPath, runtime.FileIO().Stat, common.WrapInputStatError/common.FlagErrorf) so the validation happens prior to Stat.shortcuts/slides/slides_media_upload.go (1)
44-55:⚠️ Potential issue | 🟠 MajorGuard
--filewithvalidate.SafeInputPathin validation.The
--fileflag is user-controlled, but this shortcut never runs it throughvalidate.SafeInputPathbefore theStat/upload in Execute. While the documentation states only CWD-relative paths are accepted, absolute paths and..traversal can survive to execution without explicit validation.Suggested fix
+import "github.com/larksuite/cli/internal/validate" + Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { ref, err := parsePresentationRef(runtime.Str("presentation")) if err != nil { return err } + if err := validate.SafeInputPath(runtime.Str("file")); err != nil { + return output.ErrValidation("unsafe file path: %s", err) + } if ref.Kind == "wiki" { if err := runtime.RequireScopes("wiki:node:read"); err != nil { return err } } return nil },As per coding guidelines,
**/*.go: Validate paths usingvalidate.SafeInputPathbefore any file I/O operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_media_upload.go` around lines 44 - 55, The Validate block for the shortcut currently parses the presentation ref but does not validate the user-controlled --file path; update the Validate function to call validate.SafeInputPath on runtime.Str("file") (and return its error) before returning nil so any absolute paths or .. traversal are rejected early; ensure you import/use the validate package and keep this check alongside the existing parsePresentationRef and wiki scope guard in the same Validate closure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/slides/slides_create.go`:
- Around line 53-67: The extracted placeholder paths from
extractImagePlaceholderPaths must be validated with validate.SafeInputPath
before any filesystem access; update the loop over placeholders to call
validate.SafeInputPath(path) for each path and return a wrapped error if it
fails, then proceed to runtime.FileIO().Stat(path) and the existing regular-file
check; ensure you reference the same variables and functions (placeholders,
path, validate.SafeInputPath, runtime.FileIO().Stat,
common.WrapInputStatError/common.FlagErrorf) so the validation happens prior to
Stat.
In `@shortcuts/slides/slides_media_upload.go`:
- Around line 44-55: The Validate block for the shortcut currently parses the
presentation ref but does not validate the user-controlled --file path; update
the Validate function to call validate.SafeInputPath on runtime.Str("file") (and
return its error) before returning nil so any absolute paths or .. traversal are
rejected early; ensure you import/use the validate package and keep this check
alongside the existing parsePresentationRef and wiki scope guard in the same
Validate closure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7086a7b7-945c-47a5-8c39-4f6da63c1f5f
📒 Files selected for processing (15)
shortcuts/common/runner.goshortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/slide-templates.mdskills/lark-slides/references/xml-format-guide.mdskills/lark-slides/references/xml-schema-quick-ref.md
✅ Files skipped from review due to trivial changes (6)
- skills/lark-slides/references/xml-format-guide.md
- skills/lark-slides/references/xml-schema-quick-ref.md
- skills/lark-slides/references/lark-slides-xml-presentation-slide-create.md
- skills/lark-slides/references/lark-slides-create.md
- shortcuts/slides/helpers_test.go
- skills/lark-slides/references/lark-slides-media-upload.md
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/slides/shortcuts.go
- shortcuts/slides/slides_create_test.go
ViperCai
force-pushed
the
feat/slides-image-upload
branch
from
ee3a336 to
001d597
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/slides/slides_media_upload.go (1)
44-48: Consider validating--fileexists inValidatefor faster feedback.Currently,
Validateonly checks--presentationbut not--file. WhileExecutedoes validate the file viaFileIO().Stat(which internally callsSafeInputPath), moving this check toValidatewould fail faster with a clearer error context. This matches the pattern inSlidesCreate.Validate(lines 52-60 inslides_create.go).However, since
FileIO().StatinExecutealready enforces path safety, this is a UX improvement rather than a security issue.♻️ Optional: Add file validation in Validate
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { if _, err := parsePresentationRef(runtime.Str("presentation")); err != nil { return err } + filePath := runtime.Str("file") + stat, err := runtime.FileIO().Stat(filePath) + if err != nil { + return common.WrapInputStatError(err, "file not found") + } + if !stat.Mode().IsRegular() { + return output.ErrValidation("file must be a regular file: %s", filePath) + } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/slides/slides_media_upload.go` around lines 44 - 48, The current Validate in slides_media_upload.go only checks presentation; update Validate to also verify the provided --file exists for faster, clearer feedback: after parsing presentation via parsePresentationRef(runtime.Str("presentation")), get the file path with runtime.Str("file") and call runtime.FileIO().Stat (or the same FileIO().Stat used in Execute) to ensure the path is safe and the file exists, returning any error encountered; reference the Validate function in slides_media_upload.go and mirror the approach used in SlidesCreate.Validate to implement this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/slides/slides_media_upload.go`:
- Around line 44-48: The current Validate in slides_media_upload.go only checks
presentation; update Validate to also verify the provided --file exists for
faster, clearer feedback: after parsing presentation via
parsePresentationRef(runtime.Str("presentation")), get the file path with
runtime.Str("file") and call runtime.FileIO().Stat (or the same FileIO().Stat
used in Execute) to ensure the path is safe and the file exists, returning any
error encountered; reference the Validate function in slides_media_upload.go and
mirror the approach used in SlidesCreate.Validate to implement this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dc948ce-f93e-42ff-bf6f-c75c730c13d3
📒 Files selected for processing (14)
shortcuts/slides/helpers.goshortcuts/slides/helpers_test.goshortcuts/slides/shortcuts.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-create.mdskills/lark-slides/references/lark-slides-media-upload.mdskills/lark-slides/references/lark-slides-xml-presentation-slide-create.mdskills/lark-slides/references/slide-templates.mdskills/lark-slides/references/xml-format-guide.mdskills/lark-slides/references/xml-schema-quick-ref.md
✅ Files skipped from review due to trivial changes (4)
- skills/lark-slides/references/lark-slides-xml-presentation-slide-create.md
- shortcuts/slides/shortcuts.go
- skills/lark-slides/references/lark-slides-create.md
- skills/lark-slides/references/lark-slides-media-upload.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-slides/references/xml-schema-quick-ref.md
- skills/lark-slides/references/xml-format-guide.md
ViperCai
force-pushed
the
feat/slides-image-upload
branch
from
001d597 to
053b1e4
Compare
github-actions
bot
added
domain/ccm
…rs in +create - New `slides +media-upload` shortcut: upload a local image to a slides presentation and return the file_token for use in <img src="...">. - `slides +create --slides` now supports `@./path.png` placeholders that are auto-uploaded and replaced with file_tokens. - Reject images >20 MB (multipart upload not supported for slide_file). - Support wiki URL resolution for --presentation flag.
ViperCai
force-pushed
the
feat/slides-image-upload
branch
from
053b1e4 to
a173b29
Compare
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.
Summary
Adds first-class image support to the slides skill. Users (and AI agents) can now upload local images into a presentation and embed them via XML, either by reusing a returned
file_tokenor by writing<img src=\"@./path\">placeholders inside+create --slidesand letting the CLI auto-upload + rewrite.Changes
slides +media-uploadshortcut: uploads a local image to a presentation and returns itsfile_token. Validates--fileis a CWD-relative safe path; usesslide_fileasparent_type(the only value the slides upload endpoint accepts).slides +create --slidesnow scans each slide XML for<img src=\"@./...\">placeholders, uploads the referenced files via the same code path, and rewritessrcto the resultingfile_tokenbefore creating slides.skills/lark-slides/...) expanded:SKILL.mdso it is discoverable.width:heightmust match original aspect or the image is cropped).references/slide-templates.md.SKILL.md.before_slide_idbelongs in--databody — putting it in--paramsis silently forwarded as an unknown query param and the new slide ends up appended to the end instead of inserted at the requested position.Test Plan
go test ./shortcuts/slides/... ./cmd/service/...lark-cli slides +create --slides '[...<img src=\"@./pic.png\".../>...]'uploads and embeds the imagelark-cli slides +media-upload --file ./pic.png --presentation $PIDreturns afile_tokenbefore_slide_idin--datalands the new slide at the requested position; placing it in--params(the documented anti-pattern) silently appends to the end, confirming the warning is needednode scripts/skill-format-check/index.jspassesRelated Issues
Summary by CodeRabbit
Release Notes
New Features
Documentation