Skip to content

Harden parser validation and evidence guardrails#96

Closed
DevOpsMadDog wants to merge 10 commits into
mainfrom
codex/implement-parser-hardening-and-safety-improvements
Closed

Harden parser validation and evidence guardrails#96
DevOpsMadDog wants to merge 10 commits into
mainfrom
codex/implement-parser-hardening-and-safety-improvements

Conversation

@DevOpsMadDog
Copy link
Copy Markdown
Owner

@DevOpsMadDog DevOpsMadDog commented Oct 13, 2025

Summary

  • detect open security group rules defined at both aws_security_group and aws_security_group_rule resources and expand tests
  • enforce strict SARIF parsing with size limits and Pydantic validation, including YAML root checks and new regression coverage
  • centralise evidence path normalisation with a safe resolver and apply it across the run registry and bundle packager while extending API auth tests

Testing

  • pytest -q
  • python -m fixops.cli demo --mode demo --output out/pipeline-demo.json --pretty
  • python -m fixops.cli demo --mode enterprise --output out/pipeline-enterprise.json --pretty

https://chatgpt.com/codex/tasks/task_e_68ed01a3412c8329bf48c383b61f8b4d


Summary by cubic

Strengthens input parsing and evidence storage with strict SARIF validation and safe path resolution. Adds open security group detection across both Terraform resource types and improves API auth responses.

  • New Features

    • Enforce strict SARIF parsing with Pydantic models, allowed levels only, and a 32MB payload limit.
    • Detect open security groups in both aws_security_group and aws_security_group_rule, including IPv4/IPv6 CIDRs.
    • Centralize evidence path normalization via resolve_within_root to prevent writes outside the output root (used in bundle packager and run registry).
  • Bug Fixes

    • API auth now returns 401 for missing token and 403 for invalid token; header is case-insensitive.
    • Policy loader requires a YAML mapping and raises on invalid input.
    • Transparency index append uses the safe resolver to avoid path escapes.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

5 issues found across 13 files

Prompt for AI agents (all 5 issues)

Understand the root cause of the following 5 issues and fix them.


<file name="apps/api/app.py">

<violation number="1" location="apps/api/app.py:127">
Returning 403 here treats an invalid API key as an authorization failure, but this case is still an authentication failure and should continue to return 401 to match standard HTTP semantics and existing clients.</violation>
</file>

<file name="fixops/utils/paths.py">

<violation number="1" location="fixops/utils/paths.py:12">
Using Path(...).name collapses every input to a single filename, so callers specifying nested evidence paths (e.g., &quot;reports/logs/event.json&quot;) will now write to the root as &quot;event.json&quot; and lose directory structure. This breaks valid usages without improving safety; the existing resolve/parent check already blocks escapes.</violation>
</file>

<file name="evidence/packager.py">

<violation number="1" location="evidence/packager.py:264">
Using resolve_within_root here drops tag subdirectories, so tags like `release/v1.0` and `hotfix/v1.0` both resolve to the same manifest path and overwrite each other. Please retain unique per-tag paths.</violation>

<violation number="2" location="evidence/packager.py:268">
resolve_within_root flattens tag names, so different namespaced tags now produce the same bundle filename and overwrite existing bundles. Please keep the per-tag directory structure when constructing the bundle path.</violation>
</file>

<file name="core/stage_runner.py">

<violation number="1" location="core/stage_runner.py:732">
Limit aws_security_group_rule detection to ingress rules; otherwise egress-only rules (the AWS default) are now flagged as open security groups and inflate the deploy risk score.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Ask questions if you need clarification on any suggestion

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

Comment thread apps/api/app.py Outdated
Comment thread fixops/utils/paths.py Outdated
Comment thread evidence/packager.py Outdated
yaml.safe_dump(manifest, handle, sort_keys=False)

bundle_path = bundle_dir / f"{tag}.zip"
bundle_path = resolve_within_root(bundle_dir, f"{tag}.zip")
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Oct 13, 2025

Choose a reason for hiding this comment

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

resolve_within_root flattens tag names, so different namespaced tags now produce the same bundle filename and overwrite existing bundles. Please keep the per-tag directory structure when constructing the bundle path.

Prompt for AI agents
Address the following comment on evidence/packager.py at line 268:

<comment>resolve_within_root flattens tag names, so different namespaced tags now produce the same bundle filename and overwrite existing bundles. Please keep the per-tag directory structure when constructing the bundle path.</comment>

<file context>
@@ -259,11 +261,11 @@ def create_bundle(inputs: BundleInputs) -&gt; dict[str, Any]:
         yaml.safe_dump(manifest, handle, sort_keys=False)
 
-    bundle_path = bundle_dir / f&quot;{tag}.zip&quot;
+    bundle_path = resolve_within_root(bundle_dir, f&quot;{tag}.zip&quot;)
     with ZipFile(bundle_path, &quot;w&quot;) as archive:
         for source, arcname in bundle_files:
</file context>

✅ Addressed in 7617726

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Oct 17, 2025

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Oct 17, 2025

Choose a reason for hiding this comment

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

Here's a PR: #103 Preserve per-tag directories to prevent bundle overwrites (merges into #96)

Review and merge it to apply the changes.

Comment thread evidence/packager.py Outdated
}

manifest_path = manifest_dir / f"{tag}.yaml"
manifest_path = resolve_within_root(manifest_dir, f"{tag}.yaml")
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Oct 13, 2025

Choose a reason for hiding this comment

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

Using resolve_within_root here drops tag subdirectories, so tags like release/v1.0 and hotfix/v1.0 both resolve to the same manifest path and overwrite each other. Please retain unique per-tag paths.

Prompt for AI agents
Address the following comment on evidence/packager.py at line 264:

<comment>Using resolve_within_root here drops tag subdirectories, so tags like `release/v1.0` and `hotfix/v1.0` both resolve to the same manifest path and overwrite each other. Please retain unique per-tag paths.</comment>

<file context>
@@ -259,11 +261,11 @@ def create_bundle(inputs: BundleInputs) -&gt; dict[str, Any]:
     }
 
-    manifest_path = manifest_dir / f&quot;{tag}.yaml&quot;
+    manifest_path = resolve_within_root(manifest_dir, f&quot;{tag}.yaml&quot;)
     with manifest_path.open(&quot;w&quot;, encoding=&quot;utf-8&quot;) as handle:
         yaml.safe_dump(manifest, handle, sort_keys=False)
</file context>

✅ Addressed in 88b9980

Comment thread core/stage_runner.py
cidr_values = []
if any(value == "0.0.0.0/0" for value in cidr_values):
cidr_values = _extract_cidrs(rule)
if _contains_open_rule(cidr_values):
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Oct 13, 2025

Choose a reason for hiding this comment

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

Limit aws_security_group_rule detection to ingress rules; otherwise egress-only rules (the AWS default) are now flagged as open security groups and inflate the deploy risk score.

Prompt for AI agents
Address the following comment on core/stage_runner.py at line 732:

<comment>Limit aws_security_group_rule detection to ingress rules; otherwise egress-only rules (the AWS default) are now flagged as open security groups and inflate the deploy risk score.</comment>

<file context>
@@ -695,23 +721,24 @@ def _analyse_posture(self, payload: Mapping[str, Any]) -&gt; dict[str, Any]:
-                        cidr_values = []
-                    if any(value == &quot;0.0.0.0/0&quot; for value in cidr_values):
+                    cidr_values = _extract_cidrs(rule)
+                    if _contains_open_rule(cidr_values):
                         open_security_groups.add(name)
 
</file context>
Fix with Cubic

DevOpsMadDog and others added 3 commits October 15, 2025 23:25
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
…-py-L12-1760531107

Preserve nested evidence paths instead of collapsing names (merges into #96)
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 5 files

…-py-L268-1760701090

Preserve per-tag directories to prevent bundle overwrites (merges into #96)
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 2 files

@devin-ai-integration
Copy link
Copy Markdown
Contributor

Closing as part of PR consolidation. Useful changes have been cherry-picked into PR #240.

DevOpsMadDog added a commit that referenced this pull request May 6, 2026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant