🐛 URL encode user and password. Generated PAT may contain (\). #79
Conversation
Signed-off-by: Jeff Ortel <[email protected]>
WalkthroughThe writeCreds function now URL-escapes the User and Password (using url.PathEscape) before writing http/https credential entries; all other credential-writing logic (scheme iteration, host assembly, file write, error handling, early return on missing creds) is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant G as writeCreds
participant E as url.PathEscape
participant F as File System
C->>G: writeCreds(user, pass, host)
alt user or pass missing
G-->>C: return early
else
G->>E: PathEscape(user), PathEscape(pass)
E-->>G: userEsc, passEsc
loop schemes: http, https
G->>F: Write credential entry "scheme://userEsc:passEsc@host"
F-->>G: OK / Err
end
G-->>C: Result
end
note over G,E: New — credentials are escaped with url.PathEscape before writing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Jeff Ortel <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
repository/git.go (2)
231-247: Use RFC-3986 userinfo encoding; QueryEscape is wrong here (space → '+').
net/url.QueryEscapeis for query strings and turns spaces into '+', which breaks credentials containing spaces and other userinfo-reserved chars. Build the URL withurl.URL+url.UserPassword()(or at leastPathEscape) to guarantee correct percent-encoding for the userinfo.Apply this diff to the loop body:
- entry := scheme - entry += "://" - if r.Identity.User != "" { - entry += urllib.QueryEscape(r.Identity.User) - entry += ":" - } - if r.Identity.Password != "" { - entry += urllib.QueryEscape(r.Identity.Password) - entry += "@" - } - entry += url.Host - _, err = f.Write([]byte(entry + "\n")) + u := urllib.URL{ + Scheme: scheme, + Host: url.Host, + } + // Both are guaranteed non-empty due to the early return above. + u.User = urllib.UserPassword(r.Identity.User, r.Identity.Password) + entry := u.String() + _, err = f.Write([]byte(entry + "\n")) if err != nil { err = liberr.Wrap( err, "path", path) break }
222-229: Set .git-credentials permissions to 0600.Credentials are sensitive; use restrictive perms instead of inheriting umask defaults.
Apply this diff:
- f, err := os.Create(path) + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600)
🧹 Nitpick comments (1)
repository/git.go (1)
283-307: Proxy URL builds: encode creds and honor scheme via net/url.The proxy auth is string-concatenated without encoding and the scheme is hard-coded to
http://. Build the proxy URL withurl.URL+url.UserPassword()and usep.Kindfor the scheme.Apply this diff:
- auth := "" - if p.Identity != nil { - var id *api.Identity - id, err = addon.Identity.Get(p.Identity.ID) - if err != nil { - return - } - auth = fmt.Sprintf( - "%s:%s@", - id.User, - id.Password) - } - proxy = fmt.Sprintf( - "http://%s%s", - auth, - p.Host) - if p.Port > 0 { - proxy = fmt.Sprintf( - "%s:%d", - proxy, - p.Port) - } + pu := urllib.URL{ + Scheme: p.Kind, // "http" or "https" + Host: p.Host, + } + if p.Port > 0 { + pu.Host = fmt.Sprintf("%s:%d", p.Host, p.Port) + } + if p.Identity != nil { + var id *api.Identity + id, err = addon.Identity.Get(p.Identity.ID) + if err != nil { + return + } + pu.User = urllib.UserPassword(id.User, id.Password) + } + proxy = pu.String()Please verify with a proxy identity containing
\or spaces to ensure end-to-end behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
repository/git.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-upload-for-global-ci
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
repository/git.go (2)
222-229: Harden credentials file permissions (0600).git-credentials contains secrets; create it with owner-only perms to avoid accidental disclosure under permissive umasks.
Apply:
- f, err := os.Create(path) + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600)
293-307: Encode proxy credentials too (same bug class)Proxy auth currently embeds raw user/password into the proxy URL. Special chars (e.g., backslash) will break and/or risk misparsing. Use url.UserPassword and build the URL.
Apply:
- auth := "" - if p.Identity != nil { - var id *api.Identity - id, err = addon.Identity.Get(p.Identity.ID) - if err != nil { - return - } - auth = fmt.Sprintf( - "%s:%s@", - id.User, - id.Password) - } - proxy = fmt.Sprintf( - "http://%s%s", - auth, - p.Host) - if p.Port > 0 { - proxy = fmt.Sprintf( - "%s:%d", - proxy, - p.Port) - } + if p.Identity != nil { + var id *api.Identity + id, err = addon.Identity.Get(p.Identity.ID) + if err != nil { + return + } + u := &urllib.URL{ + Scheme: "http", + Host: p.Host, + User: urllib.UserPassword(id.User, id.Password), + } + if p.Port > 0 { + u.Host = fmt.Sprintf("%s:%d", p.Host, p.Port) + } + proxy = u.String() + } else { + host := p.Host + if p.Port > 0 { + host = fmt.Sprintf("%s:%d", host, p.Port) + } + proxy = "http://" + host + }Note: If HTTPS proxies are supported, consider setting Scheme based on p.Kind.
🧹 Nitpick comments (1)
repository/git.go (1)
235-246: Prefer building the URL with url.URL + url.UserPasswordConstructing the entry via the standard URL builder avoids manual concatenation, guarantees correct userinfo encoding, and removes redundant emptiness checks.
Apply:
- for _, scheme := range []string{ - "https", - "http", - } { - entry := scheme - entry += "://" - if r.Identity.User != "" { - entry += urllib.PathEscape(r.Identity.User) - entry += ":" - } - if r.Identity.Password != "" { - entry += urllib.PathEscape(r.Identity.Password) - entry += "@" - } - entry += url.Host - _, err = f.Write([]byte(entry + "\n")) - if err != nil { - err = liberr.Wrap( - err, - "path", - path) - break - } - } + for _, scheme := range []string{"https", "http"} { + u := &urllib.URL{ + Scheme: scheme, + Host: url.Host, + User: urllib.UserPassword(r.Identity.User, r.Identity.Password), + } + if _, err = f.WriteString(u.String() + "\n"); err != nil { + err = liberr.Wrap(err, "path", path) + break + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
repository/git.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-integration / e2e-ui-integration-tests
- GitHub Check: test-integration / e2e-api-integration-tests
🔇 Additional comments (1)
repository/git.go (1)
237-244: Correct: percent-encode user/password for .git-credentialsUsing url.PathEscape on both user and password properly handles backslashes and other reserved characters. This matches Git’s expectation for percent-encoded credentials.
2 participants
closes: #77
Summary by CodeRabbit