Handle misencoding of login_source cfg in mssql #16268

zeripath · 2021-06-27T11:02:18Z

Unfortunately due a bug in xorm (see https://gitea.com/xorm/xorm/pulls/1957) updating
loginsources (and a few other places) on MSSQL causes them to become corrupted. (#16252)

Whilst waiting for the referenced PR to be merged and to handle the corrupted
loginsources correctly we need to add a wrapper to the FromDB() methods to look
for and ignore the misplaced BOMs that have been added.

Fix #16252

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

codecov-commenter · 2021-06-27T11:29:21Z

Codecov Report

Merging #16268 (ead13d5) into main (f533b5d) will decrease coverage by 0.00%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##             main   #16268      +/-   ##
==========================================
- Coverage   45.40%   45.39%   -0.01%     
==========================================
  Files         709      709              
  Lines       83496    83492       -4     
==========================================
- Hits        37911    37902       -9     
- Misses      39508    39513       +5     
  Partials     6077     6077

Impacted Files	Coverage Δ
models/login_source.go	`28.64% <45.45%> (+0.66%)`	⬆️
models/repo_unit.go	`88.09% <100.00%> (-0.67%)`	⬇️
modules/util/timer.go	`42.85% <0.00%> (-42.86%)`	⬇️
modules/queue/queue_bytefifo.go	`73.96% <0.00%> (-3.56%)`	⬇️
models/unit.go	`41.09% <0.00%> (-2.74%)`	⬇️
services/pull/pull.go	`42.42% <0.00%> (+0.43%)`	⬆️
modules/queue/workerpool.go	`55.72% <0.00%> (+0.76%)`	⬆️

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 f533b5d...ead13d5. Read the comment docs.

models/login_source.go

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

Unfortunately due a bug in xorm (see https://gitea.com/xorm/xorm/pulls/1957) updating loginsources on MSSQL causes them to become corrupted. (go-gitea#16252) Whilst waiting for the referenced PR to be merged and to handle the corrupted loginsources correctly we need to add a wrapper to the `FromDB()` methods to look for and ignore the misplaced BOMs that have been added. Fix go-gitea#16252 Signed-off-by: Andrew Thornton <art27@cantab.net>

models/login_source.go

noerw

Is Task.PayloadContent not affected? That's also serializing to JSON

zeripath · 2021-06-27T12:36:49Z

The bug only affects convert.Conversion.

zeripath · 2021-06-27T12:38:54Z

Task.PayloadContent doesn't implement convert.Conversion so as far as Xorm is concerned it's just a string.

Take a look at the linked xorm PR.

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

Backport go-gitea#16268 Unfortunately due a bug in xorm (see https://gitea.com/xorm/xorm/pulls/1957) updating loginsources on MSSQL causes them to become corrupted. (go-gitea#16252) Whilst waiting for the referenced PR to be merged and to handle the corrupted loginsources correctly we need to add a wrapper to the `FromDB()` methods to look for and ignore the misplaced BOMs that have been added. Fix go-gitea#16252 Signed-off-by: Andrew Thornton <art27@cantab.net>

Backport #16268 Unfortunately due a bug in xorm (see https://gitea.com/xorm/xorm/pulls/1957) updating loginsources on MSSQL causes them to become corrupted. (#16252) Whilst waiting for the referenced PR to be merged and to handle the corrupted loginsources correctly we need to add a wrapper to the `FromDB()` methods to look for and ignore the misplaced BOMs that have been added. Fix #16252 Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2021-07-15T22:00:45Z

models/login_source.go

+// possible that a Blob may gain an unwanted prefix of 0xff 0xfe.
+func jsonUnmarshalIgnoreErroneousBOM(bs []byte, v interface{}) error {
+	json := jsoniter.ConfigCompatibleWithStandardLibrary
+	err := json.Unmarshal(bs, &v)


This is double indirection!

Suggested change

err := json.Unmarshal(bs, &v)

err := json.Unmarshal(bs, v)

zeripath · 2021-07-15T22:01:00Z

models/login_source.go

+	json := jsoniter.ConfigCompatibleWithStandardLibrary
+	err := json.Unmarshal(bs, &v)
+	if err != nil && len(bs) > 2 && bs[0] == 0xff && bs[1] == 0xfe {
+		err = json.Unmarshal(bs[2:], &v)


This is double indirection!

Suggested change

err = json.Unmarshal(bs[2:], &v)

err = json.Unmarshal(bs[2:], v)

Unfortunately go-gitea#16268 contained a terrible error, whereby there was a double indirection taken when unmarshalling the source data. This fatally breaks authentication configuration reading. Fix go-gitea#16342 Signed-off-by: Andrew Thornton <art27@cantab.net>

Backport go-gitea#16447 Unfortunately go-gitea#16268 contained a terrible error, whereby there was a double indirection taken when unmarshalling the source data. This fatally breaks authentication configuration reading. Fix go-gitea#16342 Signed-off-by: Andrew Thornton <art27@cantab.net>

Once go-gitea#16449 is merged I think we should release 1.14.5. There are a couple of security fixes and the broken go-gitea#16268 is annoying enough that we should just release things. Signed-off-by: Andrew Thornton <art27@cantab.net>

Backport #16447 Unfortunately #16268 contained a terrible error, whereby there was a double indirection taken when unmarshalling the source data. This fatally breaks authentication configuration reading. Fix #16342 Signed-off-by: Andrew Thornton <art27@cantab.net>

Unfortunately #16268 contained a terrible error, whereby there was a double indirection taken when unmarshalling the source data. This fatally breaks authentication configuration reading. Fix #16342 Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io>

Once #16449 is merged I think we should release 1.14.5. There are a couple of security fixes and the broken #16268 is annoying enough that we should just release things. Signed-off-by: Andrew Thornton <art27@cantab.net>

One of the reasons why go-gitea#16447 was needed and why go-gitea#16268 was needed in the first place was because it appears that editing ldap configuration doesn't get tested. This PR therefore adds a basic test that will run the edit pipeline. In doing so it's now clear that go-gitea#16447 and go-gitea#16268 aren't actually solving go-gitea#16252. It turns out that what actually happens is that is that the bytes are actually double encoded. This PR now changes the json unmarshal wrapper to handle this double encode. Fix go-gitea#16252 Signed-off-by: Andrew Thornton <art27@cantab.net>

One of the reasons why #16447 was needed and why #16268 was needed in the first place was because it appears that editing ldap configuration doesn't get tested. This PR therefore adds a basic test that will run the edit pipeline. In doing so it's now clear that #16447 and #16268 aren't actually solving #16252. It turns out that what actually happens is that is that the bytes are actually double encoded. This PR now changes the json unmarshal wrapper to handle this double encode. Fix #16252 Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: 6543 <6543@obermui.de>

…#16465) Backport go-gitea#16465 One of the reasons why go-gitea#16447 was needed and why go-gitea#16268 was needed in the first place was because it appears that editing ldap configuration doesn't get tested. This PR therefore adds a basic test that will run the edit pipeline. In doing so it's now clear that go-gitea#16447 and go-gitea#16268 aren't actually solving go-gitea#16252. It turns out that what actually happens is that is that the bytes are actually double encoded. This PR now changes the json unmarshal wrapper to handle this double encode. Fix go-gitea#16252 Signed-off-by: Andrew Thornton <art27@cantab.net>

Backport #16465 One of the reasons why #16447 was needed and why #16268 was needed in the first place was because it appears that editing ldap configuration doesn't get tested. This PR therefore adds a basic test that will run the edit pipeline. In doing so it's now clear that #16447 and #16268 aren't actually solving #16252. It turns out that what actually happens is that is that the bytes are actually double encoded. This PR now changes the json unmarshal wrapper to handle this double encode. Fix #16252 Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: 6543 <6543@obermui.de>

* Handle misencoding of login_source cfg in mssql Unfortunately due a bug in xorm (see https://gitea.com/xorm/xorm/pulls/1957) updating loginsources on MSSQL causes them to become corrupted. (go-gitea#16252) Whilst waiting for the referenced PR to be merged and to handle the corrupted loginsources correctly we need to add a wrapper to the `FromDB()` methods to look for and ignore the misplaced BOMs that have been added. Fix go-gitea#16252 Signed-off-by: Andrew Thornton <art27@cantab.net> * Update models/login_source.go

Unfortunately go-gitea#16268 contained a terrible error, whereby there was a double indirection taken when unmarshalling the source data. This fatally breaks authentication configuration reading. Fix go-gitea#16342 Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io>

…#16465) One of the reasons why go-gitea#16447 was needed and why go-gitea#16268 was needed in the first place was because it appears that editing ldap configuration doesn't get tested. This PR therefore adds a basic test that will run the edit pipeline. In doing so it's now clear that go-gitea#16447 and go-gitea#16268 aren't actually solving go-gitea#16252. It turns out that what actually happens is that is that the bytes are actually double encoded. This PR now changes the json unmarshal wrapper to handle this double encode. Fix go-gitea#16252 Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: 6543 <6543@obermui.de>

zeripath added type/bug backport/v1.14 labels Jun 27, 2021

zeripath added this to the 1.15.0 milestone Jun 27, 2021

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 27, 2021

zeripath commented Jun 27, 2021

View reviewed changes

models/login_source.go Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

zeripath added a commit to zeripath/gitea that referenced this pull request Jun 27, 2021

Fix go-gitea#16252 - Equivalent to go-gitea#16268

339a74b

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

zeripath force-pushed the fix-16252-handle-bom-in-login_source branch from fb9de8e to d6c6190 Compare June 27, 2021 11:50

zeripath commented Jun 27, 2021

View reviewed changes

models/login_source.go Outdated Show resolved Hide resolved

Update models/login_source.go

ead13d5

noerw reviewed Jun 27, 2021

View reviewed changes

noerw approved these changes Jun 27, 2021

View reviewed changes

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 Jun 27, 2021

zeripath added a commit to zeripath/gitea that referenced this pull request Jun 27, 2021

fixup! Fix go-gitea#16252 - Equivalent to go-gitea#16268

88d9546

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

Merge branch 'main' into fix-16252-handle-bom-in-login_source

084f05e

techknowlogick approved these changes Jun 27, 2021

View reviewed changes

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 Jun 27, 2021

techknowlogick merged commit 9a0cd3a into go-gitea:main Jun 27, 2021

zeripath deleted the fix-16252-handle-bom-in-login_source branch June 27, 2021 20:46

zeripath mentioned this pull request Jun 27, 2021

Handle misencoding of login_source cfg in mssql (#16268) #16275

Merged

zeripath mentioned this pull request Jul 5, 2021

Application crash after authentication sources update #16342

Closed

6 tasks

zeripath commented Jul 15, 2021

View reviewed changes

zeripath mentioned this pull request Jul 15, 2021

Fix crash following ldap authentication update #16447

Merged

zeripath mentioned this pull request Jul 15, 2021

Fix crash following ldap authentication update (#16447) #16448

Merged

zeripath mentioned this pull request Jul 15, 2021

Fix crash following ldap authentication update (#16447) #16449

Merged

zeripath mentioned this pull request Jul 15, 2021

Changelog for v1.14.5 #16450

Merged

zeripath mentioned this pull request Jul 16, 2021

Add basic edit ldap auth test & actually fix #16252 #16465

Merged

zeripath mentioned this pull request Jul 20, 2021

Add basic edit ldap auth test & actually fix #16252 (#16465) #16494

Merged

zeripath mentioned this pull request Jul 20, 2021

Add basic edit ldap auth test & actually fix #16252 (#16465) #16495

Merged

zeripath added the backport/done All backports for this PR have been created label Jul 26, 2021

go-gitea locked and limited conversation to collaborators Oct 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle misencoding of login_source cfg in mssql #16268

Handle misencoding of login_source cfg in mssql #16268

zeripath commented Jun 27, 2021 •

edited

Loading

codecov-commenter commented Jun 27, 2021 •

edited

Loading

This comment has been minimized.

noerw left a comment

zeripath commented Jun 27, 2021 •

edited

Loading

zeripath commented Jun 27, 2021

zeripath Jul 15, 2021

zeripath Jul 15, 2021

zeripath Jul 15, 2021

zeripath Jul 15, 2021

	err = json.Unmarshal(bs[2:], &v)
	err = json.Unmarshal(bs[2:], v)

Handle misencoding of login_source cfg in mssql #16268

Handle misencoding of login_source cfg in mssql #16268

Conversation

zeripath commented Jun 27, 2021 • edited Loading

codecov-commenter commented Jun 27, 2021 • edited Loading

Codecov Report

This comment has been minimized.

noerw left a comment

Choose a reason for hiding this comment

zeripath commented Jun 27, 2021 • edited Loading

zeripath commented Jun 27, 2021

zeripath Jul 15, 2021

Choose a reason for hiding this comment

zeripath Jul 15, 2021

Choose a reason for hiding this comment

zeripath Jul 15, 2021

Choose a reason for hiding this comment

zeripath Jul 15, 2021

Choose a reason for hiding this comment

zeripath commented Jun 27, 2021 •

edited

Loading

codecov-commenter commented Jun 27, 2021 •

edited

Loading

zeripath commented Jun 27, 2021 •

edited

Loading