Skip to content

feat(apps): replace +html-publish cwd hard-reject with credential-file scan#1072

Merged
liangshuo-1 merged 4 commits into
mainfrom
feat/apps-sensitive-block
May 25, 2026
Merged

feat(apps): replace +html-publish cwd hard-reject with credential-file scan#1072
liangshuo-1 merged 4 commits into
mainfrom
feat/apps-sensitive-block

Conversation

@raistlin042

@raistlin042 raistlin042 commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

The previous --path == "." block in apps +html-publish was a coarse heuristic: it caught the common foot-gun of publishing a repo root, but also rejected legitimate clean cwds, and let a ./dist with a forgotten .env ship the secret through anyway (the sensitive-paths scanner was advisory and never ran on the Execute path). This PR replaces the path-shape gate with a content-based credential-file scan.

Changes

  • Move the gate to Validate. Validate now walks --path candidates and rejects publishes that include well-known credential files. Living in Validate (not DryRun) means dry-run returns non-zero on hit too, so the dry-run preview matches what Execute would do.
  • Narrow the credential pattern set. Recognized: .env, .env.*, .npmrc, .netrc, .git-credentials, .aws/credentials, .docker/config.json, .kube/config. Explicitly out of scope (false-positive rate too high, or wrong real-world path): .git/ SCM history, SSH private keys (id_rsa* / id_ed25519* etc.), *.pem / *.key, .aws/config, gcloud's config tree (real location is ~/.config/gcloud/, not ~/.gcloud/).
  • Add --allow-sensitive as the escape hatch for legitimate cases (e.g. a docs site shipping .env.example on purpose). DryRun surfaces the waived list in a sensitive_waived field so the caller can relay it to the user.
  • Drop the cwd defense-in-depth in runHTMLPublish. A clean cwd is now a valid publish target.
  • Update the lark-apps skill and the lark-apps-html-publish reference to describe the new gate, the override flag, and the patterns now explicitly out of scope.

Test Plan

  • go test ./shortcuts/apps/ ./tests/cli_e2e/apps/ ./cmd/auth/ passes
  • go vet ./... clean
  • gofmt -l shortcuts/apps/ tests/cli_e2e/apps/ clean
  • Manual: built binary and confirmed apps +html-publish --help lists --allow-sensitive
  • New unit tests cover: clean cwd is allowed; sensitive file blocks Validate (and therefore dry-run); --allow-sensitive overrides; truncation in the validation error message
  • Updated E2E dry-run tests cover the same behaviors end-to-end through the binary

Related Issues

  • None

Summary by CodeRabbit

  • New Features

    • Added an --allow-sensitive flag to optionally bypass credential-file validation for HTML app publishing.
  • Bug Fixes

    • Dry-run and publish now fail by default if known credential files are present; dry-run no longer emits advisory warnings.
    • Publishing from the current working directory is permitted when no intercepted credential files are found.
    • Validation errors include a truncated sample of matches plus a total count.
  • Documentation

    • Updated docs for validation behavior, bypass option, and path guidance.
  • Tests

    • Expanded tests covering validation, allow-sensitive override, cwd behavior, and error truncation.

Review Change Stack

…e scan

The previous --path == "." block was a coarse heuristic: it caught the
common foot-gun of publishing a repo root, but also rejected legitimate
clean cwds, and let a ./dist with a forgotten .env ship the secret
through anyway (the sensitive-paths scanner was advisory and never ran
on the Execute path).

Move the gate from path shape to path content:

- Validate now walks --path candidates and rejects publishes that
  include well-known credential files (.env / .env.* / .npmrc / .netrc
  / .git-credentials / .aws/credentials / .gcloud/credentials* /
  .docker/config.json / .kube/config). Living in Validate (not DryRun)
  means dry-run returns non-zero on hit too, so the dry-run preview
  matches Execute.
- Narrow the credential pattern set. .git/, SSH private keys, *.pem
  and *.key are out of scope -- they're not env-token files and the
  false-positive rate (public certs, docs about key formats) is high.
- Add --allow-sensitive as the escape hatch for legitimate cases
  (e.g. a docs site shipping .env.example on purpose). DryRun surfaces
  the waived list in sensitive_waived so the caller can relay it.
- Drop the cwd defense-in-depth in runHTMLPublish. A clean cwd is now
  a valid publish target.

The lark-apps skill and the html-publish reference are updated to
describe the new gate, the override flag, and the patterns now
explicitly out of scope.
@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 28e9c489-d864-4392-968a-4d689ed8b29c

📥 Commits

Reviewing files that changed from the base of the PR and between 931e1e4 and 78cd9c0.

📒 Files selected for processing (2)
  • shortcuts/apps/apps_html_publish.go
  • shortcuts/apps/sensitive_paths.go

📝 Walkthrough

Walkthrough

Refine html-publish credential validation: narrow detected sensitive paths, add an --allow-sensitive bypass, enforce detection at Validate (affects dry-run), remove the prior forbid-cwd rule, and update tests and docs accordingly.

Changes

HTML Publish Credential Validation

Layer / File(s) Summary
Sensitive path detection contract narrowing
shortcuts/apps/sensitive_paths.go, shortcuts/apps/sensitive_paths_test.go
isSensitiveRelPath now matches a focused set of credential filenames/paths (.env*, .npmrc, .netrc, .git-credentials, and parent-scoped .aws/credentials, .docker/config.json, .kube/config). Added isSensitiveCandidate wrapper that normalizes/re-anchors paths; tests updated for new in-scope and out-of-scope cases.
HTML publish validation and --allow-sensitive
shortcuts/apps/apps_html_publish.go
Add --allow-sensitive flag; Validate now blocks publish (including --dry-run) when sensitive paths are found unless waived; removed prior cwd-blocking logic; introduced sensitiveCandidatesError truncation helper.
DryRun reporting and helpers
shortcuts/apps/apps_html_publish.go
DryRun no longer emits default non-blocking warnings; when --allow-sensitive is used it emits sensitive_waived and sensitive_waived_summary. Added formatting helpers to cap inline sample lists in error output.
Unit and integration test updates
shortcuts/apps/apps_html_publish_test.go, tests/cli_e2e/apps/apps_html_publish_dryrun_test.go
Add tests for clean-cwd acceptance, sensitive-file validation blocking, --allow-sensitive bypass reporting, and truncating error messages; update e2e dry-run subtests and remove prior cwd-rejection test.
Documentation updates
skills/lark-apps/SKILL.md, skills/lark-apps/references/lark-apps-html-publish.md
Document Validate-stage credential-file interception, the --allow-sensitive bypass and dry-run waived reporting, removal of the cwd prohibition, and the narrowed detection/explicit exclusions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • larksuite/cli#1002: Related work on the apps +html-publish shortcut that this PR further modifies (sensitive-path handling and flags).

Suggested labels

size/L, feature

Suggested reviewers

  • liangshuo-1

Poem

🐰 I hop through files with careful eyes,
I sniff .env and .npmrc where danger lies.
A flag to waive when intent is clear,
Clean cwd allowed — step forward, my dear.
Tiny paws, precise guards — publish on, hooray.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: replacing the cwd hard-reject with a credential-file scanning approach for apps HTML publishing.
Description check ✅ Passed The PR description provides a comprehensive summary, itemized changes, detailed test plan with checkmarks, and follows the repository template structure with all required sections completed.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/apps-sensitive-block

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 and usage tips.

@github-actions github-actions Bot added the size/M Single-domain feat or fix with limited business impact label May 25, 2026
@github-actions

github-actions Bot commented May 25, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@78cd9c0e3a2fe33bd7f112d1c6c7eed5b71b08f5

🧩 Skill update

npx skills add larksuite/cli#feat/apps-sensitive-block -y -g

@codecov

codecov Bot commented May 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.05882% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.90%. Comparing base (708cbc2) to head (78cd9c0).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
shortcuts/apps/apps_html_publish.go 93.10% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1072      +/-   ##
==========================================
+ Coverage   67.79%   67.90%   +0.10%     
==========================================
  Files         591      592       +1     
  Lines       55237    55410     +173     
==========================================
+ Hits        37448    37625     +177     
+ Misses      14680    14673       -7     
- Partials     3109     3112       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@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.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@skills/lark-apps/SKILL.md`:
- Line 48: The documentation entry uses the glob ".gcloud/credentials*" which
implies files like "credentials_old" would be blocked, but the
implementation/tests only match "credentials" and "credentials.*"; update the
wording to replace ".gcloud/credentials*" with a precise description such as
".gcloud/credentials and .gcloud/credentials.*" (or explicitly state that only
the exact file name and dotted extensions are matched) so the doc aligns with
the actual matcher behavior and avoids false expectations.
🪄 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: 278378ff-6a15-4729-910a-39644fded08b

📥 Commits

Reviewing files that changed from the base of the PR and between f2a4c95 and ad95b1b.

📒 Files selected for processing (7)
  • shortcuts/apps/apps_html_publish.go
  • shortcuts/apps/apps_html_publish_test.go
  • shortcuts/apps/sensitive_paths.go
  • shortcuts/apps/sensitive_paths_test.go
  • skills/lark-apps/SKILL.md
  • skills/lark-apps/references/lark-apps-html-publish.md
  • tests/cli_e2e/apps/apps_html_publish_dryrun_test.go

Comment thread skills/lark-apps/SKILL.md Outdated
The .gcloud/credentials pattern matched a non-existent path: gcloud's
actual config dir is ~/.config/gcloud/ (XDG-based), and the real
credential files there are credentials.db / access_tokens.db /
application_default_credentials.json -- none of which would land under
a .gcloud/ segment in a publish payload.

Drop the rule rather than fix it: the realistic gcloud foot-gun would
require recognizing the .config/gcloud/* tree by file basename, which
is a broader change than the targeted env/cred scan in this PR. The
remaining 7 patterns (.env / .env.* / .npmrc / .netrc /
.git-credentials / .aws/credentials / .docker/config.json /
.kube/config) cover the common Node/Python/CLI-tooling foot-guns.
@raistlin042

Copy link
Copy Markdown
Collaborator Author

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… itself

isSensitiveRelPath anchors cloud-SDK matchers on adjacent parent/file
segments (.aws/credentials, .docker/config.json, .kube/config), but
walker strips that parent via filepath.Rel when --path is the conventional
parent dir (e.g. ./.aws), yielding a bare RelPath="credentials" that
slipped through silently. Same bypass for the single-file form
--path ./.aws/credentials (walker sets RelPath = Base(rootPath)).

Wrap the scan in isSensitiveCandidate: keep the fast RelPath scan, and
on miss fall back to filepath.Abs(AbsPath) so the parent segment is
visible again. isSensitiveRelPath itself is unchanged; existing tests
still pin its pure-function contract.
… lint

The previous fix called filepath.Abs(c.AbsPath) — banned by the repo's
forbidigo rule because shortcuts must not reach into the filesystem for
path resolution.

Reframe the same fix without fs access: re-prepend the root's basename
(or, for the single-file form, the parent dir's basename of rootPath)
to RelPath and re-scan only the parent-anchored credential pairs
(.aws/credentials, .docker/config.json, .kube/config). Leaf matchers
(.env / .npmrc / ...) stay scoped to RelPath — incidentally closing a
latent false-positive where --path /home/alice/.env/dist would have
flagged every file under it just because .env appeared in the
absolute path.
@liangshuo-1
liangshuo-1 merged commit e93e2a9 into main May 25, 2026
21 checks passed
@liangshuo-1
liangshuo-1 deleted the feat/apps-sensitive-block branch May 25, 2026 15:24
@liangshuo-1 liangshuo-1 mentioned this pull request May 26, 2026
3 tasks
tuxedomm pushed a commit to zhumiaoxin/cli that referenced this pull request Jun 6, 2026
…e scan (larksuite#1072)

* feat(apps): replace +html-publish cwd hard-reject with credential-file scan

The previous --path == "." block was a coarse heuristic: it caught the
common foot-gun of publishing a repo root, but also rejected legitimate
clean cwds, and let a ./dist with a forgotten .env ship the secret
through anyway (the sensitive-paths scanner was advisory and never ran
on the Execute path).

Move the gate from path shape to path content:

- Validate now walks --path candidates and rejects publishes that
  include well-known credential files (.env / .env.* / .npmrc / .netrc
  / .git-credentials / .aws/credentials / .gcloud/credentials* /
  .docker/config.json / .kube/config). Living in Validate (not DryRun)
  means dry-run returns non-zero on hit too, so the dry-run preview
  matches Execute.
- Narrow the credential pattern set. .git/, SSH private keys, *.pem
  and *.key are out of scope -- they're not env-token files and the
  false-positive rate (public certs, docs about key formats) is high.
- Add --allow-sensitive as the escape hatch for legitimate cases
  (e.g. a docs site shipping .env.example on purpose). DryRun surfaces
  the waived list in sensitive_waived so the caller can relay it.
- Drop the cwd defense-in-depth in runHTMLPublish. A clean cwd is now
  a valid publish target.

The lark-apps skill and the html-publish reference are updated to
describe the new gate, the override flag, and the patterns now
explicitly out of scope.

* feat(apps): drop .gcloud/* from credential-file scan

The .gcloud/credentials pattern matched a non-existent path: gcloud's
actual config dir is ~/.config/gcloud/ (XDG-based), and the real
credential files there are credentials.db / access_tokens.db /
application_default_credentials.json -- none of which would land under
a .gcloud/ segment in a publish payload.

Drop the rule rather than fix it: the realistic gcloud foot-gun would
require recognizing the .config/gcloud/* tree by file basename, which
is a broader change than the targeted env/cred scan in this PR. The
remaining 7 patterns (.env / .env.* / .npmrc / .netrc /
.git-credentials / .aws/credentials / .docker/config.json /
.kube/config) cover the common Node/Python/CLI-tooling foot-guns.

* fix(apps): close credential-scan bypass when --path is the parent dir itself

isSensitiveRelPath anchors cloud-SDK matchers on adjacent parent/file
segments (.aws/credentials, .docker/config.json, .kube/config), but
walker strips that parent via filepath.Rel when --path is the conventional
parent dir (e.g. ./.aws), yielding a bare RelPath="credentials" that
slipped through silently. Same bypass for the single-file form
--path ./.aws/credentials (walker sets RelPath = Base(rootPath)).

Wrap the scan in isSensitiveCandidate: keep the fast RelPath scan, and
on miss fall back to filepath.Abs(AbsPath) so the parent segment is
visible again. isSensitiveRelPath itself is unchanged; existing tests
still pin its pure-function contract.

* fix(apps): drop filepath.Abs from sensitive scan to satisfy forbidigo lint

The previous fix called filepath.Abs(c.AbsPath) — banned by the repo's
forbidigo rule because shortcuts must not reach into the filesystem for
path resolution.

Reframe the same fix without fs access: re-prepend the root's basename
(or, for the single-file form, the parent dir's basename of rootPath)
to RelPath and re-scan only the parent-anchored credential pairs
(.aws/credentials, .docker/config.json, .kube/config). Leaf matchers
(.env / .npmrc / ...) stay scoped to RelPath — incidentally closing a
latent false-positive where --path /home/alice/.env/dist would have
flagged every file under it just because .env appeared in the
absolute path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants