fix(sdk): preserve base path when building KAS rewrap client URL#3762
fix(sdk): preserve base path when building KAS rewrap client URL#3762elizabethhealy wants to merge 2 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a routing regression where the KAS client was incorrectly stripping the path component from the base URL. By preserving the path, the client now correctly routes requests to KAS instances deployed under non-root paths, preventing 403 errors that were previously misidentified as permission issues. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The path was lost in transit flow, / A KAS request with nowhere to go. / We kept the route, the slash we trimmed, / Now traffic flows as we have hymned. Footnotes
|
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesKAS base path preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
comp:sdk
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the KAS client to preserve the base path of the KAS URL and trim trailing slashes, with corresponding updates and additions to the test suite. The review feedback recommends validating that the URL scheme is strictly 'http' or 'https' to avoid downstream errors with malformed URLs, and adding test cases to cover these invalid scheme scenarios.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Preserve the base path so a KAS served under one (e.g. https://host/kas) | ||
| // still routes; Connect appends "/procedure". Trailing slash trimmed to avoid "//". | ||
| path := strings.TrimSuffix(u.Path, "/") | ||
|
|
||
| return fmt.Sprintf("%s://%s%s", u.Scheme, addr, path), nil |
There was a problem hiding this comment.
If the input URL does not contain a valid scheme (e.g., example.com/kas), url.Parse may parse the host as part of the path, leaving u.Scheme empty. Returning a URL with an empty scheme (e.g., ://example.com/kas) will cause confusing errors downstream.
We should validate that the scheme is either http or https before constructing the base URL.
if u.Scheme != "http" && u.Scheme != "https" {
return "", fmt.Errorf("invalid KAS URL scheme %q, expected http or https", u.Scheme)
}
// Preserve the base path so a KAS served under one (e.g. https://host/kas)
// still routes; Connect appends "/procedure". Trailing slash trimmed to avoid "//".
path := strings.TrimSuffix(u.Path, "/")
return fmt.Sprintf("%s://%s%s", u.Scheme, addr, path), nil| { | ||
| name: "Trailing slash is trimmed to avoid doubled path separator", | ||
| input: "https://example.com/dsp/", | ||
| expected: "https://example.com/dsp", | ||
| expectError: false, | ||
| }, |
There was a problem hiding this comment.
To ensure the new scheme validation works correctly, we should add test cases for missing and invalid schemes.
{
name: "Trailing slash is trimmed to avoid doubled path separator",
input: "https://example.com/dsp/",
expected: "https://example.com/dsp",
expectError: false,
},
{
name: "Missing scheme returns error",
input: "example.com/dsp",
expected: "",
expectError: true,
},
{
name: "Invalid scheme returns error",
input: "ftp://example.com/dsp",
expected: "",
expectError: true,
},![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sdk/kas_client.go`:
- Around line 326-330: Refactor parseBaseURL to sanitize the parsed url.URL
directly: clear User, RawQuery, and Fragment, trim the trailing slash from Path,
reset RawPath, and return u.String(). Remove the manual Hostname/Port
reconstruction so IPv6 brackets, ports, path escaping, and the preserved base
path are handled by the URL implementation.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38ac7938-3428-4587-8a75-6742ce9b1d56
📒 Files selected for processing (2)
sdk/kas_client.gosdk/kas_client_test.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
1 participant
Proposed Changes
parseBaseURLinsdk/kas_client.goreduced a KAS URL toscheme://host[:port], discarding the URL path. The result is passed tokasconnect.NewAccessServiceClientas the Connect base URL, and Connect builds each request asbaseURL + "/" + procedure. For a KAS served under a base path (e.g.https://host/kas), dropping the path sendsRewraptohttps://host/kas.AccessService/Rewrapinstead ofhttps://host/kas/kas.AccessService/Rewrap, so the request never reaches the service.u.Path(trailing slash trimmed to avoid a doubled//) when constructingthe base URL
TestParseBaseUrlcases that asserted the path was stripped, and add coveragefor base-path and trailing-slash inputs
Background
parseBaseURLbegan life asgetGRPCAddress, which returned a gRPC dial target (host:port) where a path is meaningless — so stripping was correct. The Connect migration (#2200) renamed it and repurposed the return value as an HTTP base URL, but kept the host+port-only logic. A Connect base URL must retain the path, so the path-stripping became a regression. Host-root KAS deployments have no path to lose, which is why it went unnoticed.Symptom
Because the mis-routed request is rejected by an upstream proxy/LB with a plain HTTP
403,connect-gomaps it to thepermission_deniedcode — surfacing askao unwrap failed ... permission_denied: 403 Forbiddenduring decrypt, which looks like an access-policy denial rather than a routing error.Checklist
Testing Instructions
Summary by CodeRabbit