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

feat: use uri-reference for ui_url etc. to allow relative urls #617

Merged
merged 9 commits into from Aug 10, 2020

Conversation

alsuren
Copy link
Contributor

@alsuren alsuren commented Jul 31, 2020

Currently ui_url for each of the flows is a uri. This means that we have to generate a new config file for each developer's cluster, and another one for production as they are each served from a different domain.

We are hosting kratos and our webserver behind an nginx proxy, so specifying a relative uri like /auth/login would solve this problem for us..

How do people feel about allowing relative urls like /auth/login to be specified as the ui_urls? If nobody can think of a security reason against this, I will happily continue with this PR, as it would save us a bunch of hassle generating config files.

  • I have manually checked that my other changes work by running the quickstart app on a different port
  • I still need to check that my change to whitelisted_return_urls works as expected.

Related issue

Proposed changes

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2020

CLA assistant check
All committers have signed the CLA.

@zepatrik
Copy link
Member

zepatrik commented Aug 2, 2020

I think we then have to add a base_uri somewhere?
So in case of uri-reference kratos would do a relative redirect, I see.

@aeneasr
Copy link
Member

aeneasr commented Aug 3, 2020

This should work as long as the proxy/load balancer preserve the Host HTTP Header - so it makes sense IMO.

@alsuren alsuren changed the title PoC: use uri-reference for ui_url to allow relative urls feat: use uri-reference for ui_url etc. to allow relative urls Aug 5, 2020
@alsuren alsuren marked this pull request as ready for review August 5, 2020 14:42
@aeneasr
Copy link
Member

aeneasr commented Aug 6, 2020

I still need to check that my change to whitelisted_return_urls works as expected.

Have you done that already?

@alsuren
Copy link
Contributor Author

alsuren commented Aug 6, 2020

I still need to check that my change to whitelisted_return_urls works as expected.

Have you done that already?

Not yet. Thanks for reminding me. I ended up writing a unit test for this, but haven't yet tested that bit manually. This is a bit of a last-minute addition to this PR. I will try to do some manual testing this afternoon.

@alsuren
Copy link
Contributor Author

alsuren commented Aug 6, 2020

Tested with this patch locally:

diff --git a/contrib/quickstart/kratos/email-password/.kratos.yml b/contrib/quickstart/kratos/email-password/.kratos.yml
index f545fe3f..df66303b 100644
--- a/contrib/quickstart/kratos/email-password/.kratos.yml
+++ b/contrib/quickstart/kratos/email-password/.kratos.yml
@@ -5,9 +5,9 @@ serve:
     base_url: http://kratos:4434/
 
 selfservice:
-  default_browser_return_url: http://127.0.0.1:4455/
+  default_browser_return_url: /thing
   whitelisted_return_urls:
-    - http://127.0.0.1:4455
+    - /dashboard
 
   strategies:
     password:
diff --git a/quickstart.yml b/quickstart.yml
index 21fe28f7..56100040 100644
--- a/quickstart.yml
+++ b/quickstart.yml
@@ -33,7 +33,7 @@ services:
   kratos:
     depends_on:
       - kratos-migrate
-    image: oryd/kratos:latest-sqlite
+    image: oryd/kratos:latest
     ports:
       - "4433:4433" # public
       - "4434:4434" # admin
[
  {
    "code": 400,
    "status": "Bad Request",
    "reason": "Requested return_to URL \"http://127.0.0.1:4455/dashboard\" is not whitelisted.",
    "debug": "Whitelisted domains are: [{    /dashboard  false  } {http   127.0.0.1:4455 /.ory/kratos/public/self-service  false  }]",
    "message": "The request was malformed or contained invalid parameters"
  }
]

@aeneasr
Copy link
Member

aeneasr commented Aug 10, 2020

Could you please rebase onto master so that the tests pass (hopefully 😄 )?

@aeneasr
Copy link
Member

aeneasr commented Aug 10, 2020

Sorry, e2e test fail because of something else. We're working on it.

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.

None yet

4 participants