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

Do not recommend to disable CSRF verification for Laravel & Lumen #292

Closed
nerg4l opened this issue Aug 4, 2020 · 5 comments
Closed

Do not recommend to disable CSRF verification for Laravel & Lumen #292

nerg4l opened this issue Aug 4, 2020 · 5 comments

Comments

@nerg4l
Copy link

nerg4l commented Aug 4, 2020

Is your feature request related to a problem? Please describe.
On the wiki under Laravel & Lumen Integration it is suggested to disable CSRF verification for the registered routes. In my opinion that shouldn't be the recommended way of integration.

Describe the solution you'd like
I would recommend instead suggest to register CSRF or XSRF header in the client library. Disabling the CSRF verification for the routes can be mentioned but that should not be the recommended way.

e.g.

function getCookieValue(a) {
    const b = document.cookie.match('(^|;)\\s*' + a + '\\s*=\\s*([^;]+)');
    return b ? b.pop() : '';
}

// [...]
var upload = new tus.Upload(file, {
    // [...]
    headers: { 'X-XSRF-TOKEN': decodeURIComponent(getCookieValue('XSRF-TOKEN')) },
    // [...]
});
@samundra
Copy link
Collaborator

samundra commented Aug 4, 2020

I don't think this is job of Tus-Client to verify XSRF token. This should be done outside the scope of Tus-client. Once you have done verification and trusted the sources then we can proceed or drop request.

With Laravel, I believe it can be done with route middleware like similar to how we validate auth endpoints.

@nerg4l
Copy link
Author

nerg4l commented Aug 4, 2020

In this issue I'm only talking about changing the wiki. Nothing more.

@samundra
Copy link
Collaborator

samundra commented Aug 4, 2020

All I am saying is, the suggested solution requires change in Tus-Php client. Use of X-XSRF-TOKEN is not supported out of the box in Tus-php client library. So, the below suggestion for registering token will not work unless we modify client to do so.

var upload = new tus.Upload(file, {
    // [...]
    headers: { 'X-XSRF-TOKEN': decodeURIComponent(getCookieValue('XSRF-TOKEN')) },   // <-- not supported out of the box
    // [...]
});

IMHO Changing wiki alone without change in Tus-php client would be misleading and not recommended. That could be reason why disabling CSRF/XSRF was suggested in first place.

@nerg4l
Copy link
Author

nerg4l commented Aug 4, 2020

As you said Laravel already have a middleware for that. In the linked wiki article in the last steps it is disabled explicitly. No need for modification in the tus server.

Edit: Also, it can be mentioned that for using the php client you have to disable it.

@ankitpokhrel
Copy link
Owner

ankitpokhrel commented Aug 11, 2020

Hi @nerg4l and @samundra,

Thank you for checking this out.

I amended the wiki to add a note about this. Headers can be passed to the client during initialization and is propagated to the server on each request. So something like this should already work.

$client = new \TusPhp\Tus\Client('http://tus-php-server', ['headers' => ['X-XSRF-TOKEN' => 'value']]);

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

No branches or pull requests

3 participants