yii2 icon indicating copy to clipboard operation
yii2 copied to clipboard

Improve Security on Form Actions

Open badbreze opened this issue 1 year ago • 7 comments

Q A
Is bugfix?
New feature?
Breaks BC?
Fixed issues

This PR is intended to secure the Action of the Forms by cleaning up malicious codes from the provider URL as much as possible to avoid XSS

badbreze avatar Feb 15 '24 12:02 badbreze

PR Summary

  • Introduction of getSafeUrl method in yii.js A new function called getSafeUrl has been included in the javascript file yii.js. The purpose of this function is to ensure the security and accuracy of URLs in use. It does so by strictly allowing only alphanumeric characters and a select set of symbols (&, ?, =, [, ], /, :), removing anything else to avoid any potential misuse or errors.

what-the-diff[bot] avatar Feb 15 '24 12:02 what-the-diff[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 48.02%. Comparing base (0027227) to head (9dccfc5). Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #20116   +/-   ##
=======================================
  Coverage   48.02%   48.02%           
=======================================
  Files         445      445           
  Lines       43892    43892           
=======================================
  Hits        21080    21080           
  Misses      22812    22812           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 15 '24 12:02 codecov[bot]

How is this supposed to prevent XSS?

rob006 avatar Feb 15 '24 13:02 rob006

@badbreze can you please respond to the comment by @rob006?

mtangoo avatar Feb 20 '24 08:02 mtangoo

How is this supposed to prevent XSS?

the filter prevents malicious code to be injected from the browser URL to the jQuery selector arguments. in this case the Action of the form.

badbreze avatar Feb 20 '24 10:02 badbreze

@badbreze Can you explain how do we get XSS attack when using window.location.href given that this call is to the browser and on the client side?

mtangoo avatar Feb 20 '24 10:02 mtangoo

the filter prevents malicious code to be injected from the browser URL to the jQuery selector arguments. in this case the Action of the form.

I that case jQuery should encode action attribute, which prevents XSS. XSS protection relies on proper encoding of content, not on stripping some chars from data.

rob006 avatar Feb 20 '24 13:02 rob006

We need to start banning people, this is terrorism. Totally nonsense PR. Are you really still using Yii's client-side scripts, to the point where security is a concern? Really? Please, just waste your time elsewhere, preferably learning CS

Webkadabra avatar Feb 26 '24 17:02 Webkadabra

@Webkadabra Learning discussing without name calling and intimidation is possible and encouraged in Yii community. We encourage people here to make PR. We debate it based on merits and agree to merge or close based on merits. Your comment is not with that spirit, in places you comment.

Please take note and correct that one. Welcome to Yii community!

mtangoo avatar Feb 26 '24 18:02 mtangoo

The thing is that we're passing arguments to jQuery directly from the URL, an extremely vulnerable door to the application code, so the PR is (at least in my case) a way to try securing this possible flaw, but i let you decide if this can be accepted or not.

badbreze avatar Feb 26 '24 18:02 badbreze

The thing is that we're passing arguments to jQuery directly from the URL, an extremely vulnerable door to the application code, so the PR is (at least in my case) a way to try securing this possible flaw, but i let you decide if this can be accepted or not.

I get your point. tried to check and see how JQuery deals with encoding, I could not find decent docs. But if you want to propertly encode, may be you should look into native functions for that than simple regex replacement? Something like encodURI?

mtangoo avatar Feb 26 '24 18:02 mtangoo

This is not about encoding an URL, but about encoding a HTML attribute. I think we can safely assume that the most popular JS library can do such basic task. :)

rob006 avatar Feb 26 '24 19:02 rob006

@Webkadabra Learning discussing without name calling and intimidation is possible and encouraged in Yii community. We encourage people here to make PR. We debate it based on merits and agree to merge or close based on merits. Your comment is not with that spirit, in places you comment.

Please take note and correct that one. Welcome to Yii community!

please point to the name calling you're so offended by

Webkadabra avatar Feb 26 '24 19:02 Webkadabra

This is not about encoding an URL, but about encoding a HTML attribute. I think we can safely assume that the most popular JS library can do such basic task. :)

Let me have little sleep, lol :)

mtangoo avatar Feb 26 '24 19:02 mtangoo

please point to the name calling you're so offended by

"Totally nonsense PR" The rest can be read on your comment it is still there. You couldn't discuss PR without calling it nonsense? And there are other comments in other PR, that are offensive as well.

mtangoo avatar Feb 26 '24 19:02 mtangoo

please point to the name calling you're so offended by

"Totally nonsense PR" The rest can be read on your comment it is still there. You couldn't discuss PR without calling it nonsense? And there are other comments in other PR, that are offensive as well.

this is factually a nonsense PR, too bad you're taking coding personally. You shouldv'e been trying to get good at coding, but you mastered getting offended

Webkadabra avatar Feb 26 '24 19:02 Webkadabra

@Webkadabra I suggest you to pick one of many open issues, create a PR to fix it and show us all how good coding looks like. So far all your comments were extremity unconstructive and probably wasted more people's time and mental energy than PRs you were commenting.

rob006 avatar Feb 26 '24 19:02 rob006

@Webkadabra I suggest to you to pick one of many open issues, create a PR to fix it and show us all how good coding looks like. So far all your comments were extremity unconstructive and probably wasted more people's time and mental energy than PRs you were commenting.

I'm in shock by the random PRs of my favourite framework. I bet your wife may appreciate advice on what to do in life, but you are clearly out of order telling people what to do, assumig anything. Welcome to software developoment, you're not under your mom's protection anymore lol. I've only commented on PRs that suggest irrelevant code changes while also drawing a lot of attention. I'm sorry you have to lie publicly assuming MY comments took more time, haha. Well, if you're spending as much tought process replying in technical threads as you do with your weak attempt at starting a scandal, then sure - my comments probably wasted a lot of your time. But I see a ton of PRs with completely unnecessary, irrelevant and project-specifi changes to the framework code where people spend time explaining to guy like you that 2+2 is 4 and there is no need to say that it's not 3

Webkadabra avatar Feb 26 '24 19:02 Webkadabra

@Webkadabra kindly take @rob006's advice and help us with your great skills iron bugs. It is welcome and appreciated. Bragging and discouraging people who took their time to make actual PR is discouraged and unwelcome.

Again take note that you need to be respectful towards others here, even if you think you are the genius in the room!

mtangoo avatar Feb 26 '24 19:02 mtangoo

@Webkadabra kindly take @rob006's advice and help us with your great skills iron bugs. It is welcome and appreciated. Bragging and discouraging people who took their time to make actual PR is discouraged and unwelcome.

Again take note that you need to be respectful towards others here, even if you think you are the genius in the room!

You tell me what to do, lie about name calling, then ask me to be respectful. Are you out of your mind? Who gave you the computer, did they not tell you to behave well?

Webkadabra avatar Feb 26 '24 19:02 Webkadabra

@Webkadabra I suggest you to pick one of many open issues, create a PR to fix it and show us all how good coding looks like. So far all your comments were extremity unconstructive and probably wasted more people's time and mental energy than PRs you were commenting.

Oh sir with the advice, have you seen these braindead issues? A guy creates an infinite loop and claims it's Yii memory leak. Really, pick an issue, haha. The main issue is PR and issue spamming right now, with some villagers yelling god knows what

Webkadabra avatar Feb 26 '24 19:02 Webkadabra

@samdark @bizley Just block this guy, this is leading nowhere.

schmunk42 avatar Feb 26 '24 19:02 schmunk42

@schmunk42 done. @Webkadabra if you want to be un-blocked, please contact me directly and be prepared to correct the way you communicate.

samdark avatar Feb 27 '24 06:02 samdark