Repository navigation
Harden URL trust checks and TLS discovery - #216
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Co-authored-by: sodejm <723679+sodejm@users.noreply.github.com>
[skip-tests: workflow-only change validated by YAML lint and hosted workflow execution]
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 295bd83d07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| results["iac_and_cloud"].append("Oracle Cloud") | ||
| except Exception: | ||
| pass | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
When a repository contains syntactically valid JSON with a non-object top level (for example, []), json.loads succeeds but data.get(...) raises AttributeError, which the narrowed handler does not catch. This aborts the entire repository security scan instead of merely omitting dependency evidence; validate that data is a mapping before accessing dependencies.
Useful? React with 👍 / 👎.
| pass | ||
| except (ValueError, TypeError): | ||
| # Malformed manifests provide no dependable dependency evidence. | ||
| continue |
There was a problem hiding this comment.
When package.json contains invalid JSON, this continue skips all remaining per-file processing, including the credential-pattern scan below. A malformed manifest containing a plaintext secret therefore produces no secrets_findings; the parse failure should suppress only dependency extraction and still allow the generic content checks to run.
Useful? React with 👍 / 👎.
| confidence = ConfidenceLevel.UNCERTAIN.value | ||
| except Exception: | ||
| pass | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
When imported or synthetic TLS evidence supplies a truthy non-string valid_until value, such as an integer, the preceding .replace(...) raises AttributeError, which this narrowed handler misses. Fingerprinting then aborts rather than recording malformed expiry as uncertainty, so validate the field type or catch this failure alongside the existing parse errors.
AGENTS.md reference: AGENTS.md:L94-L98
Useful? React with 👍 / 👎.
| # Repository path setup precedes standalone entry point imports. | ||
| # ruff: noqa: E402 | ||
| #!/usr/bin/env python3 |
There was a problem hiding this comment.
When this mode-100755 script is invoked directly on Unix, the kernel no longer recognizes it as Python because the shebang is preceded by two comments; the shell consequently attempts to interpret the Python source and exits with syntax errors. Keep #!/usr/bin/env python3 on line 1 and place the Ruff directive after it.
Useful? React with 👍 / 👎.
|
|
||
| def _is_arm_template(content): | ||
| try: | ||
| document = json.loads(content) |
There was a problem hiding this comment.
When any scanned .json file contains sufficiently deep nesting, still well below the 1 MB file limit, json.loads raises RecursionError, which this handler does not catch and therefore aborts the entire repository scan. Treat parser depth exhaustion as non-ARM input or apply a bounded-depth parser so an untrusted repository file cannot deny the scanner and its CI secret-scan fallback.
Useful? React with 👍 / 👎.
Code scanning flagged unsafe URL-authority checks, ARM template detection, and deprecated TLS negotiation. References now require HTTPS and an exact trusted hostname, ARM detection parses the declared schema, and live probes require TLS 1.2 or newer.
Review of the failing checks also found an inherited repository-wide Ruff baseline failure and a YAML description warning. This change repairs that baseline, regenerates synchronized skill adapters and portable evidence, and documents narrowly justified lint exceptions. Public string enum behavior is preserved.
Security and failure-handling corrections reject XML entities before parsing, propagate approval-storage failures rather than silently proceeding, retain cleanup residual failures, and report malformed or timezone-naive certificate expiry as uncertainty. Regression tests cover these behaviors and malicious hostname lookalikes.
Validation: full
make check, Ruff, YAML lint, and whitespace checks run in the isolated review checkout. Hosted CI must pass on the final head before merge.Alert review: the remaining apparent credential/hash findings use synthetic fixtures, content-integrity hashes, or redacted structured scanner output. The Foundry string assertion and SAN list-membership assertion do not implement URL trust decisions. Certificate probing deliberately inspects untrusted certificates and does not transmit application credentials. These observations do not establish a broader security assessment.
Workflow corrections update checkout to v7 and quote the issue-number argument; push and PR lint both pass. CodeQL alerts 1 and 3 were reviewed and dismissed as false positives with source-specific evidence: a public Azure permission identifier and a temporary synthetic redaction-test credential. All other security analysis remains enabled.