Skip to content

Conversation

@philjak
Copy link

@philjak philjak commented Oct 25, 2019

This PR strips the path of the APP_URL from the wrong url.intended string. This causes a correct redirect after login no matter if the APP_URL variable is set or not.

@philjak philjak changed the title Takes APP_URL on redirect after login into account. Fixes issue #1377 Bugfix - takes APP_URL on redirect after login into account. Fixes issue #1377 Oct 25, 2019
@ssddanbrown
Copy link
Member

Hi @philjak,
Thank you for offering this pull request but I'm going to close it for the following reasons:

  1. This pull request is targeted at one specific action (Login redirect) but I'd prefer there to be a fix to the base URL system if there is an issue since this kind of issue could appear elsewhere.
  2. There is no confirmation this will fix the issue URL returned after login may be wrong (rev. proxy and alias used) #1377, I think it's more nuanced than this.
  3. Your current hosting scenario, as described in issue URL returned after login may be wrong (rev. proxy and alias used) #1377, where the public folder is part of the request path, is not a setup that's supported and I'm assuming this pull request was created and tested based upon that scenario.
  4. There's a lot of logic on a single line here that will run for the majority of BookStack user which I am not too comfortable with.
  5. Minor point, but env() is not really to be used outside of the config files due to possible caching.

@philjak
Copy link
Author

philjak commented Oct 27, 2019

Quick comment:

  1. You're right. I also faced this issue when moving a page to another book.
  2. I think it would. The issue seems to be that the session variable "url.intended" (which you get redirected if you do redirect()->intended) has the APP_URL path included and in addition to that the actual intended page url.
  3. This happens no matter if it's /public or another subdirectory.
  4. Understand this.
  5. You're right.

So I understand your decision. But maybe this still helps fixing this issue in future.

Best,
philjak

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants