Modules: CR/LF in njs-produced nginx variables
Nobody has claimed this yet.
- Dominant language
- C
- Stars
- 1.6k
- Forks
- 239
- Avg merge
- 2d 9h
- Merged PRs (30d)
- 10
Description
#1111 added header-value validation to r.headersOut and to redirect targets passed through r.return().
In the review for #1111, the remaining raw conversion in ngx_http_js_request_variables() was noted as:
not a header path -- correctly left alone.
There is a consequence of that distinction when an njs-produced variable is later consumed by a directive that serializes it into an HTTP header.
The shortest case uses js_set:
function v(r) {
return r.args.v;
}
export default { v };
js_import test from test.js;
js_set $fromjs test.v;
location /set {
add_header X-Test $fromjs;
return 200 "ok\n";
}
Requesting:
/set?v=a%0d%0aInjected:%201
produces:
X-Test: a
Injected: 1
The value is already decoded when the handler sees it. Reporting the character codes of r.args.v for the same request gives:
97,13,10,...
so CR and LF are present as actual characters rather than as the encoded %0d%0a text.
The equivalent direct header store is rejected:
r.headersOut['X-Test'] = r.args.v;
// TypeError: invalid header value
while both of these preserve the CR/LF:
// js_set
return r.args.v;
// r.variables
r.variables.injected = r.args.v;
The r.variables case can also reach an upstream request header:
function up(r) {
r.variables.injected = r.args.v;
r.internalRedirect('/proxied');
}
export default { up };
js_import test from test.js;
js_var $injected;
location /up {
js_content test.up;
}
location /proxied {
internal;
proxy_set_header X-Fwd $injected;
proxy_pass http://127.0.0.1:9002;
}
Requesting:
/up?v=a%0d%0aX-Smuggled:%20yes
makes nginx send upstream:
GET /proxied HTTP/1.0
X-Fwd: a
X-Smuggled: yes
Host: 127.0.0.1:9002
Connection: close
This behavior is not unique to njs variables.
For example, $arg_v is not decoded:
add_header X-Test $arg_v;
and therefore produces:
X-Test: a%0d%0aInjected:%201
On the other hand, nginx variables can themselves contain decoded data. For example, $uri can produce the same header splitting behavior when used with add_header, with no njs involved.
So I am not assuming that generic nginx variables are required to satisfy HTTP header-value syntax.
The njs-specific case is that request data such as r.args.v is exposed decoded, and js_set or r.variables can carry that value into a generic nginx variable which is then consumed by a header-producing directive.
Given the distinction made in #1111, is preserving CR/LF in njs-produced nginx variables considered intentional even when those variables are later consumed by directives such as add_header or proxy_set_header?
As a sanity check, I locally tested rejecting only CR and LF at the points where an njs value becomes an nginx variable, including both js_set and r.variables.
With that change:
- the
js_setexample no longer emitsX-Test; - assigning the same value through
r.variablesthrowsTypeError: invalid variable value; - the upstream-header case is no longer possible;
- ordinary values remain unchanged, including
plain, UTF-8 such ascafé, and values containing a tab.
I limited the test to CR and LF rather than applying the full ngx_js_check_header_value() rules, since nginx variables are generic and may be consumed outside HTTP headers.
Tested with njs 1.0.1 built into nginx 1.28.0, using both the default engine and js_engine qjs; the behavior is identical with both.
I also ran the 104 nginx/t js and stream_js test files (1003 assertions) against the changed and unchanged builds. The pass/fail sets were identical. Twelve files fail in both builds in my ASan -j4 environment, so there was no regression delta from this change.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reading ngx_http_js_request_variables() and the paths handling js_set and r.variables. Reproduce the CR/LF cases with add_header and proxy_set_header, then compare behavior for plain, UTF-8, and tab-containing values. Run the nginx/t JavaScript and stream_js test suites; done means the intended variable behavior is resolved and covered without changing unrelated values.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, javascript, nginx
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100