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

Fix absolute paths for serveIndex plugin #1353

Closed
wants to merge 5 commits into from
Closed

Fix absolute paths for serveIndex plugin #1353

wants to merge 5 commits into from

Conversation

JamesCullum
Copy link
Contributor

Before: Running juiceshop ina subdirectory broke the links to files that are interacted with using the serve-index package (eg /ftp), as it uses an absolute link. From domain.com/subdir/ftp it will link to domain.com/ftp (which is not available).

Now: All links are rewritten to be relative, so that even if it runs in a subfolder it will continue to work.

There should be no impact for projects in the root directory.

Attribution goes again to Panasonic Information Systems Company Europe GmbH

@JamesCullum
Copy link
Contributor Author

Travis CI test is stuck - maybe you can restart it or skip that test

@J12934
Copy link
Member

J12934 commented Mar 20, 2020

Travis CI test is stuck - maybe you can restart it or skip that test

Restarted the stuck build. Hopefully it behaves this time 😅

@JamesCullum
Copy link
Contributor Author

That one Mac one doesn't want to run - I believe that's why there just was a maintenance of that environment.

@bkimminich
Copy link
Member

There seem to be some general issues with OSX builds at the moment... In another build on develop these failed as well before even doing anything.

@bkimminich
Copy link
Member

Mac builds seem to work again, I'm restarting the job for this PR: https://travis-ci.org/github/bkimminich/juice-shop/builds/665040311

@JamesCullum
Copy link
Contributor Author

Or did it? :P

@bkimminich
Copy link
Member

1 of 3 passed at least...

@JamesCullum
Copy link
Contributor Author

Initial run 2/3 passed - do we know when the issue is fixed on their side?

@JamesCullum
Copy link
Contributor Author

Can you try again?

@bkimminich
Copy link
Member

Please rebase with develop, the unit tests are passing there again now.

@bkimminich
Copy link
Member

Also I've found at least one additional link that's still pointing to an absolute path:

https://github.com/bkimminich/juice-shop/blob/master/frontend/src/app/navbar/navbar.component.ts#L188

There might be more and our test suites won't help us find any broken ones, because it doesn't run the server in a subdirectory. Meaning this is a manual testing exercise that'd need to happen before this gets merged.

@bkimminich bkimminich changed the title Fix absolute paths for serveIndex plugin Fix absolute paths for serveIndex plugin [🚧] Mar 24, 2020
@bkimminich
Copy link
Member

Another candidate? (...and I actually have no time to chase those down...)

https://github.com/bkimminich/juice-shop/blob/develop/frontend/src/app/order-history/order-history.component.ts#L96

Please do not remove the "🚧" until you are sure to have found all instances where this would cause issues for instances in subdirectories. The FTP folder, promotion video, profile etc. all need to be looked over thoroughly.

@JamesCullum
Copy link
Contributor Author

JamesCullum commented Mar 24, 2020

No, all references with hostServer beforehand work, as it is replaced with .

So we need to have tests for this or how do you want to make sure we found everything?

As it is a non-essential feature used by not many people I would fix it as we go.

@bkimminich
Copy link
Member

I don't think this really warrants effortful test automation, so I'd just rely on you having clicked on every link in the entire application... 😆👍

@bkimminich
Copy link
Member

The promotion video screen pulls the video for example from another URL, not sure that'll still work in a subdirectory.

…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>
@bkimminich bkimminich changed the base branch from develop to master March 26, 2020 13:35
@pull-request-size pull-request-size bot added size/L and removed size/S labels Mar 26, 2020
@bkimminich
Copy link
Member

Switched to master as it is in preparation for release and more up to date by now that develop. In both there were the two merge conflicts in e2e tests. The JWT is trivial to fix, the other one is too messed up for me to fix in GitHub Web UI because tests functions were moved around in the meantime.

@JamesCullum
Copy link
Contributor Author

I've fixed it and am currently running tests.
Rebased to develop - should I rebase again for master?

@bkimminich
Copy link
Member

If you did to develop no more conflicts should be on master either when you push! 👍

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

JamesCullum commented Mar 26, 2020

Full changes from commit:

  • 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

Travis appears to be stuck on this PR and doesn't recheck the newest versions

@JamesCullum JamesCullum changed the title Fix absolute paths for serveIndex plugin [🚧] Fix absolute paths for serveIndex plugin Mar 26, 2020
@bkimminich
Copy link
Member

Can you rebase again, then Travis-CI should realize it has a job to do... 👷

@JamesCullum JamesCullum reopened this Mar 26, 2020
@JamesCullum
Copy link
Contributor Author

As Travis didn't trigger for the last changes or rebases as well, I'm not positive on it. Will try a rebase and otherwise try a new PR.

@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.

None yet

3 participants