Support browser query parameters on chat start

[qtangs]

Here's my attempt to address this much-discussed issue: Make browser query parameters available in @on_chat_start

Also fix this bug: Logout Button Causing Issue w/ Header Auth

Here are the changes:

• Move authentication check from top level AppWrapper to Page (this removes errornous redirection to login, as reported by Logout Button Causing Issue w/ Header Auth #1219)
• Add logic to call header auth in Page if user is not yet authenticated
• Update both Login and Page to pass query parameters when redirecting between each other (unauthenticated Page -> Login -> Page after authentication)
• Update header_auth and password_auth tests to add the scenario where a query parameter is read and used on chat start

I have tested on my app: https://2in.ai

[dokterbob]

Hi @qtangs, thanks a bunch for this PR!

I fully agree that having access to query parameters is a hot topic which many users, myself included, are aching for. In fact, I'd like to have this in chainlit in weeks rather than months.

However, I am not sure if your proposed patch is the way to go.

First of all, as I mentioned in #1213 , providing a full Request object (either based on the original index request, or based on the websocket connection) yields a similar amount of code but enables many more use cases. Instead, if we accept your current proposal, we would break developer experience once we switch to supporting the full Request.

Secondly, having read your patch, you seem to be using the HTTP referrer header rather than the actual URL. Most notably, the referrer header is optional and not validated server-side, whereas, the request URL is validated by the server.

They represent different things and can lead to unexpected behaviour and possibly, security issues! Specifically in your case, your patch seems to contain a lot of code which related specifically to authentication rather than mere access to the query header.

Any referrer based authentication is a classic foot gun for security and not the way to go. Perhaps a resource like this might provide background on it: https://web.dev/articles/referrer-best-practices

I also see you included fixes for one pre-existing issue and some additional issues as part of this PR. That's great!

As it seems we still have some architectural decisions to take on the query params, perhaps it would be easier for us to have them as part of separate pull requests.

Any chance you're able to pry the parts not related to query params out and fire (preferably) multiple PR's, one for each separate issue, with enough information for us to replicate the issue while we review your fixes?

I know it's a lot to ask for, but reviewing all these issues at the same time makes it harder to do a thorough review and especially with auth stuff that can lead to dire consequences. Thanks for understanding!

[qtangs]

Hey @dokterbob, thanks for the super quick response to this. Really appreciate your taking time to look into these important issues.

Let me try to respond as best as I can.

I think you brought some very good points and I really like the Request object idea, in fact it'd be very useful to have it in many other handlers besides on_chat_start.
And I admit I'm trying to address a few things together in this PR:

1. Pass the query parameters around, between the paths /, /login and also /env, as users are redirected upon failing/passing authentication or env.
   I think for this one, it doesn't have conflict with Logout Button Causing Issue w/ Header Auth #1219, as the former is about passing data between different requests while your idea is about a single request, unless you want the Request object to contain data from multiple linked browser requests (all data from /, /login, /env are added to 1 Request object?).

2. Fix the bug Logout Button Causing Issue w/ Header Auth #1219
   Agree that this may benefit from a separate PR.

3. Use http_referer for on_chat_start
   This is optional, dependent on the user (dev) of chainlit, it's not enforced by chainlit so while the security concern is there, it should be handled by the user. Once Make full Request object available in chat #1213 is completed, anyone can easily update on_chat_start to use it.

Happy to discuss further.

[dokterbob]

Hi @qtangs,

This is what I ❤️ about open source. :)

1. Pass the query parameters around, between the paths /, /login and also /env, as users are redirected upon failing/passing authentication or env.
   I think for this one, it doesn't have conflict with Logout Button Causing Issue w/ Header Auth #1219, as the former is about passing data between different requests while your idea is about a single request, unless you want the Request object to contain data from multiple linked browser requests (all data from /, /login, /env are added to 1 Request object?).

Because the way chainlit is developed, as a websocket-based SPA app, the concept of a request is not so clearly defined. Rather, the websocket session (in addition to the data layer's session) is what's used to keep state. This is probably why a Request object wasn't originally made available.

However, as we're finding now, having access to headers, query parameters etc. is essential to allow dev's like ourselves to go wild in terms of amazing features we want to implement.

We don't always have a well-defined Request but we do have a well-defined user_session. In addition, on_chat_start() does always correspond to a HTTP request (namely, the one opening the websocket). So, passing a Request once to on_chat_start() would be valid, after which dev's can literally do whatever they want with the request and it's data, including but not limited to adding the entire thing to the user_session.

The auth issue is a bit of a trickier beast and, if I'm entirely honest, given the amount of issues related to auth and the insane variety of auth options, my current stance (which we still need to align on amongst maintainers!) is that we should delegate all auth to third-party libraries so we can focus on a silky smooth LLM-chat experience.

2. Fix the bug Logout Button Causing Issue w/ Header Auth #1219
   Agree that this may benefit from a separate PR.

If you would do this, this seems to be the most obvious way to include this chunk of your work at this point. Please, provide
as clear as possible testing instructions, including, ideally, a test project with deps and minimal code showing both the issue and it's disappearance or, if you're super kind and slightly crazy, a regression test in our e2e test suite.

3. Use http_referer for on_chat_start
   This is optional, dependent on the user (dev) of chainlit, it's not enforced by chainlit so while the security concern is there, it should be handled by the user. Once Make Request object available in on_chat_start() #1213 is completed, anyone can easily update on_chat_start to use it.

But your patch does add it to the tests. Tests are often used as a form of (authoritative, known to be working) example for other devs and (potentially) insecure examples are really something we should take care to stay clear of.

As for now, I will be closing the current PR in favour of a PR with an (optional) request: Request parameter in on_chat_start() as well as a separate PR for 2. I do hate to be 'rejecting' your work as I do appreciate great contribs.

To avoid that, in the future, I suggest using an issue to propose features to allow others to comment on it and to hold space to form a clearly crystallised concept before firing the PR -- that will provide a straight path to getting your code shipped out. (I hope to add this info to the contribution instructions once the process is a bit more crystallised itself. :p)

Thanks for understanding and please keep the good contrib coming!

[qtangs]

@dokterbob I understand your decision. Will see what else can be done. For the moment, at least the code will be here as a reference for a quick workaround while waiting for the proper solution once Request object is made available. Rooting for this: :)

to allow dev's like ourselves to go wild in terms of amazing features we want to implement.