Skip to content

fix: normalize trailing slashes in serverURL#5

Closed
johnstcn wants to merge 2 commits into
mainfrom
fix/normalize-server-url
Closed

fix: normalize trailing slashes in serverURL#5
johnstcn wants to merge 2 commits into
mainfrom
fix/normalize-server-url

Conversation

@johnstcn
Copy link
Copy Markdown
Member

  • Strip trailing slashes from serverURL in the RealCoderClient constructor to prevent double-slash API URLs
  • Add test for trailing-slash URL handling

🤖 This PR was created with the help of Coder Agents, and will be reviewed by my human. 🧑‍💻

…I endpoints

The constructor now normalizes serverURL by trimming trailing slashes,
preventing the dreaded double-slash debacle (https://coder.test//api/...)
that makes URLs look like they've had one too many drinks.
@johnstcn johnstcn self-assigned this Mar 20, 2026
@johnstcn
Copy link
Copy Markdown
Member Author

ready for review

@johnstcn johnstcn marked this pull request as ready for review March 20, 2026 10:17
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Normalizes the Coder deployment base URL used by RealCoderClient to avoid generating double-slash API URLs when a user provides a trailing slash, and adds a regression test to cover this input case.

Changes:

  • Strip trailing slashes from serverURL in RealCoderClient’s constructor before building request URLs.
  • Add a unit test ensuring a trailing-slash serverURL still produces correct API URLs.
  • Regenerate dist/index.js to include the runtime change in the shipped GitHub Action bundle.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
src/coder-client.ts Normalizes serverURL by removing trailing slashes before constructing request URLs.
src/coder-client.test.ts Adds coverage for trailing-slash serverURL behavior when creating chats.
dist/index.js Updates the compiled action bundle to reflect the source change (and includes additional bundler-output changes).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mafredri
mafredri previously approved these changes Mar 20, 2026
Copy link
Copy Markdown
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Agent on behalf of mafredri

Zero concerns, LGTM. The /\/+$/ regex handles both single and multiple trailing slashes. Test correctly verifies the constructed URL has no double-slash.

Clean refactor from private readonly parameter property to explicit assignment in the constructor body. The behavioral change is one line.

One regex to strip them all, one test to find them. Slash-free since 2026.

@mafredri mafredri dismissed their stale review March 20, 2026 12:42

Dismissing erroneous APPROVE. Agent should have used COMMENT, not APPROVE.

Copy link
Copy Markdown
Member

Brought into main via #29 (with attribution). Closing.

🤖 This comment was created with the help of Coder Agents.

@mafredri mafredri closed this May 11, 2026
@mafredri mafredri deleted the fix/normalize-server-url branch May 11, 2026 18:44
mafredri added a commit that referenced this pull request May 12, 2026
…ning

Three pre-v0 hardening fixes originally proposed by @johnstcn in #5, #6,
and #7. Closes those in favor of this single follow-up.

* `RealCoderClient` constructor normalizes trailing slashes on the
  `coder-url` input so concatenated API URLs cannot double-slash.
  `normalizeBaseUrl` extracted to `src/url.ts` (no module cycle).
* `githubToken` schema rejects empty strings (`.min(1)`), matching
  `coderToken`.
* `getCoderUserByGitHubId` calls `request<unknown>` (was `<unknown[]>`)
  to match the `{ users: [...] }` response shape it parses.
* `existing-chat-id` is now `ChatIdSchema.parse`-ed at runtime instead
  of a `as ChatId` cast.
* `GITHUB_URL_REGEX` anchored at the tail so URLs with extra path
  segments are rejected; query strings and fragments still accepted.
* Drop the redundant `process.exit(1)` after `core.setFailed(...)`.

🤖 Authored by Coder Agents.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants