Conversation
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR refactors configuration entry rendering across multiple web components by extracting repeated inline markup into a reusable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/app/routes/ws/runners/JobAgentCard.tsx (2)
60-78: MovesensitiveKeysoutside the.map()callback.The
new Set(...)is recreated on every iteration, which is wasteful. Define it once outside the callback.♻️ Proposed refactor
+ const sensitiveKeys = new Set(["token", "apiKey", "secret"]); return ( <Card className="scrollbar-thin scrollbar-thumb-muted-foreground/20 scrollbar-track-muted overflow-auto rounded-md bg-accent p-2"> <CardContent className="space-y-0.5 p-0"> {entries .sort((a, b) => a[0].localeCompare(b[0])) .map(([key, value]) => { - const sensitiveKeys = new Set(["token", "apiKey", "secret"]); const isSensitive = sensitiveKeys.has(key); return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/runners/JobAgentCard.tsx` around lines 60 - 78, The code recreates the Set of sensitive keys inside the .map() callback in JobAgentCard (sensitiveKeys), which is wasteful; move the declaration const sensitiveKeys = new Set(["token","apiKey","secret"]); above the .map() call (outside the render mapping function) and then use that same sensitiveKeys inside the map to compute isSensitive without reallocating each iteration.
61-62: Consider case-insensitive matching and additional sensitive keys.The current check is case-sensitive and covers a limited set of keys. Keys like
"Token","API_KEY","password", or"apiSecret"would not be redacted.💡 Optional: Broader sensitive key detection
- const sensitiveKeys = new Set(["token", "apiKey", "secret"]); - const isSensitive = sensitiveKeys.has(key); + const sensitivePatterns = ["token", "apikey", "secret", "password", "credential"]; + const isSensitive = sensitivePatterns.some((pattern) => + key.toLowerCase().includes(pattern) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/runners/JobAgentCard.tsx` around lines 61 - 62, The redaction check in JobAgentCard.tsx currently uses a case-sensitive Set (sensitiveKeys) and only a few names; update the logic in the sensitiveKeys/isSensitive area to perform case-insensitive matching and cover more variants by (1) normalizing the inspected key with key.toLowerCase() before checking, (2) expanding the sensitive list to include common names like "password", "passwd", "api_key", "apikey", "api-secret", "secret", "token", "credential", "auth", and (3) applying substring/regex matching for tokens/keys/secrets (e.g., if lowerKey.includes("token") || lowerKey.includes("key") || lowerKey.includes("secret") || lowerKey.includes("password")) so "API_KEY", "apiSecret", "Password" and similar variants are redacted by isSensitive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/app/routes/ws/runners/card-contents/ArgoCDConfig.tsx`:
- Around line 17-24: The anchor currently always prepends "https://" to
config.serverUrl, causing double-protocols; update the JSX in ArgoCDConfig (the
<a> element that uses config.serverUrl) to first normalize the URL by checking
if config.serverUrl already starts with a protocol (e.g., /^https?:\/\//i or use
the URL constructor) and only prepend "https://" when no protocol is present,
then use the normalized value for href while keeping the displayed text as
config.serverUrl (or a normalized/trimmed form if desired).
---
Nitpick comments:
In `@apps/web/app/routes/ws/runners/JobAgentCard.tsx`:
- Around line 60-78: The code recreates the Set of sensitive keys inside the
.map() callback in JobAgentCard (sensitiveKeys), which is wasteful; move the
declaration const sensitiveKeys = new Set(["token","apiKey","secret"]); above
the .map() call (outside the render mapping function) and then use that same
sensitiveKeys inside the map to compute isSensitive without reallocating each
iteration.
- Around line 61-62: The redaction check in JobAgentCard.tsx currently uses a
case-sensitive Set (sensitiveKeys) and only a few names; update the logic in the
sensitiveKeys/isSensitive area to perform case-insensitive matching and cover
more variants by (1) normalizing the inspected key with key.toLowerCase() before
checking, (2) expanding the sensitive list to include common names like
"password", "passwd", "api_key", "apikey", "api-secret", "secret", "token",
"credential", "auth", and (3) applying substring/regex matching for
tokens/keys/secrets (e.g., if lowerKey.includes("token") ||
lowerKey.includes("key") || lowerKey.includes("secret") ||
lowerKey.includes("password")) so "API_KEY", "apiSecret", "Password" and similar
variants are redacted by isSensitive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ea86414-ff91-475a-8e56-759a01114a1f
📒 Files selected for processing (4)
apps/web/app/routes/ws/runners/JobAgentCard.tsxapps/web/app/routes/ws/runners/card-contents/ArgoCDConfig.tsxapps/web/app/routes/ws/runners/card-contents/TfeConfig.tsxapps/workspace-engine/svc/controllers/jobdispatch/jobagents/terraformcloud/tfe.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/workspace-engine/svc/controllers/jobdispatch/jobagents/terraformcloud/tfe_test.go (1)
14-16: Consider adding test cleanup to restore global state.The
setTFETokenhelper mutatesconfig.Global.TFETokendirectly. While tests appear to run sequentially, adding cleanup (e.g., viat.Cleanup) would make tests more robust and prevent subtle issues if parallelism is introduced later.Optional: Add cleanup helper
func setTFEToken(token string) { config.Global.TFEToken = token } + +func setTFETokenWithCleanup(t *testing.T, token string) { + t.Helper() + old := config.Global.TFEToken + config.Global.TFEToken = token + t.Cleanup(func() { config.Global.TFEToken = old }) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobdispatch/jobagents/terraformcloud/tfe_test.go` around lines 14 - 16, The helper setTFEToken mutates config.Global.TFEToken without restoring it, which can leak state into other tests; update callers to use a version that saves the old value and registers a t.Cleanup to restore it (or change setTFEToken to accept testing.T and call t.Cleanup) so that config.Global.TFEToken is reset after each test, referencing the setTFEToken helper and the config.Global.TFEToken global and using t.Cleanup to perform the restore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.compose:
- Line 37: Remove the stray trailing single quote from the inline comment on the
TFE_WEBHOOK_SECRET line so the comment reads "# openssl rand -hex 32" instead of
"# openssl rand -hex 32'"; update the comment text adjacent to the
TFE_WEBHOOK_SECRET entry in the .env.compose file to eliminate the extraneous
quote.
---
Nitpick comments:
In
`@apps/workspace-engine/svc/controllers/jobdispatch/jobagents/terraformcloud/tfe_test.go`:
- Around line 14-16: The helper setTFEToken mutates config.Global.TFEToken
without restoring it, which can leak state into other tests; update callers to
use a version that saves the old value and registers a t.Cleanup to restore it
(or change setTFEToken to accept testing.T and call t.Cleanup) so that
config.Global.TFEToken is reset after each test, referencing the setTFEToken
helper and the config.Global.TFEToken global and using t.Cleanup to perform the
restore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 642a7db3-358a-4977-9764-4fa484947177
📒 Files selected for processing (9)
.env.composeapps/web/app/routes/ws/runners/card-contents/TfeConfig.tsxapps/workspace-engine/oapi/spec/schemas/jobs.jsonnetapps/workspace-engine/pkg/config/env.goapps/workspace-engine/svc/controllers/jobdispatch/jobagents/terraformcloud/config.goapps/workspace-engine/svc/controllers/jobdispatch/jobagents/terraformcloud/tfe.goapps/workspace-engine/svc/controllers/jobdispatch/jobagents/terraformcloud/tfe_test.godocs/integrations/job-agents/terraform-cloud.mdxpackages/trpc/src/routes/job-agents.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/workspace-engine/svc/controllers/jobdispatch/jobagents/terraformcloud/tfe.go
- apps/web/app/routes/ws/runners/card-contents/TfeConfig.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/app/routes/ws/deployments/settings/_components/DeploymentAgentCard.tsx`:
- Around line 97-101: The UI currently only checks top-level keys
(sensitiveKeys) and then JSON.stringify()s nested objects, leaking nested
secrets; add a shared renderer used in both DeploymentAgentCard render sites
that deep-redacts any object/array by recursively walking the value and
replacing any property whose key matches sensitiveKeys (or whose key name
contains common secret substrings like "token","apiKey","secret") with
"[REDACTED]" before stringifying; implement this as a helper function (e.g.,
redactValue(value, sensitiveKeys) or renderSecretValue) and use it in place of
the current ternary so both branches call the same redaction + render logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40f39fcd-0971-4754-b7f6-c882fd065e95
📒 Files selected for processing (1)
apps/web/app/routes/ws/deployments/settings/_components/DeploymentAgentCard.tsx
apps/web/app/routes/ws/deployments/settings/_components/DeploymentAgentCard.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/app/components/config-entry.tsx (1)
1-1: Case-sensitive key matching may miss sensitive fields.The
sensitiveKeyscheck uses exact case matching. Keys like"Token","apikey","API_KEY", or"Secret"from backend responses won't be redacted, potentially leaking credentials.Consider normalizing to lowercase before matching:
♻️ Proposed fix for case-insensitive matching
-const sensitiveKeys = new Set(["token", "apiKey", "secret"]); +const sensitiveKeys = new Set(["token", "apikey", "secret"]); export function ConfigEntry({ entryKey, value, className, }: { entryKey: string; value: unknown; className?: string; }) { - const isSensitive = sensitiveKeys.has(entryKey); + const isSensitive = sensitiveKeys.has(entryKey.toLowerCase());Also applies to: 12-12
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/components/config-entry.tsx` at line 1, The sensitiveKeys set uses case-sensitive matching and will miss variants like "API_KEY" or "Token"; change the set to contain lowercase entries (e.g., ["token","apikey","secret"]) and ensure all lookups normalize the inspected key to lowercase (e.g., use key.toLowerCase() when checking sensitiveKeys.has). Update any other uses of sensitiveKeys (e.g., the second occurrence) to perform the same lowercase normalization so redaction is case-insensitive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/app/components/config-entry.tsx`:
- Line 1: The sensitiveKeys set uses case-sensitive matching and will miss
variants like "API_KEY" or "Token"; change the set to contain lowercase entries
(e.g., ["token","apikey","secret"]) and ensure all lookups normalize the
inspected key to lowercase (e.g., use key.toLowerCase() when checking
sensitiveKeys.has). Update any other uses of sensitiveKeys (e.g., the second
occurrence) to perform the same lowercase normalization so redaction is
case-insensitive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 672582ed-fb70-4f59-91da-9e3774f2ea02
📒 Files selected for processing (3)
apps/web/app/components/config-entry.tsxapps/web/app/routes/ws/deployments/settings/_components/DeploymentAgentCard.tsxapps/web/app/routes/ws/runners/JobAgentCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/routes/ws/runners/JobAgentCard.tsx
Show [REDACTED] for sensitive fields (token, apiKey, secret) in job agent config displays. Extract shared ConfigEntry component used by both the runners page and deployment settings page. Also persist failure status when TFE config parsing or workspace templating fails.
331ab34 to
86c4672
Compare
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
3 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.
Summary
[REDACTED]for TFE token and ArgoCD API key in job agent config cardstoken,apiKey,secret) in the generic fallback config rendererReplaces #858 — keeps token in the per-agent config rather than moving to env var.
Summary by CodeRabbit
Bug Fixes
New Features