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

Feat: Pass encoded characters in path unchanged #489

Merged

Conversation

valentin-krasontovitsch
Copy link
Contributor

We enable passing substrings that represent encoded reserved symbols, like
%2f for /, in the path of a request.

NB: This does not enable using encoded characters in rewrite rules (e.g. in the
urlprefix directive).

Cf. #347 and #486

@CLAassistant
Copy link

CLAassistant commented Apr 25, 2018

CLA assistant check
All committers have signed the CLA.

@valentin-krasontovitsch
Copy link
Contributor Author

So it turns out that one can utilize the internal mechanisms of net/url to handle encoded parts of an uri. One just has to keep track of RawPath, whenever necessary, which is what this commit attempts to do...

As a side note, I got carried away and tried enabling encoded characters in rewrite rules as well, and in the strip prefix, which worked fine, until you combined say a rule like

route add svc / http://bar.com/a%2fc/$path

with a request for

/%20

the space in the request won't get escaped, as it's unambiguous on its own (I guess, cf. docs for function that sets Path and RawPath in net/url package), but would get escaped in the context of %2f, i.e. if one looked at the whole url http://bar.com/a%2fc/%20.

One may inspect the more involved changes on my corresponding branch - in particular, a link to the failing test case.

Since I didn't see an easy and quick fix for that, I decided that it's maybe not the most needed use-case and that supporting what we have here might be enough.

@magiconair
Copy link
Contributor

Does that mean this fixes #347 and #486?

@valentin-krasontovitsch
Copy link
Contributor Author

I'm so sorry, saw your comment, but kinda ignored it O_o - let me see, I honestly don't know anymore, gotta check out the code... did I add some test cases? : )

... yes, I did. Please take a look at those, I'm sure you're more familiar with the code base and can say with more confidence which cases I have covered, and whether I forgot any cases / should add some cases.

But at least the test cases are covered! ... and I think, to answer your original question, yes, that solves those issues, as far as I understand.

@scalp42
Copy link

scalp42 commented Aug 25, 2018

Any chance to see this merged @magiconair ?

@makepanic
Copy link

We were also hit with this issue.
Is there anything one can do to proceed with the issue/PR?

@aaronhurt
Copy link
Member

@valentin-krasontovitsch Sorry for the lag in response around this issue. If you could resolve the conflicts I'll get this merged. Otherwise I'll try and fix the simple conflict myself when I get time.

@valentin-krasontovitsch
Copy link
Contributor Author

We enable passing substrings that represent encoded reserved symbols, like
`%2f` for `/`, in the path of a request.

NB: This does not enable using encoded characters in rewrite rules (e.g. in the
urlprefix directive).

Cf. fabiolb#347 and fabiolb#486
@valentin-krasontovitsch
Copy link
Contributor Author

okay, ran a rebase, and luckily it was only a superficial conflict - a test added in the same place where i added a test. resolved, and pushed - after the travis build checks out, you will hopefully be able to merge.

@valentin-krasontovitsch
Copy link
Contributor Author

Works on go1.13, but a handful tests break on go master? Very strange.
I presume somebody more familiar with the code base might have a better chance of working out why the tests fail... I can try to take a look myself, too, though.... might just take some more time.

@aaronhurt
Copy link
Member

That happens sometimes. There is some odd race condition where vault doesn't launch before the test suite runs. I need to spend some time debugging that but for now I'll just re-run that branch. Thank you again!

@aaronhurt aaronhurt merged commit c034d4f into fabiolb:master Feb 2, 2020
@valentin-krasontovitsch
Copy link
Contributor Author

hmmm OK I see I've experienced such things myself, sure, thanks!

.... I'm not sure what vault is though, and I started looking at the first test that failed TestTableLookup_656 and tried it with master locally myself, just running the subpackage go test -count=1 ./route, and it sometimes fails, sometimes don't - could that be due to a vault thing? it seems to my naive understanding to just be a simple redirect test... or is the vault the place where the table information is stored, and it happens async?

oh and cool thanks for merging : )

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.

6 participants