4 ms·

This is a weird post to be honest. You've found a whole bunch of serious security issues, filed two PRs, one of which is adding some quotes because > Those are

by jorams 6mo ago

This is a weird post to be honest. You've found a whole bunch of serious security issues, filed two PRs, one of which is adding some quotes because > Those aren't exploitable XSS, but it doesn't hurt to have a second layer of defense. The other suggests breaking clients that aren't using the more secure version of an OAuth method because > I can't think of any OAuth client that would like to [use it] That second one is a good idea, but the maintainer is also right to ask for some discussion before introducing a breaking change. But crucially: neither of these are the kind of significant security issues you've found. Maybe lead with an actual bug?

arcfour 6mo ago
Closing the PR without providing feedback beyond "needs further discussion" does not engender said further discussion.
- PunchyHamster 6mo ago
  PR isn't a place for discussion about what or how to implement change in the first place, that should be forum/mailing list/issues and there is open issue for that discussion https://codeberg.org/forgejo/forgejo/issues/8634 https://codeberg.org/forgejo/forgejo/issues/8634
  - radlad 6mo ago
    #8634 is specifically about a breaking change that occurred in v12. It's literally the first line of what you linked: > In the v12 release of Forgejo (fixed in v12.0.1) there have been breaking changes that impact third-party authentication sources that use Forgejo as a provider. If you have been affected, please help us assessing the impact ...
- henryteeare 6mo ago
  The response was, "needs a discussion," as in a post on `https://codeberg.org/forgejo/discussions https://codeberg.org/forgejo/discussions`, rather than directly creating a PR. There also was feedback saying approximately that they've been burned by security changes in the recent past and don't want to run into similar issues without due consideration.
PunchyHamster 6mo ago
> That second one is a good idea, but the maintainer is also right to ask for some discussion before introducing a breaking change. The discussion seems to be already happening https://codeberg.org/forgejo/forgejo/issues/8634 https://codeberg.org/forgejo/forgejo/issues/8634, author of the blog just did drive-by PR rather than looking at issue tracker It's very much "I know better, do what I told you despise not thinking a second about any second order effects the change might cause" attitude that is so common with security people
- henryteeare 6mo ago
  I believe the discussion in #8634 is for a different change, but one of a similar nature.
  - embedding-shape 6mo ago
    It's not, the maintainer has pointed to that discussion multiple times to the author of the submission, saying they need to resolve that before they can just straight up deprecate authentication methods without any alternatives available to users currently using it.
    - radlad 6mo ago
      I'm really confused by this interpretation. I see a single comment by the maintainer, saying: > That mistake was made in the past (#8634), where there was still a lot of usages of a old and announced deprecated method (and even with quite some effort there is). It was a related, but separate issue, which is perhaps best-described in this upstream issue: https://github.com/python-social-auth/social-core/issues/1211 https://github.com/python-social-auth/social-core/issues/121... The "plain" setting jvoison wants to remove is described here: https://security.stackexchange.com/a/218554 https://security.stackexchange.com/a/218554 I do agree with the maintainer that a discussion is warranted before removing this setting. But I also wouldn't personally have closed the PR while waiting for said discussion to occur - and the maintainer could have created a discussion themselves. They are signaling they don't want this change, full stop.
bogwog 6mo ago
And attempting to publicly shame them into accepting a PR. Kinda reminds me of https://en.wikipedia.org/wiki/XZ_Utils_backdoor https://en.wikipedia.org/wiki/XZ_Utils_backdoor

by jorams 6mo ago

arcfour 6mo ago

Closing the PR without providing feedback beyond "needs further discussion" does not engender said further discussion.

PunchyHamster 6mo ago
PR isn't a place for discussion about what or how to implement change in the first place, that should be forum/mailing list/issues and there is open issue for that discussion https://codeberg.org/forgejo/forgejo/issues/8634 https://codeberg.org/forgejo/forgejo/issues/8634
- radlad 6mo ago
  #8634 is specifically about a breaking change that occurred in v12. It's literally the first line of what you linked: > In the v12 release of Forgejo (fixed in v12.0.1) there have been breaking changes that impact third-party authentication sources that use Forgejo as a provider. If you have been affected, please help us assessing the impact ...
henryteeare 6mo ago
The response was, "needs a discussion," as in a post on `https://codeberg.org/forgejo/discussions https://codeberg.org/forgejo/discussions`, rather than directly creating a PR. There also was feedback saying approximately that they've been burned by security changes in the recent past and don't want to run into similar issues without due consideration.

PunchyHamster 6mo ago

> That second one is a good idea, but the maintainer is also right to ask for some discussion before introducing a breaking change. The discussion seems to be already happening https://codeberg.org/forgejo/forgejo/issues/8634 https://codeberg.org/forgejo/forgejo/issues/8634, author of the blog just did drive-by PR rather than looking at issue tracker It's very much "I know better, do what I told you despise not thinking a second about any second order effects the change might cause" attitude that is so common with security people

henryteeare 6mo ago
I believe the discussion in #8634 is for a different change, but one of a similar nature.
- embedding-shape 6mo ago
  It's not, the maintainer has pointed to that discussion multiple times to the author of the submission, saying they need to resolve that before they can just straight up deprecate authentication methods without any alternatives available to users currently using it.
  - radlad 6mo ago
    I'm really confused by this interpretation. I see a single comment by the maintainer, saying: > That mistake was made in the past (#8634), where there was still a lot of usages of a old and announced deprecated method (and even with quite some effort there is). It was a related, but separate issue, which is perhaps best-described in this upstream issue: https://github.com/python-social-auth/social-core/issues/1211 https://github.com/python-social-auth/social-core/issues/121... The "plain" setting jvoison wants to remove is described here: https://security.stackexchange.com/a/218554 https://security.stackexchange.com/a/218554 I do agree with the maintainer that a discussion is warranted before removing this setting. But I also wouldn't personally have closed the PR while waiting for said discussion to occur - and the maintainer could have created a discussion themselves. They are signaling they don't want this change, full stop.

bogwog 6mo ago

And attempting to publicly shame them into accepting a PR. Kinda reminds me of https://en.wikipedia.org/wiki/XZ_Utils_backdoor https://en.wikipedia.org/wiki/XZ_Utils_backdoor