Don't replace underscores in auto-generated IDs in goldmark

[zeripath]

Fix #12196

Signed-off-by: Andrew Thornton [email protected]

[zeripath]

Technically this is breaking.

[codecov-commenter]

Codecov Report

Merging #12805 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #12805      +/-   ##
==========================================
- Coverage   43.13%   43.13%   -0.01%     
==========================================
  Files         654      654              
  Lines       72205    72205              
==========================================
- Hits        31146    31143       -3     
+ Misses      36011    36009       -2     
- Partials     5048     5053       +5     

Impacted Files	Coverage Δ
modules/markup/common/footnote.go	21.77% <100.00%> (ø)
modules/indexer/stats/db.go	43.47% <0.00%> (-17.40%)	⬇️
modules/indexer/stats/queue.go	64.70% <0.00%> (-11.77%)	⬇️
modules/git/utils.go	73.77% <0.00%> (-3.28%)	⬇️
services/pull/check.go	47.69% <0.00%> (-0.77%)	⬇️
modules/git/repo.go	49.23% <0.00%> (-0.51%)	⬇️
services/pull/pull.go	41.57% <0.00%> (-0.47%)	⬇️
modules/log/event.go	59.43% <0.00%> (+1.88%)	⬆️
modules/queue/workerpool.go	60.81% <0.00%> (+2.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55e05ad...64037e4. Read the comment docs.

[silverwind]

Are these underscores coming out of Goldmark like that or are we actually replacing them elsewhere ourselves? I have a suspicion that the latter is the case and then this fix may not be right.

BTW it may be possible to add compat with old links in JS thought I'm fine with not adding compat.

[zeripath]

@silverwind this is the place we replace them in goldmark. Outside of goldmark we simply assert that ids need the prefix user-content-

[silverwind]

So they are coming out like that of Goldmark? If so, may be a bug there because I think it strives to be compatible with GFM.

The user-content- prefix should be mostly be a thing of the past since #11903, only JS compat for it should remain.

[zeripath]

We have to protect ids - without prefixing these how do you propose this?

[zeripath]

FYI IDs are generated within our variant of goldmark using:

gitea/modules/markup/markdown/goldmark.go

Lines 197 to 217 in 29ac1f9

	// Generate generates a new element id.
	func (p *prefixedIDs) GenerateWithDefault(value []byte, dft []byte) []byte {
	result := common.CleanValue(value)
	if len(result) == 0 {
	result = dft
	}
	if !bytes.HasPrefix(result, []byte("user-content-")) {
	result = append([]byte("user-content-"), result...)
	}
	if _, ok := p.values[util.BytesToReadOnlyString(result)]; !ok {
	p.values[util.BytesToReadOnlyString(result)] = true
	return result
	}
	for i := 1; ; i++ {
	newResult := fmt.Sprintf("%s-%d", result, i)
	if _, ok := p.values[newResult]; !ok {
	p.values[newResult] = true
	return []byte(newResult)
	}
	}
	}

Goldmark's default generator is:

gitea/vendor/github.com/yuin/goldmark/parser/parser.go

Lines 75 to 115 in 29ac1f9

	func (s *ids) Generate(value []byte, kind ast.NodeKind) []byte {
	value = util.TrimLeftSpace(value)
	value = util.TrimRightSpace(value)
	result := []byte{}
	for i := 0; i < len(value); {
	v := value[i]
	l := util.UTF8Len(v)
	i += int(l)
	if l != 1 {
	continue
	}
	if util.IsAlphaNumeric(v) {
	if 'A' <= v && v <= 'Z' {
	v += 'a' - 'A'
	}
	result = append(result, v)
	} else if util.IsSpace(v) || v == '-' || v == '_' {
	result = append(result, '-')
	}
	}
	if len(result) == 0 {
	if kind == ast.KindHeading {
	result = []byte("heading")
	} else {
	result = []byte("id")
	}
	}
	if _, ok := s.values[util.BytesToReadOnlyString(result)]; !ok {
	s.values[util.BytesToReadOnlyString(result)] = true
	return result
	}
	for i := 1; ; i++ {
	newResult := fmt.Sprintf("%s-%d", result, i)
	if _, ok := s.values[newResult]; !ok {
	s.values[newResult] = true
	return []byte(newResult)
	}
	}
	}

when I moved us to Goldmark that generator was significantly different from our previous blackfriday generator therefore to make the changes as non-breaking as possible I created the above one.

---

Going back to why we have to prefix IDs:

I cannot stress this enough, we absolutely have to prefix ids.

Github also does this.

Our JS code and our CSS relies on IDs in several places - if you do not prefix user content ids you will get conflicts with multiple ids and will break rendering and potentially create security holes.

[silverwind]

we absolutely have to prefix ids.

I never understood the reason behind this. Why are user-generated ids so bad?

conflicts with multiple ids

I think browsers handle that gracefully and scroll to the first id

security holes

What benefit would an attacker gain from this exactly? As I see it, they can't really do anything dangerous when controlling ids, but of course it depends on our JS and I'd rather prefer to use classnames only in JS anyways.

[techknowlogick]

🚀