Avoid showing unnecessary JS errors when there are elements with different origin on the page (#29081) (#29089)

Backport #29081 by wxiaoguang

Try to fix #29080

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

.eslintrc.yaml

@@ -743,7 +743,7 @@ rules:
   wc/no-constructor-params: [2]
   wc/no-constructor: [2]
   wc/no-customized-built-in-elements: [2]
-  wc/no-exports-with-element: [2]
+  wc/no-exports-with-element: [0]
   wc/no-invalid-element-name: [2]
   wc/no-invalid-extends: [2]
   wc/no-method-prefixed-with-on: [2]

modules/contexttest/context_tests.go

@@ -40,8 +40,19 @@ func mockRequest(t *testing.T, reqPath string) *http.Request {
 	return req
 }
 
+type MockContextOption struct {
+	Render context.Render
+}
+
 // MockContext mock context for unit tests
-func MockContext(t *testing.T, reqPath string) (*context.Context, *httptest.ResponseRecorder) {
+func MockContext(t *testing.T, reqPath string, opts ...MockContextOption) (*context.Context, *httptest.ResponseRecorder) {
+	var opt MockContextOption
+	if len(opts) > 0 {
+		opt = opts[0]
+	}
+	if opt.Render == nil {
+		opt.Render = &MockRender{}
+	}
 	resp := httptest.NewRecorder()
 	req := mockRequest(t, reqPath)
 	base, baseCleanUp := context.NewBaseContext(resp, req)
@@ -49,7 +60,7 @@ func MockContext(t *testing.T, reqPath string) (*context.Context, *httptest.Resp
 	base.Data = middleware.GetContextData(req.Context())
 	base.Locale = &translation.MockLocale{}
 
-	ctx := context.NewWebContext(base, &MockRender{}, nil)
+	ctx := context.NewWebContext(base, opt.Render, nil)
 
 	chiCtx := chi.NewRouteContext()
 	ctx.Base.AppendContextValue(chi.RouteCtxKey, chiCtx)

modules/indexer/code/elasticsearch/elasticsearch.go

@@ -180,11 +180,17 @@ func (b *Indexer) Index(ctx context.Context, repo *repo_model.Repository, sha st
 	}
 
 	if len(reqs) > 0 {
-		_, err := b.inner.Client.Bulk().
-			Index(b.inner.VersionedIndexName()).
-			Add(reqs...).
-			Do(ctx)
-		return err
+		esBatchSize := 50
+
+		for i := 0; i < len(reqs); i += esBatchSize {
+			_, err := b.inner.Client.Bulk().
+				Index(b.inner.VersionedIndexName()).
+				Add(reqs[i:min(i+esBatchSize, len(reqs))]...).
+				Do(ctx)
+			if err != nil {
+				return err
+			}
+		}
 	}
 	return nil
 }

modules/markup/orgmode/orgmode.go

@@ -133,18 +133,18 @@ type Writer struct {
 	Ctx *markup.RenderContext
 }
 
-const mailto = "mailto:"
-
-func (r *Writer) resolveLink(l org.RegularLink) string {
-	link := html.EscapeString(l.URL)
-	if l.Protocol == "file" {
-		link = link[len("file:"):]
-	}
-	if len(link) > 0 && !markup.IsLinkStr(link) &&
-		link[0] != '#' && !strings.HasPrefix(link, mailto) {
+func (r *Writer) resolveLink(kind, link string) string {
+	link = strings.TrimPrefix(link, "file:")
+	if !strings.HasPrefix(link, "#") && // not a URL fragment
+		!markup.IsLinkStr(link) && // not an absolute URL
+		!strings.HasPrefix(link, "mailto:") {
+		if kind == "regular" {
+			// orgmode reports the link kind as "regular" for "[[ImageLink.svg][The Image Desc]]"
+			// so we need to try to guess the link kind again here
+			kind = org.RegularLink{URL: link}.Kind()
+		}
 		base := r.Ctx.Links.Base
-		switch l.Kind() {
-		case "image", "video":
+		if kind == "image" || kind == "video" {
 			base = r.Ctx.Links.ResolveMediaLink(r.Ctx.IsWiki)
 		}
 		link = util.URLJoin(base, link)
@@ -154,29 +154,29 @@ func (r *Writer) resolveLink(l org.RegularLink) string {
 
 // WriteRegularLink renders images, links or videos
 func (r *Writer) WriteRegularLink(l org.RegularLink) {
-	link := r.resolveLink(l)
+	link := r.resolveLink(l.Kind(), l.URL)
 
 	// Inspired by https://github.com/niklasfasching/go-org/blob/6eb20dbda93cb88c3503f7508dc78cbbc639378f/org/html_writer.go#L406-L427
 	switch l.Kind() {
 	case "image":
 		if l.Description == nil {
-			fmt.Fprintf(r, `<img src="%s" alt="%s" />`, link, link)
+			_, _ = fmt.Fprintf(r, `<img src="%s" alt="%s" />`, link, link)
 		} else {
-			imageSrc := r.resolveLink(l.Description[0].(org.RegularLink))
-			fmt.Fprintf(r, `<a href="%s"><img src="%s" alt="%s" /></a>`, link, imageSrc, imageSrc)
+			imageSrc := r.resolveLink(l.Kind(), org.String(l.Description...))
+			_, _ = fmt.Fprintf(r, `<a href="%s"><img src="%s" alt="%s" /></a>`, link, imageSrc, imageSrc)
 		}
 	case "video":
 		if l.Description == nil {
-			fmt.Fprintf(r, `<video src="%s">%s</video>`, link, link)
+			_, _ = fmt.Fprintf(r, `<video src="%s">%s</video>`, link, link)
 		} else {
-			videoSrc := r.resolveLink(l.Description[0].(org.RegularLink))
-			fmt.Fprintf(r, `<a href="%s"><video src="%s">%s</video></a>`, link, videoSrc, videoSrc)
+			videoSrc := r.resolveLink(l.Kind(), org.String(l.Description...))
+			_, _ = fmt.Fprintf(r, `<a href="%s"><video src="%s">%s</video></a>`, link, videoSrc, videoSrc)
 		}
 	default:
 		description := link
 		if l.Description != nil {
 			description = r.WriteNodesAsString(l.Description...)
 		}
-		fmt.Fprintf(r, `<a href="%s" title="%s">%s</a>`, link, description, description)
+		_, _ = fmt.Fprintf(r, `<a href="%s">%s</a>`, link, description)
 	}
 }

modules/markup/orgmode/orgmode_test.go

@@ -10,60 +10,53 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/markup"
 	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
 
-const (
-	AppURL    = "http://localhost:3000/"
-	Repo      = "gogits/gogs"
-	AppSubURL = AppURL + Repo + "/"
-)
+const AppURL = "http://localhost:3000/"
 
 func TestRender_StandardLinks(t *testing.T) {
 	setting.AppURL = AppURL
-	setting.AppSubURL = AppSubURL
 
 	test := func(input, expected string) {
 		buffer, err := RenderString(&markup.RenderContext{
 			Ctx: git.DefaultContext,
 			Links: markup.Links{
-				Base: setting.AppSubURL,
+				Base:       "/relative-path",
+				BranchPath: "branch/main",
 			},
 		}, input)
 		assert.NoError(t, err)
 		assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(buffer))
 	}
 
-	googleRendered := "<p><a href=\"https://google.com/\" title=\"https://google.com/\">https://google.com/</a></p>"
-	test("[[https://google.com/]]", googleRendered)
-
-	lnk := util.URLJoin(AppSubURL, "WikiPage")
-	test("[[WikiPage][WikiPage]]",
-		"<p><a href=\""+lnk+"\" title=\"WikiPage\">WikiPage</a></p>")
+	test("[[https://google.com/]]",
+		`<p><a href="https://google.com/">https://google.com/</a></p>`)
+	test("[[WikiPage][The WikiPage Desc]]",
+		`<p><a href="/relative-path/WikiPage">The WikiPage Desc</a></p>`)
+	test("[[ImageLink.svg][The Image Desc]]",
+		`<p><a href="/relative-path/media/branch/main/ImageLink.svg">The Image Desc</a></p>`)
 }
 
 func TestRender_Media(t *testing.T) {
 	setting.AppURL = AppURL
-	setting.AppSubURL = AppSubURL
 
 	test := func(input, expected string) {
 		buffer, err := RenderString(&markup.RenderContext{
 			Ctx: git.DefaultContext,
 			Links: markup.Links{
-				Base: setting.AppSubURL,
+				Base: "./relative-path",
 			},
 		}, input)
 		assert.NoError(t, err)
 		assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(buffer))
 	}
 
-	url := "../../.images/src/02/train.jpg"
-	result := util.URLJoin(AppSubURL, url)
-
-	test("[[file:"+url+"]]",
-		"<p><img src=\""+result+"\" alt=\""+result+"\" /></p>")
+	test("[[file:../../.images/src/02/train.jpg]]",
+		`<p><img src=".images/src/02/train.jpg" alt=".images/src/02/train.jpg" /></p>`)
+	test("[[file:train.jpg]]",
+		`<p><img src="relative-path/train.jpg" alt="relative-path/train.jpg" /></p>`)
 
 	// With description.
 	test("[[https://example.com][https://example.com/example.svg]]",
@@ -76,11 +69,20 @@ func TestRender_Media(t *testing.T) {
 		`<p><img src="https://example.com/example.svg" alt="https://example.com/example.svg" /></p>`)
 	test("[[https://example.com/example.mp4]]",
 		`<p><video src="https://example.com/example.mp4">https://example.com/example.mp4</video></p>`)
+
+	// test [[LINK][DESCRIPTION]] syntax with "file:" prefix
+	test(`[[https://example.com/][file:https://example.com/foo%20bar.svg]]`,
+		`<p><a href="https://example.com/"><img src="https://example.com/foo%20bar.svg" alt="https://example.com/foo%20bar.svg" /></a></p>`)
+	test(`[[file:https://example.com/foo%20bar.svg][Goto Image]]`,
+		`<p><a href="https://example.com/foo%20bar.svg">Goto Image</a></p>`)
+	test(`[[file:https://example.com/link][https://example.com/image.jpg]]`,
+		`<p><a href="https://example.com/link"><img src="https://example.com/image.jpg" alt="https://example.com/image.jpg" /></a></p>`)
+	test(`[[file:https://example.com/link][file:https://example.com/image.jpg]]`,
+		`<p><a href="https://example.com/link"><img src="https://example.com/image.jpg" alt="https://example.com/image.jpg" /></a></p>`)
 }
 
 func TestRender_Source(t *testing.T) {
 	setting.AppURL = AppURL
-	setting.AppSubURL = AppSubURL
 
 	test := func(input, expected string) {
 		buffer, err := RenderString(&markup.RenderContext{

routers/web/repo/middlewares.go

@@ -92,23 +92,15 @@ func SetWhitespaceBehavior(ctx *context.Context) {
 // SetShowOutdatedComments set the show outdated comments option as context variable
 func SetShowOutdatedComments(ctx *context.Context) {
 	showOutdatedCommentsValue := ctx.FormString("show-outdated")
-	// var showOutdatedCommentsValue string
-
 	if showOutdatedCommentsValue != "true" && showOutdatedCommentsValue != "false" {
 		// invalid or no value for this form string -> use default or stored user setting
+		showOutdatedCommentsValue = "true"
 		if ctx.IsSigned {
-			showOutdatedCommentsValue, _ = user_model.GetUserSetting(ctx, ctx.Doer.ID, user_model.SettingsKeyShowOutdatedComments, "false")
-		} else {
-			// not logged in user -> use the default value
-			showOutdatedCommentsValue = "false"
+			showOutdatedCommentsValue, _ = user_model.GetUserSetting(ctx, ctx.Doer.ID, user_model.SettingsKeyShowOutdatedComments, showOutdatedCommentsValue)
 		}
-	} else {
+	} else if ctx.IsSigned {
 		// valid value -> update user setting if user is logged in
-		if ctx.IsSigned {
-			_ = user_model.SetUserSetting(ctx, ctx.Doer.ID, user_model.SettingsKeyShowOutdatedComments, showOutdatedCommentsValue)
-		}
+		_ = user_model.SetUserSetting(ctx, ctx.Doer.ID, user_model.SettingsKeyShowOutdatedComments, showOutdatedCommentsValue)
 	}
-
-	showOutdatedComments, _ := strconv.ParseBool(showOutdatedCommentsValue)
-	ctx.Data["ShowOutdatedComments"] = showOutdatedComments
+	ctx.Data["ShowOutdatedComments"], _ = strconv.ParseBool(showOutdatedCommentsValue)
 }

routers/web/repo/pull_review.go

@@ -22,6 +22,7 @@ import (
 
 const (
 	tplDiffConversation     base.TplName = "repo/diff/conversation"
+	tplConversationOutdated base.TplName = "repo/diff/conversation_outdated"
 	tplTimelineConversation base.TplName = "repo/issue/view_content/conversation"
 	tplNewComment           base.TplName = "repo/diff/new_comment"
 )
@@ -161,8 +162,8 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
 		return
 	}
 	if len(comments) == 0 {
-		// if the comments are empty (deleted, outdated, etc), it doesn't need to render anything, just return an empty body to replace "conversation-holder" on the page
-		ctx.Resp.WriteHeader(http.StatusOK)
+		// if the comments are empty (deleted, outdated, etc), it's better to tell the users that it is outdated
+		ctx.HTML(http.StatusOK, tplConversationOutdated)
 		return
 	}
 

routers/web/repo/pull_review_test.go

@@ -0,0 +1,76 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package repo
+
+import (
+	"net/http/httptest"
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/context"
+	"code.gitea.io/gitea/modules/contexttest"
+	"code.gitea.io/gitea/modules/templates"
+	"code.gitea.io/gitea/services/pull"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRenderConversation(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	pr, _ := issues_model.GetPullRequestByID(db.DefaultContext, 2)
+	_ = pr.LoadIssue(db.DefaultContext)
+	_ = pr.Issue.LoadPoster(db.DefaultContext)
+	_ = pr.Issue.LoadRepo(db.DefaultContext)
+
+	run := func(name string, cb func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder)) {
+		t.Run(name, func(t *testing.T) {
+			ctx, resp := contexttest.MockContext(t, "/", contexttest.MockContextOption{Render: templates.HTMLRenderer()})
+			contexttest.LoadUser(t, ctx, pr.Issue.PosterID)
+			contexttest.LoadRepo(t, ctx, pr.BaseRepoID)
+			contexttest.LoadGitRepo(t, ctx)
+			defer ctx.Repo.GitRepo.Close()
+			cb(t, ctx, resp)
+		})
+	}
+
+	var preparedComment *issues_model.Comment
+	run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+		comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "content", "", false, 0, pr.HeadCommitID)
+		if !assert.NoError(t, err) {
+			return
+		}
+		comment.Invalidated = true
+		err = issues_model.UpdateCommentInvalidate(ctx, comment)
+		if !assert.NoError(t, err) {
+			return
+		}
+		preparedComment = comment
+	})
+	if !assert.NotNil(t, preparedComment) {
+		return
+	}
+	run("diff with outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+		ctx.Data["ShowOutdatedComments"] = true
+		renderConversation(ctx, preparedComment, "diff")
+		assert.Contains(t, resp.Body.String(), `<div class="content comment-container"`)
+	})
+	run("diff without outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+		ctx.Data["ShowOutdatedComments"] = false
+		renderConversation(ctx, preparedComment, "diff")
+		assert.Contains(t, resp.Body.String(), `conversation-not-existing`)
+	})
+	run("timeline with outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+		ctx.Data["ShowOutdatedComments"] = true
+		renderConversation(ctx, preparedComment, "timeline")
+		assert.Contains(t, resp.Body.String(), `<div id="code-comments-`)
+	})
+	run("timeline without outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+		ctx.Data["ShowOutdatedComments"] = false
+		renderConversation(ctx, preparedComment, "timeline")
+		assert.Contains(t, resp.Body.String(), `conversation-not-existing`)
+	})
+}

templates/repo/diff/conversation_outdated.tmpl

@@ -0,0 +1,3 @@
+<div class="ui segment conversation-holder conversation-not-existing">
+	{{ctx.Locale.Tr "repo.issues.review.outdated_description"}}
+</div>

web_src/js/modules/dirauto.js

@@ -1,5 +1,6 @@
-// for performance considerations, it only uses performant syntax
+import {isDocumentFragmentOrElementNode} from '../utils/dom.js';
 
+// for performance considerations, it only uses performant syntax
 function attachDirAuto(el) {
   if (el.type !== 'hidden' &&
       el.type !== 'checkbox' &&
@@ -18,7 +19,7 @@ export function initDirAuto() {
       const len = mutation.addedNodes.length;
       for (let i = 0; i < len; i++) {
         const addedNode = mutation.addedNodes[i];
-        if (addedNode.nodeType !== Node.ELEMENT_NODE && addedNode.nodeType !== Node.DOCUMENT_FRAGMENT_NODE) continue;
+        if (!isDocumentFragmentOrElementNode(addedNode)) continue;
         if (addedNode.nodeName === 'INPUT' || addedNode.nodeName === 'TEXTAREA') attachDirAuto(addedNode);
         const children = addedNode.querySelectorAll('input, textarea');
         const len = children.length;

web_src/js/modules/tippy.js

@@ -1,4 +1,5 @@
 import tippy, {followCursor} from 'tippy.js';
+import {isDocumentFragmentOrElementNode} from '../utils/dom.js';
 
 const visibleInstances = new Set();
 
@@ -136,8 +137,6 @@ function attachChildrenLazyTooltip(target) {
   }
 }
 
-const elementNodeTypes = new Set([Node.ELEMENT_NODE, Node.DOCUMENT_FRAGMENT_NODE]);
-
 export function initGlobalTooltips() {
   // use MutationObserver to detect new "data-tooltip-content" elements added to the DOM, or attributes changed
   const observerConnect = (observer) => observer.observe(document, {
@@ -152,11 +151,10 @@ export function initGlobalTooltips() {
       if (mutation.type === 'childList') {
         // mainly for Vue components and AJAX rendered elements
         for (const el of mutation.addedNodes) {
-          if (elementNodeTypes.has(el.nodeType)) {
-            attachChildrenLazyTooltip(el);
-            if (el.hasAttribute('data-tooltip-content')) {
-              attachLazyTooltip(el);
-            }
+          if (!isDocumentFragmentOrElementNode(el)) continue;
+          attachChildrenLazyTooltip(el);
+          if (el.hasAttribute('data-tooltip-content')) {
+            attachLazyTooltip(el);
           }
         }
       } else if (mutation.type === 'attributes') {

web_src/js/utils/dom.js

@@ -59,6 +59,17 @@ export function onDomReady(cb) {
   }
 }
 
+// checks whether an element is owned by the current document, and whether it is a document fragment or element node
+// if it is, it means it is a "normal" element managed by us, which can be modified safely.
+export function isDocumentFragmentOrElementNode(el) {
+  try {
+    return el.ownerDocument === document && el.nodeType === Node.ELEMENT_NODE || el.nodeType === Node.DOCUMENT_FRAGMENT_NODE;
+  } catch {
+    // in case the el is not in the same origin, then the access to nodeType would fail
+    return false;
+  }
+}
+
 // autosize a textarea to fit content. Based on
 // https://github.com/github/textarea-autosize
 // ---------------------------------------------------------------------

web_src/js/webcomponents/GiteaOriginUrl.js

@@ -1,17 +1,21 @@
 // Convert an absolute or relative URL to an absolute URL with the current origin
+export function toOriginUrl(urlStr) {
+  try {
+    // only process absolute HTTP/HTTPS URL or relative URLs ('/xxx' or '//host/xxx')
+    if (urlStr.startsWith('http://') || urlStr.startsWith('https://') || urlStr.startsWith('/')) {
+      const {origin, protocol, hostname, port} = window.location;
+      const url = new URL(urlStr, origin);
+      url.protocol = protocol;
+      url.hostname = hostname;
+      url.port = port || (protocol === 'https:' ? '443' : '80');
+      return url.toString();
+    }
+  } catch {}
+  return urlStr;
+}
+
 window.customElements.define('gitea-origin-url', class extends HTMLElement {
   connectedCallback() {
-    const urlStr = this.getAttribute('data-url');
-    try {
-      // only process absolute HTTP/HTTPS URL or relative URLs ('/xxx' or '//host/xxx')
-      if (urlStr.startsWith('http://') || urlStr.startsWith('https://') || urlStr.startsWith('/')) {
-        const url = new URL(urlStr, window.origin);
-        url.protocol = window.location.protocol;
-        url.host = window.location.host;
-        this.textContent = url.toString();
-        return;
-      }
-    } catch {}
-    this.textContent = urlStr;
+    this.textContent = toOriginUrl(this.getAttribute('data-url'));
   }
 });

web_src/js/webcomponents/GiteaOriginUrl.test.js

@@ -0,0 +1,17 @@
+import {toOriginUrl} from './GiteaOriginUrl.js';
+
+test('toOriginUrl', () => {
+  const oldLocation = window.location;
+  for (const origin of ['https://example.com', 'https://example.com:3000']) {
+    window.location = new URL(`${origin}/`);
+    expect(toOriginUrl('/')).toEqual(`${origin}/`);
+    expect(toOriginUrl('/org/repo.git')).toEqual(`${origin}/org/repo.git`);
+    expect(toOriginUrl('https://another.com')).toEqual(`${origin}/`);
+    expect(toOriginUrl('https://another.com/')).toEqual(`${origin}/`);
+    expect(toOriginUrl('https://another.com/org/repo.git')).toEqual(`${origin}/org/repo.git`);
+    expect(toOriginUrl('https://another.com:4000')).toEqual(`${origin}/`);
+    expect(toOriginUrl('https://another.com:4000/')).toEqual(`${origin}/`);
+    expect(toOriginUrl('https://another.com:4000/org/repo.git')).toEqual(`${origin}/org/repo.git`);
+  }
+  window.location = oldLocation;
+});