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

Update header parsing to allow double-escaped ":" #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

porkloin
Copy link

@porkloin porkloin commented Mar 9, 2021

What Does This PR Do?

  • Modifies header parsing logic to leverage regex instead of basic split

Why?

Currently, any header value that contains literal colon characters is blanket rejected by these lines of code. My first impulse as a user was to try escaping those characters. However, I quickly realized that the logic didn't support escaping chars either. This regex split should only select colon characters not preceded by a double escape. Double escape must be used instead of single escape, since JS' strings will infer a regular escape as a standard escape char for the string system and store it without those escape chars.

Questions to Think About 🤔

Negative lookbehind support (which this regex uses) isn't great across older JS versions - I believe ES2018 was when support was added. If compatibility is an issue let me know, and I can probably put together an alternative approach that still allows escaping these chars.

@hasura-bot
Copy link

Beep boop! 🤖

Hey @porkloin, thanks for your PR!

One of my human friends will review this PR and get back to you as soon as possible.

Stay awesome! 😎

@porkloin
Copy link
Author

porkloin commented Apr 15, 2021

Hey! Any thinks on this one? I know you folks are probably busy, but I'm more than happy to put something together that meets whatever reqs you might have for this. I regularly work with some headers that I have to forward to my GQL endpoints that unfortunately contain literal : and I don't have the ability to negotiate on the structure of the header. I'd love to be able to use this tool for that work!

@threeiem
Copy link

threeiem commented Jul 8, 2021

Simple enough change to allow double quotes.

@daa
Copy link
Contributor

daa commented Oct 7, 2021

I've recently met this issue and I'd like to suggest another apporach: instead of supporting colon escaping just allow colons in header value by joining by colon parts after the first : headerValue = parts.slice(1).join(':'). Also it would be useful to change parts[1].trim() below to headerValue.trimStart() to preserve possibly intentional spaces in the end of header value.

Edit: forgot that in JS split works differently from Python.

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.

4 participants