google / google/mcp-security

Fragile ISO datetime parsing antipattern .replace("Z", "+00:00") causes time shift and validation errors

Open
#291 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
528
Forks
141
Avg merge
3d 2h
Merged PRs (30d)
6

Description

## Problem Statement

Across several tool modules in `server/secops/secops_mcp/tools/`, ISO-8601 datetime strings are parsed using the ad-hoc pattern:

```python
start_dt = datetime.fromisoformat(start_time.replace("Z", "+00:00"))
```

This pattern has several functional and correctness issues:

1. **Silent Time-Shift with Non-UTC Offsets**:
If a user or LLM agent supplies an ISO timestamp with an explicit non-UTC offset (for example `"2025-01-20T12:00:00-05:00"` which is 17:00:00 UTC):
- `datetime.fromisoformat()` produces a timezone-aware datetime with `tzinfo=UTC-5`.
- When passed to Chronicle SDK methods (e.g. `list_detections`, `test_rule`), the SDK formats the datetime using:
```python
extra_params["startTime"] = start_time.strftime("%Y-%m-%dT%H:%M:%S.%fZ")
```
- `strftime` formats the **local wall-clock hours** (`12:00:00`) and attaches a literal `Z`, sending `"2025-01-20T12:00:00.000000Z"` to Chronicle API instead of the correct `"2025-01-20T17:00:00.000000Z"`.
- Without `.astimezone(timezone.utc)`, this results in a silent time shift in API queries.

2. **Brittle String Replacement**:
- A global `.replace("Z", "+00:00")` replaces any uppercase `Z` in the entire string.
- It fails on lowercase `'z'` (e.g. `"2025-01-20T00:00:00z"`), raising an uncaught `ValueError: Invalid isoformat string`.
- In Python 3.11+, `fromisoformat()` natively parses trailing `"Z"` without needing `.replace()`.

3. **Naive vs. Aware Inconsistencies**:
- Strings without an explicit timezone (e.g. `"2025-01-20T00:00:00"`) produce naive datetimes (`tzinfo=None`), which can cause `TypeError: can't compare offset-naive and offset-aware datetimes` if mixed with aware datetimes.

---

## Occurrences in the Codebase

The antipattern is currently used in the following locations:

- **`server/secops/secops_mcp/tools/log_ingestion.py`** (Lines 122, 124) in `ingest_log`:
```python
ingestion_params['log_entry_time'] = datetime.fromisoformat(log_entry_time.replace('Z', '+00:00'))
ingestion_params['collection_time'] = datetime.fromisoformat(collection_time.replace('Z', '+00:00'))
```
- **`server/secops/secops_mcp/tools/curated_rules_management.py`** (Lines 321, 322) in `list_curated_rule_detections`:
```python
start_dt = datetime.fromisoformat(start_time.replace("Z", "+00:00"))
end_dt = datetime.fromisoformat(end_time.replace("Z", "+00:00"))
```
- **`server/secops/secops_mcp/tools/security_rules.py`** (Lines 943, 948) in `test_rule`:
```python
start_dt = datetime.fromisoformat(start_time.replace("Z", "+00:00"))
end_dt = datetime.fromisoformat(end_time.replace("Z", "+00:00"))
```
- **`server/secops/secops_mcp/tools/security_rules.py`** (Lines 1227, 1232) in `create_retrohunt`:
```python
start_dt = datetime.fromisoformat(start_time.replace("Z", "+00:00"))
end_dt = datetime.fromisoformat(end_time.replace("Z", "+00:00"))
```
- **`server/secops/secops_mcp/tools/security_rules.py`** (Lines 321, 326) in PR #283 (`get_rule_detections`).

---

## Proposed Fix

1. Add a shared, robust datetime parser in `server/secops/secops_mcp/utils.py`:

```python
def parse_iso_datetime(time_str: str) -> datetime:
"""Parses an ISO 8601 string and returns a UTC timezone-aware datetime."""
if time_str.endswith(("z", "Z")):
time_str = time_str[:-1] + "+00:00"
dt = datetime.fromisoformat(time_str)
if dt.tzinfo is None:
dt = dt.replace(tzinfo=timezone.utc)
return dt.astimezone(timezone.utc)
```

2. Refactor existing tools in `log_ingestion.py`, `curated_rules_management.py`, and `security_rules.py` to use `parse_iso_datetime` (or `parse_time_range`).

3. Add unit tests covering:
- Trailing `"Z"` and `"z"`
- Positive and negative timezone offsets (`"+02:00"`, `"-05:00"`) ensuring proper normalization to UTC
- Naive ISO timestamps defaulting to UTC
- Invalid formats raising `ValueError` cleanly

Contributor guide

Open the contributing guide

Research direction

Start by reading the datetime handling in server/secops/secops_mcp/tools/log_ingestion.py, curated_rules_management.py, and security_rules.py, then inspect server/secops/secops_mcp/utils.py as the proposed shared location. Trace the listed tool entry points and add focused unit coverage for trailing Z/z, timezone offsets, naive timestamps, and invalid formats; done means all inputs normalize correctly and invalid values raise ValueError cleanly.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.