diff --git a/.github/workflows/files-changed.yml b/.github/workflows/files-changed.yml index 3c0603974e..11ff4cec68 100644 --- a/.github/workflows/files-changed.yml +++ b/.github/workflows/files-changed.yml @@ -64,6 +64,11 @@ jobs: - ".golangci.yml" - ".editorconfig" - "options/locale/locale_en-US.json" + - "models/fixtures/**" + - "tests/*.ini.tmpl" + - "tests/gitea-repositories-meta/**" + - "tests/testdata/**" + - "tools/test-integration.sh" frontend: - "*.ts" diff --git a/modules/hostmatcher/hostmatcher.go b/modules/hostmatcher/hostmatcher.go index 044dba679a..7c17bc95da 100644 --- a/modules/hostmatcher/hostmatcher.go +++ b/modules/hostmatcher/hostmatcher.go @@ -8,6 +8,7 @@ import ( "path/filepath" "slices" "strings" + "sync" ) // HostMatchList is used to check if a host or IP is in a list. @@ -23,10 +24,64 @@ type HostMatchList struct { ipNets []*net.IPNet } -// MatchBuiltinExternal A valid non-private unicast IP, all hosts on public internet are matched +// MatchBuiltinExternal A valid global-unicast IP that is neither private (see MatchBuiltinPrivate) +// nor a reserved special-purpose range (see reservedIPNets); i.e. a routable host on the public internet. const MatchBuiltinExternal = "external" -// MatchBuiltinPrivate RFC 1918 (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) and RFC 4193 (FC00::/7). Also called LAN/Intranet. +// reservedIPNets are special-purpose ranges that net.IP.IsPrivate omits but that must not be +// treated as public/external destinations (CGNAT, cloud metadata, IPv6 transition, etc.). We layer +// these on top of net.IP.IsPrivate (RFC 1918 / RFC 4193) so future additions to Go's IsPrivate are +// picked up automatically, while still covering the ranges it leaves out; otherwise the default +// allow-list would let authenticated users reach cloud metadata, internal, and IPv6 transition +// endpoints (SSRF), and a "private" block-list would fail to catch them. +var reservedIPNets = sync.OnceValue(func() []*net.IPNet { + var nets []*net.IPNet + for _, cidr := range []string{ + // IPv4 + "100.64.0.0/10", // RFC 6598 Carrier-Grade NAT + "168.63.129.16/32", // Azure WireServer metadata endpoint + "192.0.0.0/24", // RFC 6890 IETF protocol assignments + "192.0.2.0/24", // RFC 5737 TEST-NET-1 + "192.88.99.0/24", // RFC 7526 6to4 relay anycast (deprecated) + "198.18.0.0/15", // RFC 2544 benchmarking + "198.51.100.0/24", // RFC 5737 TEST-NET-2 + "203.0.113.0/24", // RFC 5737 TEST-NET-3 + // IPv6 + "100::/64", // RFC 6666 discard-only + "64:ff9b::/96", // RFC 6052 NAT64 (can embed IPv4 such as 169.254.169.254) + "64:ff9b:1::/48", // RFC 8215 local-use NAT64 + "2001::/32", // RFC 4380 Teredo tunneling (embeds IPv4) + "2001:10::/28", // RFC 4843 ORCHID (deprecated) + "2001:20::/28", // RFC 7343 ORCHIDv2 + "2001:db8::/32", // RFC 3849 documentation + "2002::/16", // RFC 3056 6to4 (embeds IPv4) + } { + _, ipNet, err := net.ParseCIDR(cidr) + if err != nil { + panic("hostmatcher: invalid reserved CIDR " + cidr + ": " + err.Error()) + } + nets = append(nets, ipNet) + } + return nets +}) + +// isPrivateIP reports whether ip falls in a private (net.IP.IsPrivate) or reserved special-purpose +// range (see reservedIPNets) that must not be considered a public/external destination. +func isPrivateIP(ip net.IP) bool { + if ip.IsPrivate() { + return true + } + for _, ipNet := range reservedIPNets() { + if ipNet.Contains(ip) { + return true + } + } + return false +} + +// MatchBuiltinPrivate RFC 1918 (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) and RFC 4193 (FC00::/7), +// plus the reserved special-purpose ranges in reservedIPNets (CGNAT, NAT64, cloud metadata, etc.). +// Also called LAN/Intranet. const MatchBuiltinPrivate = "private" // MatchBuiltinLoopback 127.0.0.0/8 for IPv4 and ::1/128 for IPv6, localhost is included. @@ -100,11 +155,11 @@ func (hl *HostMatchList) checkIP(ip net.IP) bool { for _, builtin := range hl.builtins { switch builtin { case MatchBuiltinExternal: - if ip.IsGlobalUnicast() && !ip.IsPrivate() { + if ip.IsGlobalUnicast() && !isPrivateIP(ip) { return true } case MatchBuiltinPrivate: - if ip.IsPrivate() { + if isPrivateIP(ip) { return true } case MatchBuiltinLoopback: diff --git a/modules/hostmatcher/hostmatcher_test.go b/modules/hostmatcher/hostmatcher_test.go index c781847471..61582f28d3 100644 --- a/modules/hostmatcher/hostmatcher_test.go +++ b/modules/hostmatcher/hostmatcher_test.go @@ -159,3 +159,58 @@ func TestHostOrIPMatchesList(t *testing.T) { } test(cases) } + +// TestReservedRanges ensures special-purpose ranges that net.IP.IsPrivate misses are kept out of the +// "external" allow-list (the default for webhook delivery and repository migrations) and folded into +// the "private" block-list, so they cannot be used for SSRF to metadata/internal endpoints. +func TestReservedRanges(t *testing.T) { + external := ParseHostMatchList("", "external") + private := ParseHostMatchList("", "private") + + // legitimate public destinations: external, not private + for _, ip := range []string{"8.8.8.8", "1.1.1.1", "2001:4860:4860::8888", "1000::1"} { + addr := net.ParseIP(ip) + assert.Truef(t, external.MatchIPAddr(addr), "public ip %s should be external", ip) + assert.Falsef(t, private.MatchIPAddr(addr), "public ip %s should not be private", ip) + } + + // RFC 1918 / RFC 4193 private ranges (now folded into privateIPNets instead of net.IP.IsPrivate): + // not external, blockable as private. Includes range edges to guard the CIDR boundaries. + for _, ip := range []string{ + "10.0.0.0", "10.255.255.255", // 10.0.0.0/8 + "172.16.0.0", "172.31.255.255", // 172.16.0.0/12 + "192.168.0.0", "192.168.255.255", // 192.168.0.0/16 + "fc00::", "fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", // fc00::/7 + } { + addr := net.ParseIP(ip) + assert.Falsef(t, external.MatchIPAddr(addr), "private ip %s must not be external", ip) + assert.Truef(t, private.MatchIPAddr(addr), "private ip %s should match private block-list", ip) + } + + // 172.32.0.0 is just outside 172.16.0.0/12: a public destination, not private + if addr := net.ParseIP("172.32.0.0"); assert.NotNil(t, addr) { + assert.True(t, external.MatchIPAddr(addr), "172.32.0.0 should be external") + assert.False(t, private.MatchIPAddr(addr), "172.32.0.0 should not be private") + } + + // reserved ranges that IsPrivate does not cover: not external, but blockable as private + for _, ip := range []string{ + "100.64.0.1", // CGNAT + "100.127.255.254", // CGNAT + "168.63.129.16", // Azure WireServer + "192.0.2.1", // TEST-NET-1 + "198.18.0.1", // benchmarking + "198.51.100.1", // TEST-NET-2 + "203.0.113.1", // TEST-NET-3 + "192.88.99.1", // 6to4 relay anycast + "64:ff9b::1", // NAT64 + "64:ff9b::a9fe:a9fe", // NAT64 embedding 169.254.169.254 + "2001::1", // Teredo + "2002::1", // 6to4 + "2001:db8::1", // documentation + } { + addr := net.ParseIP(ip) + assert.Falsef(t, external.MatchIPAddr(addr), "reserved ip %s must not be external", ip) + assert.Truef(t, private.MatchIPAddr(addr), "reserved ip %s should match private block-list", ip) + } +} diff --git a/modules/packages/rubygems/metadata.go b/modules/packages/rubygems/metadata.go index 8a989bbe7f..2258034ba4 100644 --- a/modules/packages/rubygems/metadata.go +++ b/modules/packages/rubygems/metadata.go @@ -8,7 +8,6 @@ import ( "compress/gzip" "io" "regexp" - "strings" "sync" "gitea.dev/modules/util" @@ -26,8 +25,14 @@ var ( ErrInvalidVersion = util.NewInvalidArgumentErrorf("package version is invalid") ) -var versionMatcher = sync.OnceValue(func() *regexp.Regexp { - return regexp.MustCompile(`\A[0-9]+(?:\.[0-9a-zA-Z]+)*(?:-[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?\z`) +var globalVars = sync.OnceValue(func() (ret struct { + nameMatcher, versionMatcher *regexp.Regexp +}, +) { + // https://github.com/rubygems/rubygems/blob/master/lib/rubygems/specification.rb (VALID_NAME_PATTERN) + ret.nameMatcher = regexp.MustCompile(`\A[\w.-]+\z`) + ret.versionMatcher = regexp.MustCompile(`\A[0-9]+(?:\.[0-9a-zA-Z]+)*(?:-[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?\z`) + return ret }) // Package represents a RubyGems package @@ -175,11 +180,11 @@ func parseMetadataFile(r io.Reader) (*Package, error) { return nil, err } - if len(spec.Name) == 0 || strings.Contains(spec.Name, "/") { + if !globalVars().nameMatcher.MatchString(spec.Name) { return nil, ErrInvalidName } - if !versionMatcher().MatchString(spec.Version.Version) { + if !globalVars().versionMatcher.MatchString(spec.Version.Version) { return nil, ErrInvalidVersion } diff --git a/modules/packages/rubygems/metadata_test.go b/modules/packages/rubygems/metadata_test.go index da75edd04a..8917bd2b36 100644 --- a/modules/packages/rubygems/metadata_test.go +++ b/modules/packages/rubygems/metadata_test.go @@ -32,6 +32,17 @@ version: assert.NoError(t, err) assert.NotNil(t, rp) }) + + t.Run("InvalidName", func(t *testing.T) { + // a name carrying a newline would be re-emitted verbatim into the + // line-based compact index, letting an upload forge extra entries + for _, quotedName := range []string{`"evil\n1.0.0"`, `"a b"`, `"a/b"`, `""`} { + content := test.CompressGzip("name: " + quotedName + "\nversion:\n version: 1\n") + rp, err := parseMetadataFile(content) + assert.ErrorIs(t, err, ErrInvalidName, "name %s should be rejected", quotedName) + assert.Nil(t, rp) + } + }) } func TestParseMetadataFile(t *testing.T) { diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index 8943ad3993..9ca5243631 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -55,7 +55,8 @@ func ListForks(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - forks, total, err := repo_service.FindForks(ctx, ctx.Repo.Repository, ctx.Doer, utils.GetListOptions(ctx)) + listOptions := utils.GetListOptions(ctx) + forks, total, err := repo_service.FindForks(ctx, ctx.Repo.Repository, ctx.Doer, listOptions) if err != nil { ctx.APIErrorInternal(err) return @@ -79,6 +80,7 @@ func ListForks(ctx *context.APIContext) { apiForks[i] = convert.ToRepo(ctx, fork, permission) } + ctx.SetLinkHeader(total, listOptions.PageSize) ctx.SetTotalCountHeader(total) ctx.JSON(http.StatusOK, apiForks) } diff --git a/templates/repo/commit_page.tmpl b/templates/repo/commit_page.tmpl index 76fbf3c159..fd9ff52ce0 100644 --- a/templates/repo/commit_page.tmpl +++ b/templates/repo/commit_page.tmpl @@ -129,11 +129,7 @@
{{if .Author}} {{ctx.AvatarUtils.Avatar .Author 20}} - {{if .Author.FullName}} - {{.Author.FullName}} - {{else}} - {{.Commit.Author.Name}} - {{end}} + {{.Author.GetShortDisplayNameLinkHTML}} {{else}} {{ctx.AvatarUtils.AvatarByEmail .Commit.Author.Email .Commit.Author.Email 20}} {{.Commit.Author.Name}} @@ -141,18 +137,19 @@
{{DateUtils.TimeSince .Commit.Committer.When}} -
- {{if or (ne .Commit.Committer.Name .Commit.Author.Name) (ne .Commit.Committer.Email .Commit.Author.Email)}} + {{$committerIsAuthor := and (eq .Commit.Committer.Name .Commit.Author.Name) (eq .Commit.Committer.Email .Commit.Author.Email)}} + {{if not $committerIsAuthor}} +
{{ctx.Locale.Tr "repo.diff.committed_by"}} - {{if and .Verification.CommittingUser .Verification.CommittingUser.ID}} + {{if and .Verification.CommittingUser}} {{ctx.AvatarUtils.Avatar .Verification.CommittingUser 20}} - {{.Commit.Committer.Name}} + {{.Verification.CommittingUser.GetShortDisplayNameLinkHTML}} {{else}} - {{ctx.AvatarUtils.AvatarByEmail .Commit.Committer.Email .Commit.Committer.Name 20}} + {{ctx.AvatarUtils.AvatarByEmail .Commit.Committer.Email .Commit.Committer.Email 20}} {{.Commit.Committer.Name}} {{end}} - {{end}} -
+
+ {{end}} {{if .CommitOtherParticipants}}
@@ -162,16 +159,12 @@ {{$gitIdentity := $participant.GitIdentity}} {{if $user}} {{ctx.AvatarUtils.Avatar $user 20}} - {{$user.GetDisplayName}} + {{$user.GetShortDisplayNameLinkHTML}} {{else}} {{$gitName := $gitIdentity.Name}} {{$gitEmail := $gitIdentity.Email}} - {{ctx.AvatarUtils.AvatarByEmail $gitEmail $gitName 20}} - {{if $gitEmail}} - {{$gitName}} - {{else}} - {{$gitName}} - {{end}} + {{ctx.AvatarUtils.AvatarByEmail $gitEmail $gitEmail 20}}{{/* use the same layout as the "author" above */}} + {{$gitName}} {{end}} {{end}}
diff --git a/tests/integration/api_fork_test.go b/tests/integration/api_fork_test.go index cf0a2e4384..4ce2c93428 100644 --- a/tests/integration/api_fork_test.go +++ b/tests/integration/api_fork_test.go @@ -89,7 +89,7 @@ func testAPIForkListLimitedAndPrivateRepos(t *testing.T) { assert.Equal(t, "0", resp.Header().Get("X-Total-Count")) }) - t.Run("Logged in", func(t *testing.T) { + t.Run("LoggedIn", func(t *testing.T) { defer tests.PrintCurrentTest(t)() req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/forks").AddTokenAuth(user1Token) @@ -107,6 +107,36 @@ func testAPIForkListLimitedAndPrivateRepos(t *testing.T) { assert.Len(t, forks, 2) assert.Equal(t, "2", resp.Header().Get("X-Total-Count")) }) + + t.Run("RespHeaderLinks", func(t *testing.T) { + t.Run("Page1", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/forks?page=1&limit=1").AddTokenAuth(user1Token) + resp := MakeRequest(t, req, http.StatusOK) + assert.Equal(t, "2", resp.Header().Get("X-Total-Count")) + + linkHeader := resp.Header().Get("Link") + assert.NotEmpty(t, linkHeader, "Link header should not be empty") + assert.Contains(t, linkHeader, `rel="next"`) + assert.Contains(t, linkHeader, `rel="last"`) + assert.Contains(t, linkHeader, `/api/v1/repos/user2/repo1/forks?limit=1&page=2>`) + + forks := DecodeJSON(t, resp, []*api.Repository{}) + assert.Len(t, forks, 1) + }) + + t.Run("Page2", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/forks?page=2&limit=1").AddTokenAuth(user1Token) + resp := MakeRequest(t, req, http.StatusOK) + assert.Equal(t, "2", resp.Header().Get("X-Total-Count")) + + forks := DecodeJSON(t, resp, []*api.Repository{}) + assert.Len(t, forks, 1) + }) + }) } func testGetPrivateReposForks(t *testing.T) { diff --git a/tests/sqlite.ini.tmpl b/tests/sqlite.ini.tmpl index 95a1df283f..a12735e06d 100644 --- a/tests/sqlite.ini.tmpl +++ b/tests/sqlite.ini.tmpl @@ -5,6 +5,7 @@ RUN_MODE = prod [database] DB_TYPE = sqlite3 PATH = gitea-test.db +SQLITE_JOURNAL_MODE = WAL [indexer] REPO_INDEXER_ENABLED = true diff --git a/web_src/js/utils/dom.test.ts b/web_src/js/utils/dom.test.ts index 3edbe94ce4..a30b29bab2 100644 --- a/web_src/js/utils/dom.test.ts +++ b/web_src/js/utils/dom.test.ts @@ -9,6 +9,8 @@ import { test('createElementFromHTML', () => { expect(createElementFromHTML('foobar').outerHTML).toEqual('foobar'); expect(createElementFromHTML('foo').outerHTML).toEqual('foo'); + expect(createElementFromHTML('foo').outerHTML).toEqual('foo'); + expect(createElementFromHTML('').outerHTML).toEqual(''); }); test('createElementFromAttrs', () => { diff --git a/web_src/js/utils/dom.ts b/web_src/js/utils/dom.ts index d6823fc895..b18f33f33d 100644 --- a/web_src/js/utils/dom.ts +++ b/web_src/js/utils/dom.ts @@ -267,9 +267,14 @@ export function isElemVisible(el: HTMLElement): boolean { export function createElementFromHTML(htmlString: string): T { htmlString = htmlString.trim(); + const isLetter = (code: number) => (code >= 65 && code <= 90) || (code >= 97 && code <= 122); + const startsWithTag = (s: string, tag: string) => { + return s.startsWith('<') && + s.substring(1, 1 + tag.length).toLowerCase() === tag.toLowerCase() && + !isLetter(s[1 + tag.length].charCodeAt(0)); + }; // There is no way to create some elements without a proper parent, jQuery's approach: https://github.com/jquery/jquery/blob/main/src/manipulation/wrapMap.js - // eslint-disable-next-line github/unescaped-html-literal - if (htmlString.startsWith('('tr')!; diff --git a/web_src/js/utils/url.ts b/web_src/js/utils/url.ts index 84328faf24..06e1aa2951 100644 --- a/web_src/js/utils/url.ts +++ b/web_src/js/utils/url.ts @@ -1,3 +1,5 @@ +import {html, htmlRaw} from './html.ts'; + export function urlQueryEscape(s: string) { // See "TestQueryEscape" in backend // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent#encoding_for_rfc3986 @@ -35,9 +37,9 @@ const urlLinkifyPattern = /(<([-\w]+)[^>]*>)|(<\/([-\w]+)[^>]*>)|(https?:\/\/[^\ const trailingPunctPattern = /[.,;:!?]+$/; // Convert URLs to clickable links in HTML, preserving existing HTML tags -export function linkifyURLs(html: string): string { +export function linkifyURLs(htmlString: string): string { let inAnchor = false; - return html.replace(urlLinkifyPattern, (match, _openTagFull, openTag, _closeTagFull, closeTag, url) => { + return htmlString.replace(urlLinkifyPattern, (match, _openTagFull, openTag, _closeTagFull, closeTag, url) => { // skip URLs inside existing tags if (openTag === 'a') { inAnchor = true; @@ -54,6 +56,6 @@ export function linkifyURLs(html: string): string { const cleanUrl = trailingPunct ? url.slice(0, -trailingPunct[0].length) : url; const trailing = trailingPunct ? trailingPunct[0] : ''; // safe because regexp only matches valid URLs (no quotes or angle brackets) - return `${cleanUrl}${trailing}`; // eslint-disable-line github/unescaped-html-literal + return html`${htmlRaw(cleanUrl)}${htmlRaw(trailing)}`; }); }