Replace '<' and '>' for swarm 'host' field. Fix for XSS attack.

[lhupfeldt]

No description provided.

[codecov]

Codecov Report

Merging #1603 into master will decrease coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1603      +/-   ##
==========================================
- Coverage   82.41%   82.30%   -0.12%     
==========================================
  Files          28       28              
  Lines        2565     2565              
  Branches      390      390              
==========================================
- Hits         2114     2111       -3     
- Misses        355      358       +3     
  Partials       96       96              

Impacted Files	Coverage Δ
locust/web.py	91.56% <100.00%> (ø)
locust/runners.py	83.84% <0.00%> (-0.58%)	⬇️

---

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 dd14aac...0d11817. Read the comment docs.

[cyberw]

Hmm... I think this might be the wrong place to escape this.

Right now host doesnt take a complete url (http:/foo.com/THISSTUFF/) to be used for relative names, but we want to change that (#1591), and in this case we might even want to allow http:/foo.com/?a=1&b=2, which would be broken by this change. Can we instead change it to escape it before showing it back to the user?

XSS is not really a major thing for us, as the web ui should never be exposed to external users (just spawning a test with 10000000 users could at least crash the machine it is running on, so that will always be a bad idea, and I'm sure there are other vulnerabilities as well)

[lhupfeldt]

I think the place is correct. It would be very hard to ensure the security by attempting to replace at every point of use. The actual error found was from injecting a script and then viewing the exception log!
Maybe html.escape is too aggressive, but I was reluctant to pull in a big input validation framework like WTForms for this one field.

 html.escape("http:/foo.com/?a=1&b=2")
'http:/foo.com/?a=1&b=2'

Maybe just replace & back to & would be a quick solution. And then it can be revisited when you allow for more complex URLs.

I agree that most people will not expose Locust to the internet, which is also why I mentioned as not being very critical, regardless of that I still think it should be fixed (I personally will not be allowed to roll out the use of Locust in our organisation until a fix is in place).

[lhupfeldt]

This was the only input validation bug found, after all there is a very limited number of input fields.

[cyberw]

html.escape() only escapes &, < and > anyways. If you change your fix to only do a string replace for < and > instead, then I'm fine with it.

[lhupfeldt]

Done

[cyberw]

Thx!

[lhupfeldt]

Thank you for the quick response

[lhupfeldt]

:( the black check failed I see, but it only says "would be refomattted". I did not see that because I had not raised the number of open files, so never got the black test.

[lhupfeldt]

Shouldn't all the tests be run on the PRs?

[cyberw]

They would have been if I hadnt been so sloppy as to merge it before they had completed :)

Fixed in master now.

[lhupfeldt]

😃

…

On Thu, 22 Oct 2020, 14:43 Lars Holmberg, ***@***.***> wrote: They would have been if I hadnt been so sloppy as to merge it before they had completed :) Fixed in master now. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1603 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMCZSISARWV3JCTKEBGDSDSMASHNANCNFSM4S3BJE6A> .

[lhupfeldt]

@cyberw I'm hoping for this to get released soon. The security team keeps asking for this to be deployed.

[cyberw]

I can do it next week

[lhupfeldt]

Thank you