Validate repository URL paths#559
Conversation
Greptile SummaryThis PR improves Confidence Score: 4/5Not safe to merge as-is: an existing test will fail due to the changed error message for malformed inputs. One P1 finding — the reordering breaks the pre-existing 'should reject malformed URLs' test assertion. The two P2 findings (duplicated domain list, incomplete SSH support) are non-blocking improvements. packages/billing/src/org-billing.ts — the normalizeRepositoryUrl-first reordering changes the error path for inputs that don't contain a recognizable host.
|
| Filename | Overview |
|---|---|
| packages/billing/src/org-billing.ts | Reorders validateAndNormalizeRepositoryUrl to normalize before parsing, and adds owner/repo path-segment check — but breaks the existing 'malformed URL' test and misses SSH normalization for GitLab/Bitbucket. |
| packages/billing/src/tests/org-billing.test.ts | Adds two new test cases for SSH URL normalization and missing owner/repo path rejection; existing malformed-URL test is not updated to match the new error message. |
Comments Outside Diff (2)
-
packages/billing/src/org-billing.ts, line 500-523 (link)Existing test broken by reordering
The pre-existing test
'should reject malformed URLs'passes'not-a-url'and expectserror === 'Repository domain not allowed'. With the old code,new URL('https://not-a-url')succeeded and the hostname failed the allowlist check. With the new code,normalizeRepositoryUrl('not-a-url')returns'not-a-url'unchanged (the function only prependshttps://for URLs that containgithub.com), thennew URL('not-a-url')throws, so the catch block returns'Invalid URL format'instead — failing that assertion.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/billing/src/org-billing.ts Line: 500-523 Comment: **Existing test broken by reordering** The pre-existing test `'should reject malformed URLs'` passes `'not-a-url'` and expects `error === 'Repository domain not allowed'`. With the old code, `new URL('https://not-a-url')` succeeded and the hostname failed the allowlist check. With the new code, `normalizeRepositoryUrl('not-a-url')` returns `'not-a-url'` unchanged (the function only prepends `https://` for URLs that contain `github.com`), then `new URL('not-a-url')` throws, so the catch block returns `'Invalid URL format'` instead — failing that assertion. How can I resolve this? If you propose a fix, please make it concise.
-
packages/billing/src/org-billing.ts, line 461-463 (link)SSH normalization is GitHub-only
normalizeRepositoryUrlconverts GitHub SSH URLs but not the equivalent GitLab (gitlab.com) or Bitbucket (bitbucket.org) SSH forms, even though both appear inallowedDomains. A GitLab SSH URL would fall through tonew URL(...)unparsed, throw, and return'Invalid URL format'. The new SSH test only exercises the GitHub path, so the gap is undetected. The SSH conversion block should be extended (or generalized via a regex) to cover all three allowed hosts.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/billing/src/org-billing.ts Line: 461-463 Comment: **SSH normalization is GitHub-only** `normalizeRepositoryUrl` converts GitHub SSH URLs but not the equivalent GitLab (`gitlab.com`) or Bitbucket (`bitbucket.org`) SSH forms, even though both appear in `allowedDomains`. A GitLab SSH URL would fall through to `new URL(...)` unparsed, throw, and return `'Invalid URL format'`. The new SSH test only exercises the GitHub path, so the gap is undetected. The SSH conversion block should be extended (or generalized via a regex) to cover all three allowed hosts. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/billing/src/org-billing.ts
Line: 500-523
Comment:
**Existing test broken by reordering**
The pre-existing test `'should reject malformed URLs'` passes `'not-a-url'` and expects `error === 'Repository domain not allowed'`. With the old code, `new URL('https://not-a-url')` succeeded and the hostname failed the allowlist check. With the new code, `normalizeRepositoryUrl('not-a-url')` returns `'not-a-url'` unchanged (the function only prepends `https://` for URLs that contain `github.com`), then `new URL('not-a-url')` throws, so the catch block returns `'Invalid URL format'` instead — failing that assertion.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/billing/src/org-billing.ts
Line: 504-508
Comment:
**Duplicated domain list — simplification opportunity**
`allowedDomains` here and `knownHosts` inside `normalizeRepositoryUrl` both hard-code the identical three values (`github.com`, `gitlab.com`, `bitbucket.org`). Extracting a single shared constant (e.g. `ALLOWED_REPO_HOSTS`) would eliminate the duplication and make future additions change-once.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/billing/src/org-billing.ts
Line: 461-463
Comment:
**SSH normalization is GitHub-only**
`normalizeRepositoryUrl` converts GitHub SSH URLs but not the equivalent GitLab (`gitlab.com`) or Bitbucket (`bitbucket.org`) SSH forms, even though both appear in `allowedDomains`. A GitLab SSH URL would fall through to `new URL(...)` unparsed, throw, and return `'Invalid URL format'`. The new SSH test only exercises the GitHub path, so the gap is undetected. The SSH conversion block should be extended (or generalized via a regex) to cover all three allowed hosts.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: validate repository URL paths" | Re-trigger Greptile
| // Whitelist allowed domains | ||
| const allowedDomains = ['github.com', 'gitlab.com', 'bitbucket.org'] | ||
| if (!allowedDomains.includes(urlObj.hostname)) { | ||
| return { isValid: false, error: 'Repository domain not allowed' } | ||
| } |
There was a problem hiding this comment.
Duplicated domain list — simplification opportunity
allowedDomains here and knownHosts inside normalizeRepositoryUrl both hard-code the identical three values (github.com, gitlab.com, bitbucket.org). Extracting a single shared constant (e.g. ALLOWED_REPO_HOSTS) would eliminate the duplication and make future additions change-once.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/billing/src/org-billing.ts
Line: 504-508
Comment:
**Duplicated domain list — simplification opportunity**
`allowedDomains` here and `knownHosts` inside `normalizeRepositoryUrl` both hard-code the identical three values (`github.com`, `gitlab.com`, `bitbucket.org`). Extracting a single shared constant (e.g. `ALLOWED_REPO_HOSTS`) would eliminate the duplication and make future additions change-once.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Validation