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

Fixed #12772 - use the APP_URL config more consistently #12793

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

snipe
Copy link
Owner

@snipe snipe commented Apr 5, 2023

I don't think this should harm anything here - we switched over to using config('app.url') a while back to better handle folks running Snipe-IT in a subdirectory, but I don't think we updated it everywhere.

Signed-off-by: snipe <snipe@snipe.net>
@what-the-diff
Copy link

what-the-diff bot commented Apr 5, 2023

PR Summary

  • Update url() helper and fix hardcoded urls
    The url() helper now uses the app configuration instead of hardcoded urls in various parts of the code.

  • Fix image path in requested.blade.php
    The image path is now fixed and more reliable for requests made through the website.

  • Use app configuration for URLs in multiple files
    Removed hardcoded URLs and now use the app's configuration for better consistency and maintainability.

  • Add baseUrl meta tag
    Introduced a new baseUrl meta tag with the app's URL value for better organization and easier management.

  • Pull logo image and site link from configuration
    Both logo image and site link in the footer now use the app's configuration for a more unified and easier-to-update approach.

Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

This absolutely looks much cleaner, but I do think we're going to have some problems here.

The main one I can think of is that if someone has APP_ALLOW_INSECURE_HOSTS enabled (which plenty of self-hosted customers may use, specifically when they have different "inside hostnames" versus "outside hostnames"). I mean, I, personally, wouldn't ever do that, but I have definitely seen some of our users do that.

Maybe there's a new Helper function we should consider for this, instead?

@marcusmoore
Copy link
Collaborator

@uberbrady would that mean that places using config('app.url') already would cause the issue now?

@uberbrady
Copy link
Collaborator

@uberbrady would that mean that places using config('app.url') already would cause the issue now?

I mean, that's a pretty fair point - it's definitely possible. I just don't want to hose people that we don't have to hose on this one.

@snipe
Copy link
Owner Author

snipe commented Apr 5, 2023

@marcusmoore - that's the argument I'm having with him right now at the annex :D

He's referring to this:

// TODO - isn't it somehow 'gauche' to check the environment directly; shouldn't we be using config() somehow?
if ( ! env('APP_ALLOW_INSECURE_HOSTS')) { // unless you set APP_ALLOW_INSECURE_HOSTS, you should PROHIBIT forging domain parts of URL via Host: headers
$url_parts = parse_url(config('app.url'));
if ($url_parts && array_key_exists('scheme', $url_parts) && array_key_exists('host', $url_parts)) { // check for the *required* parts of a bare-minimum URL
\URL::forceRootUrl(config('app.url'));
} else {
\Log::error("Your APP_URL in your .env is misconfigured - it is: ".config('app.url').". Many things will work strangely unless you fix it.");
}

But we're already using the config value there - and we already use config('app.url') in a buttload of other places, so if it was going to interfere, I'd expect it to have been half-broken the whole time.

@spencerrlongg
Copy link
Collaborator

@marcusmoore - that's the argument I'm having with him right now at the annex :D

He's referring to this:

// TODO - isn't it somehow 'gauche' to check the environment directly; shouldn't we be using config() somehow?
if ( ! env('APP_ALLOW_INSECURE_HOSTS')) { // unless you set APP_ALLOW_INSECURE_HOSTS, you should PROHIBIT forging domain parts of URL via Host: headers
$url_parts = parse_url(config('app.url'));
if ($url_parts && array_key_exists('scheme', $url_parts) && array_key_exists('host', $url_parts)) { // check for the *required* parts of a bare-minimum URL
\URL::forceRootUrl(config('app.url'));
} else {
\Log::error("Your APP_URL in your .env is misconfigured - it is: ".config('app.url').". Many things will work strangely unless you fix it.");
}

But we're already using the config value there - and we already use config('app.url') in a buttload of other places, so if it was going to interfere, I'd expect it to have been half-broken the whole time.

Yeah, was just looking at that and agree.

@marcusmoore
Copy link
Collaborator

@spencerrlongg hit submit on the comment I was typing out 😄 . I also agree.

@uberbrady
Copy link
Collaborator

MUTINY! YOU'RE ALL FIRRRRRRRREDDDD!!!!!!

@snipe
Copy link
Owner Author

snipe commented Apr 5, 2023

AND DIVORCED!!! 🤣

I think this should be good to go then - we are agreed?

@marcusmoore
Copy link
Collaborator

😆 yeah. I think we're good to go.

@snipe snipe merged commit 39e6ca8 into develop Apr 5, 2023
3 checks passed
@snipe snipe deleted the fixes/use_app_url_more_consistently branch April 5, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants