Fix sanitizer config - multiple rules #11133

cipherboy · 2020-04-19T14:15:33Z

In #9888, it was reported that my earlier pull request #9075 didn't quite function as expected. I was quite hopeful the ValuesWithShadow() worked as expected (and, I thought my testing showed it did) but I guess not. @zeripath proposed an alternative syntax which I like:

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

This was quite easy to adopt into the existing code. I've done so in a semi-backwards-compatible manner:

The value from .Value() is used for each element.
We parse [markup.sanitizer] and all [markup.sanitizer.*] sections and add them as rules.

This means that existing configs will load one rule (not all rules). It also means people can use string identifiers ([markup.sanitiser.KaTeX]) if they prefer, instead of numbered ones.

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

Resolves: 9888 Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

codecov-io · 2020-04-19T14:51:51Z

Codecov Report

Merging #11133 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master   #11133   +/-   ##
=======================================
  Coverage   43.47%   43.47%           
=======================================
  Files         600      600           
  Lines       85108    85104    -4     
=======================================
  Hits        37002    37002           
+ Misses      43537    43534    -3     
+ Partials     4569     4568    -1

Impacted Files	Coverage Δ
modules/setting/markup.go	`1.29% <0.00%> (+0.06%)`	⬆️
modules/git/repo.go	`49.79% <0.00%> (-1.26%)`	⬇️
models/gpg_key.go	`53.42% <0.00%> (-0.53%)`	⬇️
services/pull/pull.go	`35.30% <0.00%> (+1.14%)`	⬆️

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 9588d2c...4042adb. Read the comment docs.

6543 · 2020-04-19T16:34:46Z

first thought: since #9075 got into v1.11 -> we will break existing config based on that app.ini settings
so if we change the configuration we have to migrate old config if they exist 👀

lafriks · 2020-04-19T16:51:05Z

Maybe it could be possible to also support old syntax for backward compatibility

cipherboy · 2020-04-19T17:25:42Z

I think the point is that the old config doesn't work well -- it is best we migrate completely rather than support what doesn't work. In particular, this won't work:

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

Because ValuesWithShadows() returns an array of size 1 rather than of size 2. This means we can't align the values in a three rule set:

[markup.sanitizer]
ELEMENT = a
ALLOW_ATTR = target
REGEXP = $1
ELEMENT = a
ALLOW_ATTR = rel
REGEXP = $2
ELEMENT = img
ALLOW_ATTR = src
REGEXP = $3

Our data is:

ELEMENT = ['a', 'img']
ALLOW_ATTR = ['target', 'rel', 'src']
REGEX = [$1, $2, $3]

So we don't know if a is duplicated or img is duplicated!

Rewriting the ini file (with ini) thus isn't possible either. Some other tool would be needed to do this.

There's then two working scenarios:

All distinct element/allowed attrs/regexp tuples.
Only one element/attr/regexp (what's currently documented -- KaTeX ruleset).

With this PR currently, we support number two just fine. That leaves number one -- since it borders closely on what doesn't work, I'm inclined to require manual migration. Thoughts?

zeripath · 2020-04-20T02:47:16Z

Yeah the old system just doesn't work.

zeripath

Comments as above otherwise approve

docs/content/doc/advanced/external-renderers.en-us.md

docs/content/doc/advanced/config-cheat-sheet.en-us.md

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

cipherboy · 2020-04-20T15:42:32Z

Thanks @zeripath for the docs review. I wrote the docs first (and originally was thinking numbered sections) but then I looked at the code and figured numbering was a needless restriction.

I've added an example of what was broken with the old config format to the cheat sheet as well.

guillep2k · 2020-04-21T03:52:44Z

custom/conf/app.ini.sample

-; The following keys can be used multiple times to define sanitation policy rules.
+[markup.sanitizer.1]
+; The following keys can appear once to define a sanitation policy rule.
+; This section can appear with an incremenented number to define multiple rules.


Suggested change

; This section can appear with an incremenented number to define multiple rules.

; This section can appear with an incrementing numbers or any distinct alphanumeric string to define multiple rules.

I don't think "this section can appear with an incrementing numbers to define multiple rules" is correct English either.

I've made it just "This section can appear again with a unique alphanumeric string to define multiple rules".

custom/conf/app.ini.sample

guillep2k · 2020-04-21T04:01:12Z

docs/content/doc/advanced/config-cheat-sheet.en-us.md

+This was changed because the implementation with the ini parser used was flawed; the following configs were indistinguishable after parsing:
+
+```ini
+[markup.sanitizer]
+ELEMENT = a
+ALLOW_ATTR = target
+REGEXP = $1
+ELEMENT = a
+ALLOW_ATTR = rel
+REGEXP = $2
+ELEMENT = img
+ALLOW_ATTR = src
+REGEXP = $3
+```
+
+and
+
+```ini
+[markup.sanitizer]
+ELEMENT = a
+ALLOW_ATTR = target
+REGEXP = $1
+ELEMENT = img
+ALLOW_ATTR = rel
+REGEXP = $2
+ELEMENT = img
+ALLOW_ATTR = src
+REGEXP = $3
+```
+
+Because of limitations in the ini library, we are unable to automatically migrate configurations.
+
+We will still parse the first rule from a `[markup.sanitizer]` section if present, but multiple rules must be manually migrated.


Suggested change

This was changed because the implementation with the ini parser used was flawed; the following configs were indistinguishable after parsing:

```ini

[markup.sanitizer]

ELEMENT = a

ALLOW_ATTR = target

REGEXP = $1

ELEMENT = a

ALLOW_ATTR = rel

REGEXP = $2

ELEMENT = img

ALLOW_ATTR = src

REGEXP = $3

```

and

```ini

[markup.sanitizer]

ELEMENT = a

ALLOW_ATTR = target

REGEXP = $1

ELEMENT = img

ALLOW_ATTR = rel

REGEXP = $2

ELEMENT = img

ALLOW_ATTR = src

REGEXP = $3

```

Because of limitations in the ini library, we are unable to automatically migrate configurations.

We will still parse the first rule from a `[markup.sanitizer]` section if present, but multiple rules must be manually migrated.

I'd rather remove this section. If you feel like an explanation is needed, please reference the version where it was introduced and link the relevant issue or this PR (e.g. "this was changed from the original implementation in 1.11 due to severe limitations; see #1234"). But we tend to avoid making explicit version references in the docs, so maybe just remove it and let the Blog speak. 😁

If a blog entry is fine with @zeripath I could write such, but I must confess I've never looked at the blog entries and mostly read documentation. :-) On the breaking section of release notes, there's a link to the PR, but not to the blog entry -- do none of those have blog entries?

Hmm... So this bit could be dropped and instead placed in the PR description - during release we can then precis this and put it into the blog post.

guillep2k · 2020-04-21T04:02:58Z

docs/content/doc/advanced/external-renderers.en-us.md

+
+**Note**: The above section numbering policy is new; previously the section was `[markup.sanitizer]` and keys could be redefined.


Suggested change

**Note**: The above section numbering policy is new; previously the section was `[markup.sanitizer]` and keys could be redefined.

Same as above

I had specifically requested some information as this is a breaking change requiring change to config (although the previous config simply does not work.)

To avoid duplicating too much info, I've added a reference to the cheat sheet here but explicitly left this line. Its either that or add a reference to this pull request (in both places) if we don't want it in the documentation -- but to someone wanting to migrate, it isn't clear what the problem is just by looking at our extended discussion here. :-)

How about:

Suggested change

**Note**: The above section numbering policy is new; previously the section was `[markup.sanitizer]` and keys could be redefined.

**Note**: Prior to Gitea 1.12 there was a single `markup.sanitiser` section and keys could be redefined, however, there were significant problems with this method of configuration necessitating this change.

I had specifically requested some information as this is a breaking change requiring change to config (although the previous config simply does not work.)

Sorry, @zeripath. I missed your comment, which was already resolved.

modules/setting/markup.go

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

guillep2k · 2020-04-22T02:55:08Z

custom/conf/app.ini.sample

@@ -965,8 +965,8 @@ SHOW_FOOTER_TEMPLATE_LOAD_TIME = true

 [markup.sanitizer.1]
 ; The following keys can appear once to define a sanitation policy rule.
-; This section can appear with an incremenented number to define multiple rules.
-; e.g., [markup.sanitizer.1] -> [markup.sanitizer.2]
+; This section can appear again with a unique alphanmuric string to define multiple rules.


Suggested change

; This section can appear again with a unique alphanmuric string to define multiple rules.

; This section can appear multiple times by adding a unique alphanumeric suffix to define multiple rules.

guillep2k · 2020-04-22T03:00:27Z

docs/content/doc/advanced/external-renderers.en-us.md

+
+**Note**: The above section numbering policy is new; previously the section was `[markup.sanitizer]` and keys could be redefined.


I had specifically requested some information as this is a breaking change requiring change to config (although the previous config simply does not work.)

Sorry, @zeripath. I missed your comment, which was already resolved.

Signed-off-by: Andrew Thornton <art27@cantab.net>

guillep2k

Only one typo. Otherwise LG-TM.

custom/conf/app.ini.sample

Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>

zeripath · 2020-04-29T11:34:20Z

make lg-tm work

@zeripath

In go-gitea#9888, it was reported that my earlier pull request go-gitea#9075 didn't quite function as expected. I was quite hopeful the `ValuesWithShadow()` worked as expected (and, I thought my testing showed it did) but I guess not. @zeripath proposed an alternative syntax which I like: ```ini [markup.sanitizer.1] ELEMENT=a ALLOW_ATTR=target REGEXP=something [markup.sanitizer.2] ELEMENT=a ALLOW_ATTR=target REGEXP=something ``` This was quite easy to adopt into the existing code. I've done so in a semi-backwards-compatible manner: - The value from `.Value()` is used for each element. - We parse `[markup.sanitizer]` and all `[markup.sanitizer.*]` sections and add them as rules. This means that existing configs will load one rule (not all rules). It also means people can use string identifiers (`[markup.sanitiser.KaTeX]`) if they prefer, instead of numbered ones. Co-authored-by: Andrew Thornton <art27@cantab.net> Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>

cipherboy added 2 commits April 19, 2020 09:46

Update documentation

58e779a

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

Add multiple rule support to markup sanitizer

4042adb

Resolves: 9888 Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

cipherboy force-pushed the fix-sanitizer-config branch from b216247 to 4042adb Compare April 19, 2020 14:16

lunny added the type/enhancement An improvement of existing functionality label Apr 19, 2020

lunny added this to the 1.12.0 milestone Apr 19, 2020

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 19, 2020

zeripath approved these changes Apr 20, 2020

View reviewed changes

docs/content/doc/advanced/external-renderers.en-us.md Outdated Show resolved Hide resolved

docs/content/doc/advanced/config-cheat-sheet.en-us.md Outdated Show resolved Hide resolved

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 20, 2020

zeripath added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Apr 20, 2020

Clarify sanitizer documentation

70941e9

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

guillep2k reviewed Apr 21, 2020

View reviewed changes

Sanitizer Rules -- additional documentation

5226e15

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

guillep2k reviewed Apr 22, 2020

View reviewed changes

zeripath added 2 commits April 28, 2020 19:05

Fix documentation as per @guillep2k

8cceae9

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge remote-tracking branch 'origin/master' into fix-sanitizer-config

d0cc385

guillep2k approved these changes Apr 29, 2020

View reviewed changes

custom/conf/app.ini.sample Outdated Show resolved Hide resolved

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 29, 2020

zeripath and others added 2 commits April 29, 2020 07:05

Update custom/conf/app.ini.sample

adce502

Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>

Merge branch 'master' into fix-sanitizer-config

8260c4a

zeripath merged commit 1bf9e44 into go-gitea:master Apr 29, 2020

zeripath mentioned this pull request May 12, 2020

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

Closed

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sanitizer config - multiple rules #11133

Fix sanitizer config - multiple rules #11133

cipherboy commented Apr 19, 2020

codecov-io commented Apr 19, 2020

6543 commented Apr 19, 2020 •

edited

Loading

lafriks commented Apr 19, 2020

cipherboy commented Apr 19, 2020

zeripath commented Apr 20, 2020

zeripath left a comment

cipherboy commented Apr 20, 2020

guillep2k Apr 21, 2020

cipherboy Apr 21, 2020

guillep2k Apr 21, 2020

cipherboy Apr 21, 2020

zeripath Apr 21, 2020

guillep2k Apr 21, 2020

zeripath Apr 21, 2020

cipherboy Apr 21, 2020

zeripath Apr 21, 2020

guillep2k Apr 22, 2020

guillep2k Apr 22, 2020

guillep2k Apr 22, 2020

guillep2k left a comment

zeripath commented Apr 29, 2020

	; This section can appear with an incremenented number to define multiple rules.
	; This section can appear with an incrementing numbers or any distinct alphanumeric string to define multiple rules.


		Note: The above section numbering policy is new; previously the section was `[markup.sanitizer]` and keys could be redefined.


	Note: The above section numbering policy is new; previously the section was `[markup.sanitizer]` and keys could be redefined.

	Note: Prior to Gitea 1.12 there was a single `markup.sanitiser` section and keys could be redefined, however, there were significant problems with this method of configuration necessitating this change.

	; This section can appear again with a unique alphanmuric string to define multiple rules.
	; This section can appear multiple times by adding a unique alphanumeric suffix to define multiple rules.

Fix sanitizer config - multiple rules #11133

Fix sanitizer config - multiple rules #11133

Conversation

cipherboy commented Apr 19, 2020

codecov-io commented Apr 19, 2020

Codecov Report

6543 commented Apr 19, 2020 • edited Loading

lafriks commented Apr 19, 2020

cipherboy commented Apr 19, 2020

zeripath commented Apr 20, 2020

zeripath left a comment

Choose a reason for hiding this comment

cipherboy commented Apr 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillep2k left a comment

Choose a reason for hiding this comment

zeripath commented Apr 29, 2020

6543 commented Apr 19, 2020 •

edited

Loading