fix: dedupe auth logic & patch potential timing attack #31
Conversation
|
@sambarnes Thanks for the PR! - Had a tons of family stuff yesterday as well haha. Reviewing shortly! |
ci: start hooking up unit test suite
pyproject.toml
Outdated
| @@ -16,11 +16,13 @@ accelerate = "^0.23.0" | |||
| datasets = "^2.14.5" | |||
| scipy = "^1.11.3" | |||
| wandb = "^0.15.12" | |||
| httpx = "^0.26.0" | |||
There was a problem hiding this comment.
seems like we don't need this anymore? Based on your comment, I think this was needed for the e2e test.
For e2e, I wonder if we can do a modal deply runner to a e2e workspace, tail the modal URL, then call it with our JS scripts?
Also we should prob add a js script to test auth as well :-?
There was a problem hiding this comment.
ah good catch! I think I just accidentally forgot to put this in the dev deps group. IIRC it was required by the fastapi test client, and shortcircuited the unit test until I had added it
(edit: done here 1d9e5c1)
I'll try digging into the E2E + js stuff you mentioned tomorrow :)
There was a problem hiding this comment.
started the js test by simply putting a bad api key request inside the existing test-simple.ts (375823e)
but for the sake of separation & clutter, idk if it makes sense to take it much further inside this PR -- perhaps a new dedicated issue is warranted, to convert these manual scripts to a test suite with assertions?
i'm about to get on a flight, so i'll have some time to try out my own deployment & run the manual scripts to ensure what i've pushed so far works. after that i should have a better sense of how an automated CI/CD for E2E testing could look
There was a problem hiding this comment.
@louisgv aight yeh the test works as expected now, gonna re-req review
i was able to deploy & do a pnpm x scripts/test-simple.ts that resulted in two successful requests & then hit the expected failure of {"detail":"Incorrect bearer token"}
|
surprisingly airplane wifi been solid enough for my first attempt at the deployment workflow lol was able to verify that my changes indeed do not work at the moment 🙃 appears that gonna check if modal is on an older version of fastapi, or im missing some small thing elsewhere -- worstcase, i'll switch back to the older syntax edit: aye there it is, released in fastapi 0.105.0 -- while we're locked at just bumped to latest in 911e73b and gonna test again editedit: looks like modal doesn't respect the new fastapi version i locked, so i reverted that change & will be changing to the older |
sambarnes
force-pushed
the
auth-refactor
branch
from
911e73b to
01e4c12
Compare
|
@sambarnes yeah one of the constraint with the modal runner is that the modal's stub abstraction depends on a specific fastapi version (as well as pydantic) |
| let gotExpectedError = false; | ||
| try { | ||
| await completion(prompt, {model, apiKey: "BADKEY"}); | ||
| } catch (e: any) { | ||
| gotExpectedError = e.message == "Status: 401"; | ||
| } | ||
| if (!gotExpectedError) { | ||
| throw new Error("Unauthorized request returned unexpected response") | ||
| } |
There was a problem hiding this comment.
nit: we can just re-throw the error in the try if not 401, no need to do it separately IMO. Also, we should either use startsWith or strict equal.
Details
'splorin the codebase a bit more, and saw a simple refactor to share that auth logic across endpoints
haven't used fastapi in a little while, so when i went to check latest syntax on dependencies/middleware, i saw they recommend the new
Annotated[...]style in python 3.10+you can find the rationale behind a lot of the structuring in the docs here -- including their notes on the possible timing attack
Code of Conduct