@tus/server: add allowedCredentials and allowedOrigins options

[idanto]

Hi,

Uppy client supports withCredentials parameter to allow sending tokens in cookie
https://uppy.io/docs/tus/#withcredentials

Tus Server doesn't allow any way to configure it; it seems like a missing configuration, and this MR is a suggestion on how to implement it.

[changeset-bot]

🦋 Changeset detected

Latest commit: 43549b7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@tus/server	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Acconut]

Should the user also be able to enable this only for requests from certain origins? Allowing it from arbitrary origins is potentially dangerous.

[idanto]

Should the user also be able to enable this only for requests from certain origins? Allowing it from arbitrary origins is potentially dangerous.

@Acconut, isn't that what Access-Control-Allow-Origin should do in the CORS policy?

TBH, I don't see a way to limit it in Tus code. I see the opposite which looks like the client can override it by sending an origin header.

[Acconut]

If tus-node-server does not yet have a way to restrict CORS access to certain origins, then we should probably add that as well.

[idanto]

@Acconut I have also added support for allowedOrigin, which implements a better logic to validate the origin header and prevent some potential XSS attacks.

I'm not quite sure how the tests for this project work and why the failure of the s3 upload test, although I didn't change anything that should affect that. If one of the creators of this project can help figure this out, it will be great.

[Murderlon]

Instead of creating an option for some new header every time, we could expose a headers function?

Implementers can sent whatever they want (but we override headers we want to control)

new Server({
  headers(req) {
    return { 'Access-Control-Allow-Credentials': true }
  }
})

[idanto]

@Murderlon I agree in general, but this is a special case. Those are CORS options, which is a well-defined standard. In most of the server implementations, I know you usually have specific options to configure your CORS to prevent people from making mistakes (like typos and so on).

[Murderlon]

My experience is that sooner or later people run into the limitations of these locked down options. For example, people with preview deployments, where the subdomain is dynamic, will not be able to use string[] for their origins.

If we make it RegExp | string[] I'm okay with keeping the options as proposed 👍

[Murderlon]

@idanto are you still interested in getting this over the finish line?

[idanto]

Yes, I’mI did a patch in my own personal project but would love to contribute this back to the open source.Can you please guide me on the right approach and why the tests are failing?I will handle it next week after summer break is overSent from my iPhoneOn 27 Aug 2024, at 2:42 PM, Merlijn Vos ***@***.***> wrote: @idanto are you still interested in getting this over the finish line? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[idanto]

@Murderlon looks like after you merge master everything is green

[Murderlon]

Only this needs to be resolved: #636 (comment).

I can take a look later this week if you don't have time before then.

[Murderlon]

Thanks for the contribution

[idanto]

thanks for the help pushing it to the finish line @Murderlon