fix(security): Resolve confirmed Sonar findings#110
Open
ryanheffernan wants to merge 24 commits into
Open
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR updates workflow pinning and Kind integration handling, tightens container build/runtime settings, hardens air-gapped image loading, renames the NATS readiness interface, escapes log values in ZTP paths, and adjusts two UI helpers. ChangesCI automation updates
Runtime images and deployment hardening
NATS readiness interface rename
Log escaping updates
UI helper updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/nats-ready/internal/nats-ready/nats.go (1)
81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstructor name still references old interface name.
NewNatsRunnablenow returnsRunnerbut its own name still carries the oldNatsRunnablenaming, which is slightly inconsistent post-rename.♻️ Optional rename for consistency
-func NewNatsRunnable(config *NatsReadyConfig) (Runner, error) { +func NewRunner(config *NatsReadyConfig) (Runner, error) {Note: would also require updating the call site in
components/nats-ready/cmd/nats-ready/main.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/nats-ready/internal/nats-ready/nats.go` at line 81, Rename the constructor to match the new interface naming and keep the API consistent: update NewNatsRunnable in nats.go to use the Runner-oriented name, and adjust the main.go call site accordingly. Make sure the renamed function still returns Runner and preserves the same config behavior so all references follow the new naming convention.build/nv-config-manager.Dockerfile (1)
44-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment: "Copy all project files" no longer matches the targeted copies below.
The comment above still says "Copy all project files" but the block now copies only specific subdirectories. Consider updating the comment to describe the narrowed scope so future contributors don't assume the whole tree is copied.
✏️ Suggested comment fix
-# Copy all project files +# Copy only the files needed for the build (narrowed build context) COPY pyproject.toml uv.lock README.md /code/nv-config-manager/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/nv-config-manager.Dockerfile` around lines 44 - 49, The comment above the COPY instructions is outdated and says “Copy all project files,” but the Dockerfile now copies only selected files and subdirectories. Update the nearby comment in the Dockerfile to accurately describe the narrower copy scope around the COPY statements for pyproject.toml, uv.lock, src/nv_config_manager, src/tests, and components/network-templates so the intent matches the current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/public-ci.yml:
- Around line 25-31: The checkout steps are still using the default credential
persistence, which leaves the git token available to later steps. Update every
job that uses the actions/checkout step via the actions_checkout anchor to
explicitly set persist-credentials: false in that step’s with block, including
jobs that already set fetch-depth: 0 and jobs like python-lint that currently
have no with block. Use the existing checkout usages in the workflow as the
places to apply the fix.
In `@build/kea-admin.Dockerfile`:
- Around line 44-46: SECURITY.md still says Kea/kea-admin containers run as
root, but kea-admin is now explicitly configured to run as non-root with USER
1000:1000 and matches the Helm chart’s runAsUser/runAsGroup convention. Update
the accepted-risk note in SECURITY.md to remove kea-admin from the “runs as
root” statement and keep only any remaining root-related risk that still applies
to Kea components. Make sure the wording aligns with the kea-admin container
behavior referenced by the Dockerfile change.
---
Nitpick comments:
In `@build/nv-config-manager.Dockerfile`:
- Around line 44-49: The comment above the COPY instructions is outdated and
says “Copy all project files,” but the Dockerfile now copies only selected files
and subdirectories. Update the nearby comment in the Dockerfile to accurately
describe the narrower copy scope around the COPY statements for pyproject.toml,
uv.lock, src/nv_config_manager, src/tests, and components/network-templates so
the intent matches the current behavior.
In `@components/nats-ready/internal/nats-ready/nats.go`:
- Line 81: Rename the constructor to match the new interface naming and keep the
API consistent: update NewNatsRunnable in nats.go to use the Runner-oriented
name, and adjust the main.go call site accordingly. Make sure the renamed
function still returns Runner and preserves the same config behavior so all
references follow the new naming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de5a0357-2b53-48b3-a7ff-092f9a7052ed
📒 Files selected for processing (17)
.github/workflows/docs.yml.github/workflows/public-ci.ymlbuild/kea-admin.Dockerfilebuild/kea.Dockerfilebuild/nats-ready.Dockerfilebuild/nautobot-test.Dockerfilebuild/nautobot.Dockerfilebuild/nv-config-manager.Dockerfilebuild/ui.Dockerfilecomponents/nats-ready/internal/nats-ready/nats.gocomponents/nautobot/nautobot-app-overlays/development/Dockerfiledeploy/airgapped/create-airgapped.shdeploy/airgapped/manifests/README.mddeploy/airgapped/manifests/image-loader-daemonset.yamlsrc/nv_config_manager/ztp/api/device_v1.pyui/src/app/auth/discovery/route.tsui/src/app/body.tsx
ryanheffernan
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nv_config_manager/common/log.py (1)
158-160: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: consider escaping other line-breaking control characters.
Current helper only escapes
\r/\n. Some log processors/terminals also treat\x0b(VT),\x0c(FF), and Unicode line separators (\u2028/\u2029) as line breaks, which could still enable partial log-forging. Since the PR scope is specifically CR/LF, this is a nice-to-have hardening, not a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nv_config_manager/common/log.py` around lines 158 - 160, The escape_log_newlines helper currently hardens only against CR/LF, so extend it to also escape other line-breaking control characters such as vertical tab, form feed, and Unicode line/paragraph separators. Update escape_log_newlines in log.py to handle these additional separators while keeping the existing behavior for \r and \n unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/nv_config_manager/common/log.py`:
- Around line 158-160: The escape_log_newlines helper currently hardens only
against CR/LF, so extend it to also escape other line-breaking control
characters such as vertical tab, form feed, and Unicode line/paragraph
separators. Update escape_log_newlines in log.py to handle these additional
separators while keeping the existing behavior for \r and \n unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77360974-2917-48d1-a385-b656021b176d
📒 Files selected for processing (6)
src/nv_config_manager/common/log.pysrc/nv_config_manager/ztp/api/device_v1.pysrc/nv_config_manager/ztp/api/streaming.pysrc/nv_config_manager/ztp/sftp/main.pysrc/tests/ztp/api/test_main.pysrc/tests/ztp/sftp/test_main.py
✅ Files skipped from review due to trivial changes (1)
- src/nv_config_manager/ztp/sftp/main.py
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
ryanheffernan
force-pushed
the
rheffernan/sonar-fixes
branch
from
ef48383 to
fbde0cf
Compare
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Collaborator
Author
|
/kind test |
Collaborator
Author
|
/ok to test 8f6f43e |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tests/common/test_log.py (1)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering non-string input and passthrough behavior.
escape_log_newlinesacceptsvalue: objectand stringifies internally, but the tests only exercisestrinputs with an embedded separator. Adding a case for a non-string value (e.g. an exception or int) and a no-separator passthrough case would round out coverage for the actual signature.♻️ Suggested additional test
+def test_escape_log_newlines_passthrough_and_non_str() -> None: + assert escape_log_newlines("plain text") == "plain text" + assert escape_log_newlines(ValueError("bad\nvalue")) == r"bad\nvalue"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/common/test_log.py` around lines 37 - 38, The current `test_escape_log_newlines_escapes_line_separators` only covers string inputs with separators, so add coverage for `escape_log_newlines`’s broader `value: object` signature. Extend the tests in `test_log.py` to include a non-string input case (such as an int or exception object) to verify it stringifies and escapes correctly, and a passthrough case with no newline separators to confirm unchanged output when no escaping is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tests/common/test_log.py`:
- Around line 37-38: The current
`test_escape_log_newlines_escapes_line_separators` only covers string inputs
with separators, so add coverage for `escape_log_newlines`’s broader `value:
object` signature. Extend the tests in `test_log.py` to include a non-string
input case (such as an int or exception object) to verify it stringifies and
escapes correctly, and a passthrough case with no newline separators to confirm
unchanged output when no escaping is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d670ba8-f69b-4237-83c8-1e9b4a3a9f63
📒 Files selected for processing (2)
src/nv_config_manager/common/log.pysrc/tests/common/test_log.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nv_config_manager/common/log.py
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Collaborator
Author
|
/ok to test 4aaaf2e |
Collaborator
Author
|
/kind test |
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Collaborator
Author
|
/ok to test 38f0d1f |
Collaborator
Author
|
/kind test |
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/kind-integration-command.yml (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing version comment on pinned checkout.
Other workflows (docs.yml, public-ci.yml) annotate pinned SHAs with
# v4.3.1for readability; this file's checkout pins lack that comment.✏️ Suggested tweak
- - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/kind-integration-command.yml at line 30, Add the missing version comment to the pinned actions/checkout reference in the kind integration workflow so it matches the other workflows’ readability convention. Update the checkout step in the workflow and the other pinned checkout occurrence mentioned in the comment to include the same inline version tag (for example, the existing v4.3.1-style annotation) without changing the pinned SHA or step behavior.src/nv_config_manager/common/log.py (1)
62-75: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider also escaping ESC (
\x1b) to block terminal escape-sequence injection.The map neutralizes CR/LF and other line-separator characters, but not the ESC character that begins ANSI/VT terminal escape sequences. If these logs are ever viewed through a terminal, forged input containing
\x1b[...sequences could manipulate the display (hide/alter log content) even though CRLF injection is now blocked. This is a related but distinct log-forging vector (CWE-150).♻️ Optional hardening
"\x85": r"\x85", "\u2028": r"\u2028", "\u2029": r"\u2029", + "\x1b": r"\x1b", } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nv_config_manager/common/log.py` around lines 62 - 75, The log escaping map in _LOG_LINE_BREAK_ESCAPES should also neutralize the ESC character to prevent terminal escape-sequence injection. Update the translation table in log.py so the existing line-break sanitization additionally covers "\x1b", ensuring log content handled by the logging helpers cannot inject ANSI/VT control sequences when viewed in a terminal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/kind-integration-command.yml:
- Line 30: Add the missing version comment to the pinned actions/checkout
reference in the kind integration workflow so it matches the other workflows’
readability convention. Update the checkout step in the workflow and the other
pinned checkout occurrence mentioned in the comment to include the same inline
version tag (for example, the existing v4.3.1-style annotation) without changing
the pinned SHA or step behavior.
In `@src/nv_config_manager/common/log.py`:
- Around line 62-75: The log escaping map in _LOG_LINE_BREAK_ESCAPES should also
neutralize the ESC character to prevent terminal escape-sequence injection.
Update the translation table in log.py so the existing line-break sanitization
additionally covers "\x1b", ensuring log content handled by the logging helpers
cannot inject ANSI/VT control sequences when viewed in a terminal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8272c7ee-616c-4490-b97c-1e509de03cfb
📒 Files selected for processing (29)
.github/scripts/kind-integration-result.js.github/scripts/kind-integration-result.test.js.github/workflows/docs.yml.github/workflows/kind-integration-command.yml.github/workflows/kind-integration.yml.github/workflows/public-ci.ymlSECURITY.mdbuild/kea-admin.Dockerfilebuild/kea.Dockerfilebuild/nats-ready.Dockerfilebuild/nautobot-test.Dockerfilebuild/nautobot.Dockerfilebuild/nv-config-manager.Dockerfilebuild/ui.Dockerfilecomponents/nats-ready/cmd/nats-ready/main.gocomponents/nats-ready/internal/nats-ready/nats.gocomponents/nautobot/nautobot-app-overlays/development/Dockerfiledeploy/airgapped/create-airgapped.shdeploy/airgapped/manifests/README.mddeploy/airgapped/manifests/image-loader-daemonset.yamlsrc/nv_config_manager/common/log.pysrc/nv_config_manager/ztp/api/device_v1.pysrc/nv_config_manager/ztp/api/streaming.pysrc/nv_config_manager/ztp/sftp/main.pysrc/tests/common/test_log.pysrc/tests/ztp/api/test_main.pysrc/tests/ztp/sftp/test_main.pyui/src/app/auth/discovery/route.tsui/src/app/body.tsx
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Collaborator
Author
|
/ok to test 15d1a0b |
ryanheffernan
requested review from
dmathur0,
jojung1,
polarweasel,
spidercensus,
susanhooks and
zsblevins
as code owners
Collaborator
|
The escape_log_newlines mechanism would be cleaner to include in the log adapter instead of calling on every log parameter |
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Collaborator
Author
|
/ok to test e4a052a |
Collaborator
Author
|
/kind test |
Signed-off-by: rheffernan <rheffernan@nvidia.com>
Collaborator
Author
|
/ok to test 55038a2 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolve confirmed Sonar findings across container builds, deployment manifests, CI workflows, backend services, and UI code.
Validation
Passing Kind Integration run:
https://github.com/NVIDIA/nv-config-manager/actions/runs/28958011491
Ran a local integration test to confirm the non-root Nautobot image successfully cloned, synchronized, imported, and executed Git-backed jobs with correct permissions, since kind integration doesn't cover this scenario.
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit