Markdown: Sanitizier Configuration

[cipherboy]

Allowing the gitea administrator to configure sanitization policy allows
them to couple external renders and custom templates to support more
markup. In particular, the pandoc renderer allows generating KaTeX
annotations, wrapping them in <span> elements with class math and
either inline or display (depending on whether or not inline or
block mode was requested).

This iteration gives the administrator whitelisting powers; carefully
crafted regexes will thus let through only the desired attributes
necessary to support their custom markup.

Resolves: #9054

---

• I have read the contributing guidelines.
• I have used the DCO sign-off.
• I have attempted to run unit tests
  • This failed for me on === TestXSSUserFullName (integrations/xss_test.go:17), but master branch also failed for the same test case.
• I ran make golangci-lint (since make lint seemed to suggest make revive was deprecated):
  bash $ make golangci-lint golangci-lint run --timeout 5m WARN [runner] Can't run linter unused: buildssa: analysis skipped: errors in package: [-: code in directory /home/cipherboy/GitHub/cipherboy/gitea expects import "code.gitea.io/gitea"] WARN [runner] Can't run linter goanalysis_metalinter: assign: failed prerequisites: inspect@_/home/cipherboy/GitHub/cipherboy/gitea $

---

I'm currently running this on git.cipherboy.com and it works for my use case.

Would you want a test case for this? I'm not familiar with your test setup, so I'd appreciate an example for how I should test this if so.

[codecov-io]

Codecov Report

Merging #9075 into master will decrease coverage by 0.04%.
The diff coverage is 2.53%.

@@            Coverage Diff             @@
##           master    #9075      +/-   ##
==========================================
- Coverage   41.06%   41.02%   -0.05%     
==========================================
  Files         556      556              
  Lines       72582    72642      +60     
==========================================
- Hits        29809    29798      -11     
- Misses      39030    39102      +72     
+ Partials     3743     3742       -1

Impacted Files	Coverage Δ
modules/setting/markup.go	1.23% <0%> (-5.67%)	⬇️
modules/markup/sanitizer.go	84.61% <25%> (-15.39%)	⬇️
modules/task/migrate.go	25% <0%> (-3.95%)	⬇️
modules/process/manager.go	74.69% <0%> (-3.62%)	⬇️
models/repo_indexer.go	65.94% <0%> (-1.8%)	⬇️
models/error.go	32.67% <0%> (-1.19%)	⬇️
models/repo_list.go	73.14% <0%> (-0.93%)	⬇️
models/repo.go	47.67% <0%> (+0.05%)	⬆️
modules/migrations/gitea.go	9.45% <0%> (+0.63%)	⬆️
modules/migrations/migrate.go	25.4% <0%> (+1.62%)	⬆️

---

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 d672206...45963fc. Read the comment docs.

[guillep2k]

Hi, thank you for your contribution! Please check these comments.

[guillep2k]

Note: make lint is deprecacted as you've said. I meant to suggest:

make golangci-lint
make revive

They don't go through the same checks: some things pass one and not the other. Anyway, your build is already passing our CI test, which runs those two plus several others.

[cipherboy]

Ah -- I had left one additional log.Fatal that I've removed in this last push.

This should be better now and address all of the above comments.

[lunny]

Maybe set different policy for different render tool is better?

[markup.asciidoc]
ENABLED = false
; List of file extensions that should be rendered by an external command

; policy rule and the set may be redefined multiple times to add multiple
; rules.
;ELEMENT = span
;ALLOW_ATTR = class
;REGEXP = ^\s*((math(\s+|$)|inline(\s+|$)|display(\s+|$)))+

[lunny]

Or render tool could override the global one.

[cipherboy]

Maybe set different policy for different render tool is better?

I disagree.

IMO, you'd want all policy in one place so that you can see exactly what it is you're allowing. Then you don't have to scroll to different places in the file and parse each set of configuration in your head and compare them; it is all right there.

If one markdown tool adds:

[markup.asciidoc]
ENABLED = true
ELEMENT = span
ALLOW_ATTR = class
REGEXP = 

And another adds:

[markup.pandoc]
ENABLED = true
ELEMENT = span
ALLOW_ATTR = class
REGEXP = ^(info|warning|error)$

Which wins? Is it order dependent? What's the parse order of the separate markup.<UTIL> sections (I'm assuming top->bottom or in random order if the sections are stored in a map, but the ini library is a 3rd party dependency so it is best to treat it as an opaque blob since it isn't specified in the API docs AFAICT)? If the admin added these on separate dates and forgot, how would they be notified of the potentially ambiguous policy (is bluemonday sanitation is order-dependent, i.e., first-added rule wins or last-added rule wins?)? Would we have to parse each policy and ensure that, under bluemonday's interpretation, it'd be unambiguous? etc.

Best to have all policy in one section so that the admin can hopefully craft unambiguous rules, and so that it is easier to fix.

Comments are allowed in the INI file between keys, so if the admin wants to annotate them as applying to given markup, they can do so.

[lunny]

@cipherboy

I mean a render tool could override the default sanitizer configuration. That should be more flexible but it will increase the implementation complicat. I'm also OK with current change.

[markup.sanitizer]
; The following keys can be used multiple times to define sanitation policy rules.
;ELEMENT = span
;ALLOW_ATTR = class
;REGEXP = ^(info|warning|error)$

[markup.asciidoc]
ENABLED = false
; List of file extensions that should be rendered by an external command

; policy rule and the set may be redefined multiple times to add multiple
; rules.
;ELEMENT = span
;ALLOW_ATTR = class
;REGEXP = ^\s*((math(\s+|$)|inline(\s+|$)|display(\s+|$)))+

[cipherboy]

I mean a render tool could override the default sanitizer configuration. That should be more flexible but it will increase the implementation complicat. I'm also OK with current change.

Ah sorry I missed that reply. Yeah, that's better. I think my points still stand, but your way would let the admin find the right balance between "all global" and "all local".

I don't think it'd be much work to do, I'm just not personally that interested in doing it. :)

(You'd mostly just call newMarkupSanitizer on every markup.* item and add logic to deal with creating/extending ExternalSanitizerRules as necessary based on the number of the rules in this section versus the slice's actual length and capacity.)

[guillep2k]

Looks good to me 🎉.

If you need to add commits from now on, please try to avoid force-pushing them, so the review process is easier. 😄

[cipherboy]

Ah sorry, I tend to be from the crowd that does a habitual master branch update followed by a rebase and squash of any fixes. :)

[lunny]

Couldn't haveRegexp be ignored as your above code?

[cipherboy]

I'm not sure what you mean by "as my above code"? The first check is for a completely empty section; there's nothing to do so exit with only a warning.

Per above config-cheat-sheet description for REGEXP: "Must be present but may be empty for unconditional whitelisting of this attribute."

So if it is present but empty, haveRegexp = ini.Section.HasKey("REGEXP") will return true, but pattern = ini.Section.Key("REGEXP").ValueWithShadows()[i] == "" for some i.

This check is thus for if ELEMENT= and HAVEATTR= have been specified, but REGEXP hasn't been. (Well -- strictly it checks for if any haven't been specified having earlier checked that all haven't been specified).

This is because we aren't parsing the ini ourselves. We only have an external library that parses it for us, and gives us parsed results. Particularly, parsed results without any ordering between keys and without knowledge of where a missing key might be.

If we allowed:

[markup.sanitizer]
ELEMENT=span
HAVEATTR=class

The user might later add:

[markup.sanitizer]
ELEMENT=span
HAVEATTR=class
ELEMENT=div
HAVEATTR=class
REGEXP=^(error|warning|info)$

We wouldn't be able to tell which element/attr pair the regex belongs to, because the ini module doesn't expose neighbor key information. That'd surprise the user (a working config + another working config should equal a working config, since we say that we support adding configs to get more rules).

So I maintain this is desired behavior unless there's something I'm missing?

[lunny]

Please resolve the conflicts

[cipherboy]

ACK, rebased.

[6543]

Make lgtm work

[cipherboy]

Thanks for the merge!