Skip to content

fix: harden URL and path construction across helper modules#133

Open
NeekChaw wants to merge 1 commit intogoogleworkspace:mainfrom
NeekChaw:fix/harden-url-path-construction
Open

fix: harden URL and path construction across helper modules#133
NeekChaw wants to merge 1 commit intogoogleworkspace:mainfrom
NeekChaw:fix/harden-url-path-construction

Conversation

@NeekChaw
Copy link

@NeekChaw NeekChaw commented Mar 5, 2026

Closes #87

Summary

Audit of helper modules found several places where user-provided values are interpolated into URLs or file paths via format! without going through encode_path_segment() or validate_resource_name().

Changes

src/discovery.rs

  • Add validate_discovery_id() — only allows [a-zA-Z0-9._-] for service names/versions. Prevents path traversal in the local cache file path and injection in Discovery Document endpoint URLs.
  • Validate service and version at entry to fetch_discovery_document().
  • Move version query parameter in the alt-URL code path to reqwest .query() instead of string interpolation.

src/helpers/modelarmor.rs

  • Call validate_resource_name() on --template in handle_sanitize() and build_sanitize_request_data() before building URLs.
  • Validate --project, --location, --template-id in parse_create_template_args().
  • Use encode_path_segment() for templateId in build_create_template_url().

src/helpers/gmail/watch.rs

  • Extract message-URL building to build_message_url() using encode_path_segment() on the message ID.
  • Switch msg_format to reqwest .query() builder instead of string interpolation.

Testing

Added 15 new unit tests (happy-path + error-path) covering each fix:

  • 6 tests for validate_discovery_id in discovery.rs
  • 5 tests for resource name/template validation in modelarmor.rs
  • 4 tests for build_message_url in watch.rs

…oogleworkspace#87)

Audit of helper modules found several places where user-provided values are
interpolated into URLs or file paths via format! without going through
encode_path_segment() or validate_resource_name().

Changes:

src/discovery.rs
- Add validate_discovery_id() that allows only [a-zA-Z0-9._-] for service
  names and version strings, preventing path traversal in the cache directory
  and injection in Discovery Document endpoint URLs.
- Validate service and version at the entry point of fetch_discovery_document.
- Move the version query parameter in the alt-URL code path to reqwest .query().

src/helpers/modelarmor.rs
- Call validate_resource_name() on --template before embedding it in URLs
  in handle_sanitize() and build_sanitize_request_data().
- Validate --project, --location, --template-id in parse_create_template_args().
- Use encode_path_segment() for templateId in build_create_template_url().

src/helpers/gmail/watch.rs
- Extract message-URL building to build_message_url() using encode_path_segment
  on the message ID (prevents path/query injection from API-supplied IDs).
- Switch msg_format to reqwest .query() builder instead of string interpolation.

Adds 15 new unit tests (happy-path + error-path) for each fix.
@NeekChaw NeekChaw requested a review from jpoehnelt as a code owner March 5, 2026 07:15
@changeset-bot
Copy link

changeset-bot bot commented Mar 5, 2026

🦋 Changeset detected

Latest commit: ee3609f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@googleworkspace/cli Patch

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

@gemini-code-assist
Copy link
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@google-cla
Copy link

google-cla bot commented Mar 5, 2026

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.

@NeekChaw
Copy link
Author

NeekChaw commented Mar 5, 2026

@googlebot I signed it!

@jpoehnelt jpoehnelt added area: http cla: no This human has *not* signed the Contributor License Agreement. complexity: medium Moderate change, some review needed labels Mar 5, 2026
@jpoehnelt
Copy link
Member

please fix the conflicts

@jpoehnelt
Copy link
Member

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request does a great job of hardening URL and path construction across several modules by introducing validation and proper encoding for user-provided inputs. The changes significantly improve the security posture of the application against path traversal and injection attacks. I've found one area in the new validation logic in discovery.rs where the check can be made more robust to cover additional edge cases for invalid identifiers. My specific feedback is in the comment below.

Comment on lines +204 to +207
// Reject dot-only segments that could act as a traversal (e.g. "..")
if s.split('.').any(|seg| seg.is_empty()) && s.starts_with('.') {
anyhow::bail!("{label} '{s}' must not start with '.'");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The current validation for dot-separated identifiers is not comprehensive enough. It correctly rejects identifiers starting with . (like .foo, ., and ..), but it fails to reject other invalid sequences such as foo..bar or foo.. When interpolated into a hostname like {service}.googleapis.com, these would result in invalid domain names (foo..bar.googleapis.com or foo..googleapis.com).

A more robust approach is to reject any empty segment that results from splitting the string by ..

    // Reject any empty dot-separated segments, which are invalid in hostnames
    // and can be used for path traversal (e.g. '..', '.foo', 'foo.').
    if s.split('.').any(str::is_empty) {
        anyhow::bail!(
            "{label} '{s}' contains invalid dot-sequences (e.g. '..', leading/trailing '.', or empty segments)"
        );
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: http cla: no This human has *not* signed the Contributor License Agreement. complexity: medium Moderate change, some review needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden URL and path construction across helper modules

2 participants