Nginx body routing done fixes#1052
Closed
VadimZhestikov wants to merge 7 commits intonginx:masterfrom
Closed
Conversation
VadimZhestikov
changed the title
VadimZhestikov
changed the title
VadimZhestikov
force-pushed
the
nginx_body_routing_done_fixes
branch
2 times, most recently
from
9f0b758 to
6b813a5
Compare
The directive registers a JavaScript handler in the access phase, running after built-in access checkers (allow/deny, auth_basic, auth_request). r.subrequest(), ngx.fetch() and other async operations are supported. The handler defaults to NGX_OK (access granted) on normal completion, matching the behavior of other access phase modules. The r.decline() method allows the handler to return NGX_DECLINED (no opinion), deferring the decision to other access checkers under "satisfy any". The r.return() method can send any HTTP response from the access phase, including 3xx redirects for authentication flows.
Added async methods
- r.readRequestText() as string
- r.readRequestBuffer() as ArrayBuffer
- r.readRequestJSON() as object.
that return Promises resolving with the request body wrapped
as a corresponding type.
The async method parses the client request body as an HTML form and returns a Promise resolving to a form object with get(), getAll(), has(), forEach(), hasFiles() and fileFieldNames() accessors. Supports "application/x-www-form-urlencoded" and "multipart/form-data" content types. File parts are detected but their contents are not exposed. An optional maxKeys option caps the number of fields. File parts are detected but their contents are not exposed. A proper File API with streaming Blob semantics for bodies larger than memory is a significant amount of work.
VadimZhestikov
force-pushed
the
nginx_body_routing_done_fixes
branch
2 times, most recently
from
ec27c14 to
e1ef6c4
Compare
A part whose Content-Disposition carried only the extended parameter notation filename* (RFC 5987 / RFC 6266) was not recognised as a file upload: hasFiles() returned false and fileFieldNames() omitted the field. Only the plain filename parameter set is_file. Add an else-if branch for filename* in ngx_js_form_parse_disposition that sets is_file = 1 without attempting to decode the encoded value (charset, language tag, and percent-encoding are all left for application code to handle via the value's name property).
Several call sites in the form parser checked the return value of a callee with != NGX_JS_FORM_OK and then always returned the hardcoded NGX_JS_FORM_PARSE_ERROR, silently converting an allocation failure (NGX_ERROR = -1) into a parse error. Callers that distinguish the two codes (e.g. ngx_js_parse_form itself) therefore never saw NGX_ERROR bubbling up from deep in the parse chain. Fix by capturing the return value in a local rc variable and returning it unchanged in all five affected functions: ngx_js_form_parse_content_type ngx_js_form_parse_urlencoded ngx_js_form_parse_multipart ngx_js_form_parse_part_headers ngx_js_form_parse_disposition
VadimZhestikov
force-pushed
the
nginx_body_routing_done_fixes
branch
from
e1ef6c4 to
067e18d
Compare
njs_vm_object_prop() returns NULL both for a real VM exception and for a
simply absent property. ngx_http_js_request_form_max_keys() treated NULL
as an unconditional error, so readRequestForm({}) — passing an options
object without a maxKeys key — returned NJS_ERROR and caused HTTP 500.
The QJS path is not affected: JS_GetPropertyStr() returns JS_UNDEFINED for
absent properties and JS_EXCEPTION only on error, so the existing
JS_IsUndefined() check there already handles the absent-key case correctly.
Fix by collapsing the NULL and undefined checks into a single condition,
matching the established pattern for optional option properties elsewhere
in this file.
Contributor
Author
|
Closed, as all found issues where fixed in #1044 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes proposed for 'Before merge' issues mentioned in review for initial PR #1044 on top of it.
In addition all security findings are resolved: