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. |
![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: 927469bd95
ℹ️ 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".
| if user_input.get(CONF_ENABLE_EXTERNAL_CHECK): | ||
| parsed = urlparse(external_url) | ||
| if parsed.scheme not in {"http", "https"} or not parsed.netloc: |
There was a problem hiding this comment.
When the newly exposed self-check is enabled with an http:// URL, this validation accepts it and check_external_url() creates a weak-TLS finding, but serializes it to a dictionary before the coordinator appends it to scan_result.findings. _update_repairs() then passes that mixed list to group_findings(), which accesses finding.rule_id and raises AttributeError, causing the coordinator refresh to fail whenever the check produces a finding. Preserve Finding instances for internal processing or deserialize the returned dictionaries before extending the scan result.
Useful? React with 👍 / 👎.
| secrets_map.setdefault(key, mask_secret(value)) | ||
| secrets_raw_hashes.setdefault( | ||
| key, hash_for_comparison(value) | ||
| ) |
There was a problem hiding this comment.
When independent nested configurations contain their own secrets.yaml files, flattening every key into one global map lets a definition in an unrelated subtree satisfy any !secret reference. For example, a missing key referenced by packages/foo.yaml is considered available merely because esphome/secrets.yaml defines the same name; R004 checks only the global secrets_map key set and therefore suppresses the missing-secret finding. Track each store by directory and resolve references against the applicable file's local/ancestor store rather than merging all keys globally.
Useful? React with 👍 / 👎.
| filename = path.name | ||
| if filename.lower() == "secrets.yaml": | ||
| return True |
There was a problem hiding this comment.
When enable_secret_age is enabled, this unconditional exclusion prevents the root secrets.yaml from reaching any rule. R060SecretAge.evaluate_file_text() explicitly runs only when file_path == "secrets.yaml", so after this change the advertised secret-age check can never emit a finding. Exclude raw secret values from leak-oriented rules without removing the file from the rule pipeline needed by R060.
Useful? React with 👍 / 👎.
|
@codex address that feedback. Please fix all three review findings: keep external self-check findings as |
|
To use Codex here, create an environment for this repo. |
|
/apply-secretsentry-review-fixes |
1 participant
Addresses #16, #21 and #22; #11 is also covered by the existing v3.0.7 Clear All Repairs flow and will be closed after verification.
secrets.yamlstores (including ESPHome) when validating!secretreferences.secrets.yamlitself as an inline-secret leak source..cloudcredential state from user-remediable findings.Raw secret values are still not retained in the scan context; only masked values and comparison hashes are stored.