Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double escaping attribute values #143

Closed
jtran opened this issue Apr 16, 2022 · 2 comments
Closed

Double escaping attribute values #143

jtran opened this issue Apr 16, 2022 · 2 comments

Comments

@jtran
Copy link

jtran commented Apr 16, 2022

The values of attributes are getting double escaped. As far as I can tell, the problem was introduced in f0057e2. In particular, the double-quote character and non-breaking spaces won't ever make it through sanitizing.

In code, I expect the following test to pass.

func TestQuotesSanitization(t *testing.T) {
	tests := []test{
		{
			in:       `<p title="&quot;"></p>`,
			expected: `<p title="&quot;"></p>`,
		},
		{
			in:       `<p title="&nbsp;"></p>`,
			expected: `<p title="&nbsp;"></p>`,
		},
	}

	p := UGCPolicy()
	p.AllowAttrs("title").OnElements("p")

	// These tests are run concurrently to enable the race detector to pick up
	// potential issues
	wg := sync.WaitGroup{}
	wg.Add(len(tests))
	for ii, tt := range tests {
		go func(ii int, tt test) {
			out := p.Sanitize(tt.in)
			if out != tt.expected {
				t.Errorf(
					"test %d failed;\ninput   : %s\noutput  : %s\nexpected: %s",
					ii,
					tt.in,
					out,
					tt.expected,
				)
			}
			wg.Done()
		}(ii, tt)
	}
	wg.Wait()
}

However, I get this output.

=== RUN   TestQuotesSanitization
    sanitize_test.go:3713: test 1 failed;
        input   : <p title="&nbsp;"></p>
        output  : <p title="&amp;nbsp;"></p>
        expected: <p title="&nbsp;"></p>
    sanitize_test.go:3713: test 0 failed;
        input   : <p title="&quot;"></p>
        output  : <p title="&amp;quot;"></p>
        expected: <p title="&quot;"></p>
@buro9
Copy link
Member

buro9 commented Jul 1, 2022

Looks like I introduced a bug when attempting to fix HREF santization: f0057e2

However when I remove the double-sanitizing in that and test the output of the tests I wrote for that code... it doesn't pose a risk.

That is, removing the double escaping still results in sanitized and safe to use output.

As such, I'm going to remove the part of that prior commit that led to the double escaping.

@buro9 buro9 closed this as completed in cdefdb2 Jul 1, 2022
@zeripath
Copy link
Contributor

zeripath commented Jul 1, 2022

@buro9 will you be releasing a new version of bluemonday or should we (gitea) just pull a head?


Ah I see you've already released 1.0.19 - Thanks!

zeripath added a commit to zeripath/gitea that referenced this issue Jul 1, 2022
The current version of bluemonday is double escaping attributes.

This PR updates bluemonday to the version that fixes this.

(See: microcosm-cc/bluemonday#143 )

Signed-off-by: Andrew Thornton <art27@cantab.net>
6543 pushed a commit to go-gitea/gitea that referenced this issue Jul 1, 2022
The current version of bluemonday is double escaping attributes.

This PR updates bluemonday to the version that fixes this.

(See: microcosm-cc/bluemonday#143 )

Fix #19860

Signed-off-by: Andrew Thornton art27@cantab.net
zeripath added a commit to zeripath/gitea that referenced this issue Jul 3, 2022
Backport go-gitea#20199

The current version of bluemonday is double escaping attributes.

This PR updates bluemonday to the version that fixes this.

(See: microcosm-cc/bluemonday#143 )

Fix go-gitea#19860

Signed-off-by: Andrew Thornton art27@cantab.net
vsysoev pushed a commit to IntegraSDL/gitea that referenced this issue Aug 10, 2022
The current version of bluemonday is double escaping attributes.

This PR updates bluemonday to the version that fixes this.

(See: microcosm-cc/bluemonday#143 )

Fix go-gitea#19860

Signed-off-by: Andrew Thornton art27@cantab.net
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants