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

Cookie-based auth #1521

Draft
wants to merge 77 commits into
base: main
Choose a base branch
from
Draft

Cookie-based auth #1521

wants to merge 77 commits into from

Conversation

dokterbob
Copy link
Collaborator

@dokterbob dokterbob commented Nov 14, 2024

Implementation of #1520

  • Overall auth cleanup/refactor
  • Basic implementation
  • E2E-tests passing
  • Copilot testing/integration
  • Unit tests for auth methods
  • Cleanup
  • Create ticket for serving files from different root path (out of scope for this one, see below)

@dokterbob dokterbob force-pushed the dokterbob/cookie-auth branch 2 times, most recently from c49b0d8 to 38f7ea9 Compare November 19, 2024 14:02
@dokterbob
Copy link
Collaborator Author

dokterbob commented Nov 19, 2024

There's currently E2E test failures in:

  • chat_profiles
  • data_layer
  • header_auth
  • password_auth

This is kind of to be expected, given the scale of this refactor.

In addition, we need to make sure that files are served from a place which does not have API access, e.g. the files should really be untrusted. Otherwise, an LLM or whoever uploads files could call the Chainlit API on the user's behalf by crafting malicious HTML with JS.

To get there, we need to:

  • Serve the API from a single root/prefix, e.g. /api/.
  • Serve files from another, e.g. /files/.
  • Set auth cookie with path=/api/, allowing acces to the API only. Including file uploads.
  • Set another auth cookie with path=/files/, allowing only GET (and perhaps HEAD) to files.
  • For legacy support, add a permanent redirect from the current file API location to the new location (and give a deprecation warning when redirecting).

This would be a good moment to 'go all in' in terms of file security. We could also postpone this to a later PR and/or explicitly document that files in their current implementation should not come from untrusted sources (e.g. AI-generated or from 3rd parties).

@dokterbob
Copy link
Collaborator Author

Just 2 more tests missing:

────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                    
  Running:  header_auth/spec.cy.ts                                                        (18 of 37)


  Header auth
    1) should fail to auth without custom header
    ✓ should be able to auth with custom header (6430ms)


  1 passing (16s)
  1 failing

  1) Header auth
       should fail to auth without custom header:
     CypressError: The application redirected to `http://127.0.0.1:8000/login` more than 20 times. Please check if it's an intended behavior.

If so, increase `redirectionLimit` value in configuration.
      at onWindowLoad (http://127.0.0.1:8000/__cypress/runner/cypress_runner.js:137531:80)
      at $Cy.onInternalWindowLoad (http://127.0.0.1:8000/__cypress/runner/cypress_runner.js:137563:20)
      at $Cy.listener (http://127.0.0.1:8000/__cypress/runner/cypress_runner.js:86503:17)
      at ../driver/node_modules/eventemitter2/lib/eventemitter2.js.EventEmitter.emit (http://127.0.0.1:8000/__cypress/runner/cypress_runner.js:86583:19)
      at parent.<computed> [as emit] (http://127.0.0.1:8000/__cypress/runner/cypress_runner.js:155168:32)
      at $Cypress.action (http://127.0.0.1:8000/__cypress/runner/cypress_runner.js:147734:14)
      at HTMLIFrameElement.<anonymous> (http://127.0.0.1:8000/__cypress/runner/cypress_runner.js:151065:24)
  From Your Spec Code:
      at Context.eval (webpack:///./cypress/e2e/header_auth/spec.cy.ts:9:0)




  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        2                                                                                │
  │ Passing:      1                                                                                │
  │ Failing:      1                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      0                                                                                │
  │ Screenshots:  1                                                                                │
  │ Video:        false                                                                            │
  │ Duration:     15 seconds                                                                       │
  │ Spec Ran:     header_auth/spec.cy.ts                                                           │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


  (Screenshots)

  -  /home/runner/work/chainlit/chainlit/cypress/screenshots/header_auth/spec.cy.ts/H     (1280x720)
     eader auth -- should fail to auth without custom header (failed).png                           

────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                    
  Running:  data_layer/spec.cy.ts                                                         (12 of 37)


  Data Layer
    Data Features with persistence
      1) should login, submit feedback, wait for user input to create steps, browse thread history, delete a thread and then resume a thread
      2) should continue the thread after backend restarts and work with new thread as usual


  0 passing (41s)
  2 failing

  1) Data Layer
       Data Features with persistence
         should login, submit feedback, wait for user input to create steps, browse thread history, delete a thread and then resume a thread:
     AssertionError: Timed out retrying after 15000ms: expected '/' to match /^\/thread\//
      at Context.eval (webpack:///./cypress/e2e/data_layer/spec.cy.ts:16:0)

  2) Data Layer
       Data Features with persistence
         should continue the thread after backend restarts and work with new thread as usual:
     AssertionError: Timed out retrying after 15000ms: expected '/' to match /^\/thread\//
      at Context.eval (webpack:///./cypress/e2e/data_layer/spec.cy.ts:16:0)




  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        2                                                                                │
  │ Passing:      0                                                                                │
  │ Failing:      2                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      0                                                                                │
  │ Screenshots:  2                                                                                │
  │ Video:        false                                                                            │
  │ Duration:     40 seconds                                                                       │
  │ Spec Ran:     data_layer/spec.cy.ts                                                            │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


  (Screenshots)

  -  /home/runner/work/chainlit/chainlit/cypress/screenshots/data_layer/spec.cy.ts/Da     (1280x720)
     ta Layer -- Data Features with persistence -- should login, submit feedback, wai               
     t for user input to create steps, browse thread history, delete a thread and the               
     n resume a thread (failed).png                                                                 
  -  /home/runner/work/chainlit/chainlit/cypress/screenshots/data_layer/spec.cy.ts/Da     (1280x720)
     ta Layer -- Data Features with persistence -- should continue the thread after b               
     ackend restarts and work with new thread as usual (failed).png                                 


────────────────────────────────────────────────────────────────────────────────────────────────────```

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

Successfully merging this pull request may close these issues.

1 participant