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

host in @sessionToken being cached, does not always match the current $request->input('host') #218

Open
tom-rice opened this issue Oct 5, 2023 · 1 comment
Labels
help wanted Extra attention is needed

Comments

@tom-rice
Copy link
Contributor

tom-rice commented Oct 5, 2023

Looking at pull request 194, while we do need to ensure 'host' is an input when using forms in Blade, and including it in @sessionToken makes sense as a convenient way to automatically address it, I've noticed that the whole output of what is returned by that __invoke() function seems to get cached? When I edit that return line in SessionToken.php I don't see any difference until I also edit my blade template or clear the cache with php artisan view:clear - the problem with that is if I use $request->input('host') in my controller and pass it through to the view and output it as {{ $host }}, I'm often seeing a different value to what is output by @sessionToken, which is the same value across multiple browser sessions/devices. The value passed from my controller is always the correct one, whereas the value coming from @sessionToken is sometimes incorrect.

For now I have changed SessionToken.php back to just returning <input type="hidden" class="session-token" name="token" value="" /> and I'm including 'host' and 'shop' in my form directly this way instead (passed through from the controller):
<input type="hidden" name="host" value="{{ $host }}"><input type="hidden" name="shop" value="{{ $shop }}">

(in case you missed it, I started including the 'shop' value on the form as well because of issues in the POS without it, and I had done a pull request 217 for that - that inclusion is still needed, but does not address this caching problem).

EDIT: I think it's because blade directives are only evaluated at compile time, so for me I've made a new blade file for it instead called session-token.blade.php to use as an include, and added this as a new boot method to ShopifyAppProvider.php so that the host and shop values always get automatically passed to it:

    /**
     * Boot the session token view composer.
     *
     * @return void
     */
    private function bootSessionTokenComposer()
    {
        \View::composer('session-token', function ($view) {
            $view->with('host', request()->input('host'));
            $view->with('shop', request()->input('shop'));
        });
    }

Then I can just use @include('session-token') instead of @sessionToken without any caching issue.

I could do a pull request on that if you'd like but will wait to see what else I run into + if anyone suggests a better approach or has other observations first - maybe there is a way to change when the directive is evaluated instead (e.g. @csrf doesn't suffer from such an issue).

(I expect the original intention was to just use URL::tokenRoute() for the form action URL and then all of this stuff to include 'host' and 'shop' as inputs would be unnecessary, but that doesn't work at all vs including them and submitting to a standard route() and I'm not sure how to fix that)

@Kyon147
Copy link
Owner

Kyon147 commented Dec 6, 2023

Hey @tom-rice

Your PR is in 19.2.0 now 👍

For the caching issue, it is a strange one and something I never see as I don't use the package to build blade apps. So I tend to have to investigate on a test blade app I have.

It looks like the CRSF is also not immune to the issue and there's a lot of different approches people have taken.

https://laracasts.com/discuss/channels/laravel/cache-control-stops-csrf-tokens-from-working
https://stackoverflow.com/questions/52155011/laravel-how-to-make-csrf-token-work-with-html-cache

So I am open to suggestions.

@Kyon147 Kyon147 added the help wanted Extra attention is needed label Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants