mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-14 01:50:39 +09:00
Tighten publicBaseUrl port rewriting (#4553)
## Thinking Path > - Paperclip is a control plane for autonomous agent companies, so its local and authenticated deployment behavior has to stay predictable under port rebinding and worktree isolation. > - This change sits in the server/worktree configuration path that derives runtime URLs and auth origins from `auth.publicBaseUrl`. > - The original hostname-port rewrite change fixed one real gap for private/tailnet host:port worktree setups, but it widened the rewrite rule too far. > - Rewriting every explicit `auth.publicBaseUrl` can corrupt public or reverse-proxy URLs by turning a stable origin like `https://paperclip.example` into a local listen-port URL. > - Paperclip's auth and trusted-origin handling depend on that URL staying semantically correct, so this had to be narrowed before merge. > - This pull request tightens the rewrite rule to explicit-port URLs only and adds regression coverage across the CLI helper, worktree config persistence, and server startup path. > - The benefit is that private host:port worktree flows still work, while public/default-port URLs remain stable and safe. ## What Changed - Tightened `rewriteLocalUrlPort` in `cli/src/commands/worktree-lib.ts`, `server/src/worktree-config.ts`, and `server/src/index.ts` so it only rewrites URLs that already include an explicit port. - Removed the old loopback-only hostname gate from the CLI/worktree helpers and replaced it with the more precise `parsed.port` guard. - Updated CLI helper coverage to assert that explicit-port non-loopback URLs still rewrite while no-port public URLs stay unchanged. - Expanded `server/src/__tests__/worktree-config.test.ts` to cover explicit-port rewrite and no-port stability for both persisted worktree config and in-memory runtime port selection. - Added startup-path coverage in `server/src/__tests__/server-startup-feedback-export.test.ts` for `detect-port` rebinding with both explicit-port and no-port `auth.publicBaseUrl` values. ## Verification - `pnpm --filter @paperclipai/plugin-sdk build` - `npx vitest run server/src/__tests__/server-startup-feedback-export.test.ts` - `npx vitest run cli/src/__tests__/worktree.test.ts server/src/__tests__/worktree-config.test.ts` - All of the above were run locally in this issue worktree and passed. ## Risks - Low risk. The behavior change is deliberately narrower than the reviewed broad-host rewrite and is guarded by regression coverage for both the explicit-port and no-port cases. - The main remaining risk is behavioral only if another code path starts depending on port rewriting for URLs that never declared a port, which would be a separate bug. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex local agent using `gpt-5.4` with high reasoning effort, tool use, shell execution, and file editing. - Anthropic Claude local agent using `claude-opus-4-6` for follow-up code review approval on the implementation issue. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
parent
d47ffa87f0
commit
08af830430
6 changed files with 211 additions and 80 deletions
|
|
@ -190,8 +190,9 @@ describe("worktree helpers", () => {
|
|||
).toEqual(["worktree", "add", "-b", "my-worktree", "/tmp/my-worktree", "origin/main"]);
|
||||
});
|
||||
|
||||
it("rewrites loopback auth URLs to the new port only", () => {
|
||||
it("rewrites auth URLs only when they already include a port", () => {
|
||||
expect(rewriteLocalUrlPort("http://127.0.0.1:3100", 3110)).toBe("http://127.0.0.1:3110/");
|
||||
expect(rewriteLocalUrlPort("http://my-host.ts.net:3100", 3110)).toBe("http://my-host.ts.net:3110/");
|
||||
expect(rewriteLocalUrlPort("https://paperclip.example", 3110)).toBe("https://paperclip.example");
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -75,11 +75,6 @@ function nonEmpty(value: string | null | undefined): string | null {
|
|||
return typeof value === "string" && value.trim().length > 0 ? value.trim() : null;
|
||||
}
|
||||
|
||||
function isLoopbackHost(hostname: string): boolean {
|
||||
const value = hostname.trim().toLowerCase();
|
||||
return value === "127.0.0.1" || value === "localhost" || value === "::1";
|
||||
}
|
||||
|
||||
export function sanitizeWorktreeInstanceId(rawValue: string): string {
|
||||
const trimmed = rawValue.trim().toLowerCase();
|
||||
const normalized = trimmed
|
||||
|
|
@ -168,7 +163,8 @@ export function rewriteLocalUrlPort(rawUrl: string | undefined, port: number): s
|
|||
if (!rawUrl) return undefined;
|
||||
try {
|
||||
const parsed = new URL(rawUrl);
|
||||
if (!isLoopbackHost(parsed.hostname)) return rawUrl;
|
||||
// The URL API normalizes default ports like :80/:443 to "", so treat them as stable URLs.
|
||||
if (!parsed.port) return rawUrl;
|
||||
parsed.port = String(port);
|
||||
return parsed.toString();
|
||||
} catch {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue