alibaba / alibaba/ROCK

[Feature] Add parameter validation for API endpoints

Open
#984 0 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Python
Stars
485
Forks
81
Avg merge
16h 12m
Merged PRs (30d)
8

Description

## Add parameter validation for API endpoints

### Problem

Several API endpoints in the admin service accept string parameters (e.g., `sandbox_id`, `image`) without validating that they are non-empty or non-whitespace. When clients send
empty or whitespace-only strings, the request propagates deeper into the system before eventually failing with unclear or misleading errors — making debugging difficult and
polluting server-side error metrics.

For example:
- `GET /is_alive?sandbox_id=` proceeds to query sandbox status with an empty ID
- `POST /start` with `{"image": " "}` attempts to start a sandbox with a whitespace-only image name
- `POST /stop` with an empty `sandbox_id` body reaches the sandbox manager before failing

### Expected Behavior

API endpoints should validate required string parameters at the boundary and immediately return a clear `BadRequestRockError` when they are `None`, empty, or whitespace-only —
before any business logic executes.

### Scope

The following endpoints/parameters need validation:

**`sandbox_api.py`**
- `POST /start` — `image`
- `POST /start_async` — `image`
- `GET /is_alive` — `sandbox_id`
- `GET /get_sandbox_statistics` — `sandbox_id`
- `GET /get_status` — `sandbox_id`
- `POST /upload` — `sandbox_id`
- `POST /stop` — `sandbox_id`
- `POST /commit` — `sandbox_id`, `image_tag`

**`sandbox_proxy_api.py`**
- `GET /is_alive` — `sandbox_id`
- `POST /upload` — `sandbox_id`

**`gem/api.py`**
- `POST /step` — `sandbox_id`
- `POST /reset` — `sandbox_id`
- `POST /close` — `sandbox_id`

**`SandboxProxyService`**
- `get_service_status()` — `sandbox_id`
- `_update_expire_time()` — `sandbox_id`

### Additional Improvements

- **Metrics**: Distinguish client errors (`BadRequestRockError`) from server errors in the metrics decorator. Client validation failures should increment `request.client_error`
instead of `request.failure` to avoid alerting on user input mistakes.

### Acceptance Criteria

- [ ] A reusable `validate_required_str(value, param_name)` utility exists in `rock/common/validation.py`
- [ ] All listed endpoints call validation before executing business logic
- [ ] `BadRequestRockError` is raised with a message like `"{param_name} is required and must be a non-empty string"`
- [ ] Metrics decorator records `request.client_error` for `BadRequestRockError`, not `request.failure`
- [ ] Unit tests cover the validation utility, endpoint-level validation, and metrics classification

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with sandbox_api.py, sandbox_proxy_api.py, gem/api.py, SandboxProxyService, and the metrics decorator. Review existing error handling and tests, then trace each listed parameter to confirm validation occurs before business logic. Done means rock/common/validation.py has the reusable utility, all listed endpoints and service methods validate inputs, metrics classify BadRequestRockError as client errors, and unit tests cover each requirement.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.