-
Notifications
You must be signed in to change notification settings - Fork 328
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
HTTP: Ensure REQUEST_URI immutability. #1162
Conversation
Hi @andrey-zelenkov,
|
Hmm, I thought the idea was that
In which case I would have expected |
Here are a few namings:
So your understanding is correct.
The
|
src/nxt_http_rewrite.c
Outdated
r->path = nxt_mp_alloc(r->mem_pool, sizeof(nxt_str_t)); | ||
if (nxt_slow_path(r->path == NULL)) { | ||
return NXT_ERROR; | ||
} | ||
|
||
*r->path = rp.path; | ||
|
||
r->quoted_target = rp.quoted_target; | ||
r->uri_changed = 1; | ||
|
||
if (nxt_slow_path(r->log_route)) { | ||
nxt_log(task, NXT_LOG_NOTICE, "URI rewritten to \"%V\"", &r->target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
friends, just asking , any chance to get in in next release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should definitely make 1.33, I'm just waiting for this commit to be appropriately split up before reviewing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it will be in the next release.
We'll do it like this:
- Andrei adds more tests for the proxy that has been in the Tests: added $request_uri tests with proxy #1211.
- I'll create a PR split from HTTP: Ensure REQUEST_URI immutability. #1162.
- Continue on the HTTP: Ensure REQUEST_URI immutability. #1162 and related tests.
c7b412c
to
4a5e164
Compare
@andrey-zelenkov @hongzhidao: I want to double check the failing tests: https://github.com/nginx/unit/actions/runs/8718395388/job/23915553886?pr=1162#step:41:1060 I tested manually, and given the configuration in the test and the request, the log line ends up being
This is correct given the request URI. I think those tests should be changed given the outcome of the code they're testing has been changed in this PR. I'm working on a PR to this branch at hongzhidao#1, but wanted to run this by you as you're most familiar with the tests 🙂 I'll keep working on that PR as I fix the tests. |
Thank you for pointing this out! I discussed it with @hongzhidao and we agreed to include test fixes in his PR. This will demonstrate how these changes affect tests and facilitate GitHub Actions checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @javorszky,
tests: Change request_uri tests for changed behaviour Unverified
Any idea why this commit showed Unverified
on GH?
I think it's because you brought in my change, the change is of a different sha as my signature is for, so it fails. The original commit is verified if that helps. Specifically the differences between hongzhidao@ae4081f (Verified) and 6d6c68c (Unverified) include:
Each of these on their own would change the commit's digest that the original signature is for 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From 1ff64a7dc183a2f73940582cab61f125e59e341a Mon Sep 17 00:00:00 2001
From: Zhidao HONG <z.hong@f5.com>
Date: Tue, 30 Apr 2024 14:30:24 +0800
Subject: [PATCH] http: Use consistent target in nxt_h1p_peer_header_send()
No functional changes.
---
src/nxt_h1proto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/nxt_h1proto.c b/src/nxt_h1proto.c
index c3a656793..54fe77762 100644
--- a/src/nxt_h1proto.c
+++ b/src/nxt_h1proto.c
@@ -2306,7 +2306,7 @@ nxt_h1p_peer_header_send(nxt_task_t *task, nxt_http_peer_t *peer)
p = nxt_cpymem(p, r->method->start, r->method->length);
*p++ = ' ';
- p = nxt_cpymem(p, r->target.start, r->target.length);
+ p = nxt_cpymem(p, target.start, target.length);
p = nxt_cpymem(p, " HTTP/1.1\r\n", 11);
p = nxt_cpymem(p, "Connection: close\r\n", 19);
Hi @hongzhidao
Are you sure this has no functional change?
target
may be set to r->target
in nxt_h1p_peer_request_target()
in which case it would be no functional change. However it may also be set to something different.
I agree with the change in of itself, as that's what we use in the previous size calculation., but it looks like it may actually fix a potential issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From f8d106920ebda1671d56d6978cfc1cc9da4b5625 Mon Sep 17 00:00:00 2001
From: Zhidao HONG <z.hong@f5.com>
Date: Tue, 30 Apr 2024 14:45:18 +0800
Subject: [PATCH] http: Ensure REQUEST_URI immutability
Previously, the REQUEST_URI within Unit could be modified,
for example, during uri rewritting. We plan to make $request_uri
immutable and pass constant REQUEST_URI to applications.
Based on the new requirement, we remove `r->target` rewritting
in the rewrite module.
Closes: https://github.com/nginx/unit/issues/#916
Some small typos: s/rewritting/rewriting/g
and s/#916/916/
in the closes tag...
We plan to make $request_uri immutable and pass constant REQUEST_URI to applications.
This commit does that right? as it makes it sounds as if there is further work to do...
Yes, I think so. Before we change the rule, I mean REQUEST_URI is changeable, the
When URL rewriting happens, both path and r->target are changed. |
Good point, it is a decision already made, not something that will happen in the future. |
I did it based on your patch and committed it with |
I was more thinking about
Could |
Yes, but the verification is expected to fail. Depends what's more important to the project. In all honesty I don't care to get my patch included with me as an author, as long as the tests are fixed, this PR merged, and a new release with the request uri changes published. |
We currently don't really worry about signing commits, so I'd say don't worry about it. |
Hmm, they should be the same.
|
Just so I fully understand this. static nxt_int_t
nxt_h1p_peer_request_target(nxt_http_request_t *r, nxt_str_t *target)
{
u_char *p;
size_t size, encode;
if (!r->uri_changed) {
*target = r->target;
return NXT_OK;
}
if (!r->quoted_target && r->args->length == 0) {
*target = *r->path;
return NXT_OK; So you're saying that at this point Carrying on ...
target->start = nxt_mp_nget(r->mem_pool, size);
if (target->start == NULL) {
return NXT_ERROR;
}
if (r->quoted_target) {
p = (u_char *) nxt_encode_complex_uri(target->start, r->path->start,
r->path->length);
} else {
p = nxt_cpymem(target->start, r->path->start, r->path->length);
}
if (r->args->length > 0) {
*p++ = '?';
p = nxt_cpymem(p, r->args->start, r->args->length);
}
target->length = p - target->start;
... After all that, |
Yep, and it might be not changed, in this case, we don't need to allocate a new memory.
Correct. |
That then begs the question, why even call |
I'm afraid of forgetting something here. Let me explain it again, and let's assume we are talking about the new change that makes REQUEST_URI constant.
Without uri rewriting, they are the same. Then we decide to make REQUEST_URI constant, and internally, I keep
If we are talking about the new change of REQUEST_URI rule, I'd say not. |
OK, I think we're getting there... To clarify, I'm talking specifically about the first change
So at the point of this change, I.e before the second patch, After the second patch Assuming I've got that right, I think I'd like a little more detail in |
Makes sense, thanks. |
Hi @hongzhidao You can add my
(for the second commit they should come immedaitely after the |
Hi @ac000 @andrey-zelenkov,
|
Yes, it's a known issue. |
Ok. Let me know if it's ready to ship. |
Yes, it looks OK to me... |
This change is required for the next commit, after which target and r->target may be different. Before the next patch, target and r->target would be the same. No functional changes. Reviewed-by: Andrew Clayton <a.clayton@nginx.com> Signed-off-by: Zhidao HONG <z.hong@f5.com>
Previously, the REQUEST_URI within Unit could be modified, for example, during uri rewriting. We decide to make $request_uri immutable and pass constant REQUEST_URI to applications. Based on the new requirement, we remove `r->target` rewriting in the rewrite module. Closes: nginx#916 Reviewed-by: Andrew Clayton <a.clayton@nginx.com> Signed-off-by: Zhidao HONG <z.hong@f5.com>
Another implementation of proxy request creation.</de.$request_uri
constant.REQUEST_URI
to applications.Need to separate the PR after tests pass.
Here's a manual test.