Skip to content

Conversation

@JamesCullum
Copy link
Contributor

  • Fixed all found absolute links
  • Fixed SSTI test, which failed because it didn't trigger the serverside scoring
  • Fixed timing issues in registerSpec and contactSpec, where the XSS didn't work if the browser was too fast
  • Added code way to simulate a proxy environment ina subfolder. Run via "node test/e2eSubfolder.js"
  • Added e2e test for subfolder. Run via "npm run e2e -- subfolder"
  • Added e2e test for subfolder to travis

JamesCullum and others added 6 commits March 26, 2020 13:29
…directories

Signed-off-by: JamesCullum <JamesCullum@users.noreply.github.com>
Signed-off-by: JamesCullum <JamesCullum@users.noreply.github.com>
- Fixed SSTI test, which failed because it didnÄt trigger the serverside scoring
- Fixed timing issues in registerSpec and contactSpec, where the XSS didn't work if the browser was too fast
- Added code way to simulate a proxy environment ina  subfolder. Run via "node test/e2eSubfolder.js"
- Added e2e test for subfolder. Run via "npm run e2e -- subfolder"
- Added e2e test for subfolder to travis

Signed-off-by: JamesCullum <JamesCullum@users.noreply.github.com>
Signed-off-by: JamesCullum <JamesCullum@users.noreply.github.com>
Signed-off-by: JuiceShopBot <61591748+JuiceShopBot@users.noreply.github.com>
@JamesCullum
Copy link
Contributor Author

Travis doesn't like me apparently - can you check if it was paused or sth?

@bkimminich bkimminich changed the base branch from master to develop March 26, 2020 21:54
@bkimminich
Copy link
Member

I pushed a new branch with all changes from your PR, let's see how far Travis gets with that...

https://travis-ci.org/github/bkimminich/juice-shop/builds/667464194

@bkimminich
Copy link
Member

CI looks good on that branch! I'll merge it and close this PR instead, because I fixed one small integration test expectation there before everything passed.

@JamesCullum
Copy link
Contributor Author

Thanks a lot - saw the integration one failing as well.
Tell me if you need sth else :)

@bkimminich
Copy link
Member

Just one thing I noticed: process.env.basePath is passed in by the e2e tests, so there I understand that redirects after profile image upload and other changes of profile work fine. But on a "normal" server this variable wouldn't be set automatically, or would it?

@JamesCullum
Copy link
Contributor Author

Correct, that's why all usages for this environment variable have a default set like process.env.basePath || '/'

@bkimminich
Copy link
Member

Understood, but if you want to run the server in a subdirectory you must pass in the subsirectory as that variable yourself. I'm essentially asking for the documentation only.

@JamesCullum
Copy link
Contributor Author

JamesCullum commented Mar 27, 2020

I see what you mean, thanks for elaboration. I think it would be the best to document that if you want to run it via reverse proxy in a subfolder, you need to change the baseurl in the subfolder protractor file and launch the subfolder script. Everything should then work as in the test and there is no need to manage additional variables.

The environment variable was just used to not have to read the baseurl multiple times. If you want o have it more verbose, we can just use that snippet multiple times (or have a small include for it for example).

So instead of redirecting http://example.com/subfolder -> localhost:3000, they should redirect to localhost:3001/subfolder

What do you think?

@bkimminich bkimminich merged commit bc9e722 into juice-shop:develop Mar 27, 2020
@bkimminich
Copy link
Member

Hm, can't they just specify the name of the subdirectory in that environment variable and that'd be it?

@bkimminich
Copy link
Member

What I mean: When running in "production" the Juice Shop can neither rely on that new e2eSubfolder.js script from the "test scope", nor on any Protractor config to read base URLs from. Those do not exist in pure --prod installations of Juice Shop. It'd need to either find out where it's running on its own or get it injected - like you did with the environment variable.

@bkimminich
Copy link
Member

I added a config parameter server.basePath (defaults to '') which can be set in a custom config or be overwritten by BASE_PATH environment variable. If it is defined, the server will emit an additional message on startup:

...
info: Port 3000 is available (OK)
info: Server listening on port 3000
info: Server expecting to be proxied under /subfolder

bkimminich referenced this pull request in juice-shop/pwning-juice-shop Mar 27, 2020
(for bkimminich/juice-shop#1356)
@bkimminich
Copy link
Member

image

Does that make sense as an explanation?

@JamesCullum
Copy link
Contributor Author

I thought the two files were provided in a production - if not, it's a different situation to me. Your documentation sounds good to me.

I didn't test it using just the environment variable, just using the e2e file. But as it just runs the proxy and sets the environment variable it should be fine I guess?

bkimminich referenced this pull request in OWASP/www-project-juice-shop Mar 31, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants