# Phase 25.1: Parser Robustness & Security Hardening - Research

**Researched:** 2026-03-12
**Domain:** Rust parser security hardening, whitespace normalization, FFI boundary safety
**Confidence:** HIGH

## Summary

Phase 25.1 fixes a known tech-debt gap: `detect_ddl_kind` in `src/parse.rs` uses literal byte
prefix matching (`starts_with_ci`) that only accepts single ASCII spaces between keywords. Tabs,
newlines, carriage returns, and multiple spaces between `CREATE`, `SEMANTIC`, and `VIEW` all
cause silent detection failure — the query falls through as "not ours" and DuckDB renders its own
error. This is documented in TECH-DEBT.md item 4 and was surfaced by proptests in Phase 23.

The fix is a token-based keyword scanner that replaces the literal prefix match. The approach is
purely additive: replace `starts_with_ci(bytes, b"create semantic view")` with a function that
tokenizes on any whitespace and matches the keyword sequence. The downstream `prefix_len` coupling
(used to slice the string after the matched prefix) must also change — after token-based matching,
the code must track how many bytes were consumed by the prefix match, not hard-code a constant.

Security hardening is a second, orthogonal concern: the FFI boundary currently uses
`from_utf8_unchecked` and trusts the caller. This is safe for the DuckDB host but needs
explicit documentation and, where practical, hardening. The body parser and rewrite path have
SQL-injection-adjacent risks that are currently mitigated by the single-quote escape in
`rewrite_ddl_keyword_body` but should be audited and tested.

**Primary recommendation:** Implement a `match_keyword_tokens` helper that consumes whitespace
between tokens and returns the byte count consumed. Use it in `detect_ddl_kind` to replace all
seven `starts_with_ci` calls. Add proptest coverage for the prefix whitespace variants. Add
targeted unit tests for adversarial inputs. No new dependencies needed.

## Standard Stack

### Core
| Library | Version | Purpose | Why Standard |
|---------|---------|---------|--------------|
| (none new) | — | All required capabilities are already present | Cargo.toml already has `strsim`, `serde_json`, `proptest` |

### Supporting
| Library | Version | Purpose | When to Use |
|---------|---------|---------|-------------|
| `proptest` | 1.9 | Property-based tests for whitespace variants | Already in dev-dependencies; existing `parse_proptest.rs` establishes the pattern |

**Installation:** No new dependencies required.

## Architecture Patterns

### Recommended Project Structure

No new files needed. All changes are in existing source files:

```
src/
├── parse.rs          # detect_ddl_kind: replace starts_with_ci with token-based scan
│                     # prefix_len: replace with dynamic consumed-byte count
│                     # validate_and_rewrite: no change needed (uses detect_ddl_kind result)
└── body_parser.rs    # Already whitespace-tolerant (find_clause_bounds skips ascii_whitespace)
                      # Audit and confirm; no logic changes expected

tests/
└── parse_proptest.rs # Add TEST-07: prefix whitespace proptests
                      # Add TEST-08: adversarial input unit tests
```

### Pattern 1: Token-Based Keyword Matching

**What:** Replace literal byte comparison with a function that tokenizes on any whitespace.

**When to use:** Wherever the code needs to match a fixed sequence of SQL keywords at the start
of an input string, tolerating arbitrary inter-keyword whitespace.

**Approach:**

```rust
// Conceptual structure (implementation is the task, not the research)
//
// match_keyword_prefix(input: &[u8], keywords: &[&[u8]]) -> Option<usize>
//
// Walks the input byte-by-byte:
// 1. Skip leading whitespace (spaces, tabs, \n, \r, \v, \f)
// 2. Match keyword[0] case-insensitively
// 3. Require at least one whitespace after keyword[0] (unless it's the last)
// 4. Repeat for keywords[1..n]
// 5. Return Some(bytes_consumed) if all keywords matched, None otherwise
//
// The returned bytes_consumed replaces the hard-coded prefix_len values.
```

The seven DDL prefixes become keyword token sequences:
- `b"create semantic view"` → `[b"create", b"semantic", b"view"]`
- `b"create or replace semantic view"` → `[b"create", b"or", b"replace", b"semantic", b"view"]`
- etc.

The longest-first ordering in `detect_ddl_kind` is preserved — it prevents prefix overlap.

### Pattern 2: Downstream prefix_len Decoupling

**What:** The current `prefix_len(kind)` function returns hardcoded byte counts (e.g.,
`"create semantic view".len()` = 20). After switching to token-based matching, the actual
consumed byte count depends on the whitespace in the input and can no longer be a constant.

**Fix approach:** `detect_ddl_kind` returns `Option<DdlKind>` today. A new internal version
(or a parallel function `detect_ddl_kind_with_len`) should return `Option<(DdlKind, usize)>`,
where `usize` is the byte count consumed by the prefix match. The callers (`validate_and_rewrite`,
`rewrite_ddl`, `extract_ddl_name`) already call `prefix_len(kind)` to slice. They should
instead use the dynamically computed length.

**Public API impact:** `detect_ddl_kind` is `pub` and used in `parse_proptest.rs`. The return
type `Option<DdlKind>` should not change (backward compat with tests). The internal helper can
be a private `fn detect_ddl_prefix(query: &str) -> Option<(DdlKind, usize)>`.

### Pattern 3: Single-Quote Escaping in Rewrite Path

**What:** `rewrite_ddl_keyword_body` in `parse.rs` already applies `name.replace('\'', "''")`
for the view name and `json.replace('\'', "''")` for the JSON body. This is the correct approach
for embedding into SQL string literals delimited by single quotes.

**Status:** Existing implementation is correct. No change needed. The test coverage should
explicitly verify these escape paths with adversarial names.

### Anti-Patterns to Avoid

- **Normalizing whitespace globally before matching:** Do not call `query.split_whitespace().join(" ")` on the entire input. This destroys byte offsets used for error position reporting. The token-based match must work on the raw bytes and return a consumed-byte count.
- **Using `from_utf8_unchecked` outside of the FFI boundary:** The inner parse functions receive `&str` slices — UTF-8 validity is already guaranteed by the type. The unchecked conversion is only needed at the FFI entry point and is already gated there.
- **Recursive string allocations per keyword:** The token matcher should operate on byte slices with index arithmetic, not allocate intermediate `String` values per keyword match.

## Don't Hand-Roll

| Problem | Don't Build | Use Instead | Why |
|---------|-------------|-------------|-----|
| Unicode whitespace normalization | Custom Unicode scanner | DuckDB already applies `StripUnicodeSpaces()` before the parse hook | Only ASCII whitespace variants need handling in `detect_ddl_kind` |
| SQL injection prevention in rewrite output | Custom sanitizer | Single-quote doubling `replace('\'', "''")` | Standard SQL string literal escaping; already implemented correctly |
| Levenshtein distance for near-miss errors | Custom edit-distance | `strsim::levenshtein` (already in Cargo.toml) | Handles Unicode-correct edit distance |

**Key insight:** The scope here is deliberately narrow. DuckDB handles Unicode whitespace
normalization (`\u2003` etc.) before the hook fires. The `detect_ddl_kind` fix only needs to
handle the six ASCII whitespace characters: space (0x20), tab (0x09), newline (0x0A), vertical
tab (0x0B), form feed (0x0C), carriage return (0x0D).

## Common Pitfalls

### Pitfall 1: Destroying byte offsets by whitespace normalization

**What goes wrong:** If the code normalizes whitespace by joining tokens before matching (e.g.,
`query.split_whitespace().collect::<Vec<_>>().join(" ")`), the byte offsets used for error
position reporting become invalid. A caret error reported at position 15 would point to the
wrong character in the original query.

**Why it happens:** Error position is a byte offset into the *original* query string (before
any trimming). The existing proptests in `parse_proptest.rs` TEST-03 verify this invariant.

**How to avoid:** The token-based matcher must operate on the raw query bytes, tracking byte
position as it scans. Return the byte count consumed, not a re-indexed value from a normalized
copy.

**Warning signs:** A proptest like `position_invariant_clause_typo` starts failing with off-by-N
errors after the whitespace fix.

### Pitfall 2: prefix_len constant mismatch after token matching

**What goes wrong:** `detect_ddl_kind` returns `DdlKind::Create` for a tab-separated query, but
`prefix_len(DdlKind::Create)` still returns 20 (the byte length of `"create semantic view"`).
The caller slices `trimmed[20..]` to get the view name, but if the actual prefix was `CREATE\tSEMANTIC\tVIEW` (22 bytes), the slice is 2 bytes short and includes a trailing tab.

**Why it happens:** The current `prefix_len` function is hardcoded to the single-space prefix
lengths. It was correct when `starts_with_ci` used single-space literals.

**How to avoid:** Pair the detection with the consumed byte count. Any function that calls
`prefix_len(kind)` must be updated to use the actual matched length from `detect_ddl_prefix`.

**Warning signs:** `test_extract_name` or `test_rewrite_*` tests fail with names that include
leading whitespace characters when the prefix had extra whitespace.

### Pitfall 3: Adversarial inputs causing O(n²) behavior

**What goes wrong:** If the token matcher has a naive retry loop (try to match at offset 0,
fail, try at offset 1, fail, ...), a very long input that almost-matches the prefix causes
quadratic scan behavior.

**Why it happens:** The matcher only needs to check at offset 0 (after leading whitespace).
It should not retry at increasing offsets.

**How to avoid:** The token matcher anchors at the start of the trimmed input. A single pass
through the prefix tokens is sufficient. If any token fails to match, return `None` immediately.

**Warning signs:** A test with a 1 MB string of `A` characters hangs instead of returning
`PARSE_NOT_OURS` in microseconds.

### Pitfall 4: Null bytes in the FFI input

**What goes wrong:** The FFI entry point uses `from_utf8_unchecked`. If DuckDB ever passes a
query containing embedded null bytes (e.g., from a crafted client), the Rust `&str` created by
`slice::from_raw_parts(ptr, len)` would contain null bytes. Most Rust string operations are
null-safe (they use `len`, not null termination), but some downstream code (e.g., `find(...)`)
works correctly. The *C-side* buffer writes use null termination, so a null byte in the output
SQL would truncate the C string prematurely.

**Why it happens:** DuckDB query text is guaranteed UTF-8 and does not contain null bytes in
practice. The `from_utf8_unchecked` comment says "guaranteed valid UTF-8 by the caller." But
this is not enforced.

**How to avoid:** The adversarial input test should include a null byte in the view name and
verify it produces a clean error (not a panic). In Rust, null bytes are valid UTF-8 (they are
a valid single-byte sequence). The issue is only at the FFI output boundary. Document this
behavior rather than adding a check.

**Warning signs:** A test with `"CREATE SEMANTIC VIEW x\0y ..."` panics or produces truncated
output SQL.

### Pitfall 5: Prefix whitespace proptest and arb_whitespace scope

**What goes wrong:** The existing `arb_whitespace()` strategy generates `[ \t]{0,10}` — only
spaces and tabs, for leading whitespace. New proptests for inter-keyword whitespace need a
broader strategy that includes newlines and carriage returns.

**Why it happens:** Leading whitespace is stripped by `query.trim()` before `detect_ddl_kind`
operates. Inter-keyword whitespace is what breaks the current literal matcher.

**How to avoid:** Define a new `arb_inter_keyword_ws()` strategy generating `[ \t\n\r]{1,4}`
for the whitespace *between* keywords in the DDL prefix. Use it specifically in the new
prefix-whitespace proptests.

## Code Examples

### Current detection logic (to be replaced)

```rust
// Source: src/parse.rs, detect_ddl_kind()
// This is the CURRENT code — it only matches single-space separators.
pub fn detect_ddl_kind(query: &str) -> Option<DdlKind> {
    let trimmed = query.trim().trim_end_matches(';').trim();
    let bytes = trimmed.as_bytes();

    // These literals only match with single ASCII spaces between keywords.
    // CREATE\tSEMANTIC\tVIEW fails here.
    if starts_with_ci(bytes, b"create or replace semantic view") {
        Some(DdlKind::CreateOrReplace)
    } else if starts_with_ci(bytes, b"create semantic view if not exists") {
        Some(DdlKind::CreateIfNotExists)
    } else if starts_with_ci(bytes, b"create semantic view") {
        Some(DdlKind::Create)
    }
    // ... etc.
}
```

### Current prefix_len coupling (to be replaced)

```rust
// Source: src/parse.rs, prefix_len()
// These are hardcoded byte counts for single-space prefixes.
// After token-based matching they must become dynamic (consumed byte count).
fn prefix_len(kind: DdlKind) -> usize {
    match kind {
        DdlKind::Create => "create semantic view".len(),          // 20
        DdlKind::CreateOrReplace => "create or replace semantic view".len(), // 31
        // ... etc.
    }
}
```

### Single-quote escaping in rewrite (KEEP AS-IS)

```rust
// Source: src/parse.rs, rewrite_ddl_keyword_body()
// This is CORRECT and must be preserved.
let safe_name = name.replace('\'', "''");   // SQL-safe view name
let safe_json = json.replace('\'', "''");   // SQL-safe JSON payload
Ok(Some(format!("SELECT * FROM {fn_name}('{safe_name}', '{safe_json}')")))
```

### body_parser.rs whitespace handling (already correct)

```rust
// Source: src/body_parser.rs, find_clause_bounds()
// This already handles arbitrary inter-keyword whitespace correctly.
// Skip whitespace between tokens using is_ascii_whitespace().
while i < bytes.len() && (bytes[i] as char).is_ascii_whitespace() {
    i += 1;
}
// Collect identifier word (alphabetic only)
while i < bytes.len() && (bytes[i] as char).is_ascii_alphabetic() {
    i += 1;
}
```

### Security surface audit

The full rewrite pipeline for an AS-body DDL:
1. `detect_ddl_kind(query)` — token scan, returns `DdlKind`
2. `validate_create_body(...)` — slices after prefix, dispatches to `rewrite_ddl_keyword_body`
3. `parse_keyword_body(body_text, offset)` — parses TABLES/RELATIONSHIPS/DIMENSIONS/METRICS
4. `serde_json::to_string(&def)` — serializes to JSON (no injection risk; JSON-encodes all strings)
5. `name.replace('\'', "''")` + `json.replace('\'', "''")` — SQL string literal escaping
6. `format!("SELECT * FROM {fn_name}('{safe_name}', '{safe_json}')")` — final SQL string

The `fn_name` comes from the `DdlKind` enum, not user input — no injection risk there. The
view name and JSON payload are both escaped. The only remaining question is whether
`serde_json::to_string` already handles all injection vectors in the JSON payload (it does —
serde_json escapes all special characters including single quotes within JSON string values).

## State of the Art

| Old Approach | Current Approach | When Changed | Impact |
|--------------|------------------|--------------|--------|
| Literal byte prefix matching | Token-based prefix matching | Phase 25.1 (this phase) | Fixes tab/newline prefix whitespace |
| Hardcoded `prefix_len()` constants | Dynamic consumed-byte count | Phase 25.1 (this phase) | Required to fix the slicing bug |
| No adversarial input tests | Explicit unit tests for null bytes, long inputs, Unicode homoglyphs | Phase 25.1 (this phase) | Documents safe behavior |

**Deprecated/outdated:**
- `starts_with_ci` as the primary detection mechanism: still useful for single-token matching but should not be called with multi-word literals that contain spaces.
- `prefix_len(kind) -> usize` returning constants: must return the dynamic consumed byte count after this phase.

## Open Questions

1. **Should `detect_ddl_kind` change its return type?**
   - What we know: The function is `pub` and tests call it directly. Changing to `Option<(DdlKind, usize)>` would require updating `parse_proptest.rs`.
   - What's unclear: Whether a private `detect_ddl_prefix` helper or a wrapper approach is cleaner.
   - Recommendation: Keep `detect_ddl_kind` signature as `Option<DdlKind>` for test compat. Add a private `detect_ddl_prefix(query: &str) -> Option<(DdlKind, usize)>` that the internal callers use.

2. **Does DuckDB normalize Unicode whitespace before the parse hook?**
   - What we know: The additional context says "DuckDB applies `StripUnicodeSpaces()` before the main parser attempt, which converts Unicode whitespace to regular spaces — but this may not apply to the fallback extension path."
   - What's unclear: Whether this happens before or after the extension's `parse_function` is called.
   - Recommendation: Scope Phase 25.1 to ASCII whitespace only (the 6 ASCII control characters that `is_ascii_whitespace()` covers). Add a test comment documenting the Unicode whitespace uncertainty.

3. **Should Unicode homoglyphs produce a near-miss error or silent PARSE_NOT_OURS?**
   - What we know: A Cyrillic `С` looks like Latin `C` but is a different byte sequence. Currently, `starts_with_ci` uses `eq_ignore_ascii_case`, so Cyrillic bytes do not match.
   - What's unclear: Whether DuckDB would pass a query with Unicode homoglyphs to the extension hook at all (it likely normalizes to ASCII first for SQL keywords).
   - Recommendation: Document the expected behavior (PARSE_NOT_OURS, no near-miss) in a test comment. Do not add special handling for homoglyphs — it is out of scope and not a realistic threat vector for a SQL DDL parser.

## Validation Architecture

### Test Framework
| Property | Value |
|----------|-------|
| Framework | proptest 1.9 + Rust built-in `#[test]` |
| Config file | none (proptest uses defaults) |
| Quick run command | `cargo test` |
| Full suite command | `just test-all` |

### Phase Requirements → Test Map

Phase 25.1 has no formal requirement IDs (it is a cross-cutting quality concern). The success
criteria from ROADMAP.md map to tests as follows:

| Success Criterion | Behavior | Test Type | Automated Command | File Exists? |
|-------------------|----------|-----------|-------------------|-------------|
| SC-1: token-based prefix matching | `CREATE\tSEMANTIC\tVIEW` detects correctly | unit + proptest | `cargo test` | ❌ Wave 0 |
| SC-1: all 7 DDL forms with tab separators | detect returns correct `DdlKind` | proptest | `cargo test` | ❌ Wave 0 |
| SC-1: all 7 DDL forms with newline separators | detect returns correct `DdlKind` | proptest | `cargo test` | ❌ Wave 0 |
| SC-1: all 7 DDL forms with mixed whitespace | detect returns correct `DdlKind` | proptest | `cargo test` | ❌ Wave 0 |
| SC-2: all 7 DDL forms tolerate arbitrary inter-keyword whitespace | validate_and_rewrite succeeds | proptest | `cargo test` | ❌ Wave 0 |
| SC-3: very long input (1 MB) returns PARSE_NOT_OURS, no panic | linear scan, no hang | unit | `cargo test` | ❌ Wave 0 |
| SC-3: null byte in view name produces clean error | no panic | unit | `cargo test` | ❌ Wave 0 |
| SC-3: embedded semicolons in view name produce error or safe output | no SQL injection | unit | `cargo test` | ❌ Wave 0 |
| SC-3: Unicode homoglyphs return PARSE_NOT_OURS | no near-miss confusion | unit | `cargo test` | ❌ Wave 0 |
| SC-3: control characters in view name produce error | no panic | unit | `cargo test` | ❌ Wave 0 |
| SC-4: body_parser handles arbitrary whitespace | already tested in Phase 25 (as_body_accepts_any_inter_token_whitespace) | proptest | `cargo test` | ✅ exists |
| SC-5: just test-all passes | full integration | integration | `just test-all` | ✅ exists |

### Sampling Rate
- **Per task commit:** `cargo test`
- **Per wave merge:** `just test-all`
- **Phase gate:** `just test-all` green before `/gsd:verify-work`

### Wave 0 Gaps

All tests for this phase are new. They should be added directly to `tests/parse_proptest.rs`
as new `proptest!` blocks and standalone `#[test]` functions:

- [ ] `tests/parse_proptest.rs` — TEST-07 block: proptest for all 7 DDL forms with `arb_inter_keyword_ws` strategy covering `[ \t\n\r]{1,4}` between keywords
- [ ] `tests/parse_proptest.rs` — TEST-08 block: unit tests for adversarial inputs (very long string, null byte, semicolons in name, Unicode homoglyphs, control characters)

No new test files are required. No framework installation required.

## Sources

### Primary (HIGH confidence)
- Direct code inspection of `src/parse.rs` — `detect_ddl_kind`, `prefix_len`, `starts_with_ci`, `write_to_buffer`, `sv_validate_ddl_rust`, `sv_rewrite_ddl_rust`
- Direct code inspection of `src/body_parser.rs` — `find_clause_bounds`, `parse_keyword_body`, `find_primary_key`, `find_keyword_ci`
- Direct code inspection of `tests/parse_proptest.rs` — existing test coverage, including `as_body_accepts_any_inter_token_whitespace` comment documenting the known prefix whitespace gap
- `TECH-DEBT.md` item 4: "DDL prefix whitespace not normalized (`detect_ddl_kind`)" — canonical description of the gap
- `Cargo.toml` — dependency inventory (no new dependencies needed)
- `Justfile` — `just test-all` definition (rust + sql + ducklake-ci + vtab-crash + caret)

### Secondary (MEDIUM confidence)
- `additional_context` in the phase objective — describes the `StringUtil::CharacterIsSpace` behavior, FFI boundary behavior, and security threat model in detail (treated as HIGH since it comes from the orchestrator with prior investigation)

### Tertiary (LOW confidence)
- None

## Metadata

**Confidence breakdown:**
- Standard stack: HIGH — all dependencies already present, no new ones needed
- Architecture: HIGH — direct code inspection of every affected function; fix approach is well-understood
- Pitfalls: HIGH — Pitfall 2 (prefix_len coupling) is confirmed by code inspection; others are standard patterns for this class of problem
- Security scope: MEDIUM — the "secure against adversarial inputs" claim is bounded by what can be unit-tested; the FFI boundary audit is based on code inspection, not runtime fuzzing

**Research date:** 2026-03-12
**Valid until:** Stable for the duration of this phase. DuckDB API changes could affect FFI boundary behavior but not the pure-Rust parsing logic.
