markup.sanitizer in app.ini does not support multiple rule entries as expected

[albertjin]

ValueWithShadows() called in function newMarkupSanitizer() (file modules/setting/markup.go) always returns one element.

For example, the following section in app.ini means "rel" attribute is allowed for "a" element while "target" attribute is still ignored.

[markup.sanitizer]
ELEMENT = a
ALLOW_ATTR = target
REGEXP =
ELEMENT = a
ALLOW_ATTR = rel
REGEXP =

[albertjin]

This is related to pull request #9075. After a bit investigation, the problem is related multiple value support in gopkg.in/ini.v1.

For the function NewContext() in file modules/setting/setting.go at around line 519, change line Cfg.Append(...) to ini.ShadowLoad(...) as follows.

func NewContext() {
	Cfg = ini.Empty()

	if len(CustomPID) > 0 {
		createPIDFile(CustomPID)
	}

	if com.IsFile(CustomConf) {
		var err error
		if Cfg, err = ini.ShadowLoad(CustomConf); err != nil {
		//if err := Cfg.Append(CustomConf); err != nil {
			log.Fatal("Failed to load custom conf '%s': %v", CustomConf, err)
		}
	} else {

We'll have error as follows using the example above.

[E] All three keys in markup.sanitizer (ELEMENT, ALLOW_ATTR, REGEXP) must be defined the same number of times! Got 1, 2, and 1 respectively.

[zeripath]

I suspect shadowload here isn't right. We still need something that is going to append but with shadows

[zeripath]

I think We need to replace the Empty() here:

gitea/modules/setting/setting.go

Line 520 in 7d7ab1e

Cfg = ini.Empty()

With:

ini.ShadowLoad([]byte{})

[albertjin]

I think We need to replace the Empty() here:

gitea/modules/setting/setting.go

Line 520 in 7d7ab1e

Cfg = ini.Empty()

With:

ini.ShadowLoad([]byte{})

The same error

[E] All three keys in markup.sanitizer (ELEMENT, ALLOW_ATTR, REGEXP) must be defined the same number of times! Got 1, 2, and 1 respectively.

for this one,

[markup.sanitizer]
ELEMENT = a
ALLOW_ATTR = target
REGEXP =
ELEMENT = a
ALLOW_ATTR = rel
REGEXP =

[zeripath]

Sorry I hadn't read your next issue - I was sorting out the shadow.

This is never gonna work.

We need to represent a matched group of values. These just need to be child sections.

I don't think we can workaround this it needs an actual code change.

[cipherboy]

@albertjin / @zeripath:

Ah sorry -- I just saw this. Feel free to @mention me next time or drop me an email.

One idea would be a count parameter:

[markup.sanitizer]
RULE_COUNT = 2
ELEMENT_1 = a
ALLOW_ATTR_1 = target
REGEXP_1 = something
ELEMENT_2 = a
ALLOW_ATTR_2 = rel
REGEXP_2 = something

That'd also solve some of the clarity problems (and omitting a regex would be a blanket whitelist) and keys would now be unique.

Thoughts? Happy to implement whatever gets decided.

[zeripath]

@cipherboy sorry it's taken me so long to reply to this.

I think the best option would actually be:

[markup.sanitizer.1]
ELEMENT=a
ALLOW_ATTR=target
REGEXP=something
[markup.sanitizer.2]
ELEMENT=a
ALLOW_ATTR=target
REGEXP=something

We can grab all these child sections quite easily from go-ini and even map them quite nicely to objects to result in a simple []*SanitizerRules array

[zeripath]

Fixed by #11133