fix: add missing checks to several handlers and models (#39839) (#39847)

Backport https://github.com/go-gitea/gitea/pull/39839 by @silverwind

Several handlers and model helpers skipped checks that their siblings
already perform.

- Only match principal keys when looking up SSH certificate principals
- Return no attachments for an unsaved issue instead of every issue-less
one
- Reject issue and comment attachments when adding release attachments
- Share one permission check for editing, deleting and reacting to issue
content and for comment attachments between the web UI and the API,
refusing them on locked issues for users who can't write
- Reject issue edits, deletion and label changes in archived
repositories via the API
- Apply cross-origin protection to the repository topics endpoint
- Return 404 for the repositories tab of individual users and respect
profile visibility for the heatmap

* Fixes: https://github.com/go-gitea/gitea/issues/39226

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: bircni <bircni@icloud.com>

models/asymkey/ssh_key.go

@@ -199,7 +199,7 @@ func SearchPrincipalKey(ctx context.Context, principalKey string) (*PublicKey, e
 	// In the future, the existing principal keys should be migrated to use the fingerprint ("principal:{name}") instead of the content
 	key := new(PublicKey)
 	has, err := db.GetEngine(ctx).
-		Where("content = ?", principalKey).
+		Where("content = ? AND type = ?", principalKey, KeyTypePrincipal).
 		Get(key)
 	if err != nil {
 		return nil, err

models/asymkey/ssh_key_test.go

@@ -12,11 +12,13 @@ import (
 	"strings"
 	"testing"
 
+	"gitea.dev/models/db"
 	"gitea.dev/models/unittest"
 	"gitea.dev/modules/setting"
 
 	"github.com/42wim/sshsig"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func Test_SSHParsePublicKey(t *testing.T) {
@@ -481,6 +483,20 @@ func TestPublicKeysAreExternallyManaged(t *testing.T) {
 	assert.False(t, externals[0])
 }
 
+func TestSearchPrincipalKeyIgnoresOtherKeyTypes(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+	userKey := unittest.AssertExistsAndLoadBean(t, &PublicKey{ID: 1})
+
+	_, err := SearchPrincipalKey(t.Context(), userKey.Content)
+	assert.True(t, IsErrKeyNotExist(err))
+
+	principal := &PublicKey{OwnerID: 1, Name: userKey.Content, Content: userKey.Content, Type: KeyTypePrincipal}
+	require.NoError(t, db.Insert(t.Context(), principal))
+	key, err := SearchPrincipalKey(t.Context(), userKey.Content)
+	require.NoError(t, err)
+	assert.Equal(t, principal.ID, key.ID)
+}
+
 // TestCheckPublicKeyStringOversized tests if oversized SSH2 public key strings are rejected before triggering costly operations.
 func TestCheckPublicKeyStringOversized(t *testing.T) {
 	_, err := parseKeyString(strings.Repeat("a", maxKeyContentBytes+1))

models/repo/attachment.go

@@ -133,6 +133,9 @@ func ExistAttachmentsByUUID(ctx context.Context, uuid string) (bool, error) {
 
 // GetAttachmentsByIssueID returns all attachments of an issue.
 func GetAttachmentsByIssueID(ctx context.Context, issueID int64) ([]*Attachment, error) {
+	if issueID == 0 { // unsaved issue, would match every issue-less attachment
+		return []*Attachment{}, nil
+	}
 	attachments := make([]*Attachment, 0, 10)
 	return attachments, db.GetEngine(ctx).Where("issue_id = ? AND comment_id = 0", issueID).Find(&attachments)
 }

models/repo/attachment_test.go

@@ -36,6 +36,10 @@ func TestGetByCommentOrIssueID(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Len(t, attachments, 1)
 
+	attachments, err = repo_model.GetAttachmentsByIssueID(t.Context(), 0)
+	assert.NoError(t, err)
+	assert.Empty(t, attachments)
+
 	attachments, err = repo_model.GetAttachmentsByCommentID(t.Context(), 1)
 	assert.NoError(t, err)
 	assert.Len(t, attachments, 2)

models/repo/release.go

@@ -191,6 +191,10 @@ func AddReleaseAttachments(ctx context.Context, releaseID int64, attachmentUUIDs
 	}
 
 	for i := range attachments {
+		if attachments[i].IssueID != 0 || attachments[i].CommentID != 0 || attachments[i].ReleaseID != 0 {
+			return util.NewPermissionDeniedErrorf("attachment is already linked")
+		}
+
 		if attachments[i].RepoID == 0 && attachments[i].CreatedUnix < LegacyAttachmentMissingRepoIDCutoff {
 			attachments[i].RepoID = rel.RepoID
 			if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Cols("repo_id").Update(attachments[i]); err != nil {
@@ -201,10 +205,6 @@ func AddReleaseAttachments(ctx context.Context, releaseID int64, attachmentUUIDs
 		if attachments[i].RepoID != rel.RepoID {
 			return util.NewPermissionDeniedErrorf("attachment belongs to different repository")
 		}
-
-		if attachments[i].ReleaseID != 0 {
-			return util.NewPermissionDeniedErrorf("release permission denied")
-		}
 		attachments[i].ReleaseID = releaseID
 		// No assign value could be 0, so ignore AllCols().
 		if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Cols("release_id").Update(attachments[i]); err != nil {

models/repo/release_test.go

@@ -53,6 +53,18 @@ func TestAddReleaseAttachmentsRejectsDifferentRepo(t *testing.T) {
 	assert.Zero(t, attach.ReleaseID, "attachment should not be linked to release on failure")
 }
 
+func TestAddReleaseAttachmentsRejectsIssueAttachment(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	uuid := "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11"
+	err := AddReleaseAttachments(t.Context(), 1, []string{uuid})
+	assert.ErrorIs(t, err, util.ErrPermissionDenied)
+
+	attach, err := GetAttachmentByUUID(t.Context(), uuid)
+	assert.NoError(t, err)
+	assert.Zero(t, attach.ReleaseID)
+}
+
 func TestAddReleaseAttachmentsAllowsLegacyMissingRepoID(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())