fix(grafana): sanitize trailing slash on baseUrl in makeGrafanaRawRequest by minutechreview · Pull Request #604 · corsairdev/corsair

minutechreview · 2026-08-04T17:39:08Z

Description

makeGrafanaRawRequest built its request URL with naive string concatenation (${baseUrl}${endpoint}), so a Grafana instance URL configured with a trailing slash (e.g. https://grafana.example.com/) produced a double-slash request path. This fixes the same class of bug already fixed for Jira's cloudUrl in #503.

While fixing this, CodeRabbit's review on this PR also flagged that a non-HTTPS baseUrl would send the Grafana bearer token over cleartext HTTP. Since this PR was already touching the URL-building logic in that function, I fixed that too: makeGrafanaRawRequest now throws a GrafanaAPIError before calling fetch if the sanitized baseUrl isn't https://. Updated the function's JSDoc to document both behaviors.

I deliberately left CodeRabbit's other suggestion (an allowlist of "approved Grafana origins") out of scope — baseUrl is documented as pointing at a user's self-hosted Grafana instance, so there's no fixed origin to allowlist against without a larger product decision. Left a comment on the review thread explaining that.

Fixes #599

Checklist

I have run pnpm lint and all checks pass (3 pre-existing warnings elsewhere in the repo, unrelated to this change and present on main)
I have run pnpm typecheck and there are no TypeScript errors
I have run pnpm build and @corsair-dev/grafana builds successfully
I have run pnpm --filter @corsair-dev/grafana test and the unit tests I added pass (packages/grafana/api.test.ts is a pre-existing live-integration suite gated on real GRAFANA_URL/BEARER_TOKEN credentials that aren't available in this environment — it fails identically on main without them, confirmed by checking out the pre-fix file and re-running)
I have added or updated tests where applicable
I have added or updated necessary documentation (JSDoc on makeGrafanaRawRequest)

Screenshots / Demos

Pure request-building fix, no UI. Passing CI run for this branch (build + full test suite, all packages): https://github.com/corsairdev/corsair/actions/runs/30936881333/job/92085139297

Local test run demonstrating the fixed behavior end-to-end (mocked fetch, real makeGrafanaRawRequest call, each assertion checks the actual URL fetch was called with, or that it wasn't called at all for the rejected case):

$ pnpm --filter @corsair-dev/grafana test -- client.test.ts

PASS ./client.test.ts
  makeGrafanaRawRequest
    ✓ strips a trailing slash from baseUrl before building the request path (1 ms)
    ✓ leaves a baseUrl without a trailing slash unaffected
    ✓ refuses to send the bearer token to a non-HTTPS baseUrl (4 ms)

Test Suites: 1 passed, 1 total
Tests:       3 passed, 3 total

Additional Notes

No breaking changes, no new dependencies. Scope confined to packages/grafana/client.ts and its test file.

Summary by CodeRabbit

Bug Fixes
- Improved Grafana request URL handling by removing duplicate trailing slashes.
- Requests to malformed or non-HTTPS Grafana endpoints are now rejected before any network request is sent.
- Secure endpoint validation now correctly accepts HTTPS URLs regardless of letter casing.
- Added clearer error handling when Grafana endpoint validation fails.

…uest makeGrafanaRawRequest built its request URL by naive string concatenation, so a Grafana instance URL configured with a trailing slash (e.g. https://grafana.example.com/) produced a double-slash request path. Strip the trailing slash before concatenating, matching the sanitization already applied to the JSON path via the shared request builder. Fixes corsairdev#599

vercel · 2026-08-04T17:39:15Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
www	Skipped		Aug 5, 2026 11:46am

coderabbitai · 2026-08-04T17:40:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e45b3124-9c57-4c3e-96f6-a95e3b204f2c

📥 Commits

Reviewing files that changed from the base of the PR and between 43d14b5 and bc8a72d.

📒 Files selected for processing (2)

packages/grafana/client.test.ts
packages/grafana/client.ts

🚧 Files skipped from review as they are similar to previous changes (2)

packages/grafana/client.ts
packages/grafana/client.test.ts

📝 Walkthrough

Walkthrough

makeGrafanaRawRequest trims trailing slashes from baseUrl, requires HTTPS, and validates URLs before calling fetch. Tests cover normalized URLs, uppercase HTTPS, malformed URLs, and rejected requests.

Changes

Grafana URL sanitization

Layer / File(s)	Summary
Sanitize raw request URLs `packages/grafana/client.ts`, `packages/grafana/client.test.ts`	`makeGrafanaRawRequest` trims trailing slashes and rejects malformed or non-HTTPS base URLs. Tests verify URL construction, uppercase HTTPS acceptance, and rejection before `fetch`.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Issue 599 — Both changes update makeGrafanaRawRequest to strip trailing slashes from baseUrl and test this behavior.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the trailing-slash sanitization in makeGrafanaRawRequest, which is a primary part of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-08-04T17:41:15Z

Greptile Summary

The PR normalizes trailing slashes in Grafana raw-request base URLs and adds focused regression coverage.

Removes one or more trailing slashes before concatenating the raw endpoint.
Tests base URLs both with and without a trailing slash.

Confidence Score: 4/5

The PR appears safe to merge; the only concern is non-blocking test-mock type-safety documentation.

The URL normalization works for all current callers, which consistently supply leading-slash endpoints, while the new tests contain a maintainability issue in their broad mock assertions.

Files Needing Attention: packages/grafana/client.test.ts

Important Files Changed

Filename	Overview
packages/grafana/client.ts	Correctly normalizes trailing base URL slashes while preserving every current leading-slash endpoint call.
packages/grafana/client.test.ts	Covers both URL forms, but the fetch mocks use undocumented broad `unknown` typing and assertions.

_{Reviews (1): Last reviewed commit: "fix(grafana): sanitize trailing slash on..." | Re-trigger Greptile}

coderabbitai

Actionable comments posted: 2

🤖 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 `@packages/grafana/client.ts`:
- Around line 94-95: Validate the destination in makeGrafanaRawRequest before
attaching or sending ctx.key: parse baseUrl and require HTTPS plus an approved
Grafana origin, rejecting arbitrary base_url_override values. Preserve the
existing slash-trimming and endpoint concatenation for current slash-prefixed
callers.
- Around line 94-95: Validate the resolved Grafana base URL before fetch in the
client request flow, rejecting any non-https: URL, including values supplied
through base_url_override. Ensure the validation occurs before constructing or
sending the credential-bearing request and preserves HTTPS URL handling.

🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd81db8-6ed2-45de-b09a-a510e23f1f26

📥 Commits

Reviewing files that changed from the base of the PR and between 839b807 and 7a47795.

📒 Files selected for processing (2)

packages/grafana/client.test.ts
packages/grafana/client.ts

Sending the Grafana bearer token to a non-HTTPS baseUrl would transmit it in cleartext. Reject any baseUrl that isn't https:// before building the request, matching CodeRabbit's review on corsairdev#604. Addressed in review on corsairdev#599

minutechreview · 2026-08-04T17:55:55Z

Thanks for the review.

HTTPS enforcement (quick win): fixed in 63d5a32 — makeGrafanaRawRequest now throws a GrafanaAPIError before calling fetch if the sanitized baseUrl isn't https://, so the bearer token can no longer be sent in cleartext. Added a regression test.
Approved-origin allowlist: leaving this out of scope for this PR. baseUrl/base_url_override is documented and expected to point at a user's self-hosted Grafana instance (see the JSDoc on makeGrafanaRequest), so there's no fixed "approved Grafana origin" to allowlist against here — any origin can be legitimate. Restricting that would need a product decision (e.g. an explicit per-workspace domain allowlist configured by the user), which is a bigger change than issue fix(grafana): makeGrafanaRawRequest doesn't sanitize trailing slash on instance baseUrl #599 asked for. Happy to open a follow-up issue for that if maintainers agree it's worth pursuing.

coderabbitai

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 `@packages/grafana/client.ts`:
- Around line 95-99: Update the HTTPS validation in the Grafana client URL
handling to parse baseUrl with URL, accept any casing of the HTTPS scheme by
checking the normalized protocol equals https:, and map URL parsing failures to
GrafanaAPIError. Preserve rejection of non-HTTPS URLs and add coverage for a
mixed-case HTTPS scheme.

🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 261e03d1-68ab-46d9-a73c-5805e534bb27

📥 Commits

Reviewing files that changed from the base of the PR and between 7a47795 and 63d5a32.

📒 Files selected for processing (2)

packages/grafana/client.test.ts
packages/grafana/client.ts

vercel · 2026-08-04T18:04:22Z

@minutechreview is attempting to deploy a commit to the corsair Team on Vercel.

A member of the Team first needs to authorize it.

…eGrafanaRawRequest

Dhirenderchoudhary · 2026-08-04T20:42:01Z

LGTM trailing-slash strip + HTTPS reject before fetch looks right, tests cover it.

Optional : startsWith('https://') rejects HTTPS://…; new URL(...).protocol === 'https:' would handle that as per code rabbit reviews.

… check startsWith('https://') rejected valid but differently-cased schemes like HTTPS://. Parse with URL and compare protocol instead, per review feedback on corsairdev#604.

minutechreview · 2026-08-05T11:46:56Z

Good catch — fixed in bc8a72d: now parses baseUrl with URL and compares protocol === 'https:' instead of the case-sensitive prefix check, with a malformed-URL case also throwing a clean GrafanaAPIError instead of an uncaught TypeError. Added regression tests for both.

Dhirenderchoudhary · 2026-08-05T11:54:57Z

LGTM

vercel Bot temporarily deployed to Preview August 4, 2026 17:39 Inactive

github-actions Bot added the plugin Changes inside a plugin package label Aug 4, 2026

coderabbitai Bot reviewed Aug 4, 2026

View reviewed changes

Comment thread packages/grafana/client.ts

vercel Bot temporarily deployed to Preview August 4, 2026 17:55 Inactive

coderabbitai Bot reviewed Aug 4, 2026

View reviewed changes

Comment thread packages/grafana/client.ts Outdated

chore: retrigger Plugin PR Gate after filling out description/demo

e80ae78

docs(grafana): note the trailing-slash and HTTPS-only behavior on mak…

43d14b5

…eGrafanaRawRequest

vercel Bot temporarily deployed to Preview August 4, 2026 18:08 Inactive

chore: retrigger Plugin PR Gate against updated PR description

7ce20ba

vercel Bot temporarily deployed to Preview August 4, 2026 18:12 Inactive

fix(grafana): parse baseUrl scheme instead of a case-sensitive prefix…

bc8a72d

… check startsWith('https://') rejected valid but differently-cased schemes like HTTPS://. Parse with URL and compare protocol instead, per review feedback on corsairdev#604.

vercel Bot temporarily deployed to Preview August 5, 2026 11:46 Inactive

Navigation Menu

Uh oh!

fix(grafana): sanitize trailing slash on baseUrl in makeGrafanaRawRequest - #604

fix(grafana): sanitize trailing slash on baseUrl in makeGrafanaRawRequest#604
minutechreview wants to merge 6 commits into
corsairdev:mainfrom
minutechreview:fix/grafana-raw-request-trailing-slash

minutechreview commented Aug 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

vercel Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

greptile-apps Bot commented Aug 4, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

minutechreview commented Aug 4, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

vercel Bot commented Aug 4, 2026

Uh oh!

Dhirenderchoudhary commented Aug 4, 2026

Uh oh!

minutechreview commented Aug 5, 2026

Uh oh!

Dhirenderchoudhary commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

minutechreview commented Aug 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Screenshots / Demos

Additional Notes

Summary by CodeRabbit

Uh oh!

vercel Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

greptile-apps Bot commented Aug 4, 2026

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minutechreview commented Aug 4, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vercel Bot commented Aug 4, 2026

Uh oh!

Dhirenderchoudhary commented Aug 4, 2026

Uh oh!

minutechreview commented Aug 5, 2026

Uh oh!

Dhirenderchoudhary commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

minutechreview commented Aug 4, 2026 •

edited by coderabbitai Bot

Loading

vercel Bot commented Aug 4, 2026 •

edited

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading