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

Regression: Fix U2F modal bug - backport introduced issue into 1.15 #18096

Closed
fnetX opened this issue Dec 24, 2021 · 15 comments · Fixed by #18107
Closed

Regression: Fix U2F modal bug - backport introduced issue into 1.15 #18096

fnetX opened this issue Dec 24, 2021 · 15 comments · Fixed by #18107
Labels
Milestone

Comments

@fnetX
Copy link
Contributor

fnetX commented Dec 24, 2021

although double check that 1.15 is actually affected.

Originally posted by @zeripath in #18040 (comment)


Well ... The backport #18042 seems to have introduced the bug it was about to fix in Gitea 1.15. As far as I can reproduce locally, Gitea 1.15 was not affected before that PR was merged, but starting with the merge (commit 91f5be8), I can reproduce the issue / all modals pop up.

Reverting the backport appears to be fine.

Noticed this a few days ago, but was bumped by a user reporting this on Codeberg after 1.15.8+ was deployed.

@fnetX fnetX changed the title Please send backport - although double check that 1.15 is actually affected. Regression: Fix U2F modal bug - backport introduced issue into 1.15 Dec 24, 2021
@fnetX
Copy link
Contributor Author

fnetX commented Dec 24, 2021

The report is about the application tab, but "Security" (U2F) and "Applications" tab functionality seems to be affected equally.

@lunny
Copy link
Member

lunny commented Dec 25, 2021

The report is about the application tab, but "Security" (U2F) and "Applications" tab functionality seems to be affected equally.

Opposite, I can reproduce it before #18042 but cannot after it.

@zeripath
Copy link
Contributor

Looking at release/v1.15:

https://github.com/go-gitea/gitea/blob/6eaebda1b58040376d002b7e593dd6a2fc255030/web_src/js/index.js#L2959-2986

The id of the modal to look for should be on the modal-id attribute which matches the form.

The only place that there is a form with the id delete-registration is in security_u2f.tmpl is only ever loaded once in security.tmpl.

To my eyes that looks right and I can't see how #18042 is incorrect.

@zeripath zeripath added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Dec 25, 2021
@fnetX
Copy link
Contributor Author

fnetX commented Dec 25, 2021

I just checked git.disroot.org which also runs Gitea 1.15.8, and I can reproduce the bug there, too.

I can't say much about the code or the background of this bug, only that I can reproduce on different instances including local development and that it's not only my machine, because we got a user report, too. The issue wasn't present for me before. I doubt I can provide more information, as I'm not that familiar with web frontend stuff. 🤷

grafik

@fnetX
Copy link
Contributor Author

fnetX commented Dec 25, 2021

Just to clarify in case this lead to confusion: I'm only talking about the 1.15 branch. I can confirm that the bug is fixed on current main, but the backport seems unnecessary / broken to me.

@lunny lunny added type/bug and removed issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail labels Dec 26, 2021
@lunny lunny added this to the 1.15.9 milestone Dec 26, 2021
@zeripath
Copy link
Contributor

If I download a copy of 1.15.8 direct from gitea.io I can't reproduce this bug - my worry is that because the Javascript file changed we're seeing yet another issue with cached javascript resources.

I can't reproduce this bug on codeberg right now on chrome or firefox.

@wxiaoguang
Copy link
Contributor

Is Codeberg behind some CDN? Is there any cache policy?

@fnetX
Copy link
Contributor Author

fnetX commented Dec 26, 2021

Hmm, but why would this allow for local reproducibility by switching back and forth between resources? Or on Disroot Gitea which I only opened to confirm?
No, Codeberg has no special caching policy, and it's not specific to Codeberg anyway.

@fnetX
Copy link
Contributor Author

fnetX commented Dec 26, 2021

I can reproduce on all mentioned instances (Codeberg, Disroot, local) with both recent Firefox 95.0.2 and Chromium 90.0.4430.212.

@fnetX
Copy link
Contributor Author

fnetX commented Dec 26, 2021

Is there any further info I can try to provide?

@zeripath
Copy link
Contributor

Maybe press CTRL-F5 to forcibly reload js resources. Or check in your console what the function looks like.

If I go to codeberg and look index.js (prettified) for delete-button I see:

                $(".delete-button").on("click", Tu),

which maps to

            function Tu() {
                const e = $(this);
                let t = "";
                e.attr("modal-id") && (t += `#${e.attr("modal-id")}`);
                const i = $(`.delete.modal${t}`);
                return i.find(".name").text(e.data("name")),
...

Whereas on 1.15.7 I see:

            function Tu() {
                const e = $(this);
                let t = "";
                e.attr("id") && (t += `#${e.attr("id")}`);
                const i = $(`.delete.modal${t}`);
                return i.find(".name").text(e.data("name")),
...

My suspicion is that the JS you're seeing is still the 1.15.7 version and that's the cause of the problem.

@lunny
Copy link
Member

lunny commented Dec 26, 2021

Not the u2f UI, but others: emails, tokens, and etc. in user setting. @zeripath

@zeripath
Copy link
Contributor

Ah. I see. We should revert then!

@zeripath
Copy link
Contributor

Revert PR approved @lunny

@zeripath zeripath linked a pull request Dec 28, 2021 that will close this issue
@zeripath
Copy link
Contributor

Fixed by #18107

@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants