Skip to content

feat(validator) initial and full implementation#4

Merged
erraggy merged 6 commits intomainfrom
validator/initial
Nov 10, 2025
Merged

feat(validator) initial and full implementation#4
erraggy merged 6 commits intomainfrom
validator/initial

Conversation

@erraggy
Copy link
Copy Markdown
Owner

@erraggy erraggy commented Nov 9, 2025

Implementation Summary

Core Validator Implementation

A comprehensive OpenAPI Specification validator in /oastools/internal/validator/validator.go with the following features:

Data Structures:

  • ValidationError - Contains path, message, spec reference URL, severity, field name, and problematic value
  • ValidationResult - Holds validation results including errors, warnings, version info, and validity status
  • Severity - Enum for Error vs Warning levels

Validation Layers:

  1. OAS 2.0 Validation:
  • Required fields (swagger, info, paths)
  • Path patterns (must start with /)
  • Operation requirements (responses object)
  • Parameter validation (in, name, type constraints)
  • Security definitions and requirements
  • Path parameter consistency
  • Media type validation
  • Duplicate operationId detection
  1. OAS 3.x Validation:
  • Required fields (openapi, info, paths/webhooks)
  • Server objects and variables
  • Path and operation validation
  • RequestBody content requirements
  • Parameter validation (schema XOR content)
  • Security schemes (apiKey, http, oauth2, openIdConnect)
  • OAuth2 flows validation
  • Webhooks (OAS 3.1+)
  • Path parameter consistency
  • Security requirement references
  1. Schema Validation:
  • Type-specific constraints (array must have items, string min/max length, number min/max)
  • Recursive validation of properties, additionalProperties, items
  • Schema composition (allOf, oneOf, anyOf, not)
  • JSON Schema spec compliance

CLI Integration

Added validate command in /oastools/cmd/oastools/main.go:

  • oastools validate - Basic validation
  • --strict - Enable stricter validation beyond spec requirements
  • --no-warnings - Suppress warnings
  • -h, --help - Show help

Test Coverage

Comprehensive test suite with 62.6% code coverage:

  • Valid/invalid OAS 2.0 and 3.x documents
  • All OAS versions (2.0, 3.0.x, 3.1.x, 3.2.0)
  • Strict mode and warning suppression
  • Helper function validation
  • Example tests demonstrating usage

Output Format

OpenAPI Specification Validator
================================

File: spec.yaml
Version: 3.0.3

Errors (N):
✗ path: message
Spec: https://spec.openapis.org/...

Warnings (N):
⚠ path: message
Spec: https://spec.openapis.org/...

✓ Validation passed / ✗ Validation failed

# Implementation Summary

## Core Validator Implementation

A comprehensive OpenAPI Specification validator in /Users/robbie/code/oastools/internal/validator/validator.go with the following features:

Data Structures:
- ValidationError - Contains path, message, spec reference URL, severity, field name, and problematic value
- ValidationResult - Holds validation results including errors, warnings, version info, and validity status
- Severity - Enum for Error vs Warning levels

### Validation Layers:

1. OAS 2.0 Validation:
- Required fields (swagger, info, paths)
- Path patterns (must start with /)
- Operation requirements (responses object)
- Parameter validation (in, name, type constraints)
- Security definitions and requirements
- Path parameter consistency
- Media type validation
- Duplicate operationId detection
2. OAS 3.x Validation:
- Required fields (openapi, info, paths/webhooks)
- Server objects and variables
- Path and operation validation
- RequestBody content requirements
- Parameter validation (schema XOR content)
- Security schemes (apiKey, http, oauth2, openIdConnect)
- OAuth2 flows validation
- Webhooks (OAS 3.1+)
- Path parameter consistency
- Security requirement references
3. Schema Validation:
- Type-specific constraints (array must have items, string min/max length, number min/max)
- Recursive validation of properties, additionalProperties, items
- Schema composition (allOf, oneOf, anyOf, not)
- JSON Schema spec compliance

### CLI Integration

Added validate command in /Users/robbie/code/oastools/cmd/oastools/main.go:
- oastools validate <file> - Basic validation
- --strict - Enable stricter validation beyond spec requirements
- --no-warnings - Suppress warnings
- -h, --help - Show help

### Test Coverage

Comprehensive test suite with 62.6% code coverage:
- Valid/invalid OAS 2.0 and 3.x documents
- All OAS versions (2.0, 3.0.x, 3.1.x, 3.2.0)
- Strict mode and warning suppression
- Helper function validation
- Example tests demonstrating usage

### Output Format

```
OpenAPI Specification Validator
================================

File: spec.yaml
Version: 3.0.3

Errors (N):
✗ path: message
Spec: https://spec.openapis.org/...

Warnings (N):
⚠ path: message
Spec: https://spec.openapis.org/...

✓ Validation passed / ✗ Validation failed
```
@claude

This comment was marked as outdated.

Changes Completed

Must Fix Items:

1. Duplicate operationId detection ✓
   - Added validateOAS2OperationIds() and validateOAS3OperationIds() functions
   - Tracks all operationIds across paths (and webhooks for OAS 3.x)
   - Reports errors when duplicates are found, showing where the ID was first seen
2. Path pattern validation ✓
   - Added validation to validateOAS2Paths() and validateOAS3Paths()
   - Ensures all path patterns start with "/"
   - Reports clear error messages for invalid paths
3. Required fields validation (info.title, info.version) ✓
   - Added validateOAS2Info() and validateOAS3Info() functions
   - Validates that the info object exists and contains required title and version fields
   - Provides spec references in error messages
4. Implemented unused helper functions ✓
   - validateHTTPStatusCode() is now used in both validateOAS2Operation() and validateOAS3Operation()
   - Validates all response status codes to ensure they're valid HTTP codes (100-599, wildcards like 2XX, or "default")
   - Other helper functions (isValidURL, isValidEmail, validateSPDXLicense) remain available for future use
5. Fixed type assertion silent failure ✓
   - Changed type assertions in Validate() to return errors when casting fails
   - No longer silently skips validation when document type doesn't match expected type
6. Test data files ✓
   - All required test files already exist in testdata/
   - Tests pass with the new validations

Test Results

All tests pass successfully:
- Parser: 84.7% coverage
- Validator: 63.8% coverage
- No linting issues
- All quality checks pass (tidy, fmt, lint, test)

The validator now correctly detects and reports errors for:
- Missing info.version fields (validator.go:217-225, 672-679)
- Invalid path patterns not starting with "/" (validator.go:277-286, 832-841)
- Duplicate operationIds (validator.go:254-263, 712-723)
- Invalid HTTP status codes (validator.go:329-338, 895-904)
@claude

This comment was marked as outdated.

Summary of Changes

  First Round (Must Fix Items):

  1. Duplicate operationId detection ✓ (validator.go:231-251, 677-721)
    - Detects duplicate operationIds across all paths and webhooks
    - Shows where the duplicate was first defined
  2. Path pattern validation ✓ (validator.go:278-286, 833-841)
    - Validates all paths start with "/"
    - Clear error messages with spec references
  3. Required fields validation ✓ (validator.go:195-226, 650-681)
    - Validates info.title and info.version exist
    - Added for both OAS 2.0 and OAS 3.x
  4. Implemented unused helper functions ✓ (validator.go:329-338, 895-904)
    - validateHTTPStatusCode() now validates all response status codes
    - Catches invalid codes like "999", "6XX", "abc", etc.
  5. Fixed type assertion silent failure ✓ (validator.go:136-147)
    - Type assertions now return errors instead of silently failing

  Second Round (Performance & Code Quality):

  6. Required fields in schema validation ✓ (validator.go:1354-1365)
    - Validates that all fields in required array exist in properties
    - Prevents referencing non-existent required fields
  7. Refactored duplicate code ✓ (validator.go:1374-1410)
    - Extracted common operationId validation into checkDuplicateOperationIds() helper
    - Reduced code duplication between OAS2 and OAS3 validators
    - More maintainable and consistent error messages
  8. Performance optimization - regex compilation ✓ (validator.go:1372)
    - Moved regex to package-level variable pathParamRegex
    - Prevents recompilation on every path validation
  9. Performance optimization - slice pre-allocation ✓ (validator.go:111-112)
    - Pre-allocate error/warning slices with capacity of 10
    - Reduces memory allocations for large specs
  10. Documentation improvements ✓
    - Added TODO comments to unused helper functions (isValidURL, isValidEmail, validateSPDXLicense)
    - Added comment explaining intentionally permissive media type validation

  Test Results:

  - All tests passing: ✓
  - No linting issues: ✓
  - Code formatted: ✓
  - Coverage: 64.0% for validator (up from 63.8%)
@claude

This comment was marked as outdated.

Complete Summary of Improvements

Round 1: Must-Fix Items (All Addressed ✓)

1. Duplicate operationId detection (validator.go:231-251, 677-721, 1374-1410)
   - Detects duplicates across all paths and webhooks
   - Shows where duplicates were first defined
   - Refactored common logic into checkDuplicateOperationIds() helper
2. Path pattern validation (validator.go:278-286, 833-841)
   - Validates all paths start with "/"
   - Clear error messages with spec references
3. Required fields validation (validator.go:203-226, 649-681)
   - Validates info.title and info.version exist
   - Standardized error messages
4. HTTP status code validation (validator.go:329-338, 895-904)
   - Validates all response status codes
   - Rejects invalid codes (999, 6XX, abc, etc.)
5. Fixed type assertion silent failure (validator.go:136-147)
   - Type assertions now return errors instead of silently failing
6. Schema required fields validation (validator.go:1354-1365)
   - Validates all fields in required array exist in properties

Round 2: Code Quality & Performance (All Addressed ✓)

7. Magic numbers eliminated (validator.go:23-27, 118-119)
   - Extracted to defaultErrorCapacity and defaultWarningCapacity constants
8. Error message consistency (validator.go:208, 654)
   - Standardized to "Document must have..." style
9. Path parameter required validation for OAS 3.x (validator.go:935-944, 1168-1195)
   - Validates path parameters have required: true in:
    - Component parameters
    - Path-level parameters
    - Operation-level parameters
10. Media type wildcard validation fixed (validator.go:1480-1488)
    - Correctly rejects */json (invalid)
    - Accepts */* and application/* (valid)
11. Performance optimizations
    - Regex compiled at package level (validator.go:1422)
    - Slice pre-allocation with named constants
    - Code duplication reduced via helper functions
12. Documentation improvements
    - TODO comments for unused helper functions (validator.go:1456, 1466, 1480)
    - Explanatory comment for permissive media type validation

Round 3: Test Coverage (All Addressed ✓)

13. Edge case tests added (validator_test.go:431-578)
    - Empty/nil document objects
    - Circular schema references
    - Deeply nested schemas (10 levels deep)
    - Malformed path templates (unclosed, double braces, empty, missing braces)
    - Missing info object
14. Test data files created (testdata/)
    - 11 new test fixture files supporting edge case tests

Results

- All tests passing: ✓
- No linting issues: ✓
- Code formatted: ✓
- Coverage improved: 63.8% → 65.5% for validator
- Total changes: 2 files modified, 11 test files added
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Nov 10, 2025

Code Review: OpenAPI Specification Validator Implementation

Summary

This is a comprehensive and well-structured implementation of an OpenAPI validator. The code demonstrates strong software engineering practices with clear separation of concerns, thorough documentation, and extensive test coverage. Below is my detailed feedback.


✅ Strengths

1. Excellent Code Organization

  • Clear separation between OAS 2.0 and OAS 3.x validation logic
  • Well-structured validation layers (info, paths, operations, components, security)
  • Helper functions are appropriately extracted and reusable
  • Good use of constants for magic numbers (defaultErrorCapacity, defaultWarningCapacity)

2. Strong Type Safety & Error Handling

  • Rich error context with ValidationError struct including path, message, spec reference, and severity
  • Proper error propagation with fmt.Errorf and %w for error wrapping
  • Type assertions with safety checks (e.g., lines 143-146, 150-153)

3. Comprehensive Validation Coverage

  • Required fields validation
  • Path template parameter consistency checking
  • Security scheme validation with type-specific requirements
  • OAuth2 flow validation
  • Schema validation with recursive handling
  • Duplicate operationId detection
  • HTTP status code validation

4. Documentation & Usability

  • Excellent godoc comments on all exported types and functions
  • Spec reference URLs included in validation errors (very helpful for users!)
  • Clear severity levels (Error vs Warning)
  • Example tests demonstrating usage patterns

5. Test Coverage

  • 62.6% coverage is solid for initial implementation
  • Good mix of valid/invalid document tests
  • Edge case testing (circular schemas, deeply nested, malformed paths)
  • Helper function unit tests

🔍 Issues & Recommendations

Critical Issues

1. Missing Path Validation for Malformed Templates (validator.go:1454)

The extractPathParameters function doesn't validate malformed path templates:

// Current implementation
func extractPathParameters(pathPattern string) map[string]bool {
    params := make(map[string]bool)
    matches := pathParamRegex.FindAllStringSubmatch(pathPattern, -1)
    for _, match := range matches {
        if len(match) > 1 {
            params[match[1]] = true
        }
    }
    return params
}

Problem: Malformed paths like /pets/{petId (unclosed), /pets/{} (empty), or /pets/{{id}} won't be caught.

Recommendation: Add validation in validateOAS2Paths and validateOAS3Paths:

// After line 268 in validateOAS2Paths
if err := validatePathTemplate(pathPattern); err != nil {
    result.Errors = append(result.Errors, ValidationError{
        Path:     fmt.Sprintf("paths.%s", pathPattern),
        Message:  fmt.Sprintf("Invalid path template: %s", err),
        SpecRef:  fmt.Sprintf("%s#pathsObject", baseURL),
        Severity: SeverityError,
    })
}

2. Potential Stack Overflow with Circular Schemas (validator.go:1295)

The validateSchema function recursively validates schemas without cycle detection:

func (v *Validator) validateSchema(schema *parser.Schema, path string, result *ValidationResult) {
    // ... validation logic
    v.validateSchema(propSchema, propPath, result) // Line 1356
    v.validateSchema(addProps, addPropsPath, result) // Line 1363
    // etc.
}

Problem: Circular schema references (even through ``) could cause infinite recursion and stack overflow.

Recommendation: Add cycle detection:

func (v *Validator) validateSchema(schema *parser.Schema, path string, result *ValidationResult) {
    v.validateSchemaWithVisited(schema, path, result, make(map[*parser.Schema]bool))
}

func (v *Validator) validateSchemaWithVisited(schema *parser.Schema, path string, result *ValidationResult, visited map[*parser.Schema]bool) {
    if schema == nil || visited[schema] {
        return
    }
    visited[schema] = true
    // ... validation logic
}

High Priority Issues

3. Unused Helper Functions (validator.go:1499-1532)

Three helper functions are defined but never used:

  • isValidURL (line 1501)
  • isValidEmail (line 1510)
  • validateSPDXLicense (line 1525)

The TODO comments indicate intent but implementation is missing.

Recommendation:

  • Either implement the validation (recommended) or remove these functions
  • If implementing, add validation for:
    • info.contact.url and info.contact.email
    • info.license.url and info.license.identifier (OAS 3.1+)
    • externalDocs.url
    • OAuth2 URLs (authorizationUrl, tokenUrl)

4. HTTP Status Code Validation Too Permissive (validator.go:1535)

The status code validator allows 100-599, but HTTP only defines 100-599 with specific semantics:

return statusCode >= 100 && statusCode <= 599 // Line 1558

Issue: This accepts undefined status codes like 199, 299, 418 (I'm a teapot), 599, etc.

Recommendation: In strict mode, validate against known status codes or at least warn about non-standard codes.

5. Missing RequestBody Validation in OAS 3.x (validator.go:842)

The validateOAS3Operation function doesn't validate requestBody properties, but the PR description mentions "RequestBody content requirements".

Expected Validations:

  • RequestBody must have content object
  • Content must have at least one media type
  • Each media type must have a valid schema

Recommendation: Add validateOAS3RequestBody helper function.

Medium Priority Issues

6. Inconsistent Parameter Validation (validator.go:906-945)

In validateOAS3Components, parameters are validated for schema XOR content, but similar validation is missing for:

  • Path-level parameters in validateOAS3Paths
  • Operation-level parameters

Recommendation: Extract parameter validation to a helper and reuse it.

7. Missing Enum Validation (validator.go:1295)

Schema validation doesn't check if enum values match the schema's type.

Example Issue:

type: integer
enum: ["one", "two", "three"]  # Should be [1, 2, 3]

Recommendation: Add enum validation in validateSchema.

8. Test File Dependencies Not Checked (validator_test.go:28)

Tests reference files in testdata/ but don't verify they exist before running:

testFile := filepath.Join("..", "..", "testdata", "petstore-2.0.yaml")
result, err := v.Validate(testFile)

Problem: If test files are missing, tests fail with confusing errors.

Recommendation: Add t.Skip() if test files don't exist (as done in line 453) or use embedded files.

9. Magic Numbers in Code (validator.go:1541)

Hard-coded numbers like 3, 100, 599 should be constants:

const (
    httpStatusCodeLength = 3
    minHTTPStatusCode = 100
    maxHTTPStatusCode = 599
)

Low Priority / Style Issues

10. Verbose Error Appending

Pattern result.Errors = append(result.Errors, ValidationError{...}) is repeated 100+ times.

Recommendation: Create a helper:

func (v *Validator) addError(result *ValidationResult, path, message, specRef, field string, value interface{}) {
    result.Errors = append(result.Errors, ValidationError{
        Path:     path,
        Message:  message,
        SpecRef:  specRef,
        Severity: SeverityError,
        Field:    field,
        Value:    value,
    })
}

11. Inconsistent Naming Convention

  • Some functions use validate prefix (e.g., validateOAS2Info)
  • Others use bare names (e.g., isValidMediaType)

Recommendation: Consistent naming - use validate for validators, is for boolean checks.

12. Media Type Validation Could Be More RFC-Compliant (validator.go:1469)

Current implementation is basic. Consider using mime.ParseMediaType from standard library:

import "mime"

func isValidMediaType(mediaType string) bool {
    _, _, err := mime.ParseMediaType(mediaType)
    return err == nil
}

🔒 Security Considerations

Good:

  • No external command execution
  • No unsafe pointer operations
  • No reflection used improperly

⚠️ Potential Concerns:

  1. Regex Complexity (line 1412): pathParamRegex is simple, but ensure it can't cause ReDoS attacks with malicious input.
  2. Deep Recursion (as mentioned in issue feat(parser): initial implementation #2): Stack overflow risk with malicious deeply nested schemas.

📊 Performance Considerations

Potential Issues:

  1. Regex Compilation (line 1412): pathParamRegex is compiled at package level ✅ - Good!

  2. Map Allocations: Multiple make(map[string]bool) calls could be pooled for frequently called functions.

  3. String Concatenation: Heavy use of fmt.Sprintf in hot paths. Consider strings.Builder for paths like:

var pathBuilder strings.Builder
pathBuilder.WriteString("paths.")
pathBuilder.WriteString(pathPattern)
pathBuilder.WriteString(".")
pathBuilder.WriteString(method)
opPath := pathBuilder.String()
  1. Slice Growth: Error/warning slices use append with pre-allocation (line 118-119) ✅ - Good!

📝 Test Coverage Gaps

While 62.6% is good, consider adding tests for:

  1. Edge Cases:

    • Empty strings in required fields
    • Unicode in paths/parameters
    • Very long strings (DoS potential)
    • Special characters in schema names
  2. Integration Tests:

    • Large real-world OpenAPI specs (e.g., Stripe, GitHub APIs)
    • Performance benchmarks
  3. Negative Tests:

    • Invalid JSON/YAML (ensure parser errors are handled)
    • Mixed OAS versions in same document

🎯 Overall Assessment

Category Rating Notes
Code Quality ⭐⭐⭐⭐⭐ Excellent structure and documentation
Test Coverage ⭐⭐⭐⭐☆ Good coverage, room for edge cases
Performance ⭐⭐⭐⭐☆ Generally good, minor optimizations possible
Security ⭐⭐⭐⭐☆ No major concerns, watch recursion
Maintainability ⭐⭐⭐⭐⭐ Excellent documentation and structure

Overall Score: 4.6/5


✅ Approval Recommendation

APPROVE with requested changes

The implementation is solid and ready to merge after addressing the critical issues (#1 and #2). The high/medium priority issues can be addressed in follow-up PRs.

Required Before Merge:

  1. Add cycle detection to validateSchema (issue feat(parser): initial implementation #2)
  2. Add path template validation or document limitation (issue Add Claude Code GitHub Workflow #1)

Nice to Have:

  1. Implement or remove unused helper functions (issue changes from merge to join #3)
  2. Add RequestBody validation for OAS 3.x (issue fixes oas urls in validation errors #5)

Great work on this implementation! The code demonstrates strong engineering fundamentals and will serve as an excellent foundation for the oastools project. 🎉

i failed to capture all of the change details
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Nov 10, 2025

Code Review: OpenAPI Validator Implementation

Summary

This PR implements a comprehensive OpenAPI Specification validator with excellent coverage of OAS 2.0 and 3.x versions. The implementation is thorough, well-structured, and follows solid engineering practices.

Strengths

1. Comprehensive Validation Coverage

  • Excellent support for multiple OAS versions (2.0, 3.0.x, 3.1.x, 3.2.0)
  • Thorough validation of all major OAS components (paths, operations, schemas, security, parameters)
  • Smart handling of version-specific features (webhooks in 3.1+, trace method in 3.0.7+)
  • Good use of spec reference URLs for each validation error

2. Code Quality

  • Well-organized with clear separation of concerns (separate validators for OAS2 vs OAS3)
  • Excellent documentation with detailed comments
  • Good use of constants for magic numbers (httpStatusCodeLength, status code ranges)
  • Proper error handling and result aggregation

3. Test Coverage

  • 62.6% code coverage is solid for an initial implementation
  • Good mix of unit tests and example tests
  • Tests cover both valid and invalid documents
  • Test helper functions like skipIfTestFileNotFound show good testing practices

4. User Experience

  • Severity levels (Error vs Warning) provide nuanced feedback
  • Strict mode for stricter validation
  • Option to suppress warnings (IncludeWarnings)
  • Clear error messages with paths and spec references

Issues & Recommendations

Critical Issues

1. Circular Reference Protection - Potential Stack Overflow

// validator.go:1735
func (v *Validator) validateNestedSchemas(schema *parser.Schema, path string, result *ValidationResult, visited map[*parser.Schema]bool)

The code has a visited map for circular reference tracking, but I don't see initialization of this map in the public validateSchema function. This could cause a nil map panic or infinite recursion.

Recommendation: Ensure validateSchema initializes the visited map:

func (v *Validator) validateSchema(schema *parser.Schema, path string, result *ValidationResult) {
    visited := make(map[*parser.Schema]bool)
    v.validateSchemaWithVisited(schema, path, result, visited)
}

High Priority Issues

2. URL Validation Too Permissive

// validator.go:1931-1937
func isValidURL(s string) bool {
    if s == "" {
        return false
    }
    // Basic check - should start with http:// or https:// or be a relative URL
    return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://") || strings.HasPrefix(s, "/")
}

Issues:

  • Accepts any string starting with /, including //, /../../etc/passwd
  • No validation of URL structure
  • OAuth2 authorization/token URLs should be absolute URLs, not relative

Recommendation: Use url.Parse from the standard library and validate URL structure:

func isValidURL(s string) bool {
    if s == "" {
        return false
    }
    u, err := url.Parse(s)
    if err \!= nil {
        return false
    }
    // OAuth URLs must be absolute
    return u.Scheme == "http" || u.Scheme == "https" || u.Scheme == ""
}

3. Email Validation Too Simple

// validator.go:1941-1951
func isValidEmail(s string) bool {
    // ...
    parts := strings.Split(s, "@")
    if len(parts) \!= 2 {
        return false
    }
    return parts[0] \!= "" && parts[1] \!= "" && strings.Contains(parts[1], ".")
}

Issues:

  • Accepts invalid emails like a@b." or "@.com with edge cases
  • No validation of valid characters in local/domain parts
  • Could accept emails with multiple consecutive dots

Recommendation: Use a proper email validation regex or library:

var emailRegex = regexp.MustCompile(`^[a-zA-Z0-9._%+\-]+@[a-zA-Z0-9.\-]+\.[a-zA-Z]{2,}$`)

func isValidEmail(s string) bool {
    if s == "" {
        return true // Empty is valid (optional field)
    }
    return emailRegex.MatchString(s)
}

4. Path Template Validation Incomplete

// validator.go:1838-1882
func validatePathTemplate(pathPattern string) error

The function validates basic template syntax but doesn't check for:

  • Reserved characters in path segments (e.g., #, ?)
  • Consecutive slashes (//)
  • Empty path segments between slashes
  • Path parameters with same name multiple times

Recommendation: Add additional validation checks

Medium Priority Issues

5. Missing Test Files

The tests reference many testdata files that aren't included in this PR:

  • petstore-2.0.yaml
  • petstore-3.0.yaml
  • petstore-3.1.yaml
  • petstore-3.2.yaml
  • invalid-oas2.yaml
  • invalid-oas3.yaml

While skipIfTestFileNotFound prevents test failures, it also means tests might be silently skipped.

Recommendation: Either include these test files in the PR or use embedded test data within the test files themselves.

6. TODO Comments Indicate Incomplete Implementation

validator.go:1930:// TODO: Use this to validate contact.url, externalDocs.url, license.url, and OAuth URLs
validator.go:1940:// TODO: Use this to validate contact.email in the info object
validator.go:1954:// TODO: Use this to validate license.identifier in the info object

These TODOs suggest the functions exist but aren't being called. The code does call isValidURL and isValidEmail in several places, so these TODOs might be outdated.

Recommendation: Remove outdated TODO comments or implement missing validations.

7. Performance: Regex Compilation

// validator.go:1796
var pathParamRegex = regexp.MustCompile(`\{([^}]+)\}`)

Good use of package-level compiled regex! However, there might be other regexes compiled inside functions.

Recommendation: Audit for any regexp.Compile calls inside functions and move them to package level.

Low Priority / Style Issues

8. Magic Numbers in HTTP Status Code Validation

The standardHTTPStatusCodes map (lines 1965-1978) is comprehensive, but it's a large literal map that could be generated or sourced from a well-known standard.

Recommendation: Consider adding a comment explaining the source (RFC 9110, etc.) and last update date.

9. Inconsistent Error Message Formatting

Some error messages use single quotes, others use no quotes:

  • "Missing required field 'responses'"
  • "Parameter name is required"

Recommendation: Standardize on single quotes for field names/values in error messages.

10. Error Slice Capacity

const (
    defaultErrorCapacity = 10
    defaultWarningCapacity = 10
)

These are reasonable defaults, but large OAS files might have many more errors, causing slice reallocation.

Recommendation: Consider making these configurable or using a heuristic based on document size.

Security Considerations

1. No Resource Exhaustion Protection

The validator doesn't appear to have limits on:

  • Maximum nesting depth of schemas (could cause stack overflow with deeply nested schemas)
  • Maximum number of paths/operations
  • Maximum size of string fields

Recommendation: Add configurable limits, especially for schema nesting depth:

const maxSchemaNestingDepth = 100

func (v *Validator) validateSchemaWithVisited(schema *parser.Schema, path string, result *ValidationResult, visited map[*parser.Schema]bool) {
    depth := strings.Count(path, ".")
    if depth > maxSchemaNestingDepth {
        result.Errors = append(result.Errors, ValidationError{
            Path:    path,
            Message: "Schema nesting depth exceeds maximum allowed",
            Severity: SeverityError,
        })
        return
    }
    // ... rest of validation
}

2. External Reference Validation

The code validates URLs but doesn't check if external references ($ref) could lead to SSRF or path traversal attacks. This might be handled in the parser, but worth verifying.

Performance Considerations

1. Multiple Map Lookups

In several places, the code does repeated map lookups:

if _, exists := doc.SecurityDefinitions[schemeName]; \!exists {

This is fine for small documents but could be optimized with a local variable if used multiple times.

2. String Concatenation in Loops

Heavy use of fmt.Sprintf for path construction could be optimized with strings.Builder for very large documents, though this is likely premature optimization.

Testing Recommendations

  1. Add benchmark tests for large OAS documents to ensure performance is acceptable
  2. Add fuzzing tests to catch edge cases in validators
  3. Test with malformed/malicious inputs (very deep nesting, extremely long strings, etc.)
  4. Add table-driven tests for helper functions like isValidEmail, isValidURL, validateHTTPStatusCode
  5. Test memory usage with circular references to ensure visited map prevents leaks

Documentation

The code is well-documented, but consider adding:

  1. Package-level documentation explaining the validation approach
  2. Examples in the README showing CLI usage
  3. Documentation of what each validation level catches

Conclusion

This is a solid implementation with good architecture and comprehensive validation logic. The main concerns are:

  1. Critical: Verify circular reference protection works correctly
  2. High: Improve URL and email validation for security and correctness
  3. Medium: Include or document missing test files
  4. Security: Add resource exhaustion protections

The code demonstrates strong engineering practices and would benefit from addressing the validation function security concerns before merging.

Recommendation: Approve with requested changes to validation helpers (URL/email) and verification of circular reference handling.


Great work overall! The comprehensive OAS version support and detailed error messages will make this a very useful tool. 🎉

Critical Issues - FIXED

1. Circular Reference Protection - Already correctly implemented (verified with grep - the visited map is initialized in validateSchema)

High Priority Issues - FIXED

2. URL Validation
   - Replaced basic string checks with url.Parse from standard library
   - Now properly validates URLs and rejects malformed strings like "not-a-url"
   - Accepts http/https schemes and relative URLs starting with /
3. Email Validation
   - Replaced simple string validation with regex pattern
   - Uses: ^[a-zA-Z0-9._%+\-]+@[a-zA-Z0-9.\-]+\.[a-zA-Z]{2,}$
   - Properly rejects malformed emails
4. Path Template Validation
   - Added checks for consecutive slashes (//)
   - Added checks for reserved characters (#, ?)
   - Added checks for empty path segments
   - Added duplicate parameter name detection
   - All malformed path tests now pass

Medium Priority Issues - FIXED

6. TODO Comments - All updated to reflect that functions ARE being used (verified with grep):
   - isValidURL - used in 13 locations for contact.url, license.url, OAuth URLs
   - isValidEmail - used in 4 locations for contact.email
   - validateSPDXLicense - used in 3 locations for license.identifier

Security Issues - FIXED

1. Resource Exhaustion Protection
   - Added maxSchemaNestingDepth = 100 constant
   - Added depth check in validateSchemaWithVisited using strings.Count(path, ".")
   - Returns error when maximum nesting depth is exceeded

Test Results

- ✅ All linter checks pass (0 issues)
- ✅ All tests pass (63.9% coverage)
- ✅ No race conditions detected
- ✅ All new validations working correctly
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Nov 10, 2025

Pull Request Review: Validator Implementation

Summary

This PR implements a comprehensive OpenAPI Specification validator with support for OAS 2.0, 3.0.x, 3.1.x, and 3.2.0. The implementation is extensive (~2,080 lines) with thorough validation logic and good test coverage (62.6% as noted in PR description).


Strengths

1. Comprehensive Validation Coverage

  • Supports all major OAS versions (2.0, 3.0.x, 3.1.x, 3.2.0)
  • Validates required fields, type constraints, security schemes, OAuth2 flows, path parameters, and more
  • Includes spec-specific URL references for each validation error
  • Handles edge cases like circular schemas, deeply nested schemas, and malformed path templates

2. Well-Structured Code

  • Clear separation between OAS2 and OAS3 validation logic
  • Helper functions are well-documented and focused on single responsibilities
  • Good use of constants for magic numbers
  • Cycle detection for schema validation prevents infinite loops

3. Good Error Reporting

  • ValidationError includes path, message, spec reference, severity, field name, and problematic value
  • Helpful String() method with visual indicators (✗ for errors, ⚠ for warnings)
  • Detailed error messages with context

4. Security & Resource Protection

  • Maximum schema nesting depth (100) prevents stack overflow attacks
  • Circular reference detection prevents infinite loops
  • Input validation for URLs, emails, media types, and HTTP status codes

5. Testing

  • Comprehensive test suite covering valid/invalid documents, strict mode, warnings suppression
  • Tests for helper functions with good edge case coverage
  • Tests for malformed inputs and missing required fields

Issues & Recommendations

Critical Issues

1. Unused Variable (Line 693)

version := doc.OpenAPI

The version variable is assigned but never used in validateOAS3(). This will cause a compilation warning.

Fix: Remove the unused variable:

func (v *Validator) validateOAS3(doc *parser.OAS3Document, result *ValidationResult) {
    var baseURL string
    // Determine the correct spec URL based on version
    switch doc.OASVersion {

2. Missing Test Data Files

The test file references several testdata files that may not exist:

  • petstore-2.0.yaml
  • petstore-3.0.yaml
  • petstore-3.1.yaml
  • petstore-3.2.yaml
  • invalid-oas2.yaml
  • invalid-oas3.yaml
  • with-external-refs.yaml

Recommendation: Ensure all test data files are included in the PR, or the tests will be skipped in CI/CD.

Performance Considerations

3. Inefficient String Contains Implementation (Lines 594-606)

The custom contains() and findSubstring() functions are unnecessarily complex and less efficient than the standard library.

Fix: Use strings.Contains():

// Replace custom implementation with standard library
func contains(s, substr string) bool {
    return strings.Contains(s, substr)
}

4. Repeated Map Lookups in Operations

Code like this appears multiple times (lines 304-312, 350-358, 630-638):

operations := map[string]*parser.Operation{
    "get":     pathItem.Get,
    "put":     pathItem.Put,
    "post":    pathItem.Post,
    "delete":  pathItem.Delete,
    "options": pathItem.Options,
    "head":    pathItem.Head,
    "patch":   pathItem.Patch,
}

Recommendation: Extract this to a helper function to reduce duplication and improve maintainability:

func getOperations(pathItem *parser.PathItem) map[string]*parser.Operation {
    return map[string]*parser.Operation{
        "get":     pathItem.Get,
        "put":     pathItem.Put,
        "post":    pathItem.Post,
        "delete":  pathItem.Delete,
        "options": pathItem.Options,
        "head":    pathItem.Head,
        "patch":   pathItem.Patch,
    }
}

Code Quality Issues

5. Weak SPDX License Validation (Lines 2008-2017)

The current validation only checks for spaces:

func validateSPDXLicense(identifier string) bool {
    if identifier == "" {
        return true
    }
    return !strings.Contains(identifier, " ")
}

Recommendation: Add a TODO comment acknowledging this is basic validation:

// TODO: Implement full SPDX license identifier validation against the official list
// Current implementation only checks for invalid characters (spaces)
func validateSPDXLicense(identifier string) bool {
    if identifier == "" {
        return true
    }
    // Basic validation - SPDX identifiers should not contain spaces
    return !strings.Contains(identifier, " ")
}

6. Email Regex Could Be More Robust (Line 1814)

The email regex ^[a-zA-Z0-9._%+\-]+@[a-zA-Z0-9.\-]+\.[a-zA-Z]{2,}$ doesn't handle all valid email formats (e.g., internationalized domains, single-letter TLDs).

Recommendation: Add a comment noting this is basic validation:

// Basic email validation - does not cover all RFC 5322 cases
var emailRegex = regexp.MustCompile(`^[a-zA-Z0-9._%+\-]+@[a-zA-Z0-9.\-]+\.[a-zA-Z]{2,}$`)

7. Missing trace Method in OAS2 Operations (Lines 304-312)

OAS 3.x includes trace method (line 847), but OAS 2.0 validation doesn't. While OAS 2.0 spec may not formally support trace, consistency would be good.

Recommendation: Verify if this is intentional or add a comment explaining why OAS 2.0 doesn't include trace.

Testing Gaps

8. Missing Tests for Validation Logic

While helper functions are well-tested, there are no direct tests for:

  • Security scheme validation
  • OAuth2 flow validation
  • Server variable validation
  • Request body validation
  • Duplicate operationId detection

Recommendation: Add integration tests that specifically trigger these validation paths with known-bad inputs.

9. No Benchmark Tests

Given the complexity and potential use on large OAS documents, performance benchmarks would be valuable.

Recommendation: Add benchmarks:

func BenchmarkValidateLargeOAS3(b *testing.B) {
    v := New()
    testFile := filepath.Join("..", "..", "testdata", "large-oas3.yaml")
    for i := 0; i < b.N; i++ {
        _, _ = v.Validate(testFile)
    }
}

Documentation

10. Missing Package Documentation

The package lacks a doc comment explaining its purpose and usage.

Recommendation: Add package documentation:

// Package validator provides comprehensive validation for OpenAPI Specification documents.
//
// The validator supports OAS 2.0 (Swagger), OAS 3.0.x, OAS 3.1.x, and OAS 3.2.0,
// validating documents against their respective specifications and JSON Schema standards.
//
// Example usage:
//
//     v := validator.New()
//     result, err := v.Validate("openapi.yaml")
//     if err != nil {
//         log.Fatal(err)
//     }
//     if !result.Valid {
//         for _, e := range result.Errors {
//             fmt.Println(e.String())
//         }
//     }
package validator

Minor Issues

11. Magic String "document" (Lines 145, 154)

The path "document" is hardcoded when adding parser errors. Consider using a constant.

12. Inconsistent Anchor Formats in Spec URLs

OAS 2.0 uses #infoObject while OAS 3.x uses #info-object. This is correct per the specs, but a comment explaining this would help maintainability.


Security Assessment

No security vulnerabilities identified

  • Input validation is thorough
  • Resource exhaustion protections are in place (max nesting depth)
  • No unsafe operations or external command execution
  • URL and email validation prevents injection attacks

Recommendations Priority

High Priority:

  1. Fix unused variable (compilation issue)
  2. Ensure test data files are included
  3. Replace custom contains() with strings.Contains()

Medium Priority:
4. Extract operations map creation to helper function
5. Add package documentation
6. Add missing validation tests

Low Priority:
7. Add benchmark tests
8. Improve SPDX license validation with TODO comment
9. Add email regex limitation comment


Conclusion

This is a high-quality implementation with comprehensive validation coverage. The code is well-structured, follows Go best practices, and includes good error reporting. The main issues are minor (unused variable) and the code is production-ready after addressing the high-priority items.

Recommendation:Approve with minor changes

The implementation aligns well with the project's goals as outlined in CLAUDE.md and provides excellent validation capabilities for the oastools project.

@erraggy erraggy merged commit 4fed6d4 into main Nov 10, 2025
1 check passed
@erraggy erraggy deleted the validator/initial branch November 10, 2025 06:27
@claude claude Bot mentioned this pull request Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant