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

Use relative paths #1221

Merged
merged 7 commits into from Sep 23, 2019
Merged

Use relative paths #1221

merged 7 commits into from Sep 23, 2019

Conversation

@JamesCullum
Copy link
Contributor

JamesCullum commented Sep 18, 2019

In our company, we would like to run juice-shop as a docker below a certain folder, eg http://example.com/juice-shop (via reverse proxy). This helps us reduce overhead (creating and managing new domain, approvals etc) and helps not being enumerable via DNS.

However, at the moment juice-shop has the root directory hardcoded at a few places, so while the framework supports it, a lot of files are referenced via root.

The minor changes here have been locally tested and allow juice-shop to work in whatever directory it is located in.

This development has been sponsored by Panasonic Information Systems Company Europe.

bkimminich and others added 2 commits Sep 13, 2019
[ci skip]
…directory of a domain
@pull-request-size pull-request-size bot added the size/XS label Sep 18, 2019
@bkimminich bkimminich changed the base branch from master to develop Sep 18, 2019
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Sep 18, 2019
@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 18, 2019

That's a good one, thank you! I switched it to develop branch and will merge this if CI passes. For you to use it immediately you'd have to use the snapshot-tagged Docker image until the next official release.

@@ -124,7 +124,7 @@ import { DeluxeUserComponent } from './deluxe-user/deluxe-user.component'
import { AccountingGuard, AdminGuard, LoginGuard, DeluxeGuard } from './app.guard'

export function HttpLoaderFactory (http: HttpClient) {
return new TranslateHttpLoader(http, './../assets/i18n/', '.json')
return new TranslateHttpLoader(http, './assets/i18n/', '.json')

This comment has been minimized.

Copy link
@bkimminich

bkimminich Sep 19, 2019

Owner

This still works without navigating one directory up?

This comment has been minimized.

Copy link
@JamesCullum

JamesCullum Sep 20, 2019

Author Contributor

At least for me it works, as the "/#/" is ignored for me, so one directory up is one above the relative directory. I think for root it stays at root, so that's why it wasn't noticed.

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 19, 2019

Did you try running npm run protractor to see if all end-to-end tests still work? On Travis-CI one of them keeps failing no matter how often I re-ran them: The DOM XSS challenge.

@JamesCullum

This comment has been minimized.

Copy link
Contributor Author

JamesCullum commented Sep 20, 2019

The DOM XSS challenge is unlocked via socket and the API doesn't accept only the dot as servername, so the connection failed. Trying to fix it but I can't test properly locally, as tests keep timing out for me.

JamesCullum added 2 commits Sep 20, 2019
@bkimminich bkimminich changed the title Use relative paths Use relative paths [🚧] Sep 22, 2019
JamesCullum added 2 commits Sep 23, 2019
@JamesCullum JamesCullum reopened this Sep 23, 2019
@JamesCullum

This comment has been minimized.

Copy link
Contributor Author

JamesCullum commented Sep 23, 2019

Alright, so the issue was caused by socket.io listening on a default URL path of "socket.io", which needs to be added to the subdirectory. I also had to restart Travis once by closing and re-oping the PR, as a test failed in a different part of the application.

@JamesCullum JamesCullum changed the title Use relative paths [🚧] Use relative paths Sep 23, 2019
@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 23, 2019

Woohoo, awesome!

@bkimminich bkimminich merged commit ea681fc into bkimminich:develop Sep 23, 2019
5 checks passed
5 checks passed
WIP Ready for review
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - frontend/package.json (bkimminich) No new issues
Details
security/snyk - package.json (bkimminich) No new issues
Details
@JamesCullum JamesCullum deleted the JamesCullum:relative-path branch Sep 23, 2019
@JamesCullum

This comment has been minimized.

Copy link
Contributor Author

JamesCullum commented Sep 23, 2019

Great, thanks a lot! Would it be possible to add "Panasonic Information Systems Company Europe" to the package.json contributor list? Would be highly appreciated for the managers!

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 23, 2019

I can only add GitHub user names as contributors, not corporations. But if you'd like some swag, I can send stickers and postcards your way. Just mail me your post address.

@JamesCullum

This comment has been minimized.

Copy link
Contributor Author

JamesCullum commented Sep 24, 2019

As the company paid for the development and testing, it would be great to honor them instead of me. Why is it only possible to add Github usernames? At the moment there are normal names available as well and according to the documentation that's conform to standards.

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 24, 2019

Last time I checked corporate acknowledgements are only allowed by OWASP policy on a project Wiki page in the "Acknowledgements" tab. And only for monetary donations, not for contribution of time or effort in any form.

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 24, 2019

Also, as you are using a pseudonym account, how would I be able to verify the company you are working for? Using a GitHub account that has ties to e.g. a Panasonic Github organization would have been the easiest way to achieve what you'd like.

@JamesCullum

This comment has been minimized.

Copy link
Contributor Author

JamesCullum commented Sep 24, 2019

For eg DefectDojo corporate acknowledgements are also possible for contributions (which we used), but this project doesn't have this officially advertised indeed. To verify an identity, I can provide a work email account, so that shouldn't be an issue. Panasonic doesn't have an official Github account and we focus on sponsorship to contribute to open-source projects, as we otherwise can't get it officially signed off. As my commits for work always reference the company it should be clear that those commits are part of work and sponsored by the company.

So there is no way for you to willingly add acknowledgements for the company? Then I would probably just edit the PR to reflect it more.

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 24, 2019

I'll look into how DefectDojo is doing it and will bring this up with OWASP staff or in the OWASP Leadership meeting tomorrow during GlobalAppSec AMS. I could imagine having a section on the Wiki for corp-powered code contributions... Will let you know then!

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 24, 2019

Okay, found it: https://github.com/DefectDojo/django-DefectDojo/blob/master/SPONSORING.md

I will definitely not do anything like counting lines of code for points, as LOC is not about the quality of a contribution... 😁 But the way I see it, any company sponsoring their developer's time would be eligible for being mentioned on the Acknowledgements page as if they donated an amount <1000$ - just with name and link but no logo.

So, to conclude this: If you name-drop Panasonic somewhere in the PR, then I'll add them to the Ack-page on the Wiki. Also please let me know the exact name and link you'd like in there.

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 24, 2019

Added them as "Panasonic Information Systems Company Europe" linking to https://is-c.panasonic.co.jp/en - just let me know if you'd prefer a different link! 👍

@JamesCullum

This comment has been minimized.

Copy link
Contributor Author

JamesCullum commented Sep 25, 2019

Wow, that's great, thanks a lot! If you could this link, that'd be great (as it's for our specific branch in Europe): https://application.job.panasonic.eu/data/ruP0pHQvHrGZJKvL/rc.php?nav=jobsearch&custval12=ite&lang=EN&custval11=PBSEU_GER

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 25, 2019

Done! Thanks again for your contribution! 💯

@bkimminich

This comment has been minimized.

Copy link
Owner

bkimminich commented Sep 27, 2019

Hi again, after talking about the whole code-contribution topic with a few other OWASP people I concluded that OWASP would actually need to see some kind official "donation confirmation" from Panasonic to make this an official thing and keep them on the donors list. At the moment I basically took your word for the fact that you did this PR for Panasonic. Also I cannot be sure that your contribution on Panasonic time is not seen by the company as their intellectial property. If that's the case then that would normally be your problem to resolve with them, but by acknowledging them as corporate donors I'm kind of making this my problem. I am no lawyer and honestly don't want to spend my personal time on this kind of stuff. The following would solve this in a quick way:

  • Panasonic donates 1$+ earmarked for the Juice Shop project and they're sponsors like anybody else (preferred)
  • Panasonic gets in touch with OWASP Foundation asking for a way to formally confirm a code contribution that includes a) the equivalent monetary value and b) waives all future IP rights on the code (more complicated but might pave the way for future similar code donations to be handled in a proper way)

I'll take down the mention until this is resolved either way. Sorry for the inconvenience, but again: I assume we're both not lawyers, so it's safer for both of us that way... ;-)

bkimminich added a commit to OWASP/www-project-juice-shop that referenced this pull request Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.