fix: minor bugs in CLI#76
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR implements git-scoped configuration alongside path-scoped setups, refactors auth callback HTML into shared Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 (1)
src/api/auth.ts (1)
250-257: ⚡ Quick winEscape text fields inside
renderAuthPageto make XSS safety default.Right now only
authErrorResponseescapes input before passingsubtitle. Escaping should happen inrenderAuthPagefordocumentTitle,title,subtitle, andhintso future call sites can’t accidentally inject HTML.Suggested patch
private renderAuthPage(params: { state: "success" | "error"; documentTitle: string; title: string; subtitle: string; hint: string; autoClose?: boolean; }): string { const { state, documentTitle, title, subtitle, hint, autoClose } = params; + const safeDocumentTitle = this.escapeHtml(documentTitle); + const safeTitle = this.escapeHtml(title); + const safeSubtitle = this.escapeHtml(subtitle); + const safeHint = this.escapeHtml(hint); @@ -<title>${documentTitle}</title> +<title>${safeDocumentTitle}</title> @@ - <h1>${title}</h1> - <p class="desc">${subtitle}</p> + <h1>${safeTitle}</h1> + <p class="desc">${safeSubtitle}</p> @@ - <p class="hint">${hint}</p> + <p class="hint">${safeHint}</p>Also applies to: 272-273, 306-307, 309-309
🤖 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/api/auth.ts` around lines 250 - 257, renderAuthPage currently interpolates documentTitle, title, subtitle, and hint without escaping, risking XSS; update renderAuthPage to HTML-escape those four fields (documentTitle, title, subtitle, hint) at the start of the method (e.g., via a shared escapeHtml helper or existing escape utility) and use the escaped values for all template interpolation (also ensure autoClose rendering unaffected). Apply the same escape usage for call sites referenced around the renderAuthPage usages (the places noted near lines ~272-273, ~306-307, ~309) so every path into renderAuthPage or its templates uses escaped strings.
🤖 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 `@src/api/client.ts`:
- Around line 109-119: The code in the git-branch of configure flow swallows
real read errors by using .catch(() => null) on config.getConfigure and
returning a plain ProjectConfig when the user declines replacement, which the
caller interprets as "use git"; change the behavior so that
config.getConfigure(options, { scope: "path" }) only catches a specific
NotFound/NoConfig error (rethrow other errors), and stop returning a raw
ProjectConfig to signal intent—introduce and return an explicit result shape
(e.g., { status: "kept" | "replaced" | "canceled", effectiveScope?: "path" |
"git", config?: ProjectConfig }) from the function handling the confirm() branch
(the block that calls confirm("A path-only setup already exists...")), returning
{ status: "kept", effectiveScope: "path", config: pathSetup } when replace is
false so the downstream persister can honor the user's choice.
In `@src/lib/config.ts`:
- Around line 241-244: The removal logic only compares resolved pathKey to
setupKey, missing symlink aliases; change it to canonicalize both sides using
fs.realpath (or fs.realpathSync) so you compare and delete by real paths:
compute realPathKey = realpath(projectPath) and realSetupKey =
realpath(setupKey) (falling back to path.resolve if realpath fails), then remove
config.setups entries matching either the resolved or real canonical keys
(pathKey, realPathKey, setupKey, realSetupKey) so stale setups saved under
symlinked aliases (e.g., /var vs /private/var) are also deleted; apply the same
canonicalization approach where create/get/find path keys are produced
(functions referencing pathKey, setupKey, config.setups).
---
Nitpick comments:
In `@src/api/auth.ts`:
- Around line 250-257: renderAuthPage currently interpolates documentTitle,
title, subtitle, and hint without escaping, risking XSS; update renderAuthPage
to HTML-escape those four fields (documentTitle, title, subtitle, hint) at the
start of the method (e.g., via a shared escapeHtml helper or existing escape
utility) and use the escaped values for all template interpolation (also ensure
autoClose rendering unaffected). Apply the same escape usage for call sites
referenced around the renderAuthPage usages (the places noted near lines
~272-273, ~306-307, ~309) so every path into renderAuthPage or its templates
uses escaped strings.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86b79113-1e19-41c0-aad8-10de765f4e37
📒 Files selected for processing (10)
src/api/auth.tssrc/api/client.tssrc/cmd/configure.tssrc/lib/config.tssrc/lib/secretCache.tssrc/lib/secureStore.tstests/integration/config.test.tstests/integration/configure-command.test.tstests/integration/secret-cache.test.tstests/integration/secure-store.test.ts
💤 Files with no reviewable changes (1)
- src/lib/secureStore.ts
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
--gitwith clear error when no repo is found.Refactor
Tests