You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by bgaff <gi...@git.apache.org> on 2015/10/05 09:27:12 UTC
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
GitHub user bgaff opened a pull request:
https://github.com/apache/trafficserver/pull/300
TS-3956: Header_rewrite applies strange logic with = operator
Please see ticket TS-3956 for more information, I'm using github here so that @zwoop / @jacksontj can provide a code review.
It appears that whitespace causes weird behavior with header_rewrite, for example:
If you remove the white space before the = and the quotes it appears to behave correctly. This whitespace issue is likely to cause strange bugs and needs to be fixed.
```
cond %{READ_REQUEST_HDR_HOOK}
cond %{CLIENT-HEADER:Host} /^localhost$/ [AND]
cond %{CLIENT-HEADER:non_existent_header} = "shouldnt_exist_anyway" [AND]
add-header X-HeaderRewriteApplied true
```
With the following request:
```
curl -v localhost/
```
Header_rewrite will incorrectly apply the rule:
```
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Building resources, hook=TS_HTTP_READ_REQUEST_HDR_HOOK
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Adding TXN client request header buffers
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Getting Header: Host, field_loc: 0x7fffd02070d0
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Appending HEADER(Host) to evaluation value -> localhost
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Test regular expression ^localhost$ : localhost
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Successfully found regular expression match
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Evaluating HEADER(): localhost - rval: 1
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Getting Header: non_existent_header, field_loc: (nil)
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Evaluating HEADER(): - rval: 1
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) OperatorAddHeader::exec() invoked on header X-HeaderRewriteApplied: true
[Oct 4 20:56:49.245] Server {0x7ffff61b5700} DIAG: (header_rewrite) Adding header X-HeaderRewriteApplied
```
I have a proposed patch incoming it maintains backwards compatability while being a little more flexible with whitespace, for example the following config will parse as:
```
cond %{READ_REQUEST_HDR_HOOK}
cond %{CLIENT-HEADER:Host} /^localhost$/ [AND]
cond %{CLIENT-HEADER:Host} =a
# COMMENT!
# COMMENT
cond %{Client-HEADER:Foo} =b
cond %{Client-HEADER:Blah} = x
cond %{CLIENT-HEADER:non_existent_header} = "shouldnt_ exist _anyway" [AND]
cond %{CLIENT-HEADER:non_existent_header} = "shouldnt_ = _anyway" [AND]
cond %{CLIENT-HEADER:non_existent_header} ="=" [AND]
cond %{CLIENT-HEADER:non_existent_header} ="" [AND]
add-header X-HeaderRewriteApplied true
```
```
$1 = std::vector of length 2, capacity 2 = {"cond", "%{READ_REQUEST_HDR_HOOK}"}
$2 = std::vector of length 4, capacity 4 = {"cond", "%{CLIENT-HEADER:Host}", "/^localhost$/", "[AND]"}
$3 = std::vector of length 4, capacity 4 = {"cond", "%{CLIENT-HEADER:Host}", "=", "a"}
$4 = std::vector of length 4, capacity 4 = {"cond", "%{Client-HEADER:Foo}", "=", "b"}
$5 = std::vector of length 4, capacity 4 = {"cond", "%{Client-HEADER:Blah}", "=", "x"}
$6 = std::vector of length 5, capacity 8 = {"cond", "%{CLIENT-HEADER:non_existent_header}", "=", "shouldnt_ exist _anyway", "[AND]"}
$7 = std::vector of length 5, capacity 8 = {"cond", "%{CLIENT-HEADER:non_existent_header}", "=", "shouldnt_ = _anyway", "[AND]"}
$8 = std::vector of length 5, capacity 8 = {"cond", "%{CLIENT-HEADER:non_existent_header}", "=", "=", "[AND]"}
$9 = std::vector of length 5, capacity 8 = {"cond", "%{CLIENT-HEADER:non_existent_header}", "=", "", "[AND]"}
$10 = std::vector of length 3, capacity 4 = {"add-header", "X-HeaderRewriteApplied", "true"}
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bgaff/trafficserver master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/300.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #300
----
commit bd3f2a8a3da46b0a4c59c5cb1a2509be9b02c0dd
Author: Brian Geffon <br...@apache.org>
Date: 2015-10-05T07:23:45Z
TS-3956: Header_rewrite applies strange logic with = operator and whitespace
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/300#issuecomment-145744298
Any other comments on the implementation before I land it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/300#issuecomment-145743518
After looking into this, testing doesn't really make sense in a regression test so what i'll do is just drop in another source file called parser_tests.cc and have automake build that and link it to make check. I'll commit that when I land this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/300#issuecomment-145649988
I think we need tests for this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/300
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/300#issuecomment-145764790
I managed to screw up the diff in the second commit ;/ I'm gonna land the code change and tests (obviously not screwed up like that) ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on the pull request:
https://github.com/apache/trafficserver/pull/300#issuecomment-145647206
This looks reasonable. I'm not surprised that the existing code is broken. I wrote it in a hurry to be able to remove Boost but I guess I didn't test variations with the spacing well enough. Can we have some tests for this? Seems like a good use case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/300#issuecomment-145712386
@zwoop yah unit tests should be pretty easy to add.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3956: Header_rewrite applies strang...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/300#issuecomment-145669029
it ought to be possible to make a standalone test program, that exercises the config loading / parsing, without having to run as a plugin. Basically, something that runs as part of “make check” right ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---