Skip to content

fix: Reject repository URLs with embedded credentials [VER-533] - #36

Merged
mgunnin merged 3 commits into
stagingfrom
fix/repo-url-credentials
Aug 2, 2026
Merged

fix: Reject repository URLs with embedded credentials [VER-533]#36
mgunnin merged 3 commits into
stagingfrom
fix/repo-url-credentials

Conversation

@mgunnin

@mgunnin mgunnin commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Repository URL validation landed in #35 only enforces the http(s) scheme, so a credential-bearing URL such as https://token@github.com/org/repo is accepted, persisted, and then rendered verbatim as the project's external link, exposing the embedded secret to anyone who can view the project. This extends the shared client predicate and the authoritative server schema to reject userinfo:

export function isHttpUrl(value: string): boolean {
  try {
    const url = new URL(value);
    const isHttpProtocol = url.protocol === "http:" || url.protocol === "https:";
    return isHttpProtocol && url.username === "" && url.password === "";
  } catch {
    return false;
  }
}

server/src/routes/projects.ts applies the same check in its HttpUrl refinement, so create and update both return 400 instead of persisting. Because ProjectDetail already gates the link on isHttpUrl, any credential URL stored before this change stops rendering as a clickable link too.

Existing URLs without userinfo, including ports, paths, query strings, and fragments, are unaffected. Server and form tests are extended to cover the credential case.

Follow-up to a Qodo security finding on #35, which merged before the fix could land on that branch.


View with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is enabled.

Closes VER-533

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
eidolon Ready Ready Preview Jul 31, 2026 7:46pm

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1aaafe01-c010-4759-a8f7-f0724d5d1458

📥 Commits

Reviewing files that changed from the base of the PR and between fe1fccc and a6c2fce.

📒 Files selected for processing (5)
  • server/src/__tests__/projects.test.ts
  • server/src/routes/projects.ts
  • ui/src/components/projects/ProjectFormModal.tsx
  • ui/src/lib/urls.ts
  • ui/test/CreateProjectModal.test.tsx

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

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Reject repository URLs with embedded credentials

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Reject http(s) repository URLs that include userinfo (username/password) to prevent secret
 leakage.
• Enforce the same rule in both UI validation and server-side request schemas.
• Extend UI and API tests to cover credential-bearing URLs and ensure they are not persisted.
Diagram

graph TD
  U(["User"]) --> UI["Project form"] --> V["isHttpUrl()"] --> API["Projects API"] --> S["HttpUrl schema"] --> DB[("Projects DB")]
  V -->|"invalid userinfo"| ERR["Validation error"]
  S -->|"invalid userinfo"| ERR
  PD["ProjectDetail"] -->|"gate link"| V

  subgraph Legend
    direction LR
    _user(["User"]) ~~~ _comp["Component"] ~~~ _db[("Database")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Strip userinfo on ingestion (normalize URL)
  • ➕ Allows users who pasted credential URLs to still save a project
  • ➕ Avoids storing secrets even if user pastes them
  • ➖ Silent mutation can be surprising and hard to debug
  • ➖ Still requires careful normalization rules and messaging
2. Mask userinfo on render only
  • ➕ Prevents accidental UI exposure without breaking existing stored values
  • ➕ Low friction for users
  • ➖ Still stores secrets at rest and may leak via logs/exports/other APIs
  • ➖ Does not stop further propagation of the secret
3. Allow userinfo but store it separately/securely
  • ➕ Supports authenticated repository URLs as a feature
  • ➖ Significantly expands scope: secret management, encryption, access controls, auditing
  • ➖ Higher security and operational risk than needed for a repo link field

Recommendation: Keep the PR’s approach: reject userinfo at both the UI and server schema layers. It is the lowest-risk, defense-in-depth fix that prevents secret storage and exposure, while keeping semantics clear (invalid input is rejected rather than silently rewritten).

Files changed (5) +29 / -13

Bug fix (3) +18 / -6
projects.tsTighten server HttpUrl schema to reject userinfo +6/-3

Tighten server HttpUrl schema to reject userinfo

• Updates the server-side Zod refinement to validate http(s) scheme and also ensure username/password are empty. Improves the validation error message and documents the secret-leak rationale.

server/src/routes/projects.ts

ProjectFormModal.tsxUpdate project form validation message for credential URLs +4/-1

Update project form validation message for credential URLs

• Keeps using the shared isHttpUrl predicate but updates the refine error message to explicitly disallow embedded credentials and provide a safe example URL.

ui/src/components/projects/ProjectFormModal.tsx

urls.tsReject http(s) URLs with embedded credentials in isHttpUrl +8/-2

Reject http(s) URLs with embedded credentials in isHttpUrl

• Enhances isHttpUrl to require http(s) and reject URLs with non-empty username/password (userinfo). Adds documentation explaining why userinfo is disallowed (prevent secret leakage).

ui/src/lib/urls.ts

Tests (2) +11 / -7
projects.test.tsAdd credential-bearing repo URL to API rejection coverage +5/-1

Add credential-bearing repo URL to API rejection coverage

• Extends the existing invalid repository URL test cases to include an http(s) URL with embedded credentials. Verifies the API rejects the request and does not persist a project for this case.

server/src/tests/projects.test.ts

CreateProjectModal.test.tsxParameterize UI test for executable schemes and embedded credentials +6/-6

Parameterize UI test for executable schemes and embedded credentials

• Converts the repository URL validation test into a table-driven test covering both javascript: schemes and credential-bearing https URLs. Asserts the new validation message and ensures the createProject mutation is not invoked.

ui/test/CreateProjectModal.test.tsx

@qodo-code-review

qodo-code-review Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 2 rules

Grey Divider


Remediation recommended

1. Empty userinfo bypass ✓ Resolved 🐞 Bug ⛨ Security
Description
The new HttpUrl refinement (and shared isHttpUrl) only checks that URL.username and URL.password are
empty, so URLs with an explicit but empty userinfo section (e.g. https://@host/... or
https://:@host/...) can still pass despite containing an @ authority delimiter. This doesn’t
leak a secret by itself, but it violates the intended “no embedded credentials/userinfo” policy and
can persist/render surprising URLs.
Code

server/src/routes/projects.ts[R14-16]

+    const url = new URL(value);
+    const isHttpProtocol = url.protocol === 'http:' || url.protocol === 'https:';
+    return isHttpProtocol && url.username === '' && url.password === '';
Relevance

●●● Strong

Repo accepts subtle validation hardening to close edge-case bypasses (e.g., dynamic predicate fixes
accepted in PR #1).

PR-#1
PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The server predicate returns true for any http(s) URL where url.username and url.password are
empty, but it does not independently reject the presence of a userinfo delimiter (@) in the
authority. The same logic is shared on the client and is used by ProjectDetail to decide whether to
render a clickable external link.

server/src/routes/projects.ts[10-20]
ui/src/lib/urls.ts[1-14]
ui/src/pages/ProjectDetail.tsx[81-121]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`HttpUrl` / `isHttpUrl` treat “userinfo present but empty” as valid because they only validate that `url.username` and `url.password` are empty strings. This allows URLs like `https://@github.com/org/repo` to pass while still embedding an `@` delimiter in the authority.

### Issue Context
The PR intent is to reject any embedded credentials/userinfo to prevent persisting and rendering sensitive URLs. Empty-userinfo inputs are edge cases that can still be accepted by the current predicate.

### Fix Focus Areas
- server/src/routes/projects.ts[10-20]
- ui/src/lib/urls.ts[1-14]

### Suggested fix
- Extend the predicate to also reject an explicit empty-userinfo delimiter, e.g.:
 - After parsing, reject when the serialized URL begins with `"http://@"`, `"https://@"`, `"http://:@"`, or `"https://:@"`.
 - Alternatively, compute the authority portion and ensure it does not start with `"@"` (empty userinfo) and does not contain userinfo at all.
- Add tests covering `https://@github.com/org/repo` (and optionally `https://:@github.com/org/repo`) for both server create/update validation and UI form validation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. UI test misses :@ case ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New
Description
The new CreateProjectModal parameterized test adds coverage for https://@github.com/org/repo but
not https://:@github.com/org/repo. This leaves the client-side isHttpUrl behavior for the
empty-password userinfo form unverified by tests.
Code

ui/test/CreateProjectModal.test.tsx[79]

+    ["an empty userinfo section", "https://@github.com/org/repo"],
Relevance

●● Moderate

No direct precedent for this exact URL edge-case; team often accepts test-hardening/extra assertions
(e.g., PR #8).

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
isHttpUrl explicitly documents and implements handling for both https://@host and
https://:@host via raw authority inspection, but the updated UI test table only adds coverage for
the https://@host form.

ui/src/lib/urls.ts[10-13]
ui/test/CreateProjectModal.test.tsx[76-80]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CreateProjectModal.test.tsx` adds a new test row for an empty userinfo section (`https://@...`) but does not add the closely-related `https://:@...` case that the validator explicitly targets.

### Issue Context
`isHttpUrl` includes a raw-authority `@` check specifically to catch both `https://@host` and `https://:@host` because `URL` parsing can normalize empty userinfo away.

### Fix Focus Areas
- ui/test/CreateProjectModal.test.tsx[76-80]

### Suggested change
Add a fourth `it.each` row, e.g. `["an empty password userinfo", "https://:@github.com/org/repo"]`, and keep the same expected validation error message.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. PATCH credentials untested ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The server tests were extended to assert POST/create rejects a credential-bearing repo URL, but
there is no corresponding PATCH/update test for the same credential case. This leaves the update
path vulnerable to future regressions where updates could re-accept credential URLs without the test
suite catching it.
Code

server/src/tests/projects.test.ts[R48-55]

+    for (const repoUrl of [
+      'not-a-url',
+      'javascript:alert(document.domain)',
+      'https://token@github.com/org/repo',
+    ]) {
      await request(app)
        .post(`/api/companies/${companyId}/projects`)
        .send({ name: 'Invalid repository', repoUrl })
Relevance

●●● Strong

Team frequently accepts adding/hardening regression tests (e.g., extra persistence assertions in
tests in PR #8).

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Both create and update schemas use HttpUrl, but only the create-path invalid URL test was updated
to include a credential-bearing URL, leaving update behavior unprotected by a targeted regression
test.

server/src/routes/projects.ts[22-34]
server/src/tests/projects.test.ts[47-63]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Credential-bearing URL rejection is only regression-tested for project creation, not for project updates.

### Issue Context
`UpdateProjectBody` uses the same `HttpUrl` refinement as `CreateProjectBody`, but only the create invalid-url loop includes the credential case.

### Fix Focus Areas
- server/src/__tests__/projects.test.ts[47-136]
- server/src/routes/projects.ts[22-34]

### Suggested fix
- Add a test similar to the existing update-invalid-url test that PATCHes `repoUrl: 'https://token@github.com/org/repo'` and asserts:
 - HTTP 400
 - subsequent GET shows `repoUrl` unchanged (or still null).
- (Optional) Mirror this on the UI edit flow tests to ensure the shared form rejects credential URLs during edits too.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Previous review results

Review updated until commit a6c2fce

Results up to commit 887adb9 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Empty userinfo bypass ✓ Resolved 🐞 Bug ⛨ Security
Description
The new HttpUrl refinement (and shared isHttpUrl) only checks that URL.username and URL.password are
empty, so URLs with an explicit but empty userinfo section (e.g. https://@host/... or
https://:@host/...) can still pass despite containing an @ authority delimiter. This doesn’t
leak a secret by itself, but it violates the intended “no embedded credentials/userinfo” policy and
can persist/render surprising URLs.
Code

server/src/routes/projects.ts[R14-16]

+    const url = new URL(value);
+    const isHttpProtocol = url.protocol === 'http:' || url.protocol === 'https:';
+    return isHttpProtocol && url.username === '' && url.password === '';
Relevance

●●● Strong

Repo accepts subtle validation hardening to close edge-case bypasses (e.g., dynamic predicate fixes
accepted in PR #1).

PR-#1
PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The server predicate returns true for any http(s) URL where url.username and url.password are
empty, but it does not independently reject the presence of a userinfo delimiter (@) in the
authority. The same logic is shared on the client and is used by ProjectDetail to decide whether to
render a clickable external link.

server/src/routes/projects.ts[10-20]
ui/src/lib/urls.ts[1-14]
ui/src/pages/ProjectDetail.tsx[81-121]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`HttpUrl` / `isHttpUrl` treat “userinfo present but empty” as valid because they only validate that `url.username` and `url.password` are empty strings. This allows URLs like `https://@github.com/org/repo` to pass while still embedding an `@` delimiter in the authority.

### Issue Context
The PR intent is to reject any embedded credentials/userinfo to prevent persisting and rendering sensitive URLs. Empty-userinfo inputs are edge cases that can still be accepted by the current predicate.

### Fix Focus Areas
- server/src/routes/projects.ts[10-20]
- ui/src/lib/urls.ts[1-14]

### Suggested fix
- Extend the predicate to also reject an explicit empty-userinfo delimiter, e.g.:
 - After parsing, reject when the serialized URL begins with `"http://@"`, `"https://@"`, `"http://:@"`, or `"https://:@"`.
 - Alternatively, compute the authority portion and ensure it does not start with `"@"` (empty userinfo) and does not contain userinfo at all.
- Add tests covering `https://@github.com/org/repo` (and optionally `https://:@github.com/org/repo`) for both server create/update validation and UI form validation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational
2. PATCH credentials untested ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The server tests were extended to assert POST/create rejects a credential-bearing repo URL, but
there is no corresponding PATCH/update test for the same credential case. This leaves the update
path vulnerable to future regressions where updates could re-accept credential URLs without the test
suite catching it.
Code

server/src/tests/projects.test.ts[R48-55]

+    for (const repoUrl of [
+      'not-a-url',
+      'javascript:alert(document.domain)',
+      'https://token@github.com/org/repo',
+    ]) {
      await request(app)
        .post(`/api/companies/${companyId}/projects`)
        .send({ name: 'Invalid repository', repoUrl })
Relevance

●●● Strong

Team frequently accepts adding/hardening regression tests (e.g., extra persistence assertions in
tests in PR #8).

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Both create and update schemas use HttpUrl, but only the create-path invalid URL test was updated
to include a credential-bearing URL, leaving update behavior unprotected by a targeted regression
test.

server/src/routes/projects.ts[22-34]
server/src/tests/projects.test.ts[47-63]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Credential-bearing URL rejection is only regression-tested for project creation, not for project updates.

### Issue Context
`UpdateProjectBody` uses the same `HttpUrl` refinement as `CreateProjectBody`, but only the create invalid-url loop includes the credential case.

### Fix Focus Areas
- server/src/__tests__/projects.test.ts[47-136]
- server/src/routes/projects.ts[22-34]

### Suggested fix
- Add a test similar to the existing update-invalid-url test that PATCHes `repoUrl: 'https://token@github.com/org/repo'` and asserts:
 - HTTP 400
 - subsequent GET shows `repoUrl` unchanged (or still null).
- (Optional) Mirror this on the UI edit flow tests to ensure the shared form rejects credential URLs during edits too.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread server/src/routes/projects.ts Outdated
Comment thread server/src/__tests__/projects.test.ts
@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo Fixer

No findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page.

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Comment thread ui/test/CreateProjectModal.test.tsx
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 77c16f1

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit a6c2fce

@mgunnin mgunnin changed the title Reject repository URLs with embedded credentials fix: Reject repository URLs with embedded credentials [VER-533] Aug 1, 2026
@mgunnin
mgunnin merged commit 7e6aa82 into staging Aug 2, 2026
7 checks passed
@mgunnin
mgunnin deleted the fix/repo-url-credentials branch August 2, 2026 19:40
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.

1 participant