Python: Fix header conversion to byte-pair on scope building by morganabc · Pull Request #2125 · cloudflare/workerd

morganabc · 2024-05-14T23:17:55Z

headers: These are byte strings of the exact byte sequences sent by the client/to be sent by the server. While modern HTTP standards say that headers should be ASCII, older ones did not and allowed a wider range of characters. Frameworks/applications should decode headers as they deem appropriate.
(https://asgi.readthedocs.io/en/latest/specs/www.html#wsgi-compatibility)

Workerd uses Pyodide JSProxy object when passing fetch(request, env) to Python, but JSProxy objects translates into string, not byte-string.

In order to match ASGI specification, it needs to be translated into byte-pair when building scopes.

github-actions · 2024-05-14T23:18:06Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

morganabc · 2024-05-14T23:19:36Z

I have read the CLA Document and I hereby sign the CLA

hoodmane

Code change makes sense. Ideally we ought to add a test.

hoodmane · 2024-05-29T09:56:26Z

src/pyodide/internal/asgi.py

    from js import URL

-    headers = [tuple(x) for x in req.headers]
+    headers = [(k.lower().encode(), v.encode()) for k, v in req.headers]


In lieu of adding a test, can we add a comment here with an example that cares about this being bytes so that we can add a test ourselves later?

It assumes FastAPI so that would be a main concern, but its for ASGI spec though so should I add for FastAPI or just asserting for spec?

Like, anything for

@app.get("/example") async def example(request: Request): request.headers.get("content-type") # this will error if header is not bytes.

perhaps?

Also is there way to test this locally? I want to build miniflare with master workerd, but building workers-sdk doesnt works..

You can build workerd by itself and then set an environment variable to tell wrangler to use your custom build. @dom96 are there good instructions somewhere that we can point contributors to?

I guess I should check it out and test it myself...

May be easier to build and run workerd locally. These are probably the best instructions we have for doing this: https://github.com/cloudflare/workerd?tab=readme-ov-file#configuring-workerd.

We also have some sample Python worker configs in the repo already, for example https://github.com/cloudflare/workerd/tree/main/samples/pyodide-fastapi

Oh, so I can use workerd directly! Thanks, now I can test few things.

leobuskin · 2024-06-05T08:00:42Z

That's not the only trouble w/ ASGI and binary strings. We also need to decode resulting headers in asgi.py, if I understand correctly:

async def send(got):
        ...
        if got['type'] == 'http.response.start':
            status = got['status']
            headers = [(k.decode(), v.decode()) for k, v in got['headers']]

spikerwork · 2024-06-05T08:06:37Z

That's not the only trouble w/ ASGI and binary strings. We also need to decode resulting headers in asgi.py, if I understand correctly:
async def send(got):
        ...
        if got['type'] == 'http.response.start':
            status = got['status']
            headers = [(k.decode(), v.decode()) for k, v in got['headers']]

Confirmed! This patch fixed the problem with b'' I've seen before

morganabc · 2024-06-05T08:08:06Z

That's not the only trouble w/ ASGI and binary strings. We also need to decode resulting headers in asgi.py, if I understand correctly:
async def send(got):
        ...
        if got['type'] == 'http.response.start':
            status = got['status']
            headers = [(k.decode(), v.decode()) for k, v in got['headers']]

Yes, right. I fixed it. Thanks!

leobuskin · 2024-06-05T08:14:30Z

src/pyodide/internal/asgi.py

        if got["type"] == "http.response.start":
            status = got["status"]
-            headers = got["headers"]
+            headers = [(k.decode(), v.decode()) for k, v in got['headers']]


@morgan9e I'd use double quotes here to be compliant w/ folks' formatting standards

Working on it!

hoodmane · 2024-06-05T14:15:13Z

Since #2096 merged, we're actually running Python tests on workerd so it should be possible to add a test now.

… ASGI scope

morganabc · 2024-06-06T16:58:06Z

@hoodmane: Since #2096 merged, we're actually running Python tests on workerd so it should be possible to add a test now.

Should I add test for ASGI only or if its working with Starlette? I think it might be better if we make asgi.py independant to FastAPI

hoodmane · 2024-06-06T17:10:05Z

Agreed we should make it independent. Currently the only thing in there that is dependent on fastapi is Env. Perhaps we can just delete that for now and we can figure out the appropriate place to add it later.

dom96 · 2024-11-18T10:32:12Z

Hey @morgan9e, would love to get this merged in, are you still planning to create a test for this?

morganabc · 2024-11-19T04:25:29Z

Hey @morgan9e, would love to get this merged in, are you still planning to create a test for this?

Yep, thanks I'll do it ASAP.

morganabc · 2024-11-20T07:32:32Z

Hey @morgan9e, would love to get this merged in, are you still planning to create a test for this?

Yep, thanks I'll do it ASAP.

I added the test, I wrote sample ASGI Application rather than using FastAPI because I thought it was better to seperate it from FastAPI.

morganabc · 2024-11-20T07:41:02Z

Sorry, I accidently merged and removed upstream branch, I reopened it. #3142

github-actions bot added a commit that referenced this pull request May 14, 2024

@morgan9e has signed the CLA in #2125

c6c48e3

morganabc marked this pull request as ready for review May 14, 2024 23:21

morganabc requested review from a team as code owners May 14, 2024 23:21

morganabc requested review from hoodmane and jasnell May 14, 2024 23:21

hoodmane reviewed May 14, 2024

View reviewed changes

morganabc requested a review from hoodmane May 29, 2024 08:57

hoodmane approved these changes May 29, 2024

View reviewed changes

hoodmane reviewed May 29, 2024

View reviewed changes

jasnell added the python Issues/PRs relating to Python Workers label May 30, 2024

hoodmane mentioned this pull request May 31, 2024

Strange behavior with headers in FastAPI cloudflare/python-workers-examples#7

Open

leobuskin reviewed Jun 5, 2024

View reviewed changes

Python: Fix header type conversion between Pyodide request and Python…

a97b89b

… ASGI scope

Add test for Python ASGI header construction

2e1de8a

morganabc closed this Nov 20, 2024

morganabc deleted the patch-1 branch November 20, 2024 07:38

morganabc mentioned this pull request Nov 20, 2024

Python: Fix header conversion to byte-pair on scope building #3142

Merged

Conversation

morganabc commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morganabc commented May 14, 2024

Uh oh!

hoodmane left a comment

Choose a reason for hiding this comment

Uh oh!

hoodmane May 29, 2024

Choose a reason for hiding this comment

Uh oh!

morganabc May 29, 2024

Choose a reason for hiding this comment

Uh oh!

morganabc May 29, 2024

Choose a reason for hiding this comment

Uh oh!

hoodmane May 29, 2024

Choose a reason for hiding this comment

Uh oh!

dom96 May 29, 2024

Choose a reason for hiding this comment

Uh oh!

morganabc May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leobuskin commented Jun 5, 2024

Uh oh!

spikerwork commented Jun 5, 2024

Uh oh!

morganabc commented Jun 5, 2024

Uh oh!

leobuskin Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

morganabc Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

hoodmane commented Jun 5, 2024

Uh oh!

morganabc commented Jun 6, 2024

Uh oh!

hoodmane commented Jun 6, 2024

Uh oh!

dom96 commented Nov 18, 2024

Uh oh!

morganabc commented Nov 19, 2024

Uh oh!

morganabc commented Nov 20, 2024

Uh oh!

morganabc commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

morganabc commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

morganabc May 30, 2024 •

edited

Loading

morganabc commented Nov 20, 2024 •

edited

Loading