fix: do not fetch metadata for self-hosted Gitea/Forgejo#2788
Open
serhalp wants to merge 2 commits into
Open
Conversation
Repo URLs come from npm package metadata, so package publishers can specify any hostname. As this is effectively user-controlled input that can point at a malicious user-controlled server, this would put as at risk of Server-Side Request Forgery (SSRF). Thus we only support allowlisted hosts.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe pull request restricts Forgejo and Gitea provider detection to explicit allowlists ( ChangesHost allowlist and provider matching
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 `@shared/utils/git-providers.ts`:
- Around line 46-51: FORGEJO_HOSTS and GITEA_HOSTS are currently mutable arrays;
make them immutable to prevent runtime mutation widening the trusted hosts (and
update any derived exports like ALL_KNOWN_GIT_API_ORIGINS). Replace the mutable
exports with readonly constants (e.g., frozen arrays or TypeScript readonly
tuples/ReadonlyArray<string>) and ensure ALL_KNOWN_GIT_API_ORIGINS is produced
from those readonly values (not mutated later); update any consumers to accept
ReadonlyArray<string> if necessary and remove any in-place mutations of
FORGEJO_HOSTS or GITEA_HOSTS.
In `@test/unit/shared/utils/git-providers.spec.ts`:
- Around line 341-352: Add negative tests alongside the existing Gitea allowlist
spec to assert that lookalike/non-allowlisted hosts are rejected by
parseRepositoryInfo: add test cases (e.g., "rejects non-allowlisted Gitea-like
hosts" and "rejects Forgejo-like hosts") that call parseRepositoryInfo with URLs
such as 'https://gitea.com.evil/owner/repo' or
'https://forgejo.example/owner/repo' and assert it either returns undefined/null
or throws (match existing failure behavior), and verify no provider is set to
'gitea'/'forgejo' and no rawBaseUrl is produced; locate tests near the existing
Gitea block in test/unit/shared/utils/git-providers.spec.ts and mirror the
style/assertion pattern used for allowlisted success cases.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 310e3555-4dd3-4424-82c6-3fa9c5409590
📒 Files selected for processing (5)
shared/utils/fetch-cache-config.tsshared/utils/git-providers.tsshared/utils/repository-meta.tstest/nuxt/composables/use-repo-meta.spec.tstest/unit/shared/utils/git-providers.spec.ts
Comment on lines
+341
to
352
| describe('Gitea support', () => { | ||
| it('parses exact allowlisted Gitea hosts', () => { | ||
| const result = parseRepositoryInfo({ | ||
| url: 'git+ssh://git@forgejo.myserver.com/user/project.git', | ||
| url: 'https://gitea.com/owner/repo', | ||
| }) | ||
| expect(result).toMatchObject({ | ||
| provider: 'forgejo', | ||
| owner: 'user', | ||
| repo: 'project', | ||
| host: 'forgejo.myserver.com', | ||
| provider: 'gitea', | ||
| owner: 'owner', | ||
| repo: 'repo', | ||
| host: 'gitea.com', | ||
| rawBaseUrl: 'https://gitea.com/owner/repo/raw/branch/main', | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add explicit deny-path tests for non-allowlisted Gitea/Forgejo-like hosts.
This block only proves the allowlisted success path. Please also assert that lookalike non-allowlisted hosts are rejected, so the SSRF boundary is locked in by tests.
Proposed test addition
describe('Gitea support', () => {
it('parses exact allowlisted Gitea hosts', () => {
const result = parseRepositoryInfo({
url: 'https://gitea.com/owner/repo',
})
expect(result).toMatchObject({
provider: 'gitea',
owner: 'owner',
repo: 'repo',
host: 'gitea.com',
rawBaseUrl: 'https://gitea.com/owner/repo/raw/branch/main',
})
})
+
+ it('rejects non-allowlisted Gitea/Forgejo-like hosts', () => {
+ expect(
+ parseRepositoryInfo({ url: 'https://gitea.example.com/owner/repo' }),
+ ).toBeUndefined()
+ expect(
+ parseRepositoryInfo({ url: 'https://forgejo.example.com/owner/repo' }),
+ ).toBeUndefined()
+ })
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe('Gitea support', () => { | |
| it('parses exact allowlisted Gitea hosts', () => { | |
| const result = parseRepositoryInfo({ | |
| url: 'git+ssh://git@forgejo.myserver.com/user/project.git', | |
| url: 'https://gitea.com/owner/repo', | |
| }) | |
| expect(result).toMatchObject({ | |
| provider: 'forgejo', | |
| owner: 'user', | |
| repo: 'project', | |
| host: 'forgejo.myserver.com', | |
| provider: 'gitea', | |
| owner: 'owner', | |
| repo: 'repo', | |
| host: 'gitea.com', | |
| rawBaseUrl: 'https://gitea.com/owner/repo/raw/branch/main', | |
| }) | |
| describe('Gitea support', () => { | |
| it('parses exact allowlisted Gitea hosts', () => { | |
| const result = parseRepositoryInfo({ | |
| url: 'https://gitea.com/owner/repo', | |
| }) | |
| expect(result).toMatchObject({ | |
| provider: 'gitea', | |
| owner: 'owner', | |
| repo: 'repo', | |
| host: 'gitea.com', | |
| rawBaseUrl: 'https://gitea.com/owner/repo/raw/branch/main', | |
| }) | |
| }) | |
| it('rejects non-allowlisted Gitea/Forgejo-like hosts', () => { | |
| expect( | |
| parseRepositoryInfo({ url: 'https://gitea.example.com/owner/repo' }), | |
| ).toBeUndefined() | |
| expect( | |
| parseRepositoryInfo({ url: 'https://forgejo.example.com/owner/repo' }), | |
| ).toBeUndefined() | |
| }) | |
| }) |
🤖 Prompt for 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.
In `@test/unit/shared/utils/git-providers.spec.ts` around lines 341 - 352, Add
negative tests alongside the existing Gitea allowlist spec to assert that
lookalike/non-allowlisted hosts are rejected by parseRepositoryInfo: add test
cases (e.g., "rejects non-allowlisted Gitea-like hosts" and "rejects
Forgejo-like hosts") that call parseRepositoryInfo with URLs such as
'https://gitea.com.evil/owner/repo' or 'https://forgejo.example/owner/repo' and
assert it either returns undefined/null or throws (match existing failure
behavior), and verify no provider is set to 'gitea'/'forgejo' and no rawBaseUrl
is produced; locate tests near the existing Gitea block in
test/unit/shared/utils/git-providers.spec.ts and mirror the style/assertion
pattern used for allowlisted success cases.
Member
Author
There was a problem hiding this comment.
I thought about that but decided against it. It would be a very strange, porous heuristic test that speculates about what future patterns someone might implement. I think it would only provide false confidence.
Contributor
There was a problem hiding this comment.
@serhalp, that's a fair point. Negative tests on a handful of example hostnames wouldn't actually prove the allowlist is exhaustive — they'd just show those specific strings don't match today, which could give a false sense of security if the underlying logic ever drifted. The positive test on the allowlisted host combined with the explicit allowlist constants in the source is the right place to enforce the boundary. Happy to drop this suggestion.
(ᓀ‸ᓂ)
🧠 Learnings used
Learnt from: ghostdevv
Repo: npmx-dev/npmx.dev PR: 2195
File: test/nuxt/components/Header/MobileMenu.spec.ts:7-11
Timestamp: 2026-03-23T01:33:11.889Z
Learning: When using or mocking the `useConnector` composable in `npmx-dev/npmx.dev`, treat its public API as `isConnected` (not `connected`). Even though the internal `ConnectorState` uses a `connected` field, the composable’s returned object exposes `isConnected: computed(() => state.value.connected)`. Therefore, in tests/components, mocks and usages should reference `isConnected` and should not be flagged as incorrect for using `isConnected`.
Learnt from: serhalp
Repo: npmx-dev/npmx.dev PR: 2459
File: test/unit/server/utils/likes-leaderboard.spec.ts:137-165
Timestamp: 2026-04-26T00:02:20.876Z
Learning: In this Nuxt project (npmx-dev/npmx.dev), the `Packument` type is globally available via Nuxt auto-imports from `shared/types/` (exported from `shared/types/npm-registry.ts`). Therefore, do not raise or require missing `import type { Packument } from '`#shared/types`'` (or any equivalent) when `Packument` is referenced, including in unit test files.
ghostdevv
requested changes
May 24, 2026
| * is effectively user-controlled input that can point at a malicious user-controlled server, this | ||
| * would put us at risk of Server-Side Request Forgery (SSRF). Thus we only support allowlisted hosts. | ||
| */ | ||
| export const FORGEJO_HOSTS = ['next.forgejo.org', 'try.next.forgejo.org'] |
Contributor
There was a problem hiding this comment.
codeberg? or is that done somewhere else?
Member
Author
There was a problem hiding this comment.
codeberg is handled as a separate provider
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
N/A
🧭 Context
Repo URLs come from npm package metadata, so package publishers can specify any hostname. As this is effectively user-controlled input that can point at a malicious user-controlled server, this would put us at risk of Server-Side Request Forgery (SSRF). The scope of what SSRF here could accomplish is quite limited, but it would be better to nip this in the bud now in case of future changes or exploit chaining.
📚 Description
Only support allowlisted hosts.