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(admin-ui): Allow serving admin-ui on custom domains #1386

Conversation

ashitikov
Copy link
Contributor

@ashitikov ashitikov commented Jan 28, 2022

Suppose you have domain vendure.local and you resolve this domain to 127.0.0.1 via /etc/hosts.
Using compileUiExtensions with enabled dev mode you will be able to compile and serve, but accessing https://vendure.local:4002/admin you will get Invalid host header error.

Supplying parameter disableHostCheck to angular.json fix this issue.
Serving happens only on dev mode, so no strict safety is needed.

…llows accessing admin-ui on custom local domains, that differs from localhost
@michaelbromley
Copy link
Member

Hi,
Thanks for the PR and for bringing this issue to my attention. I did some more research on this issue and I'm not comfortable with disabling the host check by default. I know the risk is probably fairly minimal, but this thread has a good explanation of the attack that this check is designed to mitigate.

I would rather solve this by adding a new option to the AdminUiPlugin init options which allows you to pass arbitrary arguments to the compileUiExtensions function which will then get passed to ng serve or ng build. This would allow you to pass the --disable-host-check arg, and also make it easy to handle other issues that people run into with the Angular config.

@ashitikov
Copy link
Contributor Author

Good catch.

I did some improvements as you pointed, but unfortunately unable to test this on dev-server due to

[run:server] Error: Cannot find module '@vendure/admin-ui'
[run:server] Require stack:
[run:server] - /Users/me/vendure/packages/ui-devkit/compiler/scaffold.js

lerna bootstrap + lerna build is okay.

This happens each time I'm trying to use compileUiExtensions on dev config.
@michaelbromley Do you have any ideas why this happens? Can you review these changes please?

Copy link
Member

@michaelbromley michaelbromley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Just a couple of changes in the documentation is all.

Also, can you please make this PR against the minor branch? Since it is a new feature, per semver it will go in the next minor release (1.5.0).

Regarding problems getting compileUiExtensions() to work in the dev server - yes I am aware of this and I believe it relates to the way Lerna symlinks the dependencies. For testing that part, I usually use Verdaccio to publish a local version (there's even an npm script for that in the root package.json) and then install it in a different location and test that way. A bit slow I know, but it works.

packages/ui-devkit/src/compiler/types.ts Show resolved Hide resolved
@ashitikov ashitikov changed the base branch from master to minor March 5, 2022 19:35
# Conflicts:
#	packages/ui-devkit/src/compiler/compile.ts
#	packages/ui-devkit/src/compiler/types.ts
@ashitikov
Copy link
Contributor Author

Hey, fixed conflicts. Made some changes to pass arguments in more elegant way

@ashitikov ashitikov changed the base branch from minor to master March 5, 2022 22:09
@ashitikov ashitikov changed the base branch from master to minor March 5, 2022 22:10
@michaelbromley michaelbromley merged commit d47df21 into vendure-ecommerce:minor Mar 7, 2022
@michaelbromley
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants