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

CT handling of stringTest=xss #64

Closed
2 of 5 tasks
pixelzoom opened this issue Jan 17, 2019 · 4 comments
Closed
2 of 5 tasks

CT handling of stringTest=xss #64

pixelzoom opened this issue Jan 17, 2019 · 4 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 17, 2019

Over in phetsims/projectile-motion#171, CT failures were noted with header "projectile-motion : xss-fuzz : run". I assume that this means something like ?ea&stringTest=xss&fuzz. And in fact I was only able to reproduce these failures with ?ea&stringTest=xss.

From the code-review check list:

  • Does the sim stay on the sim page (doesn't redirect to an external page) when running with the query parameter stringTest=xss? This test passes if sim does not redirect, OK if sim crashes or fails to fully start. Only test on one desktop platform.

So there is no requirement to fix anything other than a redirect for stringTest=xss.

Questions:

  • (1) Does "projectile-motion : xss-fuzz : run" indeed mean ?ea&stringTest=xss&fuzz ?

  • (2) Why are we running stringTest=xss and fuzz together?

  • (3) Why are we reporting anything other than a redirect for stringTest=xss?

  • (4) Should we change our policy for stringTest=xss, or tolerate related CT failures? The latter seems problematic.

@pixelzoom
Copy link
Contributor Author

(1) yes, in continuous-server.js "xss-fuzz" uses query parameters brand=phet&ea&fuzz&stringTest=xss&memoryLimit=1000.

@jonathanolson
Copy link
Contributor

(2) Why are we running stringTest=xss and fuzz together?

Sometimes having the sim show certain screens/scenes/situation may trigger XSS failures, so there is a very slight advantage to do this (but may not be worth the additional time)

(3) Why are we reporting anything other than a redirect for stringTest=xss?

We can always update guidelines, but it would be nice to fix things so errors don't come up if possible (or we'd want to find a way to suppress things in the CT reports).

(4) Should we change our policy for stringTest=xss

What would you recommend?

@pixelzoom
Copy link
Contributor Author

Looks like this issue went into limbo because it's not assigned. It's now almost 19 months old. Re-labeling for developer meeting and assigning to @ariel-phet.

@jessegreenberg
Copy link
Contributor

8/13/20

We discussed that it is generally useful to run ?fuzz with ?stringTest=xss for the reasons in #64 (comment). It is worth the time on CT and catching the rare issue it finds is worth the time.

This test combination IS currently being run on CT. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants