Skip to content

fix(security): exclude benchmark fixture corpus from CodeQL analysis - #281

Closed
parthrohit22 wants to merge 2 commits into
Second-Origin:devfrom
parthrohit22:fix/codeql-exclude-benchmark-fixtures
Closed

fix(security): exclude benchmark fixture corpus from CodeQL analysis#281
parthrohit22 wants to merge 2 commits into
Second-Origin:devfrom
parthrohit22:fix/codeql-exclude-benchmark-fixtures

Conversation

@parthrohit22

Copy link
Copy Markdown
Collaborator

Summary

With the default branch now scanned on push (#280), the Security tab is current — last scan now at 319870a7 instead of 5 days stale — and that surfaces the real remaining warning:

⚠️ Could not process some files due to syntax errors

One per language (language:python and language:javascript-typescript), which accounts for the "1 warning" plus "CodeQL also found 1 other warning like this" on each configuration page.

Both point at the Repository Intelligence golden benchmark corpus:

adversarial/adv-py-malformed/src/broken.py    def broken(:
adversarial/adv-ts-malformed/src/broken.ts    export function broken( {

These are supposed to be unparseable. They are adversarial inputs to the RI engine, not code PARTHA executes. Each is pinned by a manifest asserting the malformed-source diagnostic:

"constructsCovered": ["py.syntax_error", "src.malformed_source"],
"diagnostics": [
  { "code": "RI-SRC-MALFORMED", "severity": "error",
    "message": "src/broken.py could not be parsed.", ... }
]

"A file that fails to parse. It must emit RI-SRC-MALFORMED while the mandatory repository root remains evidenced by the stored text source (RFC-0001 §4.3, §6.2, §8)."

So the malformed syntax is the fixture contract. GitHub's own suggestion — "check the syntax of the file and correct any invalid syntax" — is the wrong move here: it would silently delete py.syntax_error and ts.syntax_error coverage from the benchmark while making the warning disappear. The other half of GitHub's advice, "or exclude the file from analysis", is the correct one.

Linked issue

None — found while verifying #280 against the Security tab.

What changed

Infra

  • New .github/codeql/codeql-config.ymlpaths-ignore for apps/backend/tests/benchmark/fixtures, with the rationale recorded inline so the next reader doesn't "fix" the fixtures.
  • .github/workflows/codeql.yml — wires the config in via config-file on the init step.

No application code, contracts, schemas, or fixtures changed. The malformed files are deliberately left exactly as they are.

Acceptance criteria completed

Testing performed

Config and wiring asserted, including a blast-radius check that the exclusion cannot leak into application code:

paths-ignore: ['apps/backend/tests/benchmark/fixtures']
config-file : ./.github/codeql/codeql-config.yml
triggers    : ['push', 'pull_request', 'schedule'] (push preserved)

files excluded: 59 total, 23 py/ts   (all under tests/benchmark/fixtures)
application files still analysed: 282 (0 excluded)
CONFIG VALID

RI golden benchmark, confirming the fixtures still behave as their manifests require:

$ .venv/bin/python -m pytest tests/benchmark -q
82 passed

Honest limit: whether CodeQL stops emitting the warning can only be proven by GitHub running the analysis with this config. The definitive evidence is the two configuration pages dropping from "1 warning" to no warnings after this merges. I cannot produce that locally and am not claiming it.

Screenshots

Not applicable — no UI change. The relevant surface is the Security tab, which updates after merge.

Security and data considerations

Narrowly scoped and verified: the exclusion covers only apps/backend/tests/benchmark/fixtures, a directory containing exclusively benchmark fixture data. All 282 application source files under apps/backend/app and apps/frontend/src remain in scope, asserted in the test above.

This is a genuine improvement to signal quality rather than suppression. A permanent unfixable warning on the Security tab trains reviewers to ignore that surface, which is how a real finding gets missed. No permissions, secrets, egress, auth, or owner-scoping changes.

Dependencies and blocked work

None. Builds on #280 (merged).

Scope changes or remaining work

None.

Contributor checklist

  • Follows the branch naming and commit conventions
  • Targets dev
  • Verified locally with the commands above
  • No secrets, .env, dist/, or generated artifacts committed
  • No applied migration edited
  • Docs/behaviour claims match real repository state

With the default branch now scanned on push (Second-Origin#280), the Security tab shows
the real remaining warning: 'Could not process some files due to syntax
errors', once for python and once for javascript-typescript.

Both point at the Repository Intelligence golden benchmark corpus:

  adversarial/adv-py-malformed/src/broken.py   def broken(:
  adversarial/adv-ts-malformed/src/broken.ts   export function broken( {

These are adversarial inputs to the RI engine, not code PARTHA executes.
Each manifest asserts the RI-SRC-MALFORMED diagnostic per RFC-0001 4.3,
6.2 and 8, so the malformed syntax is the fixture contract. Correcting the
syntax would silently delete py.syntax_error and ts.syntax_error coverage
from the benchmark.

Add a CodeQL config excluding the fixture corpus and wire it into the init
step. Application code is unaffected: 282 source files remain in scope and
23 fixture sources leave it.
The bare directory path did not take effect: the analysis still scanned
248 of 248 Python files, the same count as before the exclusion, and
re-reported the parse errors in broken.py and broken.ts.

Add an explicit trailing glob alongside the bare path.
@parthrohit22
parthrohit22 marked this pull request as draft August 8, 2026 19:01
@parthrohit22

Copy link
Copy Markdown
Collaborator Author

Converting to draft — the fix in this PR does not work. Reporting it rather than leaving it looking merge-ready.

What I claimed vs what happened

The PR body says excluding the fixture corpus removes the warning. CI on this branch disproves it. Two attempts, identical result:

Python files scanned Warning
dev before any fix 248 / 248 present
bare dir …/fixtures 248 / 248 still present
+ explicit glob …/fixtures/** 248 / 248 still present

If the exclusion worked the count would be 235 (248 minus 13 fixture .py files). It has not moved at all, and broken.py / broken.ts are still reported by name.

What is verified

The config is loaded — the runner echoes it back verbatim:

Using configuration file input from workflow: ./.github/codeql/codeql-config.yml
Writing augmented user configuration file to …/user-config.yaml
  paths-ignore:
    - apps/backend/tests/benchmark/fixtures
    - apps/backend/tests/benchmark/fixtures/**

So this is not a typo, a wrong path, or a file GitHub failed to find. paths-ignore is accepted and then does not filter the extractor. No warning in the log explains why.

What I have not established

Why. Candidates I did not confirm: an interaction with build-mode: none, paths-ignore applying at query-result level rather than extraction for these extractors, or the diagnostic query reporting pre-filter. I stopped after two failed attempts instead of guessing a third time.

Not doing next, without a decision from @parthrohit22

The tempting move is to "fix" the fixtures' syntax. That would be wrong: their manifests assert RI-SRC-MALFORMED per RFC-0001 §4.3/§6.2/§8, and constructsCovered lists py.syntax_error / ts.syntax_error. Making them parse would delete that benchmark coverage while making the warning disappear — trading a real capability for a green tick.

Options as I see them:

  1. Dismiss the two alerts in the Security tab as used in tests — accurate, zero code change, but manual and returns if the corpus grows.
  2. Keep digging on paths-ignore semantics for build-mode: none.
  3. Accept the warning as expected and document it, since it is a tool warning, not a finding — there are still 0 open code scanning alerts.

My recommendation is (1) now and (3) documented, because the warning is cosmetic: it reports that CodeQL could not parse two files that are supposed to be unparseable. It is not hiding a vulnerability.

The #280 half of this work is unaffected and already merged — the Security tab is current (last scan now), which is what made this warning visible in the first place.

@parthrohit22

Copy link
Copy Markdown
Collaborator Author

Closing. This change does not work and should not be carried as pending config debt.

Decisive evidence — scanned-file counts before and after the exclusion are byte-identical:

Python TS / JS
dev before 248 / 248 175 / 175, 10 / 10
with paths-ignore (2 attempts) 248 / 248 175 / 175, 10 / 10

A working exclusion would show 235 Python files (248 minus 13 fixture .py). Nothing moved, and broken.py / broken.ts are still reported by name.

Why — the warning is emitted by the extraction-phase diagnostic queries (py/diagnostics/extraction-warnings, js/diagnostics/extraction-errors), which report what the extractor encountered. paths-ignore does not suppress them. Strongly evidenced by the logs, though I did not confirm it against CodeQL internals.

Roadmap decision (§23 / §28 merge gate) — this advances no §23 workstream toward a §28 exit criterion. It is cosmetic: open code scanning alerts are 0, and the warning truthfully reports that CodeQL could not parse two files that are required to be unparseable (manifests assert RI-SRC-MALFORMED, RFC-0001 §4.3/§6.2/§8; constructsCovered: py.syntax_error). Merging a no-op config file to silence a truthful tool warning is surface, not moat — and it would mislead the next reader into thinking the corpus is excluded when it is not.

The useful half of this work, #280, is merged and verified: a push-event CodeQL run now fires on dev (last scan now vs 5 days / 37 commits stale). That is what actually improved the security surface.

Correct resolution is to dismiss the two alerts as used in tests in the Security tab — accurate, no code change, no fixture damage. Documented in #282.

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