Prevent possible XSS #18289

Gusted · 2022-01-15T20:03:47Z

In the case of misuse or misunderstanding from a developer whereby, data-panel can receive user-controlled data. $(sel) can lead to the creation of a new element. Current usage is using hard-coded selectors in the templates, but nobody prevents that from expanding to user-controlled somehow.

- In the case of misuse or misunderstanding from a developer whereby, `data-panel` can receive user-controlled data. `$(sel)` can lead to the creation of a new element. Current usage is using hard-coded selectors in the templates, but nobody prevents that from expanding to user-controlled somehow.

Gusted · 2022-01-15T20:08:45Z

The code seems to originate from the BIDI feature(#17562) so I don't think this needs to be backported.

Gusted · 2022-01-15T20:10:51Z

make LGTM work

6543 · 2022-01-15T20:13:00Z

can we add some linter to catch this?

wxiaoguang · 2022-01-15T20:13:46Z

If we want to patch .... we had better patch all the code, but not one by one ....

And it's not a regression bug, a lot of old code looks like this.

Gusted · 2022-01-15T20:20:23Z

If we want to patch .... we had better patch all the code, but not one by one ....

Hmm interesting, this was instance was catched by a CodeQL query I've been running locally. Seems to get confused when it sees a usage that is indirect and inlined.

can we add some linter to catch this?

We might go a bit harsh and ban the usage of $(...)(exception of $(this)?) as it doesn't have 1 defined behavior, as it can be querySelector, but on the wrong data it can create a new element. E.g. a lot of instance like the mentioned one should be done via $.find.

wxiaoguang

I suggest to refactor all similar code together and write the rule into frontend guide, or just keep the code as it was.

Otherwise, this patch doesn't help the code base.

silverwind · 2022-01-16T01:40:53Z

I'm still of the opinion we should just replace jQuery with vanilla JS, e.g. document.querySelectorAll. It's better for performance and vanilla DOM APIs are not really that hard to learn. jQuery has no place in modern code.

Gusted · 2022-01-16T01:50:02Z

I'm still of the opinion we should just replace jQuery with vanilla JS

What's the current argument to not do this? If none, expect a PR from me soon to do this. As that would resolve this issue and future ones entirely.

silverwind · 2022-01-16T02:08:53Z

There's no blocker, it's just that some people like @zeripath seem to prefer jQuery out of habit, but I'd certainly welcome a refactor towards replacing jQuery with vanilla JS.

6543 · 2022-01-16T02:15:17Z

@silverwind @wxiaoguang

I think this pull is fine as is - I do not like heavy code refactor in freezing, if it do not fix an issue ;)

the ongoing discussion about how our frontend should move towards (framework, libs, ...) is important and we should overthing some of it ... - I would suggest to let this pull be reviewed against proposed aim & open a new issue/discurese-thread/... to talk about this - it look's like it (discussion) is needed :)

PS: I personally like typescript - but I wont vote for a specific direction as UI is not my main domain ;)

wxiaoguang

That's fast, awesome.

codecov-commenter · 2022-01-16T04:17:23Z

Codecov Report

❗ No coverage uploaded for pull request base (main@4b4884c). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main   #18289   +/-   ##
=======================================
  Coverage        ?   45.73%           
=======================================
  Files           ?      831           
  Lines           ?    92172           
  Branches        ?        0           
=======================================
  Hits            ?    42153           
  Misses          ?    43257           
  Partials        ?     6762

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 4b4884c...03c50ee. Read the comment docs.

This reverts commit 661d3d2.

* 'main' of https://github.com/go-gitea/gitea: show pull link for agit pull request also (go-gitea#18235) [skip ci] Updated translations via Crowdin Add some .ignore entries (go-gitea#18296) Remove unneeded debug messages to stdout. (go-gitea#18298) Handle missing default branch better in owner/repo/branches page (go-gitea#18290) Revert "Prevent possible XSS when using jQuery (go-gitea#18289)" (go-gitea#18293) not show double error response in git hook (go-gitea#18292) Remove accidental debugging in blob_excerpt.tmpl (go-gitea#18287) Prevent possible XSS when using jQuery (go-gitea#18289) Return nicer error if trying to pull from non-existent user (go-gitea#18288) [skip ci] Updated translations via Crowdin docs: mention client_max_body_size affects LFS (go-gitea#18291) Add lockfile-check (go-gitea#18285) Webauthn nits (go-gitea#18284)

In the case of misuse or misunderstanding from a developer whereby, if `sel` can receive user-controlled data, jQuery `$(sel)` can lead to the creation of a new element. Current usage is using hard-coded selectors in the templates, but nobody prevents that from expanding to user-controlled somehow.

…gitea#18293) This reverts commit 661d3d2.

zeripath approved these changes Jan 15, 2022

View reviewed changes

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 15, 2022

zeripath added type/bug and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 15, 2022

zeripath added this to the 1.16.0 milestone Jan 15, 2022

6543 approved these changes Jan 15, 2022

View reviewed changes

GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jan 15, 2022

zeripath added backport/v1.15 and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Jan 15, 2022

Merge branch 'main' into prevent-xss

e941e4a

GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jan 15, 2022

Gusted removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. backport/v1.15 labels Jan 15, 2022

GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jan 15, 2022

6543 added the issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself label Jan 15, 2022

wxiaoguang removed type/bug issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself labels Jan 15, 2022

wxiaoguang requested changes Jan 15, 2022

View reviewed changes

wxiaoguang removed this from the 1.16.0 milestone Jan 15, 2022

Gusted added 2 commits January 16, 2022 02:05

Improve other usages

9799550

Add docs

1ac0ef0

Gusted requested a review from wxiaoguang January 16, 2022 01:05

6543 approved these changes Jan 16, 2022

View reviewed changes

Merge branch 'main' into prevent-xss

4b2e4d9

6543 added this to the 1.16.0 milestone Jan 16, 2022

6543 added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Jan 16, 2022

wxiaoguang approved these changes Jan 16, 2022

View reviewed changes

Merge branch 'main' into prevent-xss

03c50ee

wxiaoguang merged commit 661d3d2 into go-gitea:main Jan 16, 2022

wxiaoguang added a commit that referenced this pull request Jan 16, 2022

Revert "Prevent possible XSS when using jQuery (#18289)"

1756040

This reverts commit 661d3d2.

wxiaoguang mentioned this pull request Jan 16, 2022

Revert "Prevent possible XSS" #18293

Merged

zeripath pushed a commit that referenced this pull request Jan 16, 2022

Revert "Prevent possible XSS when using jQuery (#18289)" (#18293)

4d0a72a

This reverts commit 661d3d2.

zeripath added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. and removed topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels Jan 16, 2022

Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022

Revert "Prevent possible XSS when using jQuery (go-gitea#18289)" (go-…

fa93977

…gitea#18293) This reverts commit 661d3d2.

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent possible XSS #18289

Prevent possible XSS #18289

Gusted commented Jan 15, 2022

Gusted commented Jan 15, 2022

Gusted commented Jan 15, 2022

6543 commented Jan 15, 2022

wxiaoguang commented Jan 15, 2022 •

edited

Gusted commented Jan 15, 2022

wxiaoguang left a comment

silverwind commented Jan 16, 2022

Gusted commented Jan 16, 2022

silverwind commented Jan 16, 2022

6543 commented Jan 16, 2022 •

edited

wxiaoguang left a comment

codecov-commenter commented Jan 16, 2022

Prevent possible XSS #18289

Prevent possible XSS #18289

Conversation

Gusted commented Jan 15, 2022

Gusted commented Jan 15, 2022

Gusted commented Jan 15, 2022

6543 commented Jan 15, 2022

wxiaoguang commented Jan 15, 2022 • edited

Gusted commented Jan 15, 2022

wxiaoguang left a comment

Choose a reason for hiding this comment

silverwind commented Jan 16, 2022

Gusted commented Jan 16, 2022

silverwind commented Jan 16, 2022

6543 commented Jan 16, 2022 • edited

wxiaoguang left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 16, 2022

Codecov Report

wxiaoguang commented Jan 15, 2022 •

edited

6543 commented Jan 16, 2022 •

edited