feat(apps-script): add +pull, +open, +run, +logs helpers with .clasp.json support#533
feat(apps-script): add +pull, +open, +run, +logs helpers with .clasp.json support#533qihan-bot wants to merge 4 commits intogoogleworkspace:mainfrom
Conversation
- Pin clawhub@0.7.0 for supply-chain safety - Add concurrency group to prevent parallel publishes - Add PR dry-run validation - Guard against missing CLAWHUB_TOKEN secret - Add changeset file
…json support - Add +pull: download project files to local directory with filename sanitization - Add +open: open script editor in browser - Add +run: execute functions with cloud-platform scope and contextual error hints (extracts GCP project number from OAuth client ID for setup guidance) - Add +logs: view execution logs via processes.listScriptProcesses - Add .clasp.json support: auto-resolve scriptId/rootDir for all helpers - Add clasp_config.rs module with path traversal validation - Add validate_script_filename() for safe file writes during +pull - Regenerate skills for new helper commands
🦋 Changeset detectedLatest commit: b5c8ef8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
googleworkspace-bot
added
area: skills
area: distribution
area: docs
area: core
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the Google Apps Script development experience within Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new helper commands for Google Apps Script (+pull, +open, +run, +logs) and adds support for .clasp.json configuration files, which is a great step towards providing a native gws workflow for Apps Script developers. The code is well-structured and includes important security validations for file and path handling.
My review focuses on a couple of critical areas. I've identified a potential panic in the +pull command's directory resolution logic and an overly restrictive filename validation that could affect compatibility with existing Apps Script projects. The suggestions provided aim to fix these issues to make the new helpers more robust and user-friendly.
| let output_dir = matches | ||
| .get_one::<String>("dir") | ||
| .map(|d| crate::validate::validate_safe_output_dir(d)) | ||
| .transpose()? | ||
| .unwrap_or_else(|| { | ||
| clasp_config::resolve_dir(None).unwrap_or_else(|_| std::env::current_dir().unwrap()) | ||
| }); |
There was a problem hiding this comment.
The logic for resolving the output directory in handle_pull has a critical flaw that can lead to a panic. It incorrectly uses clasp_config::resolve_dir, which is designed for existing input directories and will fail if the target directory doesn't exist. Additionally, the .unwrap() in the fallback logic will panic if it's unable to determine the current working directory.
This should be rewritten to use validate_safe_output_dir for all directory paths and to handle potential errors gracefully without panicking. When implementing or refining validate_safe_output_dir, please ensure that potential Time-of-check to time-of-use (TOCTOU) race conditions are acknowledged and documented as a known limitation if a full mitigation (e.g., using openat(O_NOFOLLOW)) is considered out of scope.
| let output_dir = matches | |
| .get_one::<String>("dir") | |
| .map(|d| crate::validate::validate_safe_output_dir(d)) | |
| .transpose()? | |
| .unwrap_or_else(|| { | |
| clasp_config::resolve_dir(None).unwrap_or_else(|_| std::env::current_dir().unwrap()) | |
| }); | |
| let output_dir = if let Some(dir) = matches.get_one::<String>("dir") { | |
| crate::validate::validate_safe_output_dir(dir)? | |
| } else { | |
| let clasp_dir = clasp_config::load_clasp_config()?.and_then(|c| c.root_dir); | |
| if let Some(dir) = clasp_dir { | |
| crate::validate::validate_safe_output_dir(&dir)? | |
| } else { | |
| std::env::current_dir().map_err(|e| GwsError::Validation(format!("Failed to determine current directory: {e}")))? | |
| } | |
| }; |
References
- When implementing file path validation, acknowledge and document potential Time-of-check to time-of-use (TOCTOU) race conditions as a known limitation if a full mitigation (e.g., using
openat(O_NOFOLLOW)) is considered out of scope.
| // Allowlist: only alphanumeric, underscore, hyphen | ||
| if !name | ||
| .chars() | ||
| .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') | ||
| { | ||
| return Err(GwsError::Validation(format!( | ||
| "Script file name contains invalid characters (allowed: a-z, A-Z, 0-9, _, -): {name}" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
The character allowlist in validate_script_filename is overly restrictive by disallowing dots (.) in filenames. Apps Script files can have names that include dots (e.g., a local file my.utils.js is pushed as a script file named my.utils). This restriction will cause +pull to fail for valid projects that use dots in their script file names, which is a common practice for namespacing and is supported by clasp.
To improve compatibility and support valid use cases, dots should be allowed in filenames. The existing check that prevents filenames from starting with a dot is sufficient to avoid creating hidden files. The test test_validate_script_filename_special_chars will also need to be updated to assert that filenames with dots are now valid.
| // Allowlist: only alphanumeric, underscore, hyphen | |
| if !name | |
| .chars() | |
| .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') | |
| { | |
| return Err(GwsError::Validation(format!( | |
| "Script file name contains invalid characters (allowed: a-z, A-Z, 0-9, _, -): {name}" | |
| ))); | |
| } | |
| // Allowlist: only alphanumeric, underscore, hyphen, and dot (but not as first char) | |
| if !name | |
| .chars() | |
| .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.') | |
| { | |
| return Err(GwsError::Validation(format!( | |
| "Script file name contains invalid characters (allowed: a-z, A-Z, 0-9, _, -, .): {name}" | |
| ))); | |
| } |
3 participants
Summary
+pull: download Apps Script project files to local directory with filename sanitization+open: open script editor in browser+run: execute functions with cloud-platform scope and contextual error hints (extracts GCP project number from OAuth client ID for setup guidance)+logs: view execution logs viaprocesses.listScriptProcesses.clasp.jsonsupport: auto-resolvescriptId/rootDirfor all helpersclasp_config.rsmodule with path traversal validationvalidate_script_filename()for safe file writes during+pullMotivation
These helpers replace the need for Google's
claspCLI tool, providing a nativegwsexperience for Apps Script development workflows.Test plan
cargo clippy